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@xxxxxxxxxx>
> 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@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. :)

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