Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1543754 > unrolled thread

thermal zones break with patch "Reimplement IDR and IDA using the radix tree" (mainline+next)

Back to article view | Back to linux.kernel

Contents

  thermal zones break with patch "Reimplement IDR and IDA using the radix tree" (mainline+next) Heiko Stuebner <heiko@sntech.de> - 2016-12-16 22:20 +0100
    RE: thermal zones break with patch "Reimplement IDR and IDA using the  radix tree" (mainline+next) Matthew Wilcox <mawilcox@microsoft.com> - 2016-12-16 22:40 +0100
      Re: thermal zones break with patch "Reimplement IDR and IDA using the radix tree" (mainline+next) Heiko Stuebner <heiko@sntech.de> - 2016-12-17 01:50 +0100
        Re: thermal zones break with patch "Reimplement IDR and IDA using  the radix tree" (mainline+next) Eduardo Valentin <edubezval@gmail.com> - 2016-12-20 05:00 +0100
      Re: thermal zones break with patch "Reimplement IDR and IDA using the  radix tree" (mainline+next) Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-18 20:50 +0100
        RE: thermal zones break with patch "Reimplement IDR and IDA using the  radix tree" (mainline+next) Matthew Wilcox <mawilcox@microsoft.com> - 2016-12-19 17:40 +0100

#1543754 — thermal zones break with patch "Reimplement IDR and IDA using the radix tree" (mainline+next)

Hi,

commit b05bbe3ea2db ("Reimplement IDR and IDA using the radix tree") seems to
break thermal zone allocation. This happens both on todays mainline and 
linux-next-20161216 and produces errors like:

[    9.397201] WARNING: CPU: 1 PID: 165 at ../fs/sysfs/dir.c:31 sysfs_warn_dup+0x64/0x74
[    9.405145] sysfs: cannot create duplicate filename '/devices/virtual/thermal/thermal_zone1'
[    9.514951] Hardware name: Rockchip (Device Tree)
[    9.522611] [<c03104a4>] (unwind_backtrace) from [<c030ba50>] (show_stack+0x10/0x14)
[    9.532925] [<c030ba50>] (show_stack) from [<c05a11fc>] (dump_stack+0x88/0x9c)
[    9.542695] [<c05a11fc>] (dump_stack) from [<c0340c68>] (__warn+0xe8/0x100)
[    9.552166] [<c0340c68>] (__warn) from [<c0340cb8>] (warn_slowpath_fmt+0x38/0x48)
[    9.562154] [<c0340cb8>] (warn_slowpath_fmt) from [<c047e944>] (sysfs_warn_dup+0x64/0x74)
[    9.572834] [<c047e944>] (sysfs_warn_dup) from [<c047ea20>] (sysfs_create_dir_ns+0x84/0x94)
[    9.583687] [<c047ea20>] (sysfs_create_dir_ns) from [<c05a2cb8>] (kobject_add_internal+0xac/0x2fc)
[    9.595149] [<c05a2cb8>] (kobject_add_internal) from [<c05a2f50>] (kobject_add+0x48/0x98)
[    9.605829] [<c05a2f50>] (kobject_add) from [<c07ee340>] (device_add+0xd4/0x5b0)
[    9.615714] [<c07ee340>] (device_add) from [<c0a43a98>] (thermal_zone_device_register+0x174/0x5cc)
[    9.627173] [<c0a43a98>] (thermal_zone_device_register) from [<c0a3c588>] (__power_supply_register+0x374/0x44c)
[    9.639772] [<c0a3c588>] (__power_supply_register) from [<c0a3c6bc>] (devm_power_supply_register+0x4c/0x7c)

System is a Rockchip rk3288-veyron and it has 4 thermal zones (3 from
the thermal ip block of the soc and one from the battery).

Reverting the patch mentioned above results in thermal zones being
registered again.

While I haven't looked to deeply into what idr exactly does, some findings:
- thermal_zone0 and thermal_zone1 are allocated correctly
- every further thermal_zone always gets allocated the number "1"
- thermal core calls idr_alloc with 0 for both start and end
- the rewrite-patch seems to change the semantics of idr_alloc
  where it orignally said "@end: the maximum id (exclusive, <= 0 for max)"
  the "<= 0" part is gone now, but I checked, simply setting INT_MAX
  as end in the thermal_core does not help


Heiko

[toc] | [next] | [standalone]

#1543766 — RE: thermal zones break with patch "Reimplement IDR and IDA using the radix tree" (mainline+next)

From: Heiko Stuebner [mailto:heiko@sntech.de]
> commit b05bbe3ea2db ("Reimplement IDR and IDA using the radix tree")
> seems to
> break thermal zone allocation. This happens both on todays mainline and
> linux-next-20161216 and produces errors like:

> While I haven't looked to deeply into what idr exactly does, some findings:
> - thermal_zone0 and thermal_zone1 are allocated correctly
> - every further thermal_zone always gets allocated the number "1"
> - thermal core calls idr_alloc with 0 for both start and end
> - the rewrite-patch seems to change the semantics of idr_alloc
>   where it orignally said "@end: the maximum id (exclusive, <= 0 for max)"
>   the "<= 0" part is gone now, but I checked, simply setting INT_MAX
>   as end in the thermal_core does not help

Hi Heiko,

Thanks for the report!  The problem is because the thermal subsystem calls idr_alloc() passing a NULL pointer for the data.  I have fixed this problem in my git tree but haven't sent the patch to Andrew yet.  This patch should fix the problem for you:

http://git.infradead.org/users/willy/linux-dax.git/commitdiff/c52eeed7b759c3fefe9b7f1b0a17a438df6950f3

Now ... thermal is actually using an IDR when it could save memory by using an IDA.  Are you interested in doing that conversion?

[toc] | [prev] | [next] | [standalone]

#1543867

Hi Matthew,

Am Freitag, 16. Dezember 2016, 21:19:37 CET schrieb Matthew Wilcox:
> From: Heiko Stuebner [mailto:heiko@sntech.de]
> 
> > commit b05bbe3ea2db ("Reimplement IDR and IDA using the radix tree")
> > seems to
> > break thermal zone allocation. This happens both on todays mainline and
> > linux-next-20161216 and produces errors like:
> 
> 
> 
> > While I haven't looked to deeply into what idr exactly does, some
> > findings:
 - thermal_zone0 and thermal_zone1 are allocated correctly
> > - every further thermal_zone always gets allocated the number "1"
> > - thermal core calls idr_alloc with 0 for both start and end
> > - the rewrite-patch seems to change the semantics of idr_alloc
> > 
> >   where it orignally said "@end: the maximum id (exclusive, <= 0 for
> >   max)"
> >   the "<= 0" part is gone now, but I checked, simply setting INT_MAX
> >   as end in the thermal_core does not help
> 
> 
> Hi Heiko,
> 
> Thanks for the report!  The problem is because the thermal subsystem calls
> idr_alloc() passing a NULL pointer for the data.  I have fixed this problem
> in my git tree but haven't sent the patch to Andrew yet.  This patch should
> fix the problem for you:
 
> http://git.infradead.org/users/willy/linux-dax.git/commitdiff/c52eeed7b759c3
> fefe9b7f1b0a17a438df6950f3

yay, this fixes the issue. This should definitly go as fix into 4.10-rc and when 
you send it to Andrew you can add my

Tested-by: Heiko Stuebner <heiko@sntech.de>

 
> Now ... thermal is actually using an IDR when it could save memory by using
> an IDA.  Are you interested in doing that conversion?

That would more be a question for Eduardo and Rui :-) . I'm just in the 
process of tracking down the smallish issues on my Rockchip board that are 
poping up during the merge-window.


Anyway, thanks for pointing to the fix
Heiko

[toc] | [prev] | [next] | [standalone]

#1544890 — Re: thermal zones break with patch "Reimplement IDR and IDA using the radix tree" (mainline+next)

Hey,

On Sat, Dec 17, 2016 at 01:48:33AM +0100, Heiko Stuebner wrote:
> Hi Matthew,
> 
> Am Freitag, 16. Dezember 2016, 21:19:37 CET schrieb Matthew Wilcox:
> > From: Heiko Stuebner [mailto:heiko@sntech.de]
> > 
> > > commit b05bbe3ea2db ("Reimplement IDR and IDA using the radix tree")
> > > seems to
> > > break thermal zone allocation. This happens both on todays mainline and
> > > linux-next-20161216 and produces errors like:
> > 
> > 
> > 
> > > While I haven't looked to deeply into what idr exactly does, some
> > > findings:
>  - thermal_zone0 and thermal_zone1 are allocated correctly
> > > - every further thermal_zone always gets allocated the number "1"
> > > - thermal core calls idr_alloc with 0 for both start and end
> > > - the rewrite-patch seems to change the semantics of idr_alloc
> > > 
> > >   where it orignally said "@end: the maximum id (exclusive, <= 0 for
> > >   max)"
> > >   the "<= 0" part is gone now, but I checked, simply setting INT_MAX
> > >   as end in the thermal_core does not help
> > 
> > 
> > Hi Heiko,
> > 
> > Thanks for the report!  The problem is because the thermal subsystem calls
> > idr_alloc() passing a NULL pointer for the data.  I have fixed this problem
> > in my git tree but haven't sent the patch to Andrew yet.  This patch should
> > fix the problem for you:
>  
> > http://git.infradead.org/users/willy/linux-dax.git/commitdiff/c52eeed7b759c3
> > fefe9b7f1b0a17a438df6950f3
> 
> yay, this fixes the issue. This should definitly go as fix into 4.10-rc and when 
> you send it to Andrew you can add my
> 
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> 
>  
> > Now ... thermal is actually using an IDR when it could save memory by using
> > an IDA.  Are you interested in doing that conversion?
> 
> That would more be a question for Eduardo and Rui :-) . I'm just in the 
> process of tracking down the smallish issues on my Rockchip board that are 
> poping up during the merge-window.

I would prefer to get the right implementation, and fixing thermal core by using IDA.

But given that this touches thermal core, we need an agreement with Rui
too.

> 
> 
> Anyway, thanks for pointing to the fix
> Heiko

[toc] | [prev] | [next] | [standalone]

#1544193 — Re: thermal zones break with patch "Reimplement IDR and IDA using the radix tree" (mainline+next)

On Fri, Dec 16, 2016 at 11:19 PM, Matthew Wilcox <mawilcox@microsoft.com> wrote:
> From: Heiko Stuebner [mailto:heiko@sntech.de]
>> commit b05bbe3ea2db ("Reimplement IDR and IDA using the radix tree")
>> seems to
>> break thermal zone allocation. This happens both on todays mainline and
>> linux-next-20161216 and produces errors like:
>
>> While I haven't looked to deeply into what idr exactly does, some findings:
>> - thermal_zone0 and thermal_zone1 are allocated correctly
>> - every further thermal_zone always gets allocated the number "1"
>> - thermal core calls idr_alloc with 0 for both start and end
>> - the rewrite-patch seems to change the semantics of idr_alloc
>>   where it orignally said "@end: the maximum id (exclusive, <= 0 for max)"
>>   the "<= 0" part is gone now, but I checked, simply setting INT_MAX
>>   as end in the thermal_core does not help
>
> Hi Heiko,
>
> Thanks for the report!  The problem is because the thermal subsystem calls idr_alloc() passing a NULL pointer for the data.  I have fixed this problem in my git tree but haven't sent the patch to Andrew yet.  This patch should fix the problem for you:
>
> http://git.infradead.org/users/willy/linux-dax.git/commitdiff/c52eeed7b759c3fefe9b7f1b0a17a438df6950f3
>
> Now ... thermal is actually using an IDR when it could save memory by using an IDA.  Are you interested in doing that conversion?

+Cc: Mika, Vinod

Same here for at least DMA Engine (and perhaps another place in
thermal subsystem).

For DMA Engine case

Tested-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1544662 — RE: thermal zones break with patch "Reimplement IDR and IDA using the radix tree" (mainline+next)

From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> On Fri, Dec 16, 2016 at 11:19 PM, Matthew Wilcox
> <mawilcox@microsoft.com> wrote:
> > Now ... thermal is actually using an IDR when it could save memory by using
> an IDA.  Are you interested in doing that conversion?
> 
> +Cc: Mika, Vinod
> 
> Same here for at least DMA Engine (and perhaps another place in
> thermal subsystem).

Hey Andy, thanks for testing!  Could you test another patch for me as well, to switch DMA Engine over to using an IDA instead of an IDR?  This should save 576 bytes on a normal setup.

The patch itself is here:
http://git.infradead.org/users/willy/linux-dax.git/commitdiff/a4f4e8554639b3fba9a62c07ad0e2423ea1f5f31

but it may be better just to test the head of this tree:
http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/idr-2016-12-16

as I keep fixing bugs :-)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web