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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx/

[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