| Message ID | 290b25827e3f0742808940719455ff0c5cb9d01d.1726329925.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] phy: renesas: rcar-gen3-usb2: Fix an error handling path in rcar_gen3_phy_usb2_probe() | expand |
Le 14/09/2024 à 18:07, Christophe JAILLET a écrit : > 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 v3: > - Use devm_add_action_or_reset() [Biju Das] > > 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..c8a3727f56aa 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) > + return ret; NACK This is still broken. Should be goto rpm_put; I'll send a v4. CJ > + > 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); > }; >
diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c index 58e123305152..c8a3727f56aa 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) + return ret; + 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 v3: - Use devm_add_action_or_reset() [Biju Das] 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(-)