Re: [PATCH v2] phy: renesas: rcar-gen3-usb2: Fix an error handling path in rcar_gen3_phy_usb2_probe()

[claudiu beznea <claudiu.beznea@tuxon.dev>]

On 09.09.2024 09:00, Biju Das wrote:
> Hi Christophe JAILLET,
> 
>> -----Original Message-----
>> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> Sent: Sunday, September 8, 2024 5:59 PM
>> Subject: Re: [PATCH v2] phy: renesas: rcar-gen3-usb2: Fix an error handling path in
>> rcar_gen3_phy_usb2_probe()
>>
>> Le 08/09/2024 à 18:39, Biju Das a écrit :
>>> Hi Christophe JAILLET,
>>>
>>> Thanks for the patch.
>>>
>>>> -----Original Message-----
>>>> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> Sent: Saturday, September 7, 2024 2:59 PM
>>>> Subject: [PATCH v2] phy: renesas: rcar-gen3-usb2: Fix an error
>>>> handling path in
>>>> rcar_gen3_phy_usb2_probe()
>>>>
>>>> If an error occurs after the rcar_gen3_phy_usb2_init_bus(),
>>>> reset_control_assert() must be called, as already done in the remove function.
>>>>
>>>> This is fine to re-use the existing error handling path, because even
>>>> if "channel->rstc" is still NULL at this point, it is safe to call reset_control_assert(NULL).
>>>>
>>>> Fixes: 4eae16375357 ("phy: renesas: rcar-gen3-usb2: Add support to
>>>> initialize the bus")
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> ---
>>>> Changes in v2:
>>>>    - Re-use 'error' to simplify the patch   [claudiu beznea]
>>>>    - Update the commit description to explain why it is safe.
>>>>
>>>> v1:
>>>> https://lore.kernel.org/all/fc9f7b444f0ca645411868992bbe16514aeccfed.
>>>> 1725652654.git.christophe.jaillet
>>>> @wanadoo.fr/
>>>> ---
>>>>   drivers/phy/renesas/phy-rcar-gen3-usb2.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>>> b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>>> index 58e123305152..ccb0b54b70f7 100644
>>>> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>>> @@ -803,6 +803,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>>>>   	return 0;
>>>>
>>>>   error:
>>>> +	reset_control_assert(channel->rstc);
>>>
>>> This will result in either kernel crash [1] or reset usage count
>>> imbalance in case of error [2] and [3] in
>>> rcar_gen3_phy_usb2_init_bus() see [4]. Also reset control API is not
>>> respected for SoCs other than RZ/G3S. For those SoC's reset assert is
>>> called without calling a get(). Maybe add a check (phy_data->init_bus)
>>> for assert api's, that guarantees assert is called after calling a get() as it valid only for
>> RZ/G3S??
>>>
>>> [1]
>>> channel->rstc = devm_reset_control_array_get_shared(dev);
>>> 	if (IS_ERR(channel->rstc))
>>> 		return PTR_ERR(channel->rstc);
>>>
>>> [2]
>>> ret = pm_runtime_resume_and_get(dev);
>>> 	if (ret)
>>> 		return ret;
>>> [3]
>>> ret = reset_control_deassert(channel->rstc);
>>> 	if (ret)
>>> 		goto rpm_put;

Indeed, I missed that. Thank you, Biju, for pointing it out.

>>>
>>> [4]
>>> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/reset/core.c
>>> #L483
>>
>> So, if I understand correctly, v1 [5] was correct. :)
> 
> Yes, I agree v1 [5] is correct, if we ignore "reset control API is not respected for SoCs".
> 
> Another solution could be using action_or_reset() for cleanup for
> rcar_gen3_phy_usb2_init_bus(), so that it is applicable only for RZ/G3S??

That seems like a cleaner solution to me.

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju
> 
>>
>>
>> I don't think that [1] would crash, because of [6]. It would only WARN_ON. But with v1, it is not
>> called.
>>
>> With v1, reset_control_assert() is not called if
>> rcar_gen3_phy_usb2_init_bus() fails. So [2] and [3] can't occur.
>>
>> I can send a v3, which is the same of v1, or you can pick v1 as-is (if I'm correct... :)) or you can
>> just ignore it if "reset control API is not respected for SoCs".
>>
>>
>> If of interest, I spotted it with one of my coccinelle script that compares functions called
>> in .remove function, but not in error handling path of probe.
>>
>>
>> CJ
>>
>> [5]:
>> https://lore.kernel.org/all/fc9f7b444f0ca645411868992bbe16514aeccfed.1725652654.git.christophe.jaillet
>> @wanadoo.fr/
>>
>> [6]:
>> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/reset/core.c#L473
>>
>>>
>>> Cheers,
>>> Biju
>>>
>>>>   	pm_runtime_disable(dev);
>>>>
>>>>   	return ret;
>>>> --
>>>> 2.46.0
>>>>
>>>
>>>
>>>
>