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

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

[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 <amelie.delaunay@st.com>
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

v2: Only overwrite the error in the pointer after checking it (Heiko
    Stübner <heiko@sntech.de>)
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);
+	}
 
 	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);
 }
 
 /*

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

[Heiko Stuebner]

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 <amelie.delaunay@st.com>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> 
> ---
> 
> v2: Only overwrite the error in the pointer after checking it (Heiko
>     Stübner <heiko@sntech.de>)
> 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);
	}
}
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tomeu Vizoso]

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 <amelie.delaunay@st.com>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>
>> ---
>>
>> v2: Only overwrite the error in the pointer after checking it (Heiko
>>      Stübner <heiko@sntech.de>)
>> 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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[kbuild test robot]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html