Re: [PATCH v4 1/5] clk: sunxi-ng: common: Support minimum and maximum rate

[Frank Oltmanns <frank@oltmanns.dev>]

Hi Robert,

I'm truly sorry for the trouble the patch has caused you and for my late
reply!

On 2024-06-14 at 23:52:08 +0000, "Pafford, Robert J." <pafford.9@buckeyemail.osu.edu> wrote:
>> The Allwinner SoC's typically have an upper and lower limit for their
>> clocks' rates. Up until now, support for that has been implemented
>> separately for each clock type.
>>
>> Implement that functionality in the sunxi-ng's common part making use of
>> the CCF rate liming capabilities, so that it is available for all clock
>> types.
>>
>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/clk/sunxi-ng/ccu_common.c | 19 +++++++++++++++++++
>>  drivers/clk/sunxi-ng/ccu_common.h |  3 +++
>>  2 files changed, 22 insertions(+)
>
> This patch appears to cause a buffer under-read bug due to the call to 'hw_to_ccu_common', which assumes all entries
> in the desc->hw_clocks->hws array are contained in ccu_common structs.
>
> However, not all clocks in the array are contained in ccu_common structs. For example, as part
> of the "sun20i-d1-ccu" driver, the "pll-video0" clock holds the 'clk_hw' struct inside of a 'clk_fixed_factor' struct,
> as it is a fixed factor clock based on the "pll-video0-4x" clock, created with the CLK_FIXED_FACTOR_HWS macro.
> This results in undefined behavior as the hw_to_ccu_common returns an invalid pointer referencing memory before the
> 'clk_fixed_factor' struct.
>

Great catch! At first glance, it seems to me that calling
clk_hw_set_rate_range() in sunxi_ccu_probe() should not have happenend
in the loop that iterates over the hw_clks.

Instead we should add one more loop that iterates over the ccu_clks.
Note, that there is already one such loop but, unfortunately, we can't
use that as it happens before the hw_clks loop and we can only call
clk_hw_set_rate_range() after the hw_clk has been registered.

Hence, I propose to move the offending code to a new loop:
	for (i = 0; i < desc->num_ccu_clks; i++) {
		struct ccu_common *cclk = desc->ccu_clks[i];

		if (!cclk)
			continue;

		if (cclk->max_rate)
			clk_hw_set_rate_range(&cclk->hw, common->min_rate,
					      common->max_rate);
		else
			WARN(cclk->min_rate,
			     "No max_rate, ignoring min_rate of clock %d - %s\n",
			     i, cclk->hw.init->name);
	}

I haven't tested (or even compiled) the above, but I'll test and send a
patch within the next few days for you to test.

Thanks again,
  Frank

>
> I have attached kernel warnings from a system based on the "sun8i-t113s.dtsi" device tree, where the memory contains
> a non-zero value for the min-rate but a zero value for the max-rate, triggering the "No max_rate, ignoring min_rate"
> warning in the 'sunxi_ccu_probe' function.
>
>
> [    0.549013] ------------[ cut here ]------------
> [    0.553727] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
> [    0.563153] No max_rate, ignoring min_rate of clock 6 - pll-periph0-div3
> [    0.569846] Modules linked in:
> [    0.572913] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.6.32-winglet #7
> [    0.579540] Hardware name: Generic DT based system
> [    0.584350]  unwind_backtrace from show_stack+0xb/0xc
> [    0.589445]  show_stack from dump_stack_lvl+0x2b/0x34
> [    0.594531]  dump_stack_lvl from __warn+0x5d/0x92
> [    0.599275]  __warn from warn_slowpath_fmt+0xd7/0x12c
> [    0.604354]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
> [    0.610299]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
> [    0.616317]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
> [    0.622681]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
> [    0.628542]  platform_probe from really_probe+0x81/0x1d0
> [    0.633862]  really_probe from __driver_probe_device+0x59/0x130
> [    0.639813]  __driver_probe_device from driver_probe_device+0x2d/0xc8
> [    0.646283]  driver_probe_device from __driver_attach+0x4d/0xf0
> [    0.652216]  __driver_attach from bus_for_each_dev+0x49/0x84
> [    0.657888]  bus_for_each_dev from bus_add_driver+0x91/0x13c
> [    0.663567]  bus_add_driver from driver_register+0x37/0xa4
> [    0.669066]  driver_register from do_one_initcall+0x41/0x1c4
> [    0.674740]  do_one_initcall from kernel_init_freeable+0x13d/0x180
> [    0.680937]  kernel_init_freeable from kernel_init+0x15/0xec
> [    0.686607]  kernel_init from ret_from_fork+0x11/0x1c
> [    0.691674] Exception stack(0xc8815fb0 to 0xc8815ff8)
> [    0.696739] 5fa0:                                     00000000 00000000 00000000 00000000
> [    0.704926] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.713111] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    0.719765] ---[ end trace 0000000000000000 ]---
> [    0.724452] ------------[ cut here ]------------
> [    0.729082] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
> [    0.738518] No max_rate, ignoring min_rate of clock 9 - pll-video0
> [    0.744730] Modules linked in:
> [    0.747801] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
> [    0.755911] Hardware name: Generic DT based system
> [    0.760696]  unwind_backtrace from show_stack+0xb/0xc
> [    0.765768]  show_stack from dump_stack_lvl+0x2b/0x34
> [    0.770859]  dump_stack_lvl from __warn+0x5d/0x92
> [    0.775600]  __warn from warn_slowpath_fmt+0xd7/0x12c
> [    0.780668]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
> [    0.786620]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
> [    0.792664]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
> [    0.799035]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
> [    0.804901]  platform_probe from really_probe+0x81/0x1d0
> [    0.810229]  really_probe from __driver_probe_device+0x59/0x130
> [    0.816171]  __driver_probe_device from driver_probe_device+0x2d/0xc8
> [    0.822624]  driver_probe_device from __driver_attach+0x4d/0xf0
> [    0.828566]  __driver_attach from bus_for_each_dev+0x49/0x84
> [    0.834237]  bus_for_each_dev from bus_add_driver+0x91/0x13c
> [    0.839925]  bus_add_driver from driver_register+0x37/0xa4
> [    0.845441]  driver_register from do_one_initcall+0x41/0x1c4
> [    0.851123]  do_one_initcall from kernel_init_freeable+0x13d/0x180
> [    0.857335]  kernel_init_freeable from kernel_init+0x15/0xec
> [    0.863022]  kernel_init from ret_from_fork+0x11/0x1c
> [    0.868096] Exception stack(0xc8815fb0 to 0xc8815ff8)
> [    0.873145] 5fa0:                                     00000000 00000000 00000000 00000000
> [    0.881332] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.889525] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    0.896165] ---[ end trace 0000000000000000 ]---
> [    0.900821] ------------[ cut here ]------------
> [    0.905471] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
> [    0.914885] No max_rate, ignoring min_rate of clock 12 - pll-video1
> [    0.921143] Modules linked in:
> [    0.924208] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
> [    0.932308] Hardware name: Generic DT based system
> [    0.937102]  unwind_backtrace from show_stack+0xb/0xc
> [    0.942173]  show_stack from dump_stack_lvl+0x2b/0x34
> [    0.947254]  dump_stack_lvl from __warn+0x5d/0x92
> [    0.952004]  __warn from warn_slowpath_fmt+0xd7/0x12c
> [    0.957081]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
> [    0.963034]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
> [    0.969052]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
> [    0.975422]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
> [    0.981288]  platform_probe from really_probe+0x81/0x1d0
> [    0.986607]  really_probe from __driver_probe_device+0x59/0x130
> [    0.992540]  __driver_probe_device from driver_probe_device+0x2d/0xc8
> [    0.999002]  driver_probe_device from __driver_attach+0x4d/0xf0
> [    1.004944]  __driver_attach from bus_for_each_dev+0x49/0x84
> [    1.010606]  bus_for_each_dev from bus_add_driver+0x91/0x13c
> [    1.016286]  bus_add_driver from driver_register+0x37/0xa4
> [    1.021785]  driver_register from do_one_initcall+0x41/0x1c4
> [    1.027467]  do_one_initcall from kernel_init_freeable+0x13d/0x180
> [    1.033679]  kernel_init_freeable from kernel_init+0x15/0xec
> [    1.039356]  kernel_init from ret_from_fork+0x11/0x1c
> [    1.044440] Exception stack(0xc8815fb0 to 0xc8815ff8)
> [    1.049496] 5fa0:                                     00000000 00000000 00000000 00000000
> [    1.057674] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    1.065850] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    1.072471] ---[ end trace 0000000000000000 ]---
> [    1.077106] ------------[ cut here ]------------
> [    1.081734] WARNING: CPU: 0 PID: 1 at drivers/clk/sunxi-ng/ccu_common.c:155 sunxi_ccu_probe+0x105/0x164
> [    1.091165] No max_rate, ignoring min_rate of clock 16 - pll-audio0
> [    1.097441] Modules linked in:
> [    1.100503] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.6.32-winglet #7
> [    1.108602] Hardware name: Generic DT based system
> [    1.113404]  unwind_backtrace from show_stack+0xb/0xc
> [    1.118474]  show_stack from dump_stack_lvl+0x2b/0x34
> [    1.123564]  dump_stack_lvl from __warn+0x5d/0x92
> [    1.128288]  __warn from warn_slowpath_fmt+0xd7/0x12c
> [    1.133356]  warn_slowpath_fmt from sunxi_ccu_probe+0x105/0x164
> [    1.139283]  sunxi_ccu_probe from devm_sunxi_ccu_probe+0x3d/0x60
> [    1.145318]  devm_sunxi_ccu_probe from sun20i_d1_ccu_probe+0xbf/0xec
> [    1.151680]  sun20i_d1_ccu_probe from platform_probe+0x3d/0x78
> [    1.157537]  platform_probe from really_probe+0x81/0x1d0
> [    1.162857]  really_probe from __driver_probe_device+0x59/0x130
> [    1.168816]  __driver_probe_device from driver_probe_device+0x2d/0xc8
> [    1.175278]  driver_probe_device from __driver_attach+0x4d/0xf0
> [    1.181219]  __driver_attach from bus_for_each_dev+0x49/0x84
> [    1.186908]  bus_for_each_dev from bus_add_driver+0x91/0x13c
> [    1.192595]  bus_add_driver from driver_register+0x37/0xa4
> [    1.198103]  driver_register from do_one_initcall+0x41/0x1c4
> [    1.203803]  do_one_initcall from kernel_init_freeable+0x13d/0x180
> [    1.210006]  kernel_init_freeable from kernel_init+0x15/0xec
> [    1.215684]  kernel_init from ret_from_fork+0x11/0x1c
> [    1.220759] Exception stack(0xc8815fb0 to 0xc8815ff8)
> [    1.225806] 5fa0:                                     00000000 00000000 00000000 00000000
> [    1.233984] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    1.242169] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    1.248818] ---[ end trace 0000000000000000 ]---