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

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

[Christophe JAILLET]

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);
 	pm_runtime_disable(dev);
 
 	return ret;
-- 
2.46.0

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

[Biju Das]

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;

[4] https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/reset/core.c#L483

Cheers,
Biju

>  	pm_runtime_disable(dev);
> 
>  	return ret;
> --
> 2.46.0
> 

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

[Christophe JAILLET]

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;
> 
> [4] https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/reset/core.c#L483

So, if I understand correctly, v1 [5] was correct. :)


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
>>
> 
> 
> 

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

[Biju Das]

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;
> >
> > [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??

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
> >>
> >
> >
> >

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

[claudiu beznea]

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
>>>>
>>>
>>>
>>>
>