[PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[Rickard Strandqvist]

Variable ar assigned a value that is never used.
I have also removed all the code that thereby serves no purpose.

This was found using a static code analysis program called cppcheck

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/staging/emxx_udc/emxx_udc.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
index eb178fc..b916fab 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -2974,10 +2974,10 @@ static int nbu2ss_ep_fifo_status(struct usb_ep *_ep)
 	spin_lock_irqsave(&udc->lock, flags);
 
 	if (ep->epnum == 0) {
-		data = _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
+		_nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
 
 	} else {
-		data = _nbu2ss_readl(&preg->EP_REGS[ep->epnum-1].EP_LEN_DCNT)
+		_nbu2ss_readl(&preg->EP_REGS[ep->epnum-1].EP_LEN_DCNT)
 			& EPn_LDATA;
 	}
 
@@ -3264,12 +3264,11 @@ static void __init nbu2ss_drv_set_ep_info(
 	if (isdigit(name[2])) {
 
 		long	num;
-		int	res;
 		char	tempbuf[2];
 
 		tempbuf[0] = name[2];
 		tempbuf[1] = '\0';
-		res = kstrtol(tempbuf, 16, &num);
+		kstrtol(tempbuf, 16, &num);
 
 		if (num == 0)
 			ep->ep.maxpacket = EP0_PACKETSIZE;
-- 
1.7.10.4

Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[Chris Rorvick]

On Wed, Jan 28, 2015 at 4:42 PM, Rickard Strandqvist
<rickard_strandqvist@spectrumdigital.se> wrote:
> Variable ar assigned a value that is never used.
> I have also removed all the code that thereby serves no purpose.

Each of these changes adds a warning ...

> diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
> index eb178fc..b916fab 100644
> --- a/drivers/staging/emxx_udc/emxx_udc.c
> +++ b/drivers/staging/emxx_udc/emxx_udc.c
> @@ -2974,10 +2974,10 @@ static int nbu2ss_ep_fifo_status(struct usb_ep *_ep)
>         spin_lock_irqsave(&udc->lock, flags);
>
>         if (ep->epnum == 0) {
> -               data = _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
> +               _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;

.../linux/drivers/staging/emxx_udc/emxx_udc.c:2977:36: warning: value
computed is not used [-Wunused-value]
   _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
                                    ^

>         } else {
> -               data = _nbu2ss_readl(&preg->EP_REGS[ep->epnum-1].EP_LEN_DCNT)
> +               _nbu2ss_readl(&preg->EP_REGS[ep->epnum-1].EP_LEN_DCNT)
>                         & EPn_LDATA;

.../linux/drivers/staging/emxx_udc/emxx_udc.c:2981:4: warning: value
computed is not used [-Wunused-value]
    & EPn_LDATA;
    ^

>         }
>
> @@ -3264,12 +3264,11 @@ static void __init nbu2ss_drv_set_ep_info(
>         if (isdigit(name[2])) {
>
>                 long    num;
> -               int     res;
>                 char    tempbuf[2];
>
>                 tempbuf[0] = name[2];
>                 tempbuf[1] = '\0';
> -               res = kstrtol(tempbuf, 16, &num);
> +               kstrtol(tempbuf, 16, &num);

.../linux/drivers/staging/emxx_udc/emxx_udc.c:3271:3: warning:
ignoring return value of ‘kstrtol’, declared with attribute
warn_unused_result [-Wunused-result]
   kstrtol(tempbuf, 16, &num);
   ^

Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[Rickard Strandqvist]

2015-01-29 3:37 GMT+01:00 Chris Rorvick <chris@rorvick.com>:
> On Wed, Jan 28, 2015 at 4:42 PM, Rickard Strandqvist
> <rickard_strandqvist@spectrumdigital.se> wrote:
>> Variable ar assigned a value that is never used.
>> I have also removed all the code that thereby serves no purpose.
>
> Each of these changes adds a warning ...
>
>> diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
>> index eb178fc..b916fab 100644
>> --- a/drivers/staging/emxx_udc/emxx_udc.c
>> +++ b/drivers/staging/emxx_udc/emxx_udc.c
>> @@ -2974,10 +2974,10 @@ static int nbu2ss_ep_fifo_status(struct usb_ep *_ep)
>>         spin_lock_irqsave(&udc->lock, flags);
>>
>>         if (ep->epnum == 0) {
>> -               data = _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
>> +               _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
>
> .../linux/drivers/staging/emxx_udc/emxx_udc.c:2977:36: warning: value
> computed is not used [-Wunused-value]
>    _nbu2ss_readl(&preg->EP0_LENGTH) & EP0_LDATA;
>                                     ^
>
>>         } else {
>> -               data = _nbu2ss_readl(&preg->EP_REGS[ep->epnum-1].EP_LEN_DCNT)
>> +               _nbu2ss_readl(&preg->EP_REGS[ep->epnum-1].EP_LEN_DCNT)
>>                         & EPn_LDATA;
>
> .../linux/drivers/staging/emxx_udc/emxx_udc.c:2981:4: warning: value
> computed is not used [-Wunused-value]
>     & EPn_LDATA;
>     ^
>
>>         }
>>
>> @@ -3264,12 +3264,11 @@ static void __init nbu2ss_drv_set_ep_info(
>>         if (isdigit(name[2])) {
>>
>>                 long    num;
>> -               int     res;
>>                 char    tempbuf[2];
>>
>>                 tempbuf[0] = name[2];
>>                 tempbuf[1] = '\0';
>> -               res = kstrtol(tempbuf, 16, &num);
>> +               kstrtol(tempbuf, 16, &num);
>
> .../linux/drivers/staging/emxx_udc/emxx_udc.c:3271:3: warning:
> ignoring return value of ‘kstrtol’, declared with attribute
> warn_unused_result [-Wunused-result]
>    kstrtol(tempbuf, 16, &num);
>    ^

Hi

The first two were my fault, stupid of me to let the "& number" remain :(

The last one was more interesting, se below.
But I can not really see how any error should be handled here?
Proposal to change to:

  if (kstrtol(tempbuf, 16, &num) == 0 && num == 0)


static inline int __must_check kstrtol(const char *s, unsigned int
base, long *res);

"The __must_check annotation makes use of the gcc warn_unused_result
attribute; it first found its way into the mainline in 2.6.8. If a
function is marked __must_check, the compiler will issue a strong
warning whenever the function is called and its return code is unused.
"


Kind regards
Rickard Strandqvist

Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[Chris Rorvick]

On Thu, Jan 29, 2015 at 3:52 PM, Rickard Strandqvist
<rickard_strandqvist@spectrumdigital.se> wrote:
> The last one was more interesting, se below.
> But I can not really see how any error should be handled here?
> Proposal to change to:
>
>   if (kstrtol(tempbuf, 16, &num) == 0 && num == 0)

That whole chunk of code looks odd.  Why the base 16 conversion when
we already know it's a decimal digit?  Seems like this would work
without the hassle of the string conversion:

-- >8 --

--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -3262,16 +3262,7 @@ static void __init nbu2ss_drv_set_ep_info(
        ep->ep.ops = &nbu2ss_ep_ops;

        if (isdigit(name[2])) {
-
-               long    num;
-               int     res;
-               char    tempbuf[2];
-
-               tempbuf[0] = name[2];
-               tempbuf[1] = '\0';
-               res = kstrtol(tempbuf, 16, &num);
-
-               if (num == 0)
+               if (name[2] == '0')
                        ep->ep.maxpacket = EP0_PACKETSIZE;
                else
                        ep->ep.maxpacket = EP_PACKETSIZE;

Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[Chris Rorvick]

On Thu, Jan 29, 2015 at 5:46 PM, Chris Rorvick <chris@rorvick.com> wrote:
> That whole chunk of code looks odd.  Why the base 16 conversion when
> we already know it's a decimal digit?  Seems like this would work
> without the hassle of the string conversion:

Hmm, or probably even better to do this where the ep0 check is less contorted.

-- >8 --

diff --git a/drivers/staging/emxx_udc/emxx_udc.c
b/drivers/staging/emxx_udc/emxx_udc.c
index eb178fc..98a1ace 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -3261,25 +3261,6 @@ static void __init nbu2ss_drv_set_ep_info(
        ep->ep.name = name;
        ep->ep.ops = &nbu2ss_ep_ops;

-       if (isdigit(name[2])) {
-
-               long    num;
-               int     res;
-               char    tempbuf[2];
-
-               tempbuf[0] = name[2];
-               tempbuf[1] = '\0';
-               res = kstrtol(tempbuf, 16, &num);
-
-               if (num == 0)
-                       ep->ep.maxpacket = EP0_PACKETSIZE;
-               else
-                       ep->ep.maxpacket = EP_PACKETSIZE;
-
-       } else {
-               ep->ep.maxpacket = EP_PACKETSIZE;
-       }
-
        list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
        INIT_LIST_HEAD(&ep->queue);
 }
@@ -3293,8 +3274,12 @@ static void __init nbu2ss_drv_ep_init(struct
nbu2ss_udc *udc)
        udc->gadget.ep0 = &udc->ep[0].ep;


-       for (i = 0; i < NUM_ENDPOINTS; i++)
-               nbu2ss_drv_set_ep_info(udc, &udc->ep[i], gp_ep_name[i]);
+       for (i = 0; i < NUM_ENDPOINTS; i++) {
+               struct nbu2ss_ep *ep = &udc->ep[i];
+
+               nbu2ss_drv_set_ep_info(udc, ep, gp_ep_name[i]);
+               ep->ep.maxpacket = i == 0 ? EP0_PACKETSIZE : EP_PACKETSIZE;
+       }

        list_del_init(&udc->ep[0].ep.ep_list);
 }

Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[Dan Carpenter]

Yes.  Please send that as a patch which can be applied.

regards,
dan carpenter

Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[Rickard Strandqvist]

2015-01-30 15:20 GMT+01:00 Dan Carpenter <dan.carpenter@oracle.com>:
> Yes.  Please send that as a patch which can be applied.
>
> regards,
> dan carpenter
>


Hi

Okay, I'll do it this weekend.
Or do you do it Chris?

Kind regards
Rickard Strandqvist

Re: [PATCH] staging: emxx_udc: emxx_udc: Removed variables that is never used

[Chris Rorvick]

On Fri, Jan 30, 2015 at 11:35 AM, Rickard Strandqvist
<rickard_strandqvist@spectrumdigital.se> wrote:
> 2015-01-30 15:20 GMT+01:00 Dan Carpenter <dan.carpenter@oracle.com>:
>> Yes.  Please send that as a patch which can be applied.
>>
>> regards,
>> dan carpenter
>>
>
>
> Hi
>
> Okay, I'll do it this weekend.
> Or do you do it Chris?

I'll send out a patch removing nbu2ss_drv_set_dp_info() in a minute.
This only inflates the init function to a few more than 20 lines while
making the code far easier to understand.

Regards,

Chris