| Message ID | 1457c401e3cdf792c5170f5c703cb24de137c19a.1726333335.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v4] phy: renesas: rcar-gen3-usb2: Fix an error handling path in rcar_gen3_phy_usb2_probe() | expand |
Hi, > -----Original Message----- > From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Sent: Saturday, September 14, 2024 6:03 PM > Subject: [PATCH v4] phy: renesas: rcar-gen3-usb2: Fix an error handling path in > rcar_gen3_phy_usb2_probe() > > If an error occurs after the reset_control_deassert(), > reset_control_assert() must be called, as already done in the remove function. > > Use devm_add_action_or_reset() to add the missing call and simplify the > .remove() function accordingly. > > Fixes: 4eae16375357 ("phy: renesas: rcar-gen3-usb2: Add support to initialize the bus") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > This patch changes the order of function calls when releasing the resources in the .remove function(). > Looks fine to me, but pm_ functions are sometimes tricky. > > Changes in v4: > - Use the error handling path in rcar_gen3_phy_usb2_init_bus() > > Changes in v3: > - Use devm_add_action_or_reset() [Biju Das] > v3: > https://lore.kernel.org/all/290b25827e3f0742808940719455ff0c5cb9d01d.1726329925.git.christophe.jaillet > @wanadoo.fr/ > > Changes in v2: (broken proposal) > - Re-use 'error' to simplify the patch [claudiu beznea] > - Update the commit description to explain why it is safe. > v2: > https://lore.kernel.org/all/4efe2d0419cbe98163e2422ebe0c7896b8a5efed.1725717505.git.christophe.jaillet > @wanadoo.fr/ > > v1: > https://lore.kernel.org/all/fc9f7b444f0ca645411868992bbe16514aeccfed.1725652654.git.christophe.jaillet > @wanadoo.fr/ > --- > drivers/phy/renesas/phy-rcar-gen3-usb2.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > index 58e123305152..f900fe42c311 100644 > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c > @@ -668,6 +668,13 @@ static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np) > return candidate; > } > > +static void rcar_gen3_reset_assert(void *data) { > + struct reset_control *rstc = data; nit: Maybe since it is void*, you could avoid this assignment and use data instead?? Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> Cheers, Biju > + > + reset_control_assert(rstc); > +} > + > static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel) { > struct device *dev = channel->dev; > @@ -686,6 +693,11 @@ static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel) > if (ret) > goto rpm_put; > > + ret = devm_add_action_or_reset(dev, rcar_gen3_reset_assert, > + channel->rstc); > + if (ret) > + goto rpm_put; > + > val = readl(channel->base + USB2_AHB_BUS_CTR); > val &= ~USB2_AHB_BUS_CTR_MBL_MASK; > val |= USB2_AHB_BUS_CTR_MBL_INCR4; > @@ -815,7 +827,6 @@ static void rcar_gen3_phy_usb2_remove(struct platform_device *pdev) > if (channel->is_otg_channel) > device_remove_file(&pdev->dev, &dev_attr_role); > > - reset_control_assert(channel->rstc); > pm_runtime_disable(&pdev->dev); > }; > > -- > 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..f900fe42c311 100644 --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c @@ -668,6 +668,13 @@ static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np) return candidate; } +static void rcar_gen3_reset_assert(void *data) +{ + struct reset_control *rstc = data; + + reset_control_assert(rstc); +} + static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel) { struct device *dev = channel->dev; @@ -686,6 +693,11 @@ static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel) if (ret) goto rpm_put; + ret = devm_add_action_or_reset(dev, rcar_gen3_reset_assert, + channel->rstc); + if (ret) + goto rpm_put; + val = readl(channel->base + USB2_AHB_BUS_CTR); val &= ~USB2_AHB_BUS_CTR_MBL_MASK; val |= USB2_AHB_BUS_CTR_MBL_INCR4; @@ -815,7 +827,6 @@ static void rcar_gen3_phy_usb2_remove(struct platform_device *pdev) if (channel->is_otg_channel) device_remove_file(&pdev->dev, &dev_attr_role); - reset_control_assert(channel->rstc); pm_runtime_disable(&pdev->dev); };
If an error occurs after the reset_control_deassert(), reset_control_assert() must be called, as already done in the remove function. Use devm_add_action_or_reset() to add the missing call and simplify the .remove() function accordingly. Fixes: 4eae16375357 ("phy: renesas: rcar-gen3-usb2: Add support to initialize the bus") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- This patch changes the order of function calls when releasing the resources in the .remove function(). Looks fine to me, but pm_ functions are sometimes tricky. Changes in v4: - Use the error handling path in rcar_gen3_phy_usb2_init_bus() Changes in v3: - Use devm_add_action_or_reset() [Biju Das] v3: https://lore.kernel.org/all/290b25827e3f0742808940719455ff0c5cb9d01d.1726329925.git.christophe.jaillet@wanadoo.fr/ Changes in v2: (broken proposal) - Re-use 'error' to simplify the patch [claudiu beznea] - Update the commit description to explain why it is safe. v2: https://lore.kernel.org/all/4efe2d0419cbe98163e2422ebe0c7896b8a5efed.1725717505.git.christophe.jaillet@wanadoo.fr/ v1: https://lore.kernel.org/all/fc9f7b444f0ca645411868992bbe16514aeccfed.1725652654.git.christophe.jaillet@wanadoo.fr/ --- drivers/phy/renesas/phy-rcar-gen3-usb2.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)