LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero
@ 2018-05-31 18:23 Steve Longerbeam
  2018-05-31 18:25 ` Marek Vasut
  0 siblings, 1 reply; 8+ messages in thread
From: Steve Longerbeam @ 2018-05-31 18:23 UTC (permalink / raw)
  To: Marek Vasut, Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Steve Longerbeam

Just return zero for a rounded rate if requested rate is zero.

This was caught by CONFIG_UBSAN:

[  192.266748] UBSAN: Undefined behaviour in drivers/clk/clk-versaclock5.c:513:17
[  192.274050] division by zero
[  192.276976] CPU: 0 PID: 2579 Comm: vsp-unit-test-0 Tainted: G    B   WC      4.14.17-02752-g13fb96f #1
[  192.286378] Hardware name: Renesas Salvator-X board based on r8a7795 ES2.0+ (DT)
[  192.293852] Call trace:
[  192.296343] [<ffff2000080900dc>] dump_backtrace+0x0/0x390
[  192.301807] [<ffff200008090480>] show_stack+0x14/0x1c
[  192.306920] [<ffff200008f66574>] dump_stack+0x134/0x1a8
[  192.312213] [<ffff2000087aaa30>] ubsan_epilogue+0x14/0x60
[  192.317677] [<ffff2000087ab4d0>] __ubsan_handle_divrem_overflow+0x11c/0x170
[  192.324720] [<ffff200008852120>] vc5_fod_round_rate+0x68/0x148
[  192.330620] [<ffff20000884567c>] clk_calc_new_rates+0x238/0x3fc
[  192.336607] [<ffff2000088456e0>] clk_calc_new_rates+0x29c/0x3fc
[  192.342595] [<ffff2000088483ac>] clk_core_set_rate_nolock+0x48/0x11c
[  192.349019] [<ffff2000088484b4>] clk_set_rate+0x34/0x4c
[  192.354307] [<ffff20000895e304>] rcar_du_pm_suspend+0x274/0x2f4
[  192.360297] [<ffff20000898feac>] platform_pm_suspend+0x78/0xb8
[  192.366198] [<ffff2000089a5604>] dpm_run_callback+0x584/0xa18
[  192.372010] [<ffff2000089a69e0>] __device_suspend+0x1a8/0x534
[  192.377822] [<ffff2000089adc48>] dpm_suspend+0x130/0xea0
[  192.383197] [<ffff2000089b0344>] dpm_suspend_start+0x130/0x138
[  192.389099] [<ffff20000817f584>] suspend_devices_and_enter+0xf0/0x1778
[  192.395700] [<ffff200008183014>] pm_suspend+0x2408/0x245c
[  192.401162] [<ffff20000817c0a4>] state_store+0xf0/0x130
[  192.406451] [<ffff200008f6f19c>] kobj_attr_store+0x5c/0x6c
[  192.412002] [<ffff2000084f4c94>] sysfs_kf_write+0xe8/0xfc
[  192.417466] [<ffff2000084f30b0>] kernfs_fop_write+0x22c/0x2e4
[  192.423281] [<ffff2000083e46d4>] __vfs_write+0x104/0x34c
[  192.428656] [<ffff2000083e4cc4>] vfs_write+0x134/0x2d8
[  192.433857] [<ffff2000083e5150>] SyS_write+0xbc/0x12c
[  192.438967] Exception stack(0xffff8006cd1cfec0 to 0xffff8006cd1d0000)
[  192.445480] fec0: 0000000000000001 000000001e303f00 0000000000000004 0000ffff959a5000
[  192.453397] fee0: 0000000000000000 0000000155510004 0000000000000003 000000000000006d
[  192.461314] ff00: 0000000000000040 0000000000000000 0000ffffcc304800 0000000000000020
[  192.469230] ff20: 0000000000000000 0000000000000000 0000000000000001 0000000000000008
[  192.477148] ff40: 00000000004eb3b8 0000ffff958bb840 000000000000003d 0000000000000001
[  192.485065] ff60: 000000001e303f00 0000ffff959a1508 0000000000000004 000000001e303f00
[  192.492982] ff80: 0000000000000004 00000000004d4c68 0000000000000001 0000000000000000
[  192.500899] ffa0: 000000001e30d5c0 0000ffffcc304820 0000ffff958bec64 0000ffffcc304820
[  192.508816] ffc0: 0000ffff95912898 0000000020000000 0000000000000001 0000000000000040
[  192.516733] ffe0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[  192.524650] [<ffff200008083ef0>] el0_svc_naked+0x24/0x28

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/clk/clk-versaclock5.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
index decffb3..113523d 100644
--- a/drivers/clk/clk-versaclock5.c
+++ b/drivers/clk/clk-versaclock5.c
@@ -509,6 +509,10 @@ static long vc5_fod_round_rate(struct clk_hw *hw, unsigned long rate,
 	u32 div_int;
 	u64 div_frc;
 
+	/* prevent div-by-0 */
+	if (rate == 0)
+		return 0;
+
 	/* Determine integer part, which is 12 bit wide */
 	div_int = f_in / rate;
 	/*
-- 
2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero
  2018-05-31 18:23 [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero Steve Longerbeam
@ 2018-05-31 18:25 ` Marek Vasut
  2018-05-31 18:32   ` Steve Longerbeam
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2018-05-31 18:25 UTC (permalink / raw)
  To: Steve Longerbeam, Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Steve Longerbeam

On 05/31/2018 08:23 PM, Steve Longerbeam wrote:
> Just return zero for a rounded rate if requested rate is zero.
> 
> This was caught by CONFIG_UBSAN:
> 
> [  192.266748] UBSAN: Undefined behaviour in drivers/clk/clk-versaclock5.c:513:17
> [  192.274050] division by zero
> [  192.276976] CPU: 0 PID: 2579 Comm: vsp-unit-test-0 Tainted: G    B   WC      4.14.17-02752-g13fb96f #1
> [  192.286378] Hardware name: Renesas Salvator-X board based on r8a7795 ES2.0+ (DT)
> [  192.293852] Call trace:
> [  192.296343] [<ffff2000080900dc>] dump_backtrace+0x0/0x390
> [  192.301807] [<ffff200008090480>] show_stack+0x14/0x1c
> [  192.306920] [<ffff200008f66574>] dump_stack+0x134/0x1a8
> [  192.312213] [<ffff2000087aaa30>] ubsan_epilogue+0x14/0x60
> [  192.317677] [<ffff2000087ab4d0>] __ubsan_handle_divrem_overflow+0x11c/0x170
> [  192.324720] [<ffff200008852120>] vc5_fod_round_rate+0x68/0x148
> [  192.330620] [<ffff20000884567c>] clk_calc_new_rates+0x238/0x3fc
> [  192.336607] [<ffff2000088456e0>] clk_calc_new_rates+0x29c/0x3fc
> [  192.342595] [<ffff2000088483ac>] clk_core_set_rate_nolock+0x48/0x11c
> [  192.349019] [<ffff2000088484b4>] clk_set_rate+0x34/0x4c
> [  192.354307] [<ffff20000895e304>] rcar_du_pm_suspend+0x274/0x2f4
> [  192.360297] [<ffff20000898feac>] platform_pm_suspend+0x78/0xb8
> [  192.366198] [<ffff2000089a5604>] dpm_run_callback+0x584/0xa18
> [  192.372010] [<ffff2000089a69e0>] __device_suspend+0x1a8/0x534
> [  192.377822] [<ffff2000089adc48>] dpm_suspend+0x130/0xea0
> [  192.383197] [<ffff2000089b0344>] dpm_suspend_start+0x130/0x138
> [  192.389099] [<ffff20000817f584>] suspend_devices_and_enter+0xf0/0x1778
> [  192.395700] [<ffff200008183014>] pm_suspend+0x2408/0x245c
> [  192.401162] [<ffff20000817c0a4>] state_store+0xf0/0x130
> [  192.406451] [<ffff200008f6f19c>] kobj_attr_store+0x5c/0x6c
> [  192.412002] [<ffff2000084f4c94>] sysfs_kf_write+0xe8/0xfc
> [  192.417466] [<ffff2000084f30b0>] kernfs_fop_write+0x22c/0x2e4
> [  192.423281] [<ffff2000083e46d4>] __vfs_write+0x104/0x34c
> [  192.428656] [<ffff2000083e4cc4>] vfs_write+0x134/0x2d8
> [  192.433857] [<ffff2000083e5150>] SyS_write+0xbc/0x12c
> [  192.438967] Exception stack(0xffff8006cd1cfec0 to 0xffff8006cd1d0000)
> [  192.445480] fec0: 0000000000000001 000000001e303f00 0000000000000004 0000ffff959a5000
> [  192.453397] fee0: 0000000000000000 0000000155510004 0000000000000003 000000000000006d
> [  192.461314] ff00: 0000000000000040 0000000000000000 0000ffffcc304800 0000000000000020
> [  192.469230] ff20: 0000000000000000 0000000000000000 0000000000000001 0000000000000008
> [  192.477148] ff40: 00000000004eb3b8 0000ffff958bb840 000000000000003d 0000000000000001
> [  192.485065] ff60: 000000001e303f00 0000ffff959a1508 0000000000000004 000000001e303f00
> [  192.492982] ff80: 0000000000000004 00000000004d4c68 0000000000000001 0000000000000000
> [  192.500899] ffa0: 000000001e30d5c0 0000ffffcc304820 0000ffff958bec64 0000ffffcc304820
> [  192.508816] ffc0: 0000ffff95912898 0000000020000000 0000000000000001 0000000000000040
> [  192.516733] ffe0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [  192.524650] [<ffff200008083ef0>] el0_svc_naked+0x24/0x28
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/clk/clk-versaclock5.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index decffb3..113523d 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -509,6 +509,10 @@ static long vc5_fod_round_rate(struct clk_hw *hw, unsigned long rate,
>  	u32 div_int;
>  	u64 div_frc;
>  
> +	/* prevent div-by-0 */
> +	if (rate == 0)
> +		return 0;
> +
>  	/* Determine integer part, which is 12 bit wide */
>  	div_int = f_in / rate;
>  	/*
> 
Can this actually happen ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero
  2018-05-31 18:25 ` Marek Vasut
@ 2018-05-31 18:32   ` Steve Longerbeam
  2018-05-31 18:35     ` Marek Vasut
  0 siblings, 1 reply; 8+ messages in thread
From: Steve Longerbeam @ 2018-05-31 18:32 UTC (permalink / raw)
  To: Marek Vasut, Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Steve Longerbeam

Hi Marek,


On 05/31/2018 11:25 AM, Marek Vasut wrote:
> On 05/31/2018 08:23 PM, Steve Longerbeam wrote:
>> Just return zero for a rounded rate if requested rate is zero.
>>
>> This was caught by CONFIG_UBSAN:
>>
>> [  192.266748] UBSAN: Undefined behaviour in drivers/clk/clk-versaclock5.c:513:17
>> [  192.274050] division by zero
>> [  192.276976] CPU: 0 PID: 2579 Comm: vsp-unit-test-0 Tainted: G    B   WC      4.14.17-02752-g13fb96f #1
>> [  192.286378] Hardware name: Renesas Salvator-X board based on r8a7795 ES2.0+ (DT)
>> [  192.293852] Call trace:
>> [  192.296343] [<ffff2000080900dc>] dump_backtrace+0x0/0x390
>> [  192.301807] [<ffff200008090480>] show_stack+0x14/0x1c
>> [  192.306920] [<ffff200008f66574>] dump_stack+0x134/0x1a8
>> [  192.312213] [<ffff2000087aaa30>] ubsan_epilogue+0x14/0x60
>> [  192.317677] [<ffff2000087ab4d0>] __ubsan_handle_divrem_overflow+0x11c/0x170
>> [  192.324720] [<ffff200008852120>] vc5_fod_round_rate+0x68/0x148
>> [  192.330620] [<ffff20000884567c>] clk_calc_new_rates+0x238/0x3fc
>> [  192.336607] [<ffff2000088456e0>] clk_calc_new_rates+0x29c/0x3fc
>> [  192.342595] [<ffff2000088483ac>] clk_core_set_rate_nolock+0x48/0x11c
>> [  192.349019] [<ffff2000088484b4>] clk_set_rate+0x34/0x4c
>> [  192.354307] [<ffff20000895e304>] rcar_du_pm_suspend+0x274/0x2f4
>> [  192.360297] [<ffff20000898feac>] platform_pm_suspend+0x78/0xb8
>> [  192.366198] [<ffff2000089a5604>] dpm_run_callback+0x584/0xa18
>> [  192.372010] [<ffff2000089a69e0>] __device_suspend+0x1a8/0x534
>> [  192.377822] [<ffff2000089adc48>] dpm_suspend+0x130/0xea0
>> [  192.383197] [<ffff2000089b0344>] dpm_suspend_start+0x130/0x138
>> [  192.389099] [<ffff20000817f584>] suspend_devices_and_enter+0xf0/0x1778
>> [  192.395700] [<ffff200008183014>] pm_suspend+0x2408/0x245c
>> [  192.401162] [<ffff20000817c0a4>] state_store+0xf0/0x130
>> [  192.406451] [<ffff200008f6f19c>] kobj_attr_store+0x5c/0x6c
>> [  192.412002] [<ffff2000084f4c94>] sysfs_kf_write+0xe8/0xfc
>> [  192.417466] [<ffff2000084f30b0>] kernfs_fop_write+0x22c/0x2e4
>> [  192.423281] [<ffff2000083e46d4>] __vfs_write+0x104/0x34c
>> [  192.428656] [<ffff2000083e4cc4>] vfs_write+0x134/0x2d8
>> [  192.433857] [<ffff2000083e5150>] SyS_write+0xbc/0x12c
>> [  192.438967] Exception stack(0xffff8006cd1cfec0 to 0xffff8006cd1d0000)
>> [  192.445480] fec0: 0000000000000001 000000001e303f00 0000000000000004 0000ffff959a5000
>> [  192.453397] fee0: 0000000000000000 0000000155510004 0000000000000003 000000000000006d
>> [  192.461314] ff00: 0000000000000040 0000000000000000 0000ffffcc304800 0000000000000020
>> [  192.469230] ff20: 0000000000000000 0000000000000000 0000000000000001 0000000000000008
>> [  192.477148] ff40: 00000000004eb3b8 0000ffff958bb840 000000000000003d 0000000000000001
>> [  192.485065] ff60: 000000001e303f00 0000ffff959a1508 0000000000000004 000000001e303f00
>> [  192.492982] ff80: 0000000000000004 00000000004d4c68 0000000000000001 0000000000000000
>> [  192.500899] ffa0: 000000001e30d5c0 0000ffffcc304820 0000ffff958bec64 0000ffffcc304820
>> [  192.508816] ffc0: 0000ffff95912898 0000000020000000 0000000000000001 0000000000000040
>> [  192.516733] ffe0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> [  192.524650] [<ffff200008083ef0>] el0_svc_naked+0x24/0x28
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   drivers/clk/clk-versaclock5.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
>> index decffb3..113523d 100644
>> --- a/drivers/clk/clk-versaclock5.c
>> +++ b/drivers/clk/clk-versaclock5.c
>> @@ -509,6 +509,10 @@ static long vc5_fod_round_rate(struct clk_hw *hw, unsigned long rate,
>>   	u32 div_int;
>>   	u64 div_frc;
>>   
>> +	/* prevent div-by-0 */
>> +	if (rate == 0)
>> +		return 0;
>> +
>>   	/* Determine integer part, which is 12 bit wide */
>>   	div_int = f_in / rate;
>>   	/*
>>
> Can this actually happen ?

We caught this using the Renesas 3.6.0 BSP release, when performing
a suspend of rcar-du driver. The rcar_du_pm_suspend() in 3.6.0 BSP is 
modified
from mainline version, including calling clk_set_rate() on the crtc 
clocks with a
rate of zero. So this is not actually reproducible (yet) in mainline.

Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero
  2018-05-31 18:32   ` Steve Longerbeam
@ 2018-05-31 18:35     ` Marek Vasut
  2018-05-31 18:52       ` Steve Longerbeam
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2018-05-31 18:35 UTC (permalink / raw)
  To: Steve Longerbeam, Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Steve Longerbeam, Laurent Pinchart,
	Kieran Bingham

On 05/31/2018 08:32 PM, Steve Longerbeam wrote:
> Hi Marek,
> 
> 
> On 05/31/2018 11:25 AM, Marek Vasut wrote:
>> On 05/31/2018 08:23 PM, Steve Longerbeam wrote:
>>> Just return zero for a rounded rate if requested rate is zero.
>>>
>>> This was caught by CONFIG_UBSAN:
>>>
>>> [  192.266748] UBSAN: Undefined behaviour in
>>> drivers/clk/clk-versaclock5.c:513:17
>>> [  192.274050] division by zero
>>> [  192.276976] CPU: 0 PID: 2579 Comm: vsp-unit-test-0 Tainted: G   
>>> B   WC      4.14.17-02752-g13fb96f #1
>>> [  192.286378] Hardware name: Renesas Salvator-X board based on
>>> r8a7795 ES2.0+ (DT)
>>> [  192.293852] Call trace:
>>> [  192.296343] [<ffff2000080900dc>] dump_backtrace+0x0/0x390
>>> [  192.301807] [<ffff200008090480>] show_stack+0x14/0x1c
>>> [  192.306920] [<ffff200008f66574>] dump_stack+0x134/0x1a8
>>> [  192.312213] [<ffff2000087aaa30>] ubsan_epilogue+0x14/0x60
>>> [  192.317677] [<ffff2000087ab4d0>]
>>> __ubsan_handle_divrem_overflow+0x11c/0x170
>>> [  192.324720] [<ffff200008852120>] vc5_fod_round_rate+0x68/0x148
>>> [  192.330620] [<ffff20000884567c>] clk_calc_new_rates+0x238/0x3fc
>>> [  192.336607] [<ffff2000088456e0>] clk_calc_new_rates+0x29c/0x3fc
>>> [  192.342595] [<ffff2000088483ac>] clk_core_set_rate_nolock+0x48/0x11c
>>> [  192.349019] [<ffff2000088484b4>] clk_set_rate+0x34/0x4c
>>> [  192.354307] [<ffff20000895e304>] rcar_du_pm_suspend+0x274/0x2f4
>>> [  192.360297] [<ffff20000898feac>] platform_pm_suspend+0x78/0xb8
>>> [  192.366198] [<ffff2000089a5604>] dpm_run_callback+0x584/0xa18
>>> [  192.372010] [<ffff2000089a69e0>] __device_suspend+0x1a8/0x534
>>> [  192.377822] [<ffff2000089adc48>] dpm_suspend+0x130/0xea0
>>> [  192.383197] [<ffff2000089b0344>] dpm_suspend_start+0x130/0x138
>>> [  192.389099] [<ffff20000817f584>]
>>> suspend_devices_and_enter+0xf0/0x1778
>>> [  192.395700] [<ffff200008183014>] pm_suspend+0x2408/0x245c
>>> [  192.401162] [<ffff20000817c0a4>] state_store+0xf0/0x130
>>> [  192.406451] [<ffff200008f6f19c>] kobj_attr_store+0x5c/0x6c
>>> [  192.412002] [<ffff2000084f4c94>] sysfs_kf_write+0xe8/0xfc
>>> [  192.417466] [<ffff2000084f30b0>] kernfs_fop_write+0x22c/0x2e4
>>> [  192.423281] [<ffff2000083e46d4>] __vfs_write+0x104/0x34c
>>> [  192.428656] [<ffff2000083e4cc4>] vfs_write+0x134/0x2d8
>>> [  192.433857] [<ffff2000083e5150>] SyS_write+0xbc/0x12c
>>> [  192.438967] Exception stack(0xffff8006cd1cfec0 to 0xffff8006cd1d0000)
>>> [  192.445480] fec0: 0000000000000001 000000001e303f00
>>> 0000000000000004 0000ffff959a5000
>>> [  192.453397] fee0: 0000000000000000 0000000155510004
>>> 0000000000000003 000000000000006d
>>> [  192.461314] ff00: 0000000000000040 0000000000000000
>>> 0000ffffcc304800 0000000000000020
>>> [  192.469230] ff20: 0000000000000000 0000000000000000
>>> 0000000000000001 0000000000000008
>>> [  192.477148] ff40: 00000000004eb3b8 0000ffff958bb840
>>> 000000000000003d 0000000000000001
>>> [  192.485065] ff60: 000000001e303f00 0000ffff959a1508
>>> 0000000000000004 000000001e303f00
>>> [  192.492982] ff80: 0000000000000004 00000000004d4c68
>>> 0000000000000001 0000000000000000
>>> [  192.500899] ffa0: 000000001e30d5c0 0000ffffcc304820
>>> 0000ffff958bec64 0000ffffcc304820
>>> [  192.508816] ffc0: 0000ffff95912898 0000000020000000
>>> 0000000000000001 0000000000000040
>>> [  192.516733] ffe0: 0000000000000000 0000000000000000
>>> 0000000000000000 0000000000000000
>>> [  192.524650] [<ffff200008083ef0>] el0_svc_naked+0x24/0x28
>>>
>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>> ---
>>>   drivers/clk/clk-versaclock5.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/clk/clk-versaclock5.c
>>> b/drivers/clk/clk-versaclock5.c
>>> index decffb3..113523d 100644
>>> --- a/drivers/clk/clk-versaclock5.c
>>> +++ b/drivers/clk/clk-versaclock5.c
>>> @@ -509,6 +509,10 @@ static long vc5_fod_round_rate(struct clk_hw
>>> *hw, unsigned long rate,
>>>       u32 div_int;
>>>       u64 div_frc;
>>>   +    /* prevent div-by-0 */
>>> +    if (rate == 0)
>>> +        return 0;
>>> +
>>>       /* Determine integer part, which is 12 bit wide */
>>>       div_int = f_in / rate;
>>>       /*
>>>
>> Can this actually happen ?
> 
> We caught this using the Renesas 3.6.0 BSP release, when performing
> a suspend of rcar-du driver. The rcar_du_pm_suspend() in 3.6.0 BSP is
> modified
> from mainline version, including calling clk_set_rate() on the crtc
> clocks with a
> rate of zero. So this is not actually reproducible (yet) in mainline.

So it sets clock to 0 ? Anyway, this looks sane, although maybe the
whole driver could use a once-over to see if there could be more of this.

Or should the clock framework even let us set clock to 0 Hz ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero
  2018-05-31 18:35     ` Marek Vasut
@ 2018-05-31 18:52       ` Steve Longerbeam
  2018-05-31 19:37         ` Marek Vasut
  0 siblings, 1 reply; 8+ messages in thread
From: Steve Longerbeam @ 2018-05-31 18:52 UTC (permalink / raw)
  To: Marek Vasut, Steve Longerbeam, Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Laurent Pinchart, Kieran Bingham



On 05/31/2018 11:35 AM, Marek Vasut wrote:
> On 05/31/2018 08:32 PM, Steve Longerbeam wrote:
>> Hi Marek,
>>
>>
>> On 05/31/2018 11:25 AM, Marek Vasut wrote:
>>> On 05/31/2018 08:23 PM, Steve Longerbeam wrote:
>>>> Just return zero for a rounded rate if requested rate is zero.
>>>>
>>>> This was caught by CONFIG_UBSAN:
>>>>
>>>> [  192.266748] UBSAN: Undefined behaviour in
>>>> drivers/clk/clk-versaclock5.c:513:17
>>>> [  192.274050] division by zero
>>>> [  192.276976] CPU: 0 PID: 2579 Comm: vsp-unit-test-0 Tainted: G
>>>> B   WC      4.14.17-02752-g13fb96f #1
>>>> [  192.286378] Hardware name: Renesas Salvator-X board based on
>>>> r8a7795 ES2.0+ (DT)
>>>> [  192.293852] Call trace:
>>>> [  192.296343] [<ffff2000080900dc>] dump_backtrace+0x0/0x390
>>>> [  192.301807] [<ffff200008090480>] show_stack+0x14/0x1c
>>>> [  192.306920] [<ffff200008f66574>] dump_stack+0x134/0x1a8
>>>> [  192.312213] [<ffff2000087aaa30>] ubsan_epilogue+0x14/0x60
>>>> [  192.317677] [<ffff2000087ab4d0>]
>>>> __ubsan_handle_divrem_overflow+0x11c/0x170
>>>> [  192.324720] [<ffff200008852120>] vc5_fod_round_rate+0x68/0x148
>>>> [  192.330620] [<ffff20000884567c>] clk_calc_new_rates+0x238/0x3fc
>>>> [  192.336607] [<ffff2000088456e0>] clk_calc_new_rates+0x29c/0x3fc
>>>> [  192.342595] [<ffff2000088483ac>] clk_core_set_rate_nolock+0x48/0x11c
>>>> [  192.349019] [<ffff2000088484b4>] clk_set_rate+0x34/0x4c
>>>> [  192.354307] [<ffff20000895e304>] rcar_du_pm_suspend+0x274/0x2f4
>>>> [  192.360297] [<ffff20000898feac>] platform_pm_suspend+0x78/0xb8
>>>> [  192.366198] [<ffff2000089a5604>] dpm_run_callback+0x584/0xa18
>>>> [  192.372010] [<ffff2000089a69e0>] __device_suspend+0x1a8/0x534
>>>> [  192.377822] [<ffff2000089adc48>] dpm_suspend+0x130/0xea0
>>>> [  192.383197] [<ffff2000089b0344>] dpm_suspend_start+0x130/0x138
>>>> [  192.389099] [<ffff20000817f584>]
>>>> suspend_devices_and_enter+0xf0/0x1778
>>>> [  192.395700] [<ffff200008183014>] pm_suspend+0x2408/0x245c
>>>> [  192.401162] [<ffff20000817c0a4>] state_store+0xf0/0x130
>>>> [  192.406451] [<ffff200008f6f19c>] kobj_attr_store+0x5c/0x6c
>>>> [  192.412002] [<ffff2000084f4c94>] sysfs_kf_write+0xe8/0xfc
>>>> [  192.417466] [<ffff2000084f30b0>] kernfs_fop_write+0x22c/0x2e4
>>>> [  192.423281] [<ffff2000083e46d4>] __vfs_write+0x104/0x34c
>>>> [  192.428656] [<ffff2000083e4cc4>] vfs_write+0x134/0x2d8
>>>> [  192.433857] [<ffff2000083e5150>] SyS_write+0xbc/0x12c
>>>> [  192.438967] Exception stack(0xffff8006cd1cfec0 to 0xffff8006cd1d0000)
>>>> [  192.445480] fec0: 0000000000000001 000000001e303f00
>>>> 0000000000000004 0000ffff959a5000
>>>> [  192.453397] fee0: 0000000000000000 0000000155510004
>>>> 0000000000000003 000000000000006d
>>>> [  192.461314] ff00: 0000000000000040 0000000000000000
>>>> 0000ffffcc304800 0000000000000020
>>>> [  192.469230] ff20: 0000000000000000 0000000000000000
>>>> 0000000000000001 0000000000000008
>>>> [  192.477148] ff40: 00000000004eb3b8 0000ffff958bb840
>>>> 000000000000003d 0000000000000001
>>>> [  192.485065] ff60: 000000001e303f00 0000ffff959a1508
>>>> 0000000000000004 000000001e303f00
>>>> [  192.492982] ff80: 0000000000000004 00000000004d4c68
>>>> 0000000000000001 0000000000000000
>>>> [  192.500899] ffa0: 000000001e30d5c0 0000ffffcc304820
>>>> 0000ffff958bec64 0000ffffcc304820
>>>> [  192.508816] ffc0: 0000ffff95912898 0000000020000000
>>>> 0000000000000001 0000000000000040
>>>> [  192.516733] ffe0: 0000000000000000 0000000000000000
>>>> 0000000000000000 0000000000000000
>>>> [  192.524650] [<ffff200008083ef0>] el0_svc_naked+0x24/0x28
>>>>
>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>> ---
>>>>    drivers/clk/clk-versaclock5.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/clk-versaclock5.c
>>>> b/drivers/clk/clk-versaclock5.c
>>>> index decffb3..113523d 100644
>>>> --- a/drivers/clk/clk-versaclock5.c
>>>> +++ b/drivers/clk/clk-versaclock5.c
>>>> @@ -509,6 +509,10 @@ static long vc5_fod_round_rate(struct clk_hw
>>>> *hw, unsigned long rate,
>>>>        u32 div_int;
>>>>        u64 div_frc;
>>>>    +    /* prevent div-by-0 */
>>>> +    if (rate == 0)
>>>> +        return 0;
>>>> +
>>>>        /* Determine integer part, which is 12 bit wide */
>>>>        div_int = f_in / rate;
>>>>        /*
>>>>
>>> Can this actually happen ?
>> We caught this using the Renesas 3.6.0 BSP release, when performing
>> a suspend of rcar-du driver. The rcar_du_pm_suspend() in 3.6.0 BSP is
>> modified
>> from mainline version, including calling clk_set_rate() on the crtc
>> clocks with a
>> rate of zero. So this is not actually reproducible (yet) in mainline.
> So it sets clock to 0 ?

Yep, see

https://kernel.googlesource.com/pub/scm/linux/kernel/git/horms/renesas-bsp/+/rcar-3.6.0/drivers/gpu/drm/rcar-du/rcar_du_drv.c#359

>   Anyway, this looks sane, although maybe the
> whole driver could use a once-over to see if there could be more of this.

Actually I do see more potential divide-by-zeros due to a passed rate
of zero, including vc5_pfd_round_rate() and vc5_pfd_set_rate().

I can resubmit this patch fixing all cases in clk-versaclock5.c if you
like (and probably remove the misleading backtrace in the commit
message since it is a Renesas 3.6.0 kernel backtrace not a mainline
backtrace).

Or perhaps just treat this as a heads-up, I'll leave it up to you.

>
> Or should the clock framework even let us set clock to 0 Hz ?

That is a good question, it might make sense for the core clock framework
to not allow passing a rate of 0 on to the clock ops, and instead treat
it generically.

Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero
  2018-05-31 18:52       ` Steve Longerbeam
@ 2018-05-31 19:37         ` Marek Vasut
  2018-06-01  1:46           ` Steve Longerbeam
  0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2018-05-31 19:37 UTC (permalink / raw)
  To: Steve Longerbeam, Steve Longerbeam, Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Laurent Pinchart, Kieran Bingham

On 05/31/2018 08:52 PM, Steve Longerbeam wrote:
> 
> 
> On 05/31/2018 11:35 AM, Marek Vasut wrote:
>> On 05/31/2018 08:32 PM, Steve Longerbeam wrote:
>>> Hi Marek,
>>>
>>>
>>> On 05/31/2018 11:25 AM, Marek Vasut wrote:
>>>> On 05/31/2018 08:23 PM, Steve Longerbeam wrote:
>>>>> Just return zero for a rounded rate if requested rate is zero.
>>>>>
>>>>> This was caught by CONFIG_UBSAN:
>>>>>
>>>>> [  192.266748] UBSAN: Undefined behaviour in
>>>>> drivers/clk/clk-versaclock5.c:513:17
>>>>> [  192.274050] division by zero
>>>>> [  192.276976] CPU: 0 PID: 2579 Comm: vsp-unit-test-0 Tainted: G
>>>>> B   WC      4.14.17-02752-g13fb96f #1
>>>>> [  192.286378] Hardware name: Renesas Salvator-X board based on
>>>>> r8a7795 ES2.0+ (DT)
>>>>> [  192.293852] Call trace:
>>>>> [  192.296343] [<ffff2000080900dc>] dump_backtrace+0x0/0x390
>>>>> [  192.301807] [<ffff200008090480>] show_stack+0x14/0x1c
>>>>> [  192.306920] [<ffff200008f66574>] dump_stack+0x134/0x1a8
>>>>> [  192.312213] [<ffff2000087aaa30>] ubsan_epilogue+0x14/0x60
>>>>> [  192.317677] [<ffff2000087ab4d0>]
>>>>> __ubsan_handle_divrem_overflow+0x11c/0x170
>>>>> [  192.324720] [<ffff200008852120>] vc5_fod_round_rate+0x68/0x148
>>>>> [  192.330620] [<ffff20000884567c>] clk_calc_new_rates+0x238/0x3fc
>>>>> [  192.336607] [<ffff2000088456e0>] clk_calc_new_rates+0x29c/0x3fc
>>>>> [  192.342595] [<ffff2000088483ac>]
>>>>> clk_core_set_rate_nolock+0x48/0x11c
>>>>> [  192.349019] [<ffff2000088484b4>] clk_set_rate+0x34/0x4c
>>>>> [  192.354307] [<ffff20000895e304>] rcar_du_pm_suspend+0x274/0x2f4
>>>>> [  192.360297] [<ffff20000898feac>] platform_pm_suspend+0x78/0xb8
>>>>> [  192.366198] [<ffff2000089a5604>] dpm_run_callback+0x584/0xa18
>>>>> [  192.372010] [<ffff2000089a69e0>] __device_suspend+0x1a8/0x534
>>>>> [  192.377822] [<ffff2000089adc48>] dpm_suspend+0x130/0xea0
>>>>> [  192.383197] [<ffff2000089b0344>] dpm_suspend_start+0x130/0x138
>>>>> [  192.389099] [<ffff20000817f584>]
>>>>> suspend_devices_and_enter+0xf0/0x1778
>>>>> [  192.395700] [<ffff200008183014>] pm_suspend+0x2408/0x245c
>>>>> [  192.401162] [<ffff20000817c0a4>] state_store+0xf0/0x130
>>>>> [  192.406451] [<ffff200008f6f19c>] kobj_attr_store+0x5c/0x6c
>>>>> [  192.412002] [<ffff2000084f4c94>] sysfs_kf_write+0xe8/0xfc
>>>>> [  192.417466] [<ffff2000084f30b0>] kernfs_fop_write+0x22c/0x2e4
>>>>> [  192.423281] [<ffff2000083e46d4>] __vfs_write+0x104/0x34c
>>>>> [  192.428656] [<ffff2000083e4cc4>] vfs_write+0x134/0x2d8
>>>>> [  192.433857] [<ffff2000083e5150>] SyS_write+0xbc/0x12c
>>>>> [  192.438967] Exception stack(0xffff8006cd1cfec0 to
>>>>> 0xffff8006cd1d0000)
>>>>> [  192.445480] fec0: 0000000000000001 000000001e303f00
>>>>> 0000000000000004 0000ffff959a5000
>>>>> [  192.453397] fee0: 0000000000000000 0000000155510004
>>>>> 0000000000000003 000000000000006d
>>>>> [  192.461314] ff00: 0000000000000040 0000000000000000
>>>>> 0000ffffcc304800 0000000000000020
>>>>> [  192.469230] ff20: 0000000000000000 0000000000000000
>>>>> 0000000000000001 0000000000000008
>>>>> [  192.477148] ff40: 00000000004eb3b8 0000ffff958bb840
>>>>> 000000000000003d 0000000000000001
>>>>> [  192.485065] ff60: 000000001e303f00 0000ffff959a1508
>>>>> 0000000000000004 000000001e303f00
>>>>> [  192.492982] ff80: 0000000000000004 00000000004d4c68
>>>>> 0000000000000001 0000000000000000
>>>>> [  192.500899] ffa0: 000000001e30d5c0 0000ffffcc304820
>>>>> 0000ffff958bec64 0000ffffcc304820
>>>>> [  192.508816] ffc0: 0000ffff95912898 0000000020000000
>>>>> 0000000000000001 0000000000000040
>>>>> [  192.516733] ffe0: 0000000000000000 0000000000000000
>>>>> 0000000000000000 0000000000000000
>>>>> [  192.524650] [<ffff200008083ef0>] el0_svc_naked+0x24/0x28
>>>>>
>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>> ---
>>>>>    drivers/clk/clk-versaclock5.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/clk-versaclock5.c
>>>>> b/drivers/clk/clk-versaclock5.c
>>>>> index decffb3..113523d 100644
>>>>> --- a/drivers/clk/clk-versaclock5.c
>>>>> +++ b/drivers/clk/clk-versaclock5.c
>>>>> @@ -509,6 +509,10 @@ static long vc5_fod_round_rate(struct clk_hw
>>>>> *hw, unsigned long rate,
>>>>>        u32 div_int;
>>>>>        u64 div_frc;
>>>>>    +    /* prevent div-by-0 */
>>>>> +    if (rate == 0)
>>>>> +        return 0;
>>>>> +
>>>>>        /* Determine integer part, which is 12 bit wide */
>>>>>        div_int = f_in / rate;
>>>>>        /*
>>>>>
>>>> Can this actually happen ?
>>> We caught this using the Renesas 3.6.0 BSP release, when performing
>>> a suspend of rcar-du driver. The rcar_du_pm_suspend() in 3.6.0 BSP is
>>> modified
>>> from mainline version, including calling clk_set_rate() on the crtc
>>> clocks with a
>>> rate of zero. So this is not actually reproducible (yet) in mainline.
>> So it sets clock to 0 ?
> 
> Yep, see
> 
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/horms/renesas-bsp/+/rcar-3.6.0/drivers/gpu/drm/rcar-du/rcar_du_drv.c#359
> 
> 
>>   Anyway, this looks sane, although maybe the
>> whole driver could use a once-over to see if there could be more of this.
> 
> Actually I do see more potential divide-by-zeros due to a passed rate
> of zero, including vc5_pfd_round_rate() and vc5_pfd_set_rate().
> 
> I can resubmit this patch fixing all cases in clk-versaclock5.c if you
> like (and probably remove the misleading backtrace in the commit
> message since it is a Renesas 3.6.0 kernel backtrace not a mainline
> backtrace).
> 
> Or perhaps just treat this as a heads-up, I'll leave it up to you.

It'd be nice if you resubmitted it fixing all the cases.

>> Or should the clock framework even let us set clock to 0 Hz ?
> 
> That is a good question, it might make sense for the core clock framework
> to not allow passing a rate of 0 on to the clock ops, and instead treat
> it generically.

Jupp

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero
  2018-05-31 19:37         ` Marek Vasut
@ 2018-06-01  1:46           ` Steve Longerbeam
  2018-06-01  6:36             ` Marek Vasut
  0 siblings, 1 reply; 8+ messages in thread
From: Steve Longerbeam @ 2018-06-01  1:46 UTC (permalink / raw)
  To: Marek Vasut, Steve Longerbeam, Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Laurent Pinchart, Kieran Bingham



On 05/31/2018 12:37 PM, Marek Vasut wrote:
> On 05/31/2018 08:52 PM, Steve Longerbeam wrote:
>>
>> On 05/31/2018 11:35 AM, Marek Vasut wrote:
>>> On 05/31/2018 08:32 PM, Steve Longerbeam wrote:
>>>> Hi Marek,
>>>>
>>>>
>>>> On 05/31/2018 11:25 AM, Marek Vasut wrote:
>>>>> On 05/31/2018 08:23 PM, Steve Longerbeam wrote:
>>>>>> Just return zero for a rounded rate if requested rate is zero.
>>>>>>
>>>>>> This was caught by CONFIG_UBSAN:
>>>>>>
>>>>>> [  192.266748] UBSAN: Undefined behaviour in
>>>>>> drivers/clk/clk-versaclock5.c:513:17
>>>>>> [  192.274050] division by zero
>>>>>> [  192.276976] CPU: 0 PID: 2579 Comm: vsp-unit-test-0 Tainted: G
>>>>>> B   WC      4.14.17-02752-g13fb96f #1
>>>>>> [  192.286378] Hardware name: Renesas Salvator-X board based on
>>>>>> r8a7795 ES2.0+ (DT)
>>>>>> [  192.293852] Call trace:
>>>>>> [  192.296343] [<ffff2000080900dc>] dump_backtrace+0x0/0x390
>>>>>> [  192.301807] [<ffff200008090480>] show_stack+0x14/0x1c
>>>>>> [  192.306920] [<ffff200008f66574>] dump_stack+0x134/0x1a8
>>>>>> [  192.312213] [<ffff2000087aaa30>] ubsan_epilogue+0x14/0x60
>>>>>> [  192.317677] [<ffff2000087ab4d0>]
>>>>>> __ubsan_handle_divrem_overflow+0x11c/0x170
>>>>>> [  192.324720] [<ffff200008852120>] vc5_fod_round_rate+0x68/0x148
>>>>>> [  192.330620] [<ffff20000884567c>] clk_calc_new_rates+0x238/0x3fc
>>>>>> [  192.336607] [<ffff2000088456e0>] clk_calc_new_rates+0x29c/0x3fc
>>>>>> [  192.342595] [<ffff2000088483ac>]
>>>>>> clk_core_set_rate_nolock+0x48/0x11c
>>>>>> [  192.349019] [<ffff2000088484b4>] clk_set_rate+0x34/0x4c
>>>>>> [  192.354307] [<ffff20000895e304>] rcar_du_pm_suspend+0x274/0x2f4
>>>>>> [  192.360297] [<ffff20000898feac>] platform_pm_suspend+0x78/0xb8
>>>>>> [  192.366198] [<ffff2000089a5604>] dpm_run_callback+0x584/0xa18
>>>>>> [  192.372010] [<ffff2000089a69e0>] __device_suspend+0x1a8/0x534
>>>>>> [  192.377822] [<ffff2000089adc48>] dpm_suspend+0x130/0xea0
>>>>>> [  192.383197] [<ffff2000089b0344>] dpm_suspend_start+0x130/0x138
>>>>>> [  192.389099] [<ffff20000817f584>]
>>>>>> suspend_devices_and_enter+0xf0/0x1778
>>>>>> [  192.395700] [<ffff200008183014>] pm_suspend+0x2408/0x245c
>>>>>> [  192.401162] [<ffff20000817c0a4>] state_store+0xf0/0x130
>>>>>> [  192.406451] [<ffff200008f6f19c>] kobj_attr_store+0x5c/0x6c
>>>>>> [  192.412002] [<ffff2000084f4c94>] sysfs_kf_write+0xe8/0xfc
>>>>>> [  192.417466] [<ffff2000084f30b0>] kernfs_fop_write+0x22c/0x2e4
>>>>>> [  192.423281] [<ffff2000083e46d4>] __vfs_write+0x104/0x34c
>>>>>> [  192.428656] [<ffff2000083e4cc4>] vfs_write+0x134/0x2d8
>>>>>> [  192.433857] [<ffff2000083e5150>] SyS_write+0xbc/0x12c
>>>>>> [  192.438967] Exception stack(0xffff8006cd1cfec0 to
>>>>>> 0xffff8006cd1d0000)
>>>>>> [  192.445480] fec0: 0000000000000001 000000001e303f00
>>>>>> 0000000000000004 0000ffff959a5000
>>>>>> [  192.453397] fee0: 0000000000000000 0000000155510004
>>>>>> 0000000000000003 000000000000006d
>>>>>> [  192.461314] ff00: 0000000000000040 0000000000000000
>>>>>> 0000ffffcc304800 0000000000000020
>>>>>> [  192.469230] ff20: 0000000000000000 0000000000000000
>>>>>> 0000000000000001 0000000000000008
>>>>>> [  192.477148] ff40: 00000000004eb3b8 0000ffff958bb840
>>>>>> 000000000000003d 0000000000000001
>>>>>> [  192.485065] ff60: 000000001e303f00 0000ffff959a1508
>>>>>> 0000000000000004 000000001e303f00
>>>>>> [  192.492982] ff80: 0000000000000004 00000000004d4c68
>>>>>> 0000000000000001 0000000000000000
>>>>>> [  192.500899] ffa0: 000000001e30d5c0 0000ffffcc304820
>>>>>> 0000ffff958bec64 0000ffffcc304820
>>>>>> [  192.508816] ffc0: 0000ffff95912898 0000000020000000
>>>>>> 0000000000000001 0000000000000040
>>>>>> [  192.516733] ffe0: 0000000000000000 0000000000000000
>>>>>> 0000000000000000 0000000000000000
>>>>>> [  192.524650] [<ffff200008083ef0>] el0_svc_naked+0x24/0x28
>>>>>>
>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>> ---
>>>>>>     drivers/clk/clk-versaclock5.c | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/clk/clk-versaclock5.c
>>>>>> b/drivers/clk/clk-versaclock5.c
>>>>>> index decffb3..113523d 100644
>>>>>> --- a/drivers/clk/clk-versaclock5.c
>>>>>> +++ b/drivers/clk/clk-versaclock5.c
>>>>>> @@ -509,6 +509,10 @@ static long vc5_fod_round_rate(struct clk_hw
>>>>>> *hw, unsigned long rate,
>>>>>>         u32 div_int;
>>>>>>         u64 div_frc;
>>>>>>     +    /* prevent div-by-0 */
>>>>>> +    if (rate == 0)
>>>>>> +        return 0;
>>>>>> +
>>>>>>         /* Determine integer part, which is 12 bit wide */
>>>>>>         div_int = f_in / rate;
>>>>>>         /*
>>>>>>
>>>>> Can this actually happen ?
>>>> We caught this using the Renesas 3.6.0 BSP release, when performing
>>>> a suspend of rcar-du driver. The rcar_du_pm_suspend() in 3.6.0 BSP is
>>>> modified
>>>> from mainline version, including calling clk_set_rate() on the crtc
>>>> clocks with a
>>>> rate of zero. So this is not actually reproducible (yet) in mainline.
>>> So it sets clock to 0 ?
>> Yep, see
>>
>> https://kernel.googlesource.com/pub/scm/linux/kernel/git/horms/renesas-bsp/+/rcar-3.6.0/drivers/gpu/drm/rcar-du/rcar_du_drv.c#359
>>
>>
>>>    Anyway, this looks sane, although maybe the
>>> whole driver could use a once-over to see if there could be more of this.
>> Actually I do see more potential divide-by-zeros due to a passed rate
>> of zero, including vc5_pfd_round_rate() and vc5_pfd_set_rate().
>>
>> I can resubmit this patch fixing all cases in clk-versaclock5.c if you
>> like (and probably remove the misleading backtrace in the commit
>> message since it is a Renesas 3.6.0 kernel backtrace not a mainline
>> backtrace).
>>
>> Or perhaps just treat this as a heads-up, I'll leave it up to you.
> It'd be nice if you resubmitted it fixing all the cases.
>

Ok. Please disregard this patch since there won't be a v2, the
new patch will have a different title.

Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero
  2018-06-01  1:46           ` Steve Longerbeam
@ 2018-06-01  6:36             ` Marek Vasut
  0 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2018-06-01  6:36 UTC (permalink / raw)
  To: Steve Longerbeam, Steve Longerbeam, Michael Turquette, Stephen Boyd
  Cc: linux-clk, linux-kernel, Laurent Pinchart, Kieran Bingham

On 06/01/2018 03:46 AM, Steve Longerbeam wrote:
> 
> 
> On 05/31/2018 12:37 PM, Marek Vasut wrote:
>> On 05/31/2018 08:52 PM, Steve Longerbeam wrote:
>>>
>>> On 05/31/2018 11:35 AM, Marek Vasut wrote:
>>>> On 05/31/2018 08:32 PM, Steve Longerbeam wrote:
>>>>> Hi Marek,
>>>>>
>>>>>
>>>>> On 05/31/2018 11:25 AM, Marek Vasut wrote:
>>>>>> On 05/31/2018 08:23 PM, Steve Longerbeam wrote:
>>>>>>> Just return zero for a rounded rate if requested rate is zero.
>>>>>>>
>>>>>>> This was caught by CONFIG_UBSAN:
>>>>>>>
>>>>>>> [  192.266748] UBSAN: Undefined behaviour in
>>>>>>> drivers/clk/clk-versaclock5.c:513:17
>>>>>>> [  192.274050] division by zero
>>>>>>> [  192.276976] CPU: 0 PID: 2579 Comm: vsp-unit-test-0 Tainted: G
>>>>>>> B   WC      4.14.17-02752-g13fb96f #1
>>>>>>> [  192.286378] Hardware name: Renesas Salvator-X board based on
>>>>>>> r8a7795 ES2.0+ (DT)
>>>>>>> [  192.293852] Call trace:
>>>>>>> [  192.296343] [<ffff2000080900dc>] dump_backtrace+0x0/0x390
>>>>>>> [  192.301807] [<ffff200008090480>] show_stack+0x14/0x1c
>>>>>>> [  192.306920] [<ffff200008f66574>] dump_stack+0x134/0x1a8
>>>>>>> [  192.312213] [<ffff2000087aaa30>] ubsan_epilogue+0x14/0x60
>>>>>>> [  192.317677] [<ffff2000087ab4d0>]
>>>>>>> __ubsan_handle_divrem_overflow+0x11c/0x170
>>>>>>> [  192.324720] [<ffff200008852120>] vc5_fod_round_rate+0x68/0x148
>>>>>>> [  192.330620] [<ffff20000884567c>] clk_calc_new_rates+0x238/0x3fc
>>>>>>> [  192.336607] [<ffff2000088456e0>] clk_calc_new_rates+0x29c/0x3fc
>>>>>>> [  192.342595] [<ffff2000088483ac>]
>>>>>>> clk_core_set_rate_nolock+0x48/0x11c
>>>>>>> [  192.349019] [<ffff2000088484b4>] clk_set_rate+0x34/0x4c
>>>>>>> [  192.354307] [<ffff20000895e304>] rcar_du_pm_suspend+0x274/0x2f4
>>>>>>> [  192.360297] [<ffff20000898feac>] platform_pm_suspend+0x78/0xb8
>>>>>>> [  192.366198] [<ffff2000089a5604>] dpm_run_callback+0x584/0xa18
>>>>>>> [  192.372010] [<ffff2000089a69e0>] __device_suspend+0x1a8/0x534
>>>>>>> [  192.377822] [<ffff2000089adc48>] dpm_suspend+0x130/0xea0
>>>>>>> [  192.383197] [<ffff2000089b0344>] dpm_suspend_start+0x130/0x138
>>>>>>> [  192.389099] [<ffff20000817f584>]
>>>>>>> suspend_devices_and_enter+0xf0/0x1778
>>>>>>> [  192.395700] [<ffff200008183014>] pm_suspend+0x2408/0x245c
>>>>>>> [  192.401162] [<ffff20000817c0a4>] state_store+0xf0/0x130
>>>>>>> [  192.406451] [<ffff200008f6f19c>] kobj_attr_store+0x5c/0x6c
>>>>>>> [  192.412002] [<ffff2000084f4c94>] sysfs_kf_write+0xe8/0xfc
>>>>>>> [  192.417466] [<ffff2000084f30b0>] kernfs_fop_write+0x22c/0x2e4
>>>>>>> [  192.423281] [<ffff2000083e46d4>] __vfs_write+0x104/0x34c
>>>>>>> [  192.428656] [<ffff2000083e4cc4>] vfs_write+0x134/0x2d8
>>>>>>> [  192.433857] [<ffff2000083e5150>] SyS_write+0xbc/0x12c
>>>>>>> [  192.438967] Exception stack(0xffff8006cd1cfec0 to
>>>>>>> 0xffff8006cd1d0000)
>>>>>>> [  192.445480] fec0: 0000000000000001 000000001e303f00
>>>>>>> 0000000000000004 0000ffff959a5000
>>>>>>> [  192.453397] fee0: 0000000000000000 0000000155510004
>>>>>>> 0000000000000003 000000000000006d
>>>>>>> [  192.461314] ff00: 0000000000000040 0000000000000000
>>>>>>> 0000ffffcc304800 0000000000000020
>>>>>>> [  192.469230] ff20: 0000000000000000 0000000000000000
>>>>>>> 0000000000000001 0000000000000008
>>>>>>> [  192.477148] ff40: 00000000004eb3b8 0000ffff958bb840
>>>>>>> 000000000000003d 0000000000000001
>>>>>>> [  192.485065] ff60: 000000001e303f00 0000ffff959a1508
>>>>>>> 0000000000000004 000000001e303f00
>>>>>>> [  192.492982] ff80: 0000000000000004 00000000004d4c68
>>>>>>> 0000000000000001 0000000000000000
>>>>>>> [  192.500899] ffa0: 000000001e30d5c0 0000ffffcc304820
>>>>>>> 0000ffff958bec64 0000ffffcc304820
>>>>>>> [  192.508816] ffc0: 0000ffff95912898 0000000020000000
>>>>>>> 0000000000000001 0000000000000040
>>>>>>> [  192.516733] ffe0: 0000000000000000 0000000000000000
>>>>>>> 0000000000000000 0000000000000000
>>>>>>> [  192.524650] [<ffff200008083ef0>] el0_svc_naked+0x24/0x28
>>>>>>>
>>>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>>>> ---
>>>>>>>     drivers/clk/clk-versaclock5.c | 4 ++++
>>>>>>>     1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/clk/clk-versaclock5.c
>>>>>>> b/drivers/clk/clk-versaclock5.c
>>>>>>> index decffb3..113523d 100644
>>>>>>> --- a/drivers/clk/clk-versaclock5.c
>>>>>>> +++ b/drivers/clk/clk-versaclock5.c
>>>>>>> @@ -509,6 +509,10 @@ static long vc5_fod_round_rate(struct clk_hw
>>>>>>> *hw, unsigned long rate,
>>>>>>>         u32 div_int;
>>>>>>>         u64 div_frc;
>>>>>>>     +    /* prevent div-by-0 */
>>>>>>> +    if (rate == 0)
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>>         /* Determine integer part, which is 12 bit wide */
>>>>>>>         div_int = f_in / rate;
>>>>>>>         /*
>>>>>>>
>>>>>> Can this actually happen ?
>>>>> We caught this using the Renesas 3.6.0 BSP release, when performing
>>>>> a suspend of rcar-du driver. The rcar_du_pm_suspend() in 3.6.0 BSP is
>>>>> modified
>>>>> from mainline version, including calling clk_set_rate() on the crtc
>>>>> clocks with a
>>>>> rate of zero. So this is not actually reproducible (yet) in mainline.
>>>> So it sets clock to 0 ?
>>> Yep, see
>>>
>>> https://kernel.googlesource.com/pub/scm/linux/kernel/git/horms/renesas-bsp/+/rcar-3.6.0/drivers/gpu/drm/rcar-du/rcar_du_drv.c#359
>>>
>>>
>>>
>>>>    Anyway, this looks sane, although maybe the
>>>> whole driver could use a once-over to see if there could be more of
>>>> this.
>>> Actually I do see more potential divide-by-zeros due to a passed rate
>>> of zero, including vc5_pfd_round_rate() and vc5_pfd_set_rate().
>>>
>>> I can resubmit this patch fixing all cases in clk-versaclock5.c if you
>>> like (and probably remove the misleading backtrace in the commit
>>> message since it is a Renesas 3.6.0 kernel backtrace not a mainline
>>> backtrace).
>>>
>>> Or perhaps just treat this as a heads-up, I'll leave it up to you.
>> It'd be nice if you resubmitted it fixing all the cases.
>>
> 
> Ok. Please disregard this patch since there won't be a v2, the
> new patch will have a different title.

Thanks!

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-06-01  7:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-31 18:23 [PATCH] clk: vc5: Fix div-by-0 when rounding a rate of zero Steve Longerbeam
2018-05-31 18:25 ` Marek Vasut
2018-05-31 18:32   ` Steve Longerbeam
2018-05-31 18:35     ` Marek Vasut
2018-05-31 18:52       ` Steve Longerbeam
2018-05-31 19:37         ` Marek Vasut
2018-06-01  1:46           ` Steve Longerbeam
2018-06-01  6:36             ` Marek Vasut

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).