| Message ID | 20210610113745.7710-1-yuan.fang@verisilicon.com |
|---|---|
| State | New |
| Headers | show |
| Series | driver/usb/dwc3: Add error handle for dwc3 get usb phy | expand |
Hi, Yuan Fang <fangyuanseu@gmail.com> writes: > Add error handle for dwc3 core get usb phy, because > without error handle will lead to bug. > > consider case like this, when both dwc->usb2_phy/usb3_phy > and dwc->usb2_generic_phy/usb3_generic_phy do not exist > current driver code set them all to NULL and return 0 > to caller which means usb phy is ready. it's unclear what the problem is. Care to describe what's happening? > Signed-off-by: Yuan Fang <yuan.fang@verisilicon.com> > --- > Hi maintainer, > > I find this issue when i try to add a new usb phy driver for > dwc3 usb core, this issue looks like a bug, please kindly have > a review, many thanks. > > Best regards, > Yuan Fang > > drivers/usb/dwc3/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index b6e53d8212cd..4d527d313a4c 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1184,6 +1184,7 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > ret = PTR_ERR(dwc->usb3_generic_phy); > if (ret == -ENOSYS || ret == -ENODEV) { > dwc->usb3_generic_phy = NULL; > + return dev_err_probe(dev, ret, "no usb phy device\n"); no, this is incorrect. Some platforms don't require a PHY driver at all because there's no SW control for the PHY. Moreover, generic PHY framework protects against NULL phy. I don't see what the problem you're referring to is here.
Hi, I agree with you that some platforms don't require a PHY driver. But in the case I mentioned, in the platforms that do need a PHY driver and PHY driver is not bring up successfully, the driver code right now will return 0 and go on set the dwc->phys_ready=ture, which looks odd in this case, right? B.R Yuan Fang -----邮件原件----- 发件人: Felipe Balbi [mailto:balbi@kernel.org] 发送时间: 2021年6月10日 19:47 收件人: Yuan Fang; linux-usb@vger.kernel.org 抄送: Fang, Yuan 主题: Re: [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy Hi, Yuan Fang <fangyuanseu@gmail.com> writes: > Add error handle for dwc3 core get usb phy, because without error > handle will lead to bug. > > consider case like this, when both dwc->usb2_phy/usb3_phy and > dwc->usb2_generic_phy/usb3_generic_phy do not exist current driver > code set them all to NULL and return 0 to caller which means usb phy > is ready. it's unclear what the problem is. Care to describe what's happening? > Signed-off-by: Yuan Fang <yuan.fang@verisilicon.com> > --- > Hi maintainer, > > I find this issue when i try to add a new usb phy driver for > dwc3 usb core, this issue looks like a bug, please kindly have > a review, many thanks. > > Best regards, > Yuan Fang > > drivers/usb/dwc3/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index > b6e53d8212cd..4d527d313a4c 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1184,6 +1184,7 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > ret = PTR_ERR(dwc->usb3_generic_phy); > if (ret == -ENOSYS || ret == -ENODEV) { > dwc->usb3_generic_phy = NULL; > + return dev_err_probe(dev, ret, "no usb phy device\n"); no, this is incorrect. Some platforms don't require a PHY driver at all because there's no SW control for the PHY. Moreover, generic PHY framework protects against NULL phy. I don't see what the problem you're referring to is here. -- balbi
Add to previous email, generic PHY framework DO protects against NULL phy, but which is not good enough, for a good design, should return once error was detect and pass correct message to developer, do you agree with me? In the case I mentioned, PHY is not ready but I got a PHY time out error and with dwc->phys_ready=ture, which is incorrect and passing error message. B.R Yuan Fang -----邮件原件----- 发件人: Felipe Balbi [mailto:balbi@kernel.org] 发送时间: 2021年6月10日 19:47 收件人: Yuan Fang; linux-usb@vger.kernel.org 抄送: Fang, Yuan 主题: Re: [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy Hi, Yuan Fang <fangyuanseu@gmail.com> writes: > Add error handle for dwc3 core get usb phy, because without error > handle will lead to bug. > > consider case like this, when both dwc->usb2_phy/usb3_phy and > dwc->usb2_generic_phy/usb3_generic_phy do not exist current driver > code set them all to NULL and return 0 to caller which means usb phy > is ready. it's unclear what the problem is. Care to describe what's happening? > Signed-off-by: Yuan Fang <yuan.fang@verisilicon.com> > --- > Hi maintainer, > > I find this issue when i try to add a new usb phy driver for > dwc3 usb core, this issue looks like a bug, please kindly have > a review, many thanks. > > Best regards, > Yuan Fang > > drivers/usb/dwc3/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index > b6e53d8212cd..4d527d313a4c 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1184,6 +1184,7 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > ret = PTR_ERR(dwc->usb3_generic_phy); > if (ret == -ENOSYS || ret == -ENODEV) { > dwc->usb3_generic_phy = NULL; > + return dev_err_probe(dev, ret, "no usb phy device\n"); no, this is incorrect. Some platforms don't require a PHY driver at all because there's no SW control for the PHY. Moreover, generic PHY framework protects against NULL phy. I don't see what the problem you're referring to is here. -- balbi
Hi, (please don't top-post. Also, make sure your email client breaks lines at 80 columns "Fang, Yuan" <Yuan.Fang@verisilicon.com> writes: > I agree with you that some platforms don't require a PHY driver. > > But in the case I mentioned, in the platforms that do need a PHY > driver and PHY driver is not bring up successfully, the driver code > right now will return 0 and go on set the dwc->phys_ready=ture, which > looks odd in this case, right? Your new PHY driver shouldn't return -ENODEV. You can see dwc3 filters only -ENOSYS (framework disabled) and -ENODEV (device doesn't exist). Stop returning -ENODEV and you should get the error propagated to dwc3 properly.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index b6e53d8212cd..4d527d313a4c 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1184,6 +1184,7 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) ret = PTR_ERR(dwc->usb3_generic_phy); if (ret == -ENOSYS || ret == -ENODEV) { dwc->usb3_generic_phy = NULL; + return dev_err_probe(dev, ret, "no usb phy device\n"); } else { return dev_err_probe(dev, ret, "no usb3 phy configured\n"); }
Add error handle for dwc3 core get usb phy, because without error handle will lead to bug. consider case like this, when both dwc->usb2_phy/usb3_phy and dwc->usb2_generic_phy/usb3_generic_phy do not exist current driver code set them all to NULL and return 0 to caller which means usb phy is ready. Signed-off-by: Yuan Fang <yuan.fang@verisilicon.com> --- Hi maintainer, I find this issue when i try to add a new usb phy driver for dwc3 usb core, this issue looks like a bug, please kindly have a review, many thanks. Best regards, Yuan Fang drivers/usb/dwc3/core.c | 1 + 1 file changed, 1 insertion(+)