diff mbox

BUG in bleeding edge c560f3d

Message ID CAKohpomDLWFP5tTnhTidOOrkEhW_z5Ht_MzbbnzDaq-i+-1aEg@mail.gmail.com (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Viresh Kumar Feb. 5, 2013, 7:08 a.m. UTC
On 5 February 2013 11:15, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 5 February 2013 08:13, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Monday, February 04, 2013 04:05:27 PM Dirk Brandewie wrote:
>
>>> Rebased a couple of hours ago testing now.  ATM it looks like it is related to
>>> cpufreq_stats handling of the sysfs files
>>
>> This looks like a problem with commit b8eed8a.  Viresh?
>
> I had some direct chat with Dirk to understand his problem. He has got lots
> of minor changes, which by themselves looks incorrect, for ex:
> - He is required to cpufreq_frequency_table_put_attr() from his drivers
>   init() code to make it NULL... But it should already have been NULL as per
>   my understanding. I really want him to test his driver without any additional
>   target/set_policy() patches he has.. to see if linux-next works for
> him or not.
>
> He will do this testing tomorrow and will let us know of any issues he faces.
>
> We have already tested linux-next and these patches on ARM TC2 and STE
> u8500 (by Fabio), with reboots and lots of hotplugging.
>
> Though i am still going to review my own patches once more to see if there is
> scope for improvement.

The system on which Dirk is testing his patches has following configuration:
1 Package, 4 cpus, 8 virtual cores...

Though they share their clock line in some way, it is handled by the
cpufreq driver
only and for the outside world (cpufreq core), it looks as there are 8
independent cpus,
and so 8 policy structures.

I tried to reproduce the issue at my end by removing extra cpus from
my clusters and
just keeping one per cluster alive from DT.

And i *wasn't* able to reproduce the problem. Though i was able to
find a small bug/issue
in the current code for which i have following patch (attached too):

@Dirk: Please give this one a try. Atleast on my system with various
configurations, i
couldn't see any different this patch has made, but is more logical to me.

commit 9bdd6d47403e696d05953870019e791806f8d6bf
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Tue Feb 5 12:28:18 2013 +0530

    cpufreq: Don't remove sysfs link for policy->cpu

    "cpufreq" directory in policy->cpu is never created using
sysfs_create_link(),
    but using kobject_init_and_add(). And so we shouldn't call
sysfs_remove_link()
    for policy->cpu(). sysfs stuff for policy->cpu is automatically
removed when we
    call kobject_put() for dying policy.

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

                sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
@@ -1072,7 +1074,6 @@ static int __cpufreq_remove_dev(struct device
*dev, struct subsys_interface *sif
        pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
        cpufreq_cpu_put(data);
        unlock_policy_rwsem_write(cpu);
-       sysfs_remove_link(&dev->kobj, "cpufreq");

        /* If cpu is last user of policy, free policy */
        if (cpus == 1) {

Comments

Rafael Wysocki Feb. 5, 2013, 10:08 a.m. UTC | #1
On Tuesday, February 05, 2013 12:38:44 PM Viresh Kumar wrote:
> On 5 February 2013 11:15, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 5 February 2013 08:13, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> On Monday, February 04, 2013 04:05:27 PM Dirk Brandewie wrote:
> >
> >>> Rebased a couple of hours ago testing now.  ATM it looks like it is related to
> >>> cpufreq_stats handling of the sysfs files
> >>
> >> This looks like a problem with commit b8eed8a.  Viresh?
> >
> > I had some direct chat with Dirk to understand his problem. He has got lots
> > of minor changes, which by themselves looks incorrect, for ex:
> > - He is required to cpufreq_frequency_table_put_attr() from his drivers
> >   init() code to make it NULL... But it should already have been NULL as per
> >   my understanding. I really want him to test his driver without any additional
> >   target/set_policy() patches he has.. to see if linux-next works for
> > him or not.
> >
> > He will do this testing tomorrow and will let us know of any issues he faces.
> >
> > We have already tested linux-next and these patches on ARM TC2 and STE
> > u8500 (by Fabio), with reboots and lots of hotplugging.
> >
> > Though i am still going to review my own patches once more to see if there is
> > scope for improvement.
> 
> The system on which Dirk is testing his patches has following configuration:
> 1 Package, 4 cpus, 8 virtual cores...
> 
> Though they share their clock line in some way, it is handled by the
> cpufreq driver
> only and for the outside world (cpufreq core), it looks as there are 8
> independent cpus,
> and so 8 policy structures.
> 
> I tried to reproduce the issue at my end by removing extra cpus from
> my clusters and
> just keeping one per cluster alive from DT.
> 
> And i *wasn't* able to reproduce the problem. Though i was able to
> find a small bug/issue
> in the current code for which i have following patch (attached too):

Thanks for working on this!

> @Dirk: Please give this one a try. Atleast on my system with various
> configurations, i
> couldn't see any different this patch has made, but is more logical to me.

It looks good, I'm going to take it.

Thanks,
Rafael


> commit 9bdd6d47403e696d05953870019e791806f8d6bf
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Tue Feb 5 12:28:18 2013 +0530
> 
>     cpufreq: Don't remove sysfs link for policy->cpu
> 
>     "cpufreq" directory in policy->cpu is never created using
> sysfs_create_link(),
>     but using kobject_init_and_add(). And so we shouldn't call
> sysfs_remove_link()
>     for policy->cpu(). sysfs stuff for policy->cpu is automatically
> removed when we
>     call kobject_put() for dying policy.
> 
>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7aacfbf..9567451 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1047,7 +1047,9 @@ static int __cpufreq_remove_dev(struct device
> *dev, struct subsys_interface *sif
>         cpus = cpumask_weight(data->cpus);
>         cpumask_clear_cpu(cpu, data->cpus);
> 
> -       if (unlikely((cpu == data->cpu) && (cpus > 1))) {
> +       if (cpu != data->cpu) {
> +               sysfs_remove_link(&dev->kobj, "cpufreq");
> +       } else if (cpus > 1) {
>                 /* first sibling now owns the new sysfs dir */
>                 cpu_dev = get_cpu_device(cpumask_first(data->cpus));
>                 sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
> @@ -1072,7 +1074,6 @@ static int __cpufreq_remove_dev(struct device
> *dev, struct subsys_interface *sif
>         pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
>         cpufreq_cpu_put(data);
>         unlock_policy_rwsem_write(cpu);
> -       sysfs_remove_link(&dev->kobj, "cpufreq");
> 
>         /* If cpu is last user of policy, free policy */
>         if (cpus == 1) {
dirk.brandewie@gmail.com Feb. 5, 2013, 6:45 p.m. UTC | #2
On 02/04/2013 11:08 PM, Viresh Kumar wrote:
> @Dirk: Please give this one a try. Atleast on my system with various
> configurations, i
> couldn't see any different this patch has made, but is more logical to me.
>

Tested works fine with and without my driver present.

I added it to my patch set with my Tested-by tag it seems gmail decided
to swallow it :-(

Please feel to add my Tested-by:

--Dirk
> commit 9bdd6d47403e696d05953870019e791806f8d6bf
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Tue Feb 5 12:28:18 2013 +0530
>
>      cpufreq: Don't remove sysfs link for policy->cpu
>
>      "cpufreq" directory in policy->cpu is never created using
> sysfs_create_link(),
>      but using kobject_init_and_add(). And so we shouldn't call
> sysfs_remove_link()
>      for policy->cpu(). sysfs stuff for policy->cpu is automatically
> removed when we
>      call kobject_put() for dying policy.
>
>      Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/cpufreq.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7aacfbf..9567451 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1047,7 +1047,9 @@ static int __cpufreq_remove_dev(struct device
> *dev, struct subsys_interface *sif
>          cpus = cpumask_weight(data->cpus);
>          cpumask_clear_cpu(cpu, data->cpus);
>
> -       if (unlikely((cpu == data->cpu) && (cpus > 1))) {
> +       if (cpu != data->cpu) {
> +               sysfs_remove_link(&dev->kobj, "cpufreq");
> +       } else if (cpus > 1) {
>                  /* first sibling now owns the new sysfs dir */
>                  cpu_dev = get_cpu_device(cpumask_first(data->cpus));
>                  sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
> @@ -1072,7 +1074,6 @@ static int __cpufreq_remove_dev(struct device
> *dev, struct subsys_interface *sif
>          pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
>          cpufreq_cpu_put(data);
>          unlock_policy_rwsem_write(cpu);
> -       sysfs_remove_link(&dev->kobj, "cpufreq");
>
>          /* If cpu is last user of policy, free policy */
>          if (cpus == 1) {
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Feb. 5, 2013, 9:30 p.m. UTC | #3
On Tuesday, February 05, 2013 10:45:03 AM Dirk Brandewie wrote:
> On 02/04/2013 11:08 PM, Viresh Kumar wrote:
> > @Dirk: Please give this one a try. Atleast on my system with various
> > configurations, i
> > couldn't see any different this patch has made, but is more logical to me.
> >
> 
> Tested works fine with and without my driver present.
> 
> I added it to my patch set with my Tested-by tag it seems gmail decided
> to swallow it :-(
> 
> Please feel to add my Tested-by:

I will, thanks Dirk!

Rafael


> > commit 9bdd6d47403e696d05953870019e791806f8d6bf
> > Author: Viresh Kumar <viresh.kumar@linaro.org>
> > Date:   Tue Feb 5 12:28:18 2013 +0530
> >
> >      cpufreq: Don't remove sysfs link for policy->cpu
> >
> >      "cpufreq" directory in policy->cpu is never created using
> > sysfs_create_link(),
> >      but using kobject_init_and_add(). And so we shouldn't call
> > sysfs_remove_link()
> >      for policy->cpu(). sysfs stuff for policy->cpu is automatically
> > removed when we
> >      call kobject_put() for dying policy.
> >
> >      Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >   drivers/cpufreq/cpufreq.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 7aacfbf..9567451 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1047,7 +1047,9 @@ static int __cpufreq_remove_dev(struct device
> > *dev, struct subsys_interface *sif
> >          cpus = cpumask_weight(data->cpus);
> >          cpumask_clear_cpu(cpu, data->cpus);
> >
> > -       if (unlikely((cpu == data->cpu) && (cpus > 1))) {
> > +       if (cpu != data->cpu) {
> > +               sysfs_remove_link(&dev->kobj, "cpufreq");
> > +       } else if (cpus > 1) {
> >                  /* first sibling now owns the new sysfs dir */
> >                  cpu_dev = get_cpu_device(cpumask_first(data->cpus));
> >                  sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
> > @@ -1072,7 +1074,6 @@ static int __cpufreq_remove_dev(struct device
> > *dev, struct subsys_interface *sif
> >          pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
> >          cpufreq_cpu_put(data);
> >          unlock_policy_rwsem_write(cpu);
> > -       sysfs_remove_link(&dev->kobj, "cpufreq");
> >
> >          /* If cpu is last user of policy, free policy */
> >          if (cpus == 1) {
> >
>
Viresh Kumar Feb. 6, 2013, 1:34 a.m. UTC | #4
On 6 February 2013 00:15, Dirk Brandewie <dirk.brandewie@gmail.com> wrote:
> On 02/04/2013 11:08 PM, Viresh Kumar wrote:
>>
>> @Dirk: Please give this one a try. Atleast on my system with various
>> configurations, i
>> couldn't see any different this patch has made, but is more logical to me.
>>
>
> Tested works fine with and without my driver present.

Wow!! I was shooting in the dark without knowing where the right target is
and luckily i found the target :)
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7aacfbf..9567451 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1047,7 +1047,9 @@  static int __cpufreq_remove_dev(struct device
*dev, struct subsys_interface *sif
        cpus = cpumask_weight(data->cpus);
        cpumask_clear_cpu(cpu, data->cpus);

-       if (unlikely((cpu == data->cpu) && (cpus > 1))) {
+       if (cpu != data->cpu) {
+               sysfs_remove_link(&dev->kobj, "cpufreq");
+       } else if (cpus > 1) {
                /* first sibling now owns the new sysfs dir */
                cpu_dev = get_cpu_device(cpumask_first(data->cpus));