LinuxLists.cc - [PATCH] usb: dwc2: dwc2_vbus_supply

[permalink] [raw]

Subject: Re: [PATCH] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi Tomeu,

Am Donnerstag, 22. M?rz 2018, 12:39:13 CET schrieb Heiko St?bner:
> Am Donnerstag, 22. M?rz 2018, 10:39:43 CET schrieb Tomeu Vizoso:
> > devm_regulator_get_optional returns -ENODEV if the regulator isn't
> > there, so if that's the case we have to make sure not to leave -ENODEV
> > in the regulator pointer.
> >
> > Also, make sure we return 0 in that case, but correctly propagate any
> > other errors. Also propagate the error from _dwc2_hcd_start.
> >
> > Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
> > supply") Cc: Amelie Delaunay <[email protected]>
> > Signed-off-by: Tomeu Vizoso <[email protected]>
>
> Thanks for catching that oops in your tests.
> Reviewed-by: Heiko Stuebner <[email protected]>

I take that back :-)
see below

> > ---
> >
> > drivers/usb/dwc2/hcd.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> > index dcfda5eb4cac..4ae211f65e85 100644
> > --- a/drivers/usb/dwc2/hcd.c
> > +++ b/drivers/usb/dwc2/hcd.c
> > @@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg
> > *hsotg) static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
> >
> > {
> >
> > hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
> >
> > - if (IS_ERR(hsotg->vbus_supply))
> > - return 0;
> > + if (IS_ERR(hsotg->vbus_supply)) {
> > + hsotg->vbus_supply = NULL;
> > + if (PTR_ERR(hsotg->vbus_supply) == -ENODEV)

hsotg->vbus_supply is already NULL here

2018-03-22 12:24:00

by Amelie Delaunay

[permalink] [raw]

Subject: Re: [PATCH] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi Heiko,
Hi Tomeu,

On 03/22/2018 12:41 PM, Heiko Stübner wrote:
> Hi Tomeu,
>
> Am Donnerstag, 22. März 2018, 12:39:13 CET schrieb Heiko Stübner:
>> Am Donnerstag, 22. März 2018, 10:39:43 CET schrieb Tomeu Vizoso:
>>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>>> there, so if that's the case we have to make sure not to leave -ENODEV
>>> in the regulator pointer.
>>>
>>> Also, make sure we return 0 in that case, but correctly propagate any
>>> other errors. Also propagate the error from _dwc2_hcd_start.
>>>
>>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>>> supply") Cc: Amelie Delaunay <[email protected]>
>>> Signed-off-by: Tomeu Vizoso <[email protected]>
>>
>> Thanks for catching that oops in your tests.
>> Reviewed-by: Heiko Stuebner <[email protected]>
>
> I take that back :-)
> see below
>
>>> ---
>>>
>>> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>> index dcfda5eb4cac..4ae211f65e85 100644
>>> --- a/drivers/usb/dwc2/hcd.c
>>> +++ b/drivers/usb/dwc2/hcd.c
>>> @@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg
>>> *hsotg) static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>>>
>>> {
>>>
>>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>>>
>>> - if (IS_ERR(hsotg->vbus_supply))
>>> - return 0;
>>> + if (IS_ERR(hsotg->vbus_supply)) {
>>> + hsotg->vbus_supply = NULL;
>>> + if (PTR_ERR(hsotg->vbus_supply) == -ENODEV)
>
> hsotg->vbus_supply is already NULL here
>

Tomeu is right, here, hsotg->vbus_supply = ERR_PTR(-ENODEV).
/**
* regulator_get_optional - obtain optional access to a regulator.
* ...
* Returns a struct regulator corresponding to the regulator producer,
* or IS_ERR() condition containing errno.
* ...
*/

You can add my Reviewed-by: Amelie Delaunay <[email protected]>

Thanks for the fix,
Amelie

2018-03-22 13:09:47

[permalink] [raw]

Subject: [PATCH v2] usb: dwc2: dwc2_vbus_supply_init: fix error check

devm_regulator_get_optional returns -ENODEV if the regulator isn't
there, so if that's the case we have to make sure not to leave -ENODEV
in the regulator pointer.

Also, make sure we return 0 in that case, but correctly propagate any
other errors. Also propagate the error from _dwc2_hcd_start.

Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
Cc: Amelie Delaunay <[email protected]>
Signed-off-by: Tomeu Vizoso <[email protected]>

---

v2: Only overwrite the error in the pointer after checking it (Heiko
Stübner <[email protected]>)
---
arch/arm/configs/multi_v7_defconfig | 3 +++
drivers/usb/dwc2/hcd.c | 11 +++++++----
scripts/setlocalversion | 9 ---------
3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 846ce7bb24bc..33148fcabd17 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -1029,3 +1029,6 @@ CONFIG_VIRTIO=y
CONFIG_VIRTIO_PCI=y
CONFIG_VIRTIO_PCI_LEGACY=y
CONFIG_VIRTIO_MMIO=y
+CONFIG_LOCALVERSION_AUTO=n
+CONFIG_LOCALVERSION=""
+
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index dcfda5eb4cac..863aed20517f 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
{
hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
- if (IS_ERR(hsotg->vbus_supply))
+ if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
+ hsotg->vbus_supply = NULL;
return 0;
+ } else if (IS_ERR(hsotg->vbus_supply)) {
+ hsotg->vbus_supply = NULL;
+ return PTR_ERR(hsotg->vbus_supply);
+ }

return regulator_enable(hsotg->vbus_supply);
}
@@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)

spin_unlock_irqrestore(&hsotg->lock, flags);

- dwc2_vbus_supply_init(hsotg);
-
- return 0;
+ return dwc2_vbus_supply_init(hsotg);
}

/*
diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 71f39410691b..cbc36d3b4d0f 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -161,15 +161,6 @@ res="${res}${CONFIG_LOCALVERSION}${LOCALVERSION}"
if test "$CONFIG_LOCALVERSION_AUTO" = "y"; then
# full scm version string
res="$res$(scm_version)"
-else
- # append a plus sign if the repository is not in a clean
- # annotated or signed tagged state (as git describe only
- # looks at signed or annotated tags - git tag -a/-s) and
- # LOCALVERSION= is not specified
- if test "${LOCALVERSION+set}" != "set"; then
- scm=$(scm_version --short)
- res="$res${scm:++}"
- fi
fi

echo "$res"
--
2.14.3

2018-03-22 13:17:34

[permalink] [raw]

Subject: [PATCH v3] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-03-22 13:27:50

[permalink] [raw]

Subject: Re: [PATCH v3] usb: dwc2: dwc2_vbus_supply_init: fix error check

Am Donnerstag, 22. M?rz 2018, 14:14:51 CET schrieb Tomeu Vizoso:
> devm_regulator_get_optional returns -ENODEV if the regulator isn't
> there, so if that's the case we have to make sure not to leave -ENODEV
> in the regulator pointer.
>
> Also, make sure we return 0 in that case, but correctly propagate any
> other errors. Also propagate the error from _dwc2_hcd_start.
>
> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
> supply") Cc: Amelie Delaunay <[email protected]>
> Signed-off-by: Tomeu Vizoso <[email protected]>
>
> ---
>
> v2: Only overwrite the error in the pointer after checking it (Heiko
> St?bner <[email protected]>)
> v3: Remove hunks that shouldn't be in this patch
> ---
> drivers/usb/dwc2/hcd.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index dcfda5eb4cac..863aed20517f 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
> {
> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
> - if (IS_ERR(hsotg->vbus_supply))
> + if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
> + hsotg->vbus_supply = NULL;
> return 0;
> + } else if (IS_ERR(hsotg->vbus_supply)) {
> + hsotg->vbus_supply = NULL;
> + return PTR_ERR(hsotg->vbus_supply);
> + }

my personal cluelessnes, but can you use PTR_ERR without checking for
IS_ERR first?

I would've expected something along the line of

if (IS_ERR(hsotg->vbus_supply)) {
if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
hsotg->vbus_supply = NULL;
return 0;
} else {
return PTR_ERR(hsotg->vbus_supply);
}
}

2018-03-22 13:36:46

[permalink] [raw]

Subject: Re: [PATCH v3] usb: dwc2: dwc2_vbus_supply_init: fix error check

On 03/22/2018 02:26 PM, Heiko Stübner wrote:
> Am Donnerstag, 22. März 2018, 14:14:51 CET schrieb Tomeu Vizoso:
>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>> there, so if that's the case we have to make sure not to leave -ENODEV
>> in the regulator pointer.
>>
>> Also, make sure we return 0 in that case, but correctly propagate any
>> other errors. Also propagate the error from _dwc2_hcd_start.
>>
>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>> supply") Cc: Amelie Delaunay <[email protected]>
>> Signed-off-by: Tomeu Vizoso <[email protected]>
>>
>> ---
>>
>> v2: Only overwrite the error in the pointer after checking it (Heiko
>> Stübner <[email protected]>)
>> v3: Remove hunks that shouldn't be in this patch
>> ---
>> drivers/usb/dwc2/hcd.c | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index dcfda5eb4cac..863aed20517f 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -359,8 +359,13 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>> {
>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>> - if (IS_ERR(hsotg->vbus_supply))
>> + if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
>> + hsotg->vbus_supply = NULL;
>> return 0;
>> + } else if (IS_ERR(hsotg->vbus_supply)) {
>> + hsotg->vbus_supply = NULL;
>> + return PTR_ERR(hsotg->vbus_supply);
>> + }
>
> my personal cluelessnes, but can you use PTR_ERR without checking for
> IS_ERR first?

It's just a cast, so it should be fine.

> I would've expected something along the line of
>
> if (IS_ERR(hsotg->vbus_supply)) {
> if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
> hsotg->vbus_supply = NULL;
> return 0;
> } else {
> return PTR_ERR(hsotg->vbus_supply);
> }
> }

I kind of liked to avoid one indentation level. Also wanted to play safe
and NULLify the pointer in both code paths.

Thanks,

Tomeu

2018-03-24 00:26:34

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v3] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi Tomeu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on balbi-usb/next]
[also build test WARNING on next-20180323]
[cannot apply to v4.16-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Tomeu-Vizoso/usb-dwc2-dwc2_vbus_supply_init-fix-error-check/20180324-063752
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next

coccinelle warnings: (new ones prefixed by >>)

>> drivers/usb/dwc2/hcd.c:367:9-16: ERROR: PTR_ERR applied after initialization to constant on line 366

vim +367 drivers/usb/dwc2/hcd.c

358
359 static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
360 {
361 hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
362 if (PTR_ERR(hsotg->vbus_supply) == -ENODEV) {
363 hsotg->vbus_supply = NULL;
364 return 0;
365 } else if (IS_ERR(hsotg->vbus_supply)) {
> 366 hsotg->vbus_supply = NULL;
> 367 return PTR_ERR(hsotg->vbus_supply);
368 }
369
370 return regulator_enable(hsotg->vbus_supply);
371 }
372

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

2018-03-26 09:02:52

[permalink] [raw]

Subject: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-03-26 10:57:36

by Amelie Delaunay

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi,

On 03/26/2018 11:00 AM, Tomeu Vizoso wrote:
> devm_regulator_get_optional returns -ENODEV if the regulator isn't
> there, so if that's the case we have to make sure not to leave -ENODEV
> in the regulator pointer.
>
> Also, make sure we return 0 in that case, but correctly propagate any
> other errors. Also propagate the error from _dwc2_hcd_start.
>
> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
> Cc: Amelie Delaunay <[email protected]>
> Signed-off-by: Tomeu Vizoso <[email protected]>
>
> ---
>
> v2: Only overwrite the error in the pointer after checking it (Heiko
> Stübner <[email protected]>)
> v3: Remove hunks that shouldn't be in this patch
> v4: Don't overwrite the error code before returning it (kbuild test
> robot <[email protected]>)
> ---
> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 190f95964000..c51b73b3e048 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>
> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
> {
> + int ret;
> +
> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
> - if (IS_ERR(hsotg->vbus_supply))
> - return 0;
> + if (IS_ERR(hsotg->vbus_supply)) {
> + ret = PTR_ERR(hsotg->vbus_supply);
> + hsotg->vbus_supply = NULL;
> + return ret == -ENODEV ? 0 : ret;
> + }
>
> return regulator_enable(hsotg->vbus_supply);
> }
> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
> - dwc2_vbus_supply_init(hsotg);
> -
> - return 0;
> + return dwc2_vbus_supply_init(hsotg);
> }
>
> /*
>

Reviewed-by: Amelie Delaunay <[email protected]>

Thanks,
Amelie

2018-03-26 11:52:28

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

2018-04-04 13:35:04

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Could this patch be picked up, please?

Thanks,

Tomeu

On 26 March 2018 at 13:51, Heiko Stübner <[email protected]> wrote:
> Am Montag, 26. März 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>> there, so if that's the case we have to make sure not to leave -ENODEV
>> in the regulator pointer.
>>
>> Also, make sure we return 0 in that case, but correctly propagate any
>> other errors. Also propagate the error from _dwc2_hcd_start.
>>
>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>> supply") Cc: Amelie Delaunay <[email protected]>
>> Signed-off-by: Tomeu Vizoso <[email protected]>
>
> Reviewed-by: Heiko Stuebner <[email protected]>

2018-04-05 07:56:18

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi Tomeu,

On 3/26/2018 1:01 PM, Tomeu Vizoso wrote:
> devm_regulator_get_optional returns -ENODEV if the regulator isn't
> there, so if that's the case we have to make sure not to leave -ENODEV
> in the regulator pointer.
>
> Also, make sure we return 0 in that case, but correctly propagate any
> other errors. Also propagate the error from _dwc2_hcd_start.
>
> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
> Cc: Amelie Delaunay <[email protected]>
> Signed-off-by: Tomeu Vizoso <[email protected]>
>
> ---
>
> v2: Only overwrite the error in the pointer after checking it (Heiko
> St?bner <[email protected]>)
> v3: Remove hunks that shouldn't be in this patch
> v4: Don't overwrite the error code before returning it (kbuild test
> robot <[email protected]>)
> ---
> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 190f95964000..c51b73b3e048 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>
> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
> {
> + int ret;
> +
> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
> - if (IS_ERR(hsotg->vbus_supply))
> - return 0;
> + if (IS_ERR(hsotg->vbus_supply)) {
> + ret = PTR_ERR(hsotg->vbus_supply);
> + hsotg->vbus_supply = NULL;
> + return ret == -ENODEV ? 0 : ret;
> + }
>
> return regulator_enable(hsotg->vbus_supply);
> }
> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
> - dwc2_vbus_supply_init(hsotg);
> -
> - return 0;
> + return dwc2_vbus_supply_init(hsotg);
> }
>
> /*
>

Not able to apply patch. Please rebase to balbi/next

Thanks,
Minas

2018-04-05 08:12:23

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi Minas,

On 04/05/2018 09:54 AM, Minas Harutyunyan wrote:
> Hi Tomeu,
>
> On 3/26/2018 1:01 PM, Tomeu Vizoso wrote:
>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>> there, so if that's the case we have to make sure not to leave -ENODEV
>> in the regulator pointer.
>>
>> Also, make sure we return 0 in that case, but correctly propagate any
>> other errors. Also propagate the error from _dwc2_hcd_start.
>>
>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
>> Cc: Amelie Delaunay <[email protected]>
>> Signed-off-by: Tomeu Vizoso <[email protected]>
>>
>> ---
>>
>> v2: Only overwrite the error in the pointer after checking it (Heiko
>> Stübner <[email protected]>)
>> v3: Remove hunks that shouldn't be in this patch
>> v4: Don't overwrite the error code before returning it (kbuild test
>> robot <[email protected]>)
>> ---
>> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 190f95964000..c51b73b3e048 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>>
>> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>> {
>> + int ret;
>> +
>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>> - if (IS_ERR(hsotg->vbus_supply))
>> - return 0;
>> + if (IS_ERR(hsotg->vbus_supply)) {
>> + ret = PTR_ERR(hsotg->vbus_supply);
>> + hsotg->vbus_supply = NULL;
>> + return ret == -ENODEV ? 0 : ret;
>> + }
>>
>> return regulator_enable(hsotg->vbus_supply);
>> }
>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>
>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> - dwc2_vbus_supply_init(hsotg);
>> -
>> - return 0;
>> + return dwc2_vbus_supply_init(hsotg);
>> }
>>
>> /*
>>
>
> Not able to apply patch. Please rebase to balbi/next

Are you sure? Just rebased and the resulting patch is identical to what
was sent.

Thanks,

Tomeu

2018-04-05 09:00:34

by Grigor Tovmasyan

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

On 3/26/2018 1:01 PM, Tomeu Vizoso wrote:
> devm_regulator_get_optional returns -ENODEV if the regulator isn't
> there, so if that's the case we have to make sure not to leave -ENODEV
> in the regulator pointer.
>
> Also, make sure we return 0 in that case, but correctly propagate any
> other errors. Also propagate the error from _dwc2_hcd_start.
>
> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
> Cc: Amelie Delaunay <[email protected]>
> Signed-off-by: Tomeu Vizoso <[email protected]>
>
> ---
>
> v2: Only overwrite the error in the pointer after checking it (Heiko
> St?bner <[email protected]>)
> v3: Remove hunks that shouldn't be in this patch
> v4: Don't overwrite the error code before returning it (kbuild test
> robot <[email protected]>)
> ---
> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 190f95964000..c51b73b3e048 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>
> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
> {
> + int ret;
> +
> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
> - if (IS_ERR(hsotg->vbus_supply))
> - return 0;
> + if (IS_ERR(hsotg->vbus_supply)) {
> + ret = PTR_ERR(hsotg->vbus_supply);
> + hsotg->vbus_supply = NULL;
> + return ret == -ENODEV ? 0 : ret;
> + }
>
> return regulator_enable(hsotg->vbus_supply);
> }
> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
> - dwc2_vbus_supply_init(hsotg);
> -
> - return 0;
> + return dwc2_vbus_supply_init(hsotg);
> }
>
> /*
>

Reviewed-by: Grigor Tovmasyan <[email protected]>

2018-04-05 09:03:39

by Grigor Tovmasyan

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

On 4/5/2018 12:11 PM, Tomeu Vizoso wrote:
> Hi Minas,
>
> On 04/05/2018 09:54 AM, Minas Harutyunyan wrote:
>> Hi Tomeu,
>>
>> On 3/26/2018 1:01 PM, Tomeu Vizoso wrote:
>>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>>> there, so if that's the case we have to make sure not to leave -ENODEV
>>> in the regulator pointer.
>>>
>>> Also, make sure we return 0 in that case, but correctly propagate any
>>> other errors. Also propagate the error from _dwc2_hcd_start.
>>>
>>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
>>> Cc: Amelie Delaunay <[email protected]>
>>> Signed-off-by: Tomeu Vizoso <[email protected]>
>>>
>>> ---
>>>
>>> v2: Only overwrite the error in the pointer after checking it (Heiko
>>> St?bner <[email protected]>)
>>> v3: Remove hunks that shouldn't be in this patch
>>> v4: Don't overwrite the error code before returning it (kbuild test
>>> robot <[email protected]>)
>>> ---
>>> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>> index 190f95964000..c51b73b3e048 100644
>>> --- a/drivers/usb/dwc2/hcd.c
>>> +++ b/drivers/usb/dwc2/hcd.c
>>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>>>
>>> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>>> {
>>> + int ret;
>>> +
>>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>>> - if (IS_ERR(hsotg->vbus_supply))
>>> - return 0;
>>> + if (IS_ERR(hsotg->vbus_supply)) {
>>> + ret = PTR_ERR(hsotg->vbus_supply);
>>> + hsotg->vbus_supply = NULL;
>>> + return ret == -ENODEV ? 0 : ret;
>>> + }
>>>
>>> return regulator_enable(hsotg->vbus_supply);
>>> }
>>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>>
>>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>>
>>> - dwc2_vbus_supply_init(hsotg);
>>> -
>>> - return 0;
>>> + return dwc2_vbus_supply_init(hsotg);
>>> }
>>>
>>> /*
>>>
>>
>> Not able to apply patch. Please rebase to balbi/next
>
> Are you sure? Just rebased and the resulting patch is identical to what
> was sent.

Sorry for inconvenience. We fixed the problem.
No need to rebase.

Thanks,
Grigor.

>
> Thanks,
>
> Tomeu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=K1ULVL1slpLXpMJJlAXSOxws4tRq0IkTBqxDkyW2hUQ&m=q-x0UcPJc6m4O6WQ6ZklCRyxp1ZT8QOQDRuaA_B6m6c&s=F7VAPO-hcO1TMfvPHhEkJYgX-8exTHxrioO3C6oNubs&e=
>

2018-04-05 09:53:13

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

On 4/5/2018 12:59 PM, Grigor Tovmasyan wrote:
> On 3/26/2018 1:01 PM, Tomeu Vizoso wrote:
>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>> there, so if that's the case we have to make sure not to leave -ENODEV
>> in the regulator pointer.
>>
>> Also, make sure we return 0 in that case, but correctly propagate any
>> other errors. Also propagate the error from _dwc2_hcd_start.
>>
>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
>> Cc: Amelie Delaunay <[email protected]>
>> Signed-off-by: Tomeu Vizoso <[email protected]>
>>
>> ---
>>
>> v2: Only overwrite the error in the pointer after checking it (Heiko
>> St?bner <[email protected]>)
>> v3: Remove hunks that shouldn't be in this patch
>> v4: Don't overwrite the error code before returning it (kbuild test
>> robot <[email protected]>)
>> ---
>> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 190f95964000..c51b73b3e048 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>>
>> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>> {
>> + int ret;
>> +
>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>> - if (IS_ERR(hsotg->vbus_supply))
>> - return 0;
>> + if (IS_ERR(hsotg->vbus_supply)) {
>> + ret = PTR_ERR(hsotg->vbus_supply);
>> + hsotg->vbus_supply = NULL;
>> + return ret == -ENODEV ? 0 : ret;
>> + }
>>
>> return regulator_enable(hsotg->vbus_supply);
>> }
>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>
>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> - dwc2_vbus_supply_init(hsotg);
>> -
>> - return 0;
>> + return dwc2_vbus_supply_init(hsotg);
>> }
>>
>> /*
>>
>
> Reviewed-by: Grigor Tovmasyan <[email protected]>
>

Acked-by: Minas Harutyunyan <[email protected]>

2018-04-10 12:35:34

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Am Montag, 26. M?rz 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
> devm_regulator_get_optional returns -ENODEV if the regulator isn't
> there, so if that's the case we have to make sure not to leave -ENODEV
> in the regulator pointer.
>
> Also, make sure we return 0 in that case, but correctly propagate any
> other errors. Also propagate the error from _dwc2_hcd_start.
>
> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
> Cc: Amelie Delaunay <[email protected]>
> Signed-off-by: Tomeu Vizoso <[email protected]>

The patch that gets fixed here also breaks display-output on dwc2-based
Rockchip devices (likely even more), probably due to making the regulator
framework hickup.

With this patch applied, apart from not seeing the NULL-ptr, I also get
display output on my rk3288-veycron Chromebook again, so

Tested-by: Heiko Stuebner <[email protected]>

> v2: Only overwrite the error in the pointer after checking it (Heiko
> St?bner <[email protected]>)
> v3: Remove hunks that shouldn't be in this patch
> v4: Don't overwrite the error code before returning it (kbuild test
> robot <[email protected]>)
> ---
> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 190f95964000..c51b73b3e048 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>
> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
> {
> + int ret;
> +
> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
> - if (IS_ERR(hsotg->vbus_supply))
> - return 0;
> + if (IS_ERR(hsotg->vbus_supply)) {
> + ret = PTR_ERR(hsotg->vbus_supply);
> + hsotg->vbus_supply = NULL;
> + return ret == -ENODEV ? 0 : ret;
> + }
>
> return regulator_enable(hsotg->vbus_supply);
> }
> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>
> spin_unlock_irqrestore(&hsotg->lock, flags);
>
> - dwc2_vbus_supply_init(hsotg);
> -
> - return 0;
> + return dwc2_vbus_supply_init(hsotg);
> }
>
> /*
>

2018-04-10 13:57:36

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi Heiko,

On 4/10/2018 4:28 PM, Heiko Stuebner wrote:
> Am Montag, 26. M?rz 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>> there, so if that's the case we have to make sure not to leave -ENODEV
>> in the regulator pointer.
>>
>> Also, make sure we return 0 in that case, but correctly propagate any
>> other errors. Also propagate the error from _dwc2_hcd_start.
>>
>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus supply")
>> Cc: Amelie Delaunay <[email protected]>
>> Signed-off-by: Tomeu Vizoso <[email protected]>
>
> The patch that gets fixed here also breaks display-output on dwc2-based
> Rockchip devices (likely even more), probably due to making the regulator
> framework hickup.
>
Could you please elaborate what mean "breaks display-output".
On which Kernel version you apply this patch?

Thanks,
Minas

> With this patch applied, apart from not seeing the NULL-ptr, I also get
> display output on my rk3288-veycron Chromebook again, so
>
> Tested-by: Heiko Stuebner <[email protected]>
>
>
>> v2: Only overwrite the error in the pointer after checking it (Heiko
>> St?bner <[email protected]>)
>> v3: Remove hunks that shouldn't be in this patch
>> v4: Don't overwrite the error code before returning it (kbuild test
>> robot <[email protected]>)
>> ---
>> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 190f95964000..c51b73b3e048 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
>>
>> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>> {
>> + int ret;
>> +
>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>> - if (IS_ERR(hsotg->vbus_supply))
>> - return 0;
>> + if (IS_ERR(hsotg->vbus_supply)) {
>> + ret = PTR_ERR(hsotg->vbus_supply);
>> + hsotg->vbus_supply = NULL;
>> + return ret == -ENODEV ? 0 : ret;
>> + }
>>
>> return regulator_enable(hsotg->vbus_supply);
>> }
>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>
>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> - dwc2_vbus_supply_init(hsotg);
>> -
>> - return 0;
>> + return dwc2_vbus_supply_init(hsotg);
>> }
>>
>> /*
>>
>
>
>

2018-04-10 15:42:08

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Am Dienstag, 10. April 2018, 15:52:25 CEST schrieb Minas Harutyunyan:
> Hi Heiko,
>
> On 4/10/2018 4:28 PM, Heiko Stuebner wrote:
> > Am Montag, 26. M?rz 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
> >> devm_regulator_get_optional returns -ENODEV if the regulator isn't
> >> there, so if that's the case we have to make sure not to leave -ENODEV
> >> in the regulator pointer.
> >>
> >> Also, make sure we return 0 in that case, but correctly propagate any
> >> other errors. Also propagate the error from _dwc2_hcd_start.
> >>
> >> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
> >> supply") Cc: Amelie Delaunay <[email protected]>
> >> Signed-off-by: Tomeu Vizoso <[email protected]>
> >
> > The patch that gets fixed here also breaks display-output on dwc2-based
> > Rockchip devices (likely even more), probably due to making the regulator
> > framework hickup.
>
> Could you please elaborate what mean "breaks display-output".
> On which Kernel version you apply this patch?

I think I may have written that poorly. _Without_ this patch I get
display breakage on the most recent torvalds/master (merge-window)
where "usb: dwc2: add support for host mode external vbus supply" is
applied and this patch fixes the issue.

"breaks display output" means both hdmi + edp output are missing
also including the backlight staying off.

The patch we're fixing here, causes a null-pointer dereference in the
regulator framework, which seems to also cause issues when other
regulators are enabled, which I think is what I'm seeing here.

Heiko

>
> Thanks,
> Minas
>
> > With this patch applied, apart from not seeing the NULL-ptr, I also get
> > display output on my rk3288-veycron Chromebook again, so
> >
> > Tested-by: Heiko Stuebner <[email protected]>
> >
> >> v2: Only overwrite the error in the pointer after checking it (Heiko
> >>
> >> St?bner <[email protected]>)
> >>
> >> v3: Remove hunks that shouldn't be in this patch
> >> v4: Don't overwrite the error code before returning it (kbuild test
> >>
> >> robot <[email protected]>)
> >>
> >> ---
> >>
> >> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
> >> 1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> >> index 190f95964000..c51b73b3e048 100644
> >> --- a/drivers/usb/dwc2/hcd.c
> >> +++ b/drivers/usb/dwc2/hcd.c
> >> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg
> >> *hsotg)>>
> >> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
> >> {
> >>
> >> + int ret;
> >> +
> >>
> >> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
> >>
> >> - if (IS_ERR(hsotg->vbus_supply))
> >> - return 0;
> >> + if (IS_ERR(hsotg->vbus_supply)) {
> >> + ret = PTR_ERR(hsotg->vbus_supply);
> >> + hsotg->vbus_supply = NULL;
> >> + return ret == -ENODEV ? 0 : ret;
> >> + }
> >>
> >> return regulator_enable(hsotg->vbus_supply);
> >>
> >> }
> >>
> >> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
> >>
> >> spin_unlock_irqrestore(&hsotg->lock, flags);
> >>
> >> - dwc2_vbus_supply_init(hsotg);
> >> -
> >> - return 0;
> >> + return dwc2_vbus_supply_init(hsotg);
> >>
> >> }
> >>
> >> /*

2018-04-11 06:54:23

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi Heiko,

On 4/10/2018 7:37 PM, Heiko St?bner wrote:
> Am Dienstag, 10. April 2018, 15:52:25 CEST schrieb Minas Harutyunyan:
>> Hi Heiko,
>>
>> On 4/10/2018 4:28 PM, Heiko Stuebner wrote:
>>> Am Montag, 26. M?rz 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
>>>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>>>> there, so if that's the case we have to make sure not to leave -ENODEV
>>>> in the regulator pointer.
>>>>
>>>> Also, make sure we return 0 in that case, but correctly propagate any
>>>> other errors. Also propagate the error from _dwc2_hcd_start.
>>>>
>>>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>>>> supply") Cc: Amelie Delaunay <[email protected]>
>>>> Signed-off-by: Tomeu Vizoso <[email protected]>
>>>
>>> The patch that gets fixed here also breaks display-output on dwc2-based
>>> Rockchip devices (likely even more), probably due to making the regulator
>>> framework hickup.
>>
>> Could you please elaborate what mean "breaks display-output".
>> On which Kernel version you apply this patch?
>
> I think I may have written that poorly. _Without_ this patch I get
> display breakage on the most recent torvalds/master (merge-window)
> where "usb: dwc2: add support for host mode external vbus supply" is
> applied and this patch fixes the issue.
>
> "breaks display output" means both hdmi + edp output are missing
> also including the backlight staying off.
>
> The patch we're fixing here, causes a null-pointer dereference in the
> regulator framework, which seems to also cause issues when other
> regulators are enabled, which I think is what I'm seeing here.
>
>
Thank you for clarification. Earlier you added "reviewed" tag, this
feedback can assumed as "tested" tag.

Thanks,
Minas

> Heiko
>
>>
>> Thanks,
>> Minas
>>
>>> With this patch applied, apart from not seeing the NULL-ptr, I also get
>>> display output on my rk3288-veycron Chromebook again, so
>>>
>>> Tested-by: Heiko Stuebner <[email protected]>
>>>
>>>> v2: Only overwrite the error in the pointer after checking it (Heiko
>>>>
>>>> St?bner <[email protected]>)
>>>>
>>>> v3: Remove hunks that shouldn't be in this patch
>>>> v4: Don't overwrite the error code before returning it (kbuild test
>>>>
>>>> robot <[email protected]>)
>>>>
>>>> ---
>>>>
>>>> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
>>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>> index 190f95964000..c51b73b3e048 100644
>>>> --- a/drivers/usb/dwc2/hcd.c
>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg
>>>> *hsotg)>>
>>>> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>>>> {
>>>>
>>>> + int ret;
>>>> +
>>>>
>>>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>>>>
>>>> - if (IS_ERR(hsotg->vbus_supply))
>>>> - return 0;
>>>> + if (IS_ERR(hsotg->vbus_supply)) {
>>>> + ret = PTR_ERR(hsotg->vbus_supply);
>>>> + hsotg->vbus_supply = NULL;
>>>> + return ret == -ENODEV ? 0 : ret;
>>>> + }
>>>>
>>>> return regulator_enable(hsotg->vbus_supply);
>>>>
>>>> }
>>>>
>>>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>>>
>>>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>>>
>>>> - dwc2_vbus_supply_init(hsotg);
>>>> -
>>>> - return 0;
>>>> + return dwc2_vbus_supply_init(hsotg);
>>>>
>>>> }
>>>>
>>>> /*
>
>
>

2018-04-23 13:26:24

[permalink] [raw]

Subject: Re: [PATCH v4] usb: dwc2: dwc2_vbus_supply_init: fix error check

Hi,

could this patch be picked up, please? Or if for some reason it cannot
be, could the commit that introduced the regression be reverted?

It's causing some tests in KernelCI to fail:

https://storage.kernelci.org/next/master/next-20180423/arm/multi_v7_defconfig/lab-collabora/sleep-rk3288-veyron-jaq.html

Thanks,

Tomeu

On 11 April 2018 at 08:50, Minas Harutyunyan
<[email protected]> wrote:
> Hi Heiko,
>
> On 4/10/2018 7:37 PM, Heiko Stübner wrote:
>> Am Dienstag, 10. April 2018, 15:52:25 CEST schrieb Minas Harutyunyan:
>>> Hi Heiko,
>>>
>>> On 4/10/2018 4:28 PM, Heiko Stuebner wrote:
>>>> Am Montag, 26. März 2018, 11:00:01 CEST schrieb Tomeu Vizoso:
>>>>> devm_regulator_get_optional returns -ENODEV if the regulator isn't
>>>>> there, so if that's the case we have to make sure not to leave -ENODEV
>>>>> in the regulator pointer.
>>>>>
>>>>> Also, make sure we return 0 in that case, but correctly propagate any
>>>>> other errors. Also propagate the error from _dwc2_hcd_start.
>>>>>
>>>>> Fixes: 531ef5ebea96 ("usb: dwc2: add support for host mode external vbus
>>>>> supply") Cc: Amelie Delaunay <[email protected]>
>>>>> Signed-off-by: Tomeu Vizoso <[email protected]>
>>>>
>>>> The patch that gets fixed here also breaks display-output on dwc2-based
>>>> Rockchip devices (likely even more), probably due to making the regulator
>>>> framework hickup.
>>>
>>> Could you please elaborate what mean "breaks display-output".
>>> On which Kernel version you apply this patch?
>>
>> I think I may have written that poorly. _Without_ this patch I get
>> display breakage on the most recent torvalds/master (merge-window)
>> where "usb: dwc2: add support for host mode external vbus supply" is
>> applied and this patch fixes the issue.
>>
>> "breaks display output" means both hdmi + edp output are missing
>> also including the backlight staying off.
>>
>> The patch we're fixing here, causes a null-pointer dereference in the
>> regulator framework, which seems to also cause issues when other
>> regulators are enabled, which I think is what I'm seeing here.
>>
>>
> Thank you for clarification. Earlier you added "reviewed" tag, this
> feedback can assumed as "tested" tag.
>
> Thanks,
> Minas
>
>> Heiko
>>
>>>
>>> Thanks,
>>> Minas
>>>
>>>> With this patch applied, apart from not seeing the NULL-ptr, I also get
>>>> display output on my rk3288-veycron Chromebook again, so
>>>>
>>>> Tested-by: Heiko Stuebner <[email protected]>
>>>>
>>>>> v2: Only overwrite the error in the pointer after checking it (Heiko
>>>>>
>>>>> Stübner <[email protected]>)
>>>>>
>>>>> v3: Remove hunks that shouldn't be in this patch
>>>>> v4: Don't overwrite the error code before returning it (kbuild test
>>>>>
>>>>> robot <[email protected]>)
>>>>>
>>>>> ---
>>>>>
>>>>> drivers/usb/dwc2/hcd.c | 13 ++++++++-----
>>>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>>> index 190f95964000..c51b73b3e048 100644
>>>>> --- a/drivers/usb/dwc2/hcd.c
>>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>>> @@ -358,9 +358,14 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg
>>>>> *hsotg)>>
>>>>> static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
>>>>> {
>>>>>
>>>>> + int ret;
>>>>> +
>>>>>
>>>>> hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
>>>>>
>>>>> - if (IS_ERR(hsotg->vbus_supply))
>>>>> - return 0;
>>>>> + if (IS_ERR(hsotg->vbus_supply)) {
>>>>> + ret = PTR_ERR(hsotg->vbus_supply);
>>>>> + hsotg->vbus_supply = NULL;
>>>>> + return ret == -ENODEV ? 0 : ret;
>>>>> + }
>>>>>
>>>>> return regulator_enable(hsotg->vbus_supply);
>>>>>
>>>>> }
>>>>>
>>>>> @@ -4342,9 +4347,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
>>>>>
>>>>> spin_unlock_irqrestore(&hsotg->lock, flags);
>>>>>
>>>>> - dwc2_vbus_supply_init(hsotg);
>>>>> -
>>>>> - return 0;
>>>>> + return dwc2_vbus_supply_init(hsotg);
>>>>>
>>>>> }
>>>>>
>>>>> /*
>>
>>
>>
>

2018-04-24 14:27:25