linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy
@ 2021-06-10 11:37 Yuan Fang
  2021-06-10 11:47 ` Felipe Balbi
  0 siblings, 1 reply; 5+ messages in thread
From: Yuan Fang @ 2021-06-10 11:37 UTC (permalink / raw)
  To: linux-usb; +Cc: balbi, Yuan Fang

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(+)

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");
 		}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy
  2021-06-10 11:37 [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy Yuan Fang
@ 2021-06-10 11:47 ` Felipe Balbi
  2021-06-10 11:58   ` 答复: " Fang, Yuan
  2021-06-10 12:08   ` Fang, Yuan
  0 siblings, 2 replies; 5+ messages in thread
From: Felipe Balbi @ 2021-06-10 11:47 UTC (permalink / raw)
  To: Yuan Fang, linux-usb; +Cc: Yuan Fang

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* 答复: [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy
  2021-06-10 11:47 ` Felipe Balbi
@ 2021-06-10 11:58   ` Fang, Yuan
  2021-06-10 12:27     ` Felipe Balbi
  2021-06-10 12:08   ` Fang, Yuan
  1 sibling, 1 reply; 5+ messages in thread
From: Fang, Yuan @ 2021-06-10 11:58 UTC (permalink / raw)
  To: Felipe Balbi, linux-usb

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* 答复: [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy
  2021-06-10 11:47 ` Felipe Balbi
  2021-06-10 11:58   ` 答复: " Fang, Yuan
@ 2021-06-10 12:08   ` Fang, Yuan
  1 sibling, 0 replies; 5+ messages in thread
From: Fang, Yuan @ 2021-06-10 12:08 UTC (permalink / raw)
  To: Felipe Balbi, linux-usb

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: 答复: [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy
  2021-06-10 11:58   ` 答复: " Fang, Yuan
@ 2021-06-10 12:27     ` Felipe Balbi
  0 siblings, 0 replies; 5+ messages in thread
From: Felipe Balbi @ 2021-06-10 12:27 UTC (permalink / raw)
  To: Fang, Yuan, linux-usb

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]


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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-10 12:28 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10 11:37 [PATCH] driver/usb/dwc3: Add error handle for dwc3 get usb phy Yuan Fang
2021-06-10 11:47 ` Felipe Balbi
2021-06-10 11:58   ` 答复: " Fang, Yuan
2021-06-10 12:27     ` Felipe Balbi
2021-06-10 12:08   ` Fang, Yuan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).