Re: [PATCH v5 02/10] phy: rockchip: samsung-hdptx: Prevent divide-by-zero when computing clk rate

On 8/11/26 2:24 PM, Vinod Koul wrote:
> On 07-08-26, 17:19, Cristian Ciocaltea wrote:
>> Hi Mani,
>>
>> On 8/7/26 3:56 PM, Manivannan Sadhasivam wrote:
>>> On Thu, Jul 23, 2026 at 10:41:45PM +0300, Cristian Ciocaltea wrote:
>>>> Calculating 'sdm' fraction in rk_hdptx_phy_clk_calc_rate_from_pll_cfg()
>>>> could trigger a divide-by-zero, as it uses div_u64() with a denominator
>>>> read directly from hardware: the values ropll_hw.sdm_deno,
>>>> ropll_hw.sdc_deno, ropll_hw.sdc_n, and ropll_hw.sdc_num are populated
>>>> from PLL registers which, in theory, could be left by the bootloader
>>>> uninitialized/misconfigured.
>>>>
>>>> Provide the necessary sanitization to avoid trusting the hardware state.
>>>>
>>>> Reported-by: Sashiko <[email protected]>
>>>> Closes: https://sashiko.dev/#/patchset/[email protected]?part=1
>>>> Fixes: 3481fc04d969 ("phy: rockchip: samsung-hdptx: Compute clk rate from PLL config")
>>>> Tested-by: Diederik de Haas <[email protected]>  # NanoPC-T6 LTS
>>>> Reviewed-by: Dmitry Baryshkov <[email protected]>
>>>> Reviewed-by: Andy Yan <[email protected]>
>>>> Signed-off-by: Cristian Ciocaltea <[email protected]>
>>>> ---
>>>>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 11 ++++++++---
>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>>> index 710603afff86..44e99343f249 100644
>>>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>>> @@ -2270,10 +2270,15 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
>>>>  
>>>>  	fout = PLL_REF_CLK * ropll_hw.pms_mdiv;
>>>>  	if (ropll_hw.sdm_en) {
>>>> +		val = 16U * ropll_hw.sdm_deno *
>>>> +		      (ropll_hw.sdc_deno * ropll_hw.sdc_n - ropll_hw.sdc_num);
>>>> +		if (!val) {
>>>> +			dev_dbg(hdptx->dev, "Invalid ROPLL hw state: deno == 0\n");
>>>
>>> If the hardware state is invalid, why can't this be a hard failure?
>>
>> This is an internal helper called from the .recalc_rate clk_op, which tries to
>> compute the current rate by reading back the hardware state. 
>>
>> Since .recalc_rate returns unsigned long, we cannot propagate errors to the
>> caller.  However, per the clk_ops documentation, the driver is expected to
>> return 0 if it cannot figure out the rate.
>>
>> Moreover, this condition isn't really fatal, as it only means the PLL
>> configuration currently stored in hardware can't be translated into a rate.  The
>> next .set_rate should program a valid configuration and help with the recovery.
> 
> This is a good explanation which may not be obvious to everyone. I would
> think that it would be good idea to add a comment for this here

Ack, handled in v6:

https://lore.kernel.org/all/[email protected]/

Regards,
Cristian

_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip