| Message ID | 4efe2d0419cbe98163e2422ebe0c7896b8a5efed.1725717505.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] phy: renesas: rcar-gen3-usb2: Fix an error handling path in rcar_gen3_phy_usb2_probe() | expand |
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 >
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 >> > > >
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 > >> > > > > > >
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 >>>> >>> >>> >>> >
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;
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(+)