| Message ID | 1522671694-10229-5-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Mon, Apr 02, 2018 at 09:21:34PM +0900, Yoshihiro Shimoda wrote: > This patch fixes an issue that this driver cannot call phy_init() > if a gadget driver is alreadly loaded because usb_add_gadget_udc() > might call renesas_usb3_start() via .udc_start. > > Fixes: 279d4bc64060 ("usb: gadget: udc: renesas_usb3: add support for generic phy") > Cc: <stable@vger.kernel.org> # v4.15+ > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/usb/gadget/udc/renesas_usb3.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Simon Horman <horms+renesas@verge.net.au> Please see some suggestions for follow-up lower-priority patches below. > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > index 738b734..671bac3 100644 > --- a/drivers/usb/gadget/udc/renesas_usb3.c > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > @@ -2632,6 +2632,14 @@ static int renesas_usb3_probe(struct platform_device *pdev) > if (ret < 0) > goto err_alloc_prd; > > + /* > + * This is an optional. So, if this driver cannot get a phy, > + * this driver will not handle a phy anymore. > + */ nit: s/an optional/optional/ > + usb3->phy = devm_phy_get(&pdev->dev, "usb"); > + if (IS_ERR(usb3->phy)) > + usb3->phy = NULL; I think this will unintentionally ignore errors other than the non-existence of the device, f.e. a memory allocation failure in devm_phy_get(). So perhaps something like this, as per phy_optional_get(), would be better? usb3->phy = devm_phy_get(&pdev->dev, "usb"); if (IS_ERR(usb3->phy) && (PTR_ERR(usb3->phy) == -ENODEV)) usb3->phy = NULL; > + > pm_runtime_enable(&pdev->dev); > ret = usb_add_gadget_udc(&pdev->dev, &usb3->gadget); > if (ret < 0) > @@ -2641,14 +2649,6 @@ static int renesas_usb3_probe(struct platform_device *pdev) > if (ret < 0) > goto err_dev_create; > > - /* > - * This is an optional. So, if this driver cannot get a phy, > - * this driver will not handle a phy anymore. > - */ > - usb3->phy = devm_phy_get(&pdev->dev, "usb"); > - if (IS_ERR(usb3->phy)) > - usb3->phy = NULL; > - > usb3->workaround_for_vbus = priv->workaround_for_vbus; > > renesas_usb3_debugfs_init(usb3, &pdev->dev); > -- > 1.9.1 >
Hi Simon-san, > From: Simon Horman, Sent: Monday, April 9, 2018 8:58 PM > > On Mon, Apr 02, 2018 at 09:21:34PM +0900, Yoshihiro Shimoda wrote: > > This patch fixes an issue that this driver cannot call phy_init() > > if a gadget driver is alreadly loaded because usb_add_gadget_udc() > > might call renesas_usb3_start() via .udc_start. > > > > Fixes: 279d4bc64060 ("usb: gadget: udc: renesas_usb3: add support for generic phy") > > Cc: <stable@vger.kernel.org> # v4.15+ > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > drivers/usb/gadget/udc/renesas_usb3.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > > > Please see some suggestions for follow-up lower-priority patches below. Thank you for the review and suggestions! > > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > > index 738b734..671bac3 100644 > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > @@ -2632,6 +2632,14 @@ static int renesas_usb3_probe(struct platform_device *pdev) > > if (ret < 0) > > goto err_alloc_prd; > > > > + /* > > + * This is an optional. So, if this driver cannot get a phy, > > + * this driver will not handle a phy anymore. > > + */ > > nit: s/an optional/optional/ Oops. I will fix and submit v2 patches. > > + usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > + if (IS_ERR(usb3->phy)) > > + usb3->phy = NULL; > > I think this will unintentionally ignore errors other than the > non-existence of the device, f.e. a memory allocation failure > in devm_phy_get(). > > So perhaps something like this, as per phy_optional_get(), would be better? > > usb3->phy = devm_phy_get(&pdev->dev, "usb"); > if (IS_ERR(usb3->phy) && (PTR_ERR(usb3->phy) == -ENODEV)) > usb3->phy = NULL; Thank you for the suggestion. I agree with you. I think I should use devm_phy_optional_get() instead of dev_phy_get(), as you mentioned :) So, I will fix the code by using devm_phy_optional_get() first, and then fix this. Best regards, Yoshihiro Shimoda > > + > > pm_runtime_enable(&pdev->dev); > > ret = usb_add_gadget_udc(&pdev->dev, &usb3->gadget); > > if (ret < 0) > > @@ -2641,14 +2649,6 @@ static int renesas_usb3_probe(struct platform_device *pdev) > > if (ret < 0) > > goto err_dev_create; > > > > - /* > > - * This is an optional. So, if this driver cannot get a phy, > > - * this driver will not handle a phy anymore. > > - */ > > - usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > - if (IS_ERR(usb3->phy)) > > - usb3->phy = NULL; > > - > > usb3->workaround_for_vbus = priv->workaround_for_vbus; > > > > renesas_usb3_debugfs_init(usb3, &pdev->dev); > > -- > > 1.9.1 > >
On Tue, Apr 10, 2018 at 01:28:33AM +0000, Yoshihiro Shimoda wrote: > Hi Simon-san, > > > From: Simon Horman, Sent: Monday, April 9, 2018 8:58 PM > > > > On Mon, Apr 02, 2018 at 09:21:34PM +0900, Yoshihiro Shimoda wrote: > > > This patch fixes an issue that this driver cannot call phy_init() > > > if a gadget driver is alreadly loaded because usb_add_gadget_udc() > > > might call renesas_usb3_start() via .udc_start. > > > > > > Fixes: 279d4bc64060 ("usb: gadget: udc: renesas_usb3: add support for generic phy") > > > Cc: <stable@vger.kernel.org> # v4.15+ > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > --- > > > drivers/usb/gadget/udc/renesas_usb3.c | 16 ++++++++-------- > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > > > > > > Please see some suggestions for follow-up lower-priority patches below. > > Thank you for the review and suggestions! > > > > > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > > > index 738b734..671bac3 100644 > > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > > @@ -2632,6 +2632,14 @@ static int renesas_usb3_probe(struct platform_device *pdev) > > > if (ret < 0) > > > goto err_alloc_prd; > > > > > > + /* > > > + * This is an optional. So, if this driver cannot get a phy, > > > + * this driver will not handle a phy anymore. > > > + */ > > > > nit: s/an optional/optional/ > > Oops. I will fix and submit v2 patches. > > > > + usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > > + if (IS_ERR(usb3->phy)) > > > + usb3->phy = NULL; > > > > I think this will unintentionally ignore errors other than the > > non-existence of the device, f.e. a memory allocation failure > > in devm_phy_get(). > > > > So perhaps something like this, as per phy_optional_get(), would be better? > > > > usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > if (IS_ERR(usb3->phy) && (PTR_ERR(usb3->phy) == -ENODEV)) > > usb3->phy = NULL; > > Thank you for the suggestion. I agree with you. > I think I should use devm_phy_optional_get() instead of dev_phy_get(), as you mentioned :) > So, I will fix the code by using devm_phy_optional_get() first, and then fix this. Thanks, I agree that would be a good fix. But perhaps it is better as a follow-up?
Hi Simon-san, > From: Simon Horman, Sent: Tuesday, April 10, 2018 6:34 PM > > On Tue, Apr 10, 2018 at 01:28:33AM +0000, Yoshihiro Shimoda wrote: > > Hi Simon-san, > > > > > From: Simon Horman, Sent: Monday, April 9, 2018 8:58 PM > > > > > > On Mon, Apr 02, 2018 at 09:21:34PM +0900, Yoshihiro Shimoda wrote: > > > > This patch fixes an issue that this driver cannot call phy_init() > > > > if a gadget driver is alreadly loaded because usb_add_gadget_udc() > > > > might call renesas_usb3_start() via .udc_start. > > > > > > > > Fixes: 279d4bc64060 ("usb: gadget: udc: renesas_usb3: add support for generic phy") > > > > Cc: <stable@vger.kernel.org> # v4.15+ > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > --- > > > > drivers/usb/gadget/udc/renesas_usb3.c | 16 ++++++++-------- > > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > > > > > > > > > Please see some suggestions for follow-up lower-priority patches below. > > > > Thank you for the review and suggestions! > > > > > > > > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > > > > index 738b734..671bac3 100644 > > > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > > > @@ -2632,6 +2632,14 @@ static int renesas_usb3_probe(struct platform_device *pdev) > > > > if (ret < 0) > > > > goto err_alloc_prd; > > > > > > > > + /* > > > > + * This is an optional. So, if this driver cannot get a phy, > > > > + * this driver will not handle a phy anymore. > > > > + */ > > > > > > nit: s/an optional/optional/ > > > > Oops. I will fix and submit v2 patches. > > > > > > + usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > > > + if (IS_ERR(usb3->phy)) > > > > + usb3->phy = NULL; > > > > > > I think this will unintentionally ignore errors other than the > > > non-existence of the device, f.e. a memory allocation failure > > > in devm_phy_get(). > > > > > > So perhaps something like this, as per phy_optional_get(), would be better? > > > > > > usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > > if (IS_ERR(usb3->phy) && (PTR_ERR(usb3->phy) == -ENODEV)) > > > usb3->phy = NULL; > > > > Thank you for the suggestion. I agree with you. > > I think I should use devm_phy_optional_get() instead of dev_phy_get(), as you mentioned :) > > So, I will fix the code by using devm_phy_optional_get() first, and then fix this. > > Thanks, I agree that would be a good fix. > > But perhaps it is better as a follow-up? Oops. Yes, I changed my mind after prepared the patch v2 and submitted v2. I should have sent an email about this to you... Anyway, thank you very much for reviewing the v2 patches! Best regards, Yoshihiro Shimoda
On Tue, Apr 10, 2018 at 09:44:31AM +0000, Yoshihiro Shimoda wrote: > Hi Simon-san, > > > From: Simon Horman, Sent: Tuesday, April 10, 2018 6:34 PM > > > > On Tue, Apr 10, 2018 at 01:28:33AM +0000, Yoshihiro Shimoda wrote: > > > Hi Simon-san, > > > > > > > From: Simon Horman, Sent: Monday, April 9, 2018 8:58 PM > > > > > > > > On Mon, Apr 02, 2018 at 09:21:34PM +0900, Yoshihiro Shimoda wrote: > > > > > This patch fixes an issue that this driver cannot call phy_init() > > > > > if a gadget driver is alreadly loaded because usb_add_gadget_udc() > > > > > might call renesas_usb3_start() via .udc_start. > > > > > > > > > > Fixes: 279d4bc64060 ("usb: gadget: udc: renesas_usb3: add support for generic phy") > > > > > Cc: <stable@vger.kernel.org> # v4.15+ > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > --- > > > > > drivers/usb/gadget/udc/renesas_usb3.c | 16 ++++++++-------- > > > > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > > > > > Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > > > > > > > > > > > > Please see some suggestions for follow-up lower-priority patches below. > > > > > > Thank you for the review and suggestions! > > > > > > > > > > > > > diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c > > > > > index 738b734..671bac3 100644 > > > > > --- a/drivers/usb/gadget/udc/renesas_usb3.c > > > > > +++ b/drivers/usb/gadget/udc/renesas_usb3.c > > > > > @@ -2632,6 +2632,14 @@ static int renesas_usb3_probe(struct platform_device *pdev) > > > > > if (ret < 0) > > > > > goto err_alloc_prd; > > > > > > > > > > + /* > > > > > + * This is an optional. So, if this driver cannot get a phy, > > > > > + * this driver will not handle a phy anymore. > > > > > + */ > > > > > > > > nit: s/an optional/optional/ > > > > > > Oops. I will fix and submit v2 patches. > > > > > > > > + usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > > > > + if (IS_ERR(usb3->phy)) > > > > > + usb3->phy = NULL; > > > > > > > > I think this will unintentionally ignore errors other than the > > > > non-existence of the device, f.e. a memory allocation failure > > > > in devm_phy_get(). > > > > > > > > So perhaps something like this, as per phy_optional_get(), would be better? > > > > > > > > usb3->phy = devm_phy_get(&pdev->dev, "usb"); > > > > if (IS_ERR(usb3->phy) && (PTR_ERR(usb3->phy) == -ENODEV)) > > > > usb3->phy = NULL; > > > > > > Thank you for the suggestion. I agree with you. > > > I think I should use devm_phy_optional_get() instead of dev_phy_get(), as you mentioned :) > > > So, I will fix the code by using devm_phy_optional_get() first, and then fix this. > > > > Thanks, I agree that would be a good fix. > > > > But perhaps it is better as a follow-up? > > Oops. Yes, I changed my mind after prepared the patch v2 and submitted v2. > I should have sent an email about this to you... > Anyway, thank you very much for reviewing the v2 patches! v2 looks good to me :)
diff --git a/drivers/usb/gadget/udc/renesas_usb3.c b/drivers/usb/gadget/udc/renesas_usb3.c index 738b734..671bac3 100644 --- a/drivers/usb/gadget/udc/renesas_usb3.c +++ b/drivers/usb/gadget/udc/renesas_usb3.c @@ -2632,6 +2632,14 @@ static int renesas_usb3_probe(struct platform_device *pdev) if (ret < 0) goto err_alloc_prd; + /* + * This is an optional. So, if this driver cannot get a phy, + * this driver will not handle a phy anymore. + */ + usb3->phy = devm_phy_get(&pdev->dev, "usb"); + if (IS_ERR(usb3->phy)) + usb3->phy = NULL; + pm_runtime_enable(&pdev->dev); ret = usb_add_gadget_udc(&pdev->dev, &usb3->gadget); if (ret < 0) @@ -2641,14 +2649,6 @@ static int renesas_usb3_probe(struct platform_device *pdev) if (ret < 0) goto err_dev_create; - /* - * This is an optional. So, if this driver cannot get a phy, - * this driver will not handle a phy anymore. - */ - usb3->phy = devm_phy_get(&pdev->dev, "usb"); - if (IS_ERR(usb3->phy)) - usb3->phy = NULL; - usb3->workaround_for_vbus = priv->workaround_for_vbus; renesas_usb3_debugfs_init(usb3, &pdev->dev);
This patch fixes an issue that this driver cannot call phy_init() if a gadget driver is alreadly loaded because usb_add_gadget_udc() might call renesas_usb3_start() via .udc_start. Fixes: 279d4bc64060 ("usb: gadget: udc: renesas_usb3: add support for generic phy") Cc: <stable@vger.kernel.org> # v4.15+ Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/usb/gadget/udc/renesas_usb3.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)