Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1325355
| From | "Rafael J. Wysocki" <rafael@kernel.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops |
| Date | 2016-02-03 13:50 +0100 |
| Message-ID | <qY52H-212-43@gated-at.bofh.it> (permalink) |
| References | <qXGQG-1uV-11@gated-at.bofh.it> <qXGQI-1uV-51@gated-at.bofh.it> <qXQGo-FZ-47@gated-at.bofh.it> <qXZzY-6PO-3@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
On Wed, Feb 3, 2016 at 7:58 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 02-02-16, 22:23, Rafael J. Wysocki wrote:
>> On Tue, Feb 2, 2016 at 11:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
>> "The ondemand and conservative governors use the global-attr or
>> freq-attr structures to represent sysfs attributes corresponding to
>> their tunables
>
>> (which of them is actually used depends on whether or
>> not different policy objects can use different governors at the same
>> time
>
> Not exactly. Different policies can always use different governors.
> What made the difference was that different policies using same
> governor, with different tunables or separate governor directories.
>
> I have reworded this para like:
>
> The ondemand and conservative governors use the global-attr or freq-attr
> structures to represent sysfs attributes corresponding to their tunables
> (which of them is actually used depends on whether or not different
> policy objects can use same governor with different tunables at the same
> time and, consequently, on where those attributes are located in sysfs).
>
> Please let me know if isn't clear.
That's OK. IMO you should say "use the same governor", but that's
easily fixable. :-)
>> > --- a/drivers/cpufreq/cpufreq_governor.c
>> > + ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type,
>> > + get_governor_parent_kobj(policy),
>> > + attr_group->name);
>> > + if (ret) {
>> > + pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret);
>>
>> pr_debug() would be better here.
>
> Its a real error, why pr_debug for that ?
What's the value of printing that on user systems? It contains debug
information only and it is not useful to anyone unfamiliar with the
code in question anyway.
>> > goto reset_gdbs_data;
>> > + }
>> >
>> > return 0;
>> >
>> > @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy *policy,
>> > return -EBUSY;
>> >
>> > if (!--dbs_data->usage_count) {
>> > - sysfs_remove_group(get_governor_parent_kobj(policy),
>> > - get_sysfs_attr(dbs_data));
>> > + kobject_put(&dbs_data->kobj);
>>
>> Don't we need a ->release callback for this kobject?
>
> There is nothing that we need to free from the ->release() callback.
> We are using the kobject here just to get separate show/store
> callbacks.
Well, I guess the answer should be that there can't be more active
references to the kobject, so it is safe to free it synchronously
later.
> Here is the new version based on the review comments received until
> now:
>
> -------------------------8<-------------------------
>
> From: Viresh Kumar <viresh.kumar@linaro.org>
> Date: Tue, 2 Feb 2016 12:35:01 +0530
> Subject: [PATCH] cpufreq: governor: New sysfs show/store callbacks for
> governor tunables
>
[cut]
> @@ -22,14 +22,62 @@
>
> #include "cpufreq_governor.h"
>
> -static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data)
> +static inline struct dbs_data *to_dbs_data(struct kobject *kobj)
> {
> - if (have_governor_per_policy())
> - return dbs_data->cdata->attr_group_gov_pol;
> - else
> - return dbs_data->cdata->attr_group_gov_sys;
> + return container_of(kobj, struct dbs_data, kobj);
> +}
> +
> +static inline struct governor_attr *to_gov_attr(struct attribute *attr)
> +{
> + return container_of(attr, struct governor_attr, attr);
> +}
> +
> +static ssize_t governor_show(struct kobject *kobj, struct attribute *attr,
> + char *buf)
> +{
> + struct dbs_data *dbs_data = to_dbs_data(kobj);
> + struct governor_attr *gattr = to_gov_attr(attr);
> + int ret = -EIO;
> +
> + down_read(&dbs_data->rwsem);
> +
> + if (gattr->show)
> + ret = gattr->show(dbs_data, buf);
> +
> + up_read(&dbs_data->rwsem);
Do we need the lock here too?
show() is only going to read the value, isn't it? And everything u32
or smaller is read atomically anyway.
Apart from this it looks good to me.
When you're ready, please resend the whole series without patch [5/5]
which is premature IMO.
Thanks,
Rafael
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
[PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
Re: [PATCH 3/5] cpufreq: governor: Remove unused sysfs attribute macros "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 22:40 +0100
[PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 23:00 +0100
Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 07:00 +0100
Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 13:30 +0100
Re: [PATCH 4/5] cpufreq: Don't drop rwsem before calling CPUFREQ_GOV_POLICY_EXIT Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 14:10 +0100
[PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Juri Lelli <juri.lelli@arm.com> - 2016-02-02 17:50 +0100
Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 07:10 +0100
Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Juri Lelli <juri.lelli@arm.com> - 2016-02-03 12:10 +0100
Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 12:10 +0100
Re: [PATCH 5/5] cpufreq: Get rid of ->governor_enabled and its lock "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 23:00 +0100
[PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-02 12:00 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Juri Lelli <juri.lelli@arm.com> - 2016-02-02 16:50 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 17:40 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Juri Lelli <juri.lelli@arm.com> - 2016-02-02 18:10 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 20:50 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-02 23:30 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 00:50 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 02:10 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-03 02:40 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 03:00 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-03 05:10 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 08:00 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-03 21:10 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 08:00 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Juri Lelli <juri.lelli@arm.com> - 2016-02-03 12:00 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 12:00 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Saravana Kannan <skannan@codeaurora.org> - 2016-02-03 21:20 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 08:00 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 07:40 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 22:30 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 08:00 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 13:50 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 14:30 +0100
Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-03 14:40 +0100
Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Juri Lelli <juri.lelli@arm.com> - 2016-02-02 12:30 +0100
Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups "Rafael J. Wysocki" <rafael@kernel.org> - 2016-02-02 21:10 +0100
Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 03:30 +0100
Re: [PATCH 0/5] cpufreq: governors: Solve the ABBA lockups Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-03 12:40 +0100
csiph-web