[PATCH] usb: fotg210-hcd: Fix an error message

[PATCH] usb: fotg210-hcd: Fix an error message

[Christophe JAILLET]

'retval' is known to be -ENODEV here.
This is a hard-coded default error code which is not useful in the error
message. Moreover, another error message is printed at the end of the
error handling path. The corresponding error code (-ENOMEM) is more
informative.

So remove simplify the first error message.

While at it, also remove the useless initialization of 'retval'.

Fixes: 7d50195f6c50 ("usb: host: Faraday fotg210-hcd driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/usb/host/fotg210-hcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
index 6cac642520fc..9c2eda0918e1 100644
--- a/drivers/usb/host/fotg210-hcd.c
+++ b/drivers/usb/host/fotg210-hcd.c
@@ -5568,7 +5568,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
 	struct usb_hcd *hcd;
 	struct resource *res;
 	int irq;
-	int retval = -ENODEV;
+	int retval;
 	struct fotg210_hcd *fotg210;
 
 	if (usb_disabled())
@@ -5588,7 +5588,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
 	hcd = usb_create_hcd(&fotg210_fotg210_hc_driver, dev,
 			dev_name(dev));
 	if (!hcd) {
-		dev_err(dev, "failed to create hcd with err %d\n", retval);
+		dev_err(dev, "failed to create hcd\n");
 		retval = -ENOMEM;
 		goto fail_create_hcd;
 	}
-- 
2.30.2

Re: [PATCH] usb: fotg210-hcd: Fix an error message

[Chunfeng Yun]

On Thu, 2021-05-06 at 22:39 +0200, Christophe JAILLET wrote:
> 'retval' is known to be -ENODEV here.
> This is a hard-coded default error code which is not useful in the error
> message. Moreover, another error message is printed at the end of the
> error handling path. The corresponding error code (-ENOMEM) is more
> informative.
> 
> So remove simplify the first error message.
> 
> While at it, also remove the useless initialization of 'retval'.
> 
> Fixes: 7d50195f6c50 ("usb: host: Faraday fotg210-hcd driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/usb/host/fotg210-hcd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index 6cac642520fc..9c2eda0918e1 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -5568,7 +5568,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
>  	struct usb_hcd *hcd;
>  	struct resource *res;
>  	int irq;
> -	int retval = -ENODEV;
> +	int retval;
>  	struct fotg210_hcd *fotg210;
>  
>  	if (usb_disabled())
> @@ -5588,7 +5588,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
>  	hcd = usb_create_hcd(&fotg210_fotg210_hc_driver, dev,
>  			dev_name(dev));
>  	if (!hcd) {
> -		dev_err(dev, "failed to create hcd with err %d\n", retval);
> +		dev_err(dev, "failed to create hcd\n");
>  		retval = -ENOMEM;
How about moving this line before dev_err()? then could keep error log
unchanged.

>  		goto fail_create_hcd;
>  	}

Re: [PATCH] usb: fotg210-hcd: Fix an error message

[Christophe JAILLET]

Le 07/05/2021 à 04:20, Chunfeng Yun a écrit :
> On Thu, 2021-05-06 at 22:39 +0200, Christophe JAILLET wrote:
>> 'retval' is known to be -ENODEV here.
>> This is a hard-coded default error code which is not useful in the error
>> message. Moreover, another error message is printed at the end of the
>> error handling path. The corresponding error code (-ENOMEM) is more
>> informative.
>>
>> So remove simplify the first error message.
>>
>> While at it, also remove the useless initialization of 'retval'.
>>
>> Fixes: 7d50195f6c50 ("usb: host: Faraday fotg210-hcd driver")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/usb/host/fotg210-hcd.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
>> index 6cac642520fc..9c2eda0918e1 100644
>> --- a/drivers/usb/host/fotg210-hcd.c
>> +++ b/drivers/usb/host/fotg210-hcd.c
>> @@ -5568,7 +5568,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
>>   	struct usb_hcd *hcd;
>>   	struct resource *res;
>>   	int irq;
>> -	int retval = -ENODEV;
>> +	int retval;
>>   	struct fotg210_hcd *fotg210;
>>   
>>   	if (usb_disabled())
>> @@ -5588,7 +5588,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
>>   	hcd = usb_create_hcd(&fotg210_fotg210_hc_driver, dev,
>>   			dev_name(dev));
>>   	if (!hcd) {
>> -		dev_err(dev, "failed to create hcd with err %d\n", retval);
>> +		dev_err(dev, "failed to create hcd\n");
>>   		retval = -ENOMEM;
> How about moving this line before dev_err()? then could keep error log
> unchanged.
> 

Mostly a matter of taste.
I don't think it add any useful information (this is not something 
coming from a call chain or that can have different values, it is just a 
hard-coded constant) and the line after we will already have:
	dev_err(dev, "init %s fail, %d\n", dev_name(dev), retval);
where retval = -ENOMEM

So the -ENOMEM error code is already reported.

Moreover, having error code reported or not is already not consistent in 
the function. For example "failed to enable PCLK\n" where 'retval' could 
be reported as well.



BTW, is it useful to have 'dev_name(dev)' in a dev_err?

CJ

>>   		goto fail_create_hcd;
>>   	}
> 

Re: [PATCH] usb: fotg210-hcd: Fix an error message

[Chunfeng Yun]

On Fri, 2021-05-07 at 09:06 +0200, Christophe JAILLET wrote:
> Le 07/05/2021 à 04:20, Chunfeng Yun a écrit :
> > On Thu, 2021-05-06 at 22:39 +0200, Christophe JAILLET wrote:
> >> 'retval' is known to be -ENODEV here.
> >> This is a hard-coded default error code which is not useful in the error
> >> message. Moreover, another error message is printed at the end of the
> >> error handling path. The corresponding error code (-ENOMEM) is more
> >> informative.
> >>
> >> So remove simplify the first error message.
> >>
> >> While at it, also remove the useless initialization of 'retval'.
> >>
> >> Fixes: 7d50195f6c50 ("usb: host: Faraday fotg210-hcd driver")
> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >> ---
> >>   drivers/usb/host/fotg210-hcd.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> >> index 6cac642520fc..9c2eda0918e1 100644
> >> --- a/drivers/usb/host/fotg210-hcd.c
> >> +++ b/drivers/usb/host/fotg210-hcd.c
> >> @@ -5568,7 +5568,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
> >>   	struct usb_hcd *hcd;
> >>   	struct resource *res;
> >>   	int irq;
> >> -	int retval = -ENODEV;
> >> +	int retval;
> >>   	struct fotg210_hcd *fotg210;
> >>   
> >>   	if (usb_disabled())
> >> @@ -5588,7 +5588,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
> >>   	hcd = usb_create_hcd(&fotg210_fotg210_hc_driver, dev,
> >>   			dev_name(dev));
> >>   	if (!hcd) {
> >> -		dev_err(dev, "failed to create hcd with err %d\n", retval);
> >> +		dev_err(dev, "failed to create hcd\n");
> >>   		retval = -ENOMEM;
> > How about moving this line before dev_err()? then could keep error log
> > unchanged.
> > 
> 
> Mostly a matter of taste.
> I don't think it add any useful information (this is not something 
> coming from a call chain or that can have different values, it is just a 
> hard-coded constant) and the line after we will already have:
> 	dev_err(dev, "init %s fail, %d\n", dev_name(dev), retval);
> where retval = -ENOMEM
> 
> So the -ENOMEM error code is already reported.
Yes

> 
> Moreover, having error code reported or not is already not consistent in 
> the function. For example "failed to enable PCLK\n" where 'retval' could 
> be reported as well.
> 
> 
> 
> BTW, is it useful to have 'dev_name(dev)' in a dev_err?
dev_err(dev, ...) itself will print it.

> 
> CJ
> 
> >>   		goto fail_create_hcd;
> >>   	}
> > 
> 

Re: [PATCH] usb: fotg210-hcd: Fix an error message

[Dan Carpenter]

On Fri, May 07, 2021 at 10:20:30AM +0800, Chunfeng Yun wrote:
> On Thu, 2021-05-06 at 22:39 +0200, Christophe JAILLET wrote:
> > 'retval' is known to be -ENODEV here.
> > This is a hard-coded default error code which is not useful in the error
> > message. Moreover, another error message is printed at the end of the
> > error handling path. The corresponding error code (-ENOMEM) is more
> > informative.
> > 
> > So remove simplify the first error message.
> > 
> > While at it, also remove the useless initialization of 'retval'.
> > 
> > Fixes: 7d50195f6c50 ("usb: host: Faraday fotg210-hcd driver")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> >  drivers/usb/host/fotg210-hcd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> > index 6cac642520fc..9c2eda0918e1 100644
> > --- a/drivers/usb/host/fotg210-hcd.c
> > +++ b/drivers/usb/host/fotg210-hcd.c
> > @@ -5568,7 +5568,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
> >  	struct usb_hcd *hcd;
> >  	struct resource *res;
> >  	int irq;
> > -	int retval = -ENODEV;
> > +	int retval;
> >  	struct fotg210_hcd *fotg210;
> >  
> >  	if (usb_disabled())
> > @@ -5588,7 +5588,7 @@ static int fotg210_hcd_probe(struct platform_device *pdev)
> >  	hcd = usb_create_hcd(&fotg210_fotg210_hc_driver, dev,
> >  			dev_name(dev));
> >  	if (!hcd) {
> > -		dev_err(dev, "failed to create hcd with err %d\n", retval);
> > +		dev_err(dev, "failed to create hcd\n");
> >  		retval = -ENOMEM;
> How about moving this line before dev_err()? then could keep error log
> unchanged.

Then the error message would print misleading information.  The
usb_create_hcd() does not return -ENOMEM, it returns NULL.

regards,
dan carpenter