[PATCH] usb: misc: add a missing continue and refactor code

[PATCH] usb: misc: add a missing continue and refactor code

[Gustavo A. R. Silva]

Code refactoring to make the flow easier to follow and add missing
'continue' for case USB_ENDPOINT_XFER_INT.

Addresses-Coverity-ID: 1248733
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/usb/misc/usbtest.c | 50 +++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 3525626..8723e33 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,6 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct usbtest_dev *test)
 
 /*-------------------------------------------------------------------------*/
 
+static inline void try_intr(struct usb_host_endpoint *e,
+			    struct usb_host_endpoint *int_in,
+			    struct usb_host_endpoint *int_out)
+{
+	if (usb_endpoint_dir_in(&e->desc)) {
+		if (!int_in)
+			int_in = e;
+	} else {
+		if (!int_out)
+			int_out = e;
+	}
+}
+
+static inline void try_iso(struct usb_host_endpoint *e,
+			   struct usb_host_endpoint *iso_in,
+			   struct usb_host_endpoint *iso_out)
+{
+	if (usb_endpoint_dir_in(&e->desc)) {
+		if (!iso_in)
+			iso_in = e;
+	} else {
+		if (!iso_out)
+			iso_out = e;
+	}
+}
+
 static int
 get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 {
@@ -158,11 +184,12 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 				break;
 			case USB_ENDPOINT_XFER_INT:
 				if (dev->info->intr)
-					goto try_intr;
+					try_intr(e, int_in, int_out);
+				continue;
 			case USB_ENDPOINT_XFER_ISOC:
 				if (dev->info->iso)
-					goto try_iso;
-				/* FALLTHROUGH */
+					try_iso(e, iso_in, iso_out);
+				/* fall through */
 			default:
 				continue;
 			}
@@ -174,23 +201,6 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 					out = e;
 			}
 			continue;
-try_intr:
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!int_in)
-					int_in = e;
-			} else {
-				if (!int_out)
-					int_out = e;
-			}
-			continue;
-try_iso:
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!iso_in)
-					iso_in = e;
-			} else {
-				if (!iso_out)
-					iso_out = e;
-			}
 		}
 		if ((in && out)  ||  iso_in || iso_out || int_in || int_out)
 			goto found;
-- 
2.5.0

Re: [PATCH] usb: misc: add a missing continue and refactor code

[Alan Stern]

On Tue, 21 Feb 2017, Gustavo A. R. Silva wrote:

> Code refactoring to make the flow easier to follow and add missing
> 'continue' for case USB_ENDPOINT_XFER_INT.
> 
> Addresses-Coverity-ID: 1248733
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/usb/misc/usbtest.c | 50 +++++++++++++++++++++++++++-------------------
>  1 file changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> index 3525626..8723e33 100644
> --- a/drivers/usb/misc/usbtest.c
> +++ b/drivers/usb/misc/usbtest.c
> @@ -124,6 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct usbtest_dev *test)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static inline void try_intr(struct usb_host_endpoint *e,
> +			    struct usb_host_endpoint *int_in,
> +			    struct usb_host_endpoint *int_out)
> +{
> +	if (usb_endpoint_dir_in(&e->desc)) {
> +		if (!int_in)
> +			int_in = e;
> +	} else {
> +		if (!int_out)
> +			int_out = e;
> +	}
> +}
> +
> +static inline void try_iso(struct usb_host_endpoint *e,
> +			   struct usb_host_endpoint *iso_in,
> +			   struct usb_host_endpoint *iso_out)
> +{
> +	if (usb_endpoint_dir_in(&e->desc)) {
> +		if (!iso_in)
> +			iso_in = e;
> +	} else {
> +		if (!iso_out)
> +			iso_out = e;
> +	}
> +}
> +

This is not at all what I had in mind.  First, it's incorrect (can you 
see why?).  Second, by "inline" I meant moving the code to be actually 
in-line next to the conditional, not some place else in a separate 
subroutine (even if the subroutine is declared inline).

Also, the code for the USB_ENDPOINT_XFER_BULK case should look like the 
other two.

Alan Stern

Re: [PATCH] usb: misc: add a missing continue and refactor code

[Gustavo A. R. Silva]

Hi Alan,

Quoting Alan Stern <stern@rowland.harvard.edu>:

> On Tue, 21 Feb 2017, Gustavo A. R. Silva wrote:
>
>> Code refactoring to make the flow easier to follow and add missing
>> 'continue' for case USB_ENDPOINT_XFER_INT.
>>
>> Addresses-Coverity-ID: 1248733
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>>  drivers/usb/misc/usbtest.c | 50  
>> +++++++++++++++++++++++++++-------------------
>>  1 file changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
>> index 3525626..8723e33 100644
>> --- a/drivers/usb/misc/usbtest.c
>> +++ b/drivers/usb/misc/usbtest.c
>> @@ -124,6 +124,32 @@ static struct usb_device  
>> *testdev_to_usbdev(struct usbtest_dev *test)
>>
>>   
>> /*-------------------------------------------------------------------------*/
>>
>> +static inline void try_intr(struct usb_host_endpoint *e,
>> +			    struct usb_host_endpoint *int_in,
>> +			    struct usb_host_endpoint *int_out)
>> +{
>> +	if (usb_endpoint_dir_in(&e->desc)) {
>> +		if (!int_in)
>> +			int_in = e;
>> +	} else {
>> +		if (!int_out)
>> +			int_out = e;
>> +	}
>> +}
>> +
>> +static inline void try_iso(struct usb_host_endpoint *e,
>> +			   struct usb_host_endpoint *iso_in,
>> +			   struct usb_host_endpoint *iso_out)
>> +{
>> +	if (usb_endpoint_dir_in(&e->desc)) {
>> +		if (!iso_in)
>> +			iso_in = e;
>> +	} else {
>> +		if (!iso_out)
>> +			iso_out = e;
>> +	}
>> +}
>> +
>
> This is not at all what I had in mind.  First, it's incorrect (can you
> see why?).  Second, by "inline" I meant moving the code to be actually
> in-line next to the conditional, not some place else in a separate
> subroutine (even if the subroutine is declared inline).
>

Interesting... let me double check.

I thought it would've been better to have separate inline subroutines  
for those "goto".

> Also, the code for the USB_ENDPOINT_XFER_BULK case should look like the
> other two.
>

Do you mean a 'continue' instead of the 'break'?

Thanks for you comments.
--
Gustavo A. R. Silva

Re: [PATCH] usb: misc: add a missing continue and refactor code

[Gustavo A. R. Silva]

Quoting "Gustavo A. R. Silva" <garsilva@embeddedor.com>:

> Hi Alan,
>
> Quoting Alan Stern <stern@rowland.harvard.edu>:
>
>> On Tue, 21 Feb 2017, Gustavo A. R. Silva wrote:
>>
>>> Code refactoring to make the flow easier to follow and add missing
>>> 'continue' for case USB_ENDPOINT_XFER_INT.
>>>
>>> Addresses-Coverity-ID: 1248733
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>>> ---
>>> drivers/usb/misc/usbtest.c | 50  
>>> +++++++++++++++++++++++++++-------------------
>>> 1 file changed, 30 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
>>> index 3525626..8723e33 100644
>>> --- a/drivers/usb/misc/usbtest.c
>>> +++ b/drivers/usb/misc/usbtest.c
>>> @@ -124,6 +124,32 @@ static struct usb_device  
>>> *testdev_to_usbdev(struct usbtest_dev *test)
>>>
>>> /*-------------------------------------------------------------------------*/
>>>
>>> +static inline void try_intr(struct usb_host_endpoint *e,
>>> +			    struct usb_host_endpoint *int_in,
>>> +			    struct usb_host_endpoint *int_out)
>>> +{
>>> +	if (usb_endpoint_dir_in(&e->desc)) {
>>> +		if (!int_in)
>>> +			int_in = e;
>>> +	} else {
>>> +		if (!int_out)
>>> +			int_out = e;
>>> +	}
>>> +}
>>> +
>>> +static inline void try_iso(struct usb_host_endpoint *e,
>>> +			   struct usb_host_endpoint *iso_in,
>>> +			   struct usb_host_endpoint *iso_out)
>>> +{
>>> +	if (usb_endpoint_dir_in(&e->desc)) {
>>> +		if (!iso_in)
>>> +			iso_in = e;
>>> +	} else {
>>> +		if (!iso_out)
>>> +			iso_out = e;
>>> +	}
>>> +}
>>> +
>>
>> This is not at all what I had in mind.  First, it's incorrect (can you
>> see why?).  Second, by "inline" I meant moving the code to be actually
>> in-line next to the conditional, not some place else in a separate
>> subroutine (even if the subroutine is declared inline).
>>
>
> Interesting... let me double check.
>
> I thought it would've been better to have separate inline  
> subroutines for those "goto".
>
>> Also, the code for the USB_ENDPOINT_XFER_BULK case should look like the
>> other two.
>>
>
> Do you mean a 'continue' instead of the 'break'?
>

Oh I see, the following piece of code should be part of the  
USB_ENDPOINT_XFER_BULK case:

if (usb_endpoint_dir_in(&e->desc)) {
         if (!in)
                 in = e;
} else {
         if (!out)
                 out = e;
}
continue;

--
Gustavo A. R. Silva

[PATCH] usb: misc: add missing continue and refactor code

[Gustavo A. R. Silva]

-Code refactoring to make the flow easier to follow.
-Add missing 'continue' for case USB_ENDPOINT_XFER_INT.

Addresses-Coverity-ID: 1248733
Cc: Alan Stern <stern@rowloand.harvard.edu>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/usb/misc/usbtest.c | 68 +++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 37 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 3525626..382491e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,18 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct usbtest_dev *test)
 
 /*-------------------------------------------------------------------------*/
 
+static inline void endpoint_update(int edi,
+				   struct usb_host_endpoint **in,
+				   struct usb_host_endpoint **out,
+				   struct usb_host_endpoint *e)
+{
+	if (edi) {
+		if (!*in)
+			*in = e;
+	} else {
+		if (!*out)
+			*out = e;
+	}
+}
+
 static int
 get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 {
-	int				tmp;
-	struct usb_host_interface	*alt;
-	struct usb_host_endpoint	*in, *out;
-	struct usb_host_endpoint	*iso_in, *iso_out;
-	struct usb_host_endpoint	*int_in, *int_out;
-	struct usb_device		*udev;
+	int                             tmp;
+	struct usb_host_interface       *alt;
+	struct usb_host_endpoint        *in, *out;
+	struct usb_host_endpoint        *iso_in, *iso_out;
+	struct usb_host_endpoint        *int_in, *int_out;
+	struct usb_device               *udev;
 
 	for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
-		unsigned	ep;
+		unsigned        ep;
 
 		in = out = NULL;
 		iso_in = iso_out = NULL;
@@ -150,47 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 		 * ignore other endpoints and altsettings.
 		 */
 		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
-			struct usb_host_endpoint	*e;
+			struct usb_host_endpoint        *e;
+			int edi;
 
 			e = alt->endpoint + ep;
+			edi = usb_endpoint_dir_in(&e->desc);
+
 			switch (usb_endpoint_type(&e->desc)) {
 			case USB_ENDPOINT_XFER_BULK:
-				break;
+				endpoint_update(edi, &in, &out, e);
+				continue;
 			case USB_ENDPOINT_XFER_INT:
 				if (dev->info->intr)
-					goto try_intr;
+					endpoint_update(edi, &int_in, &int_out, e);
+				continue;
 			case USB_ENDPOINT_XFER_ISOC:
 				if (dev->info->iso)
-					goto try_iso;
-				/* FALLTHROUGH */
+					endpoint_update(edi, &iso_in, &iso_out, e);
+				/* fall through */
 			default:
 				continue;
 			}
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!in)
-					in = e;
-			} else {
-				if (!out)
-					out = e;
-			}
-			continue;
-try_intr:
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!int_in)
-					int_in = e;
-			} else {
-				if (!int_out)
-					int_out = e;
-			}
-			continue;
-try_iso:
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!iso_in)
-					iso_in = e;
-			} else {
-				if (!iso_out)
-					iso_out = e;
-			}
 		}
 		if ((in && out)  ||  iso_in || iso_out || int_in || int_out)
 			goto found;
-- 
2.5.0

Re: [PATCH] usb: misc: add missing continue and refactor code

[Greg Kroah-Hartman]

On Mon, Apr 03, 2017 at 09:39:53AM -0500, Gustavo A. R. Silva wrote:
> -Code refactoring to make the flow easier to follow.
> -Add missing 'continue' for case USB_ENDPOINT_XFER_INT.

Don't do multiple things in the same patch, please make these multiple
patches.  And do the "add missing continue" first, so it can be
backported to other kernels easier please.

thanks,

greg k-h

Re: [PATCH] usb: misc: add missing continue and refactor code

[Alan Stern]

On Mon, 3 Apr 2017, Greg Kroah-Hartman wrote:

> On Mon, Apr 03, 2017 at 09:39:53AM -0500, Gustavo A. R. Silva wrote:
> > -Code refactoring to make the flow easier to follow.
> > -Add missing 'continue' for case USB_ENDPOINT_XFER_INT.
> 
> Don't do multiple things in the same patch, please make these multiple
> patches.  And do the "add missing continue" first, so it can be
> backported to other kernels easier please.

Also, make sure your patch does not contain gratuitous whitespace 
changes.

Alan Stern

Re: [PATCH] usb: misc: add missing continue and refactor code

[Gustavo A. R. Silva]

Quoting Alan Stern <stern@rowland.harvard.edu>:

> On Mon, 3 Apr 2017, Greg Kroah-Hartman wrote:
>
>> On Mon, Apr 03, 2017 at 09:39:53AM -0500, Gustavo A. R. Silva wrote:
>> > -Code refactoring to make the flow easier to follow.
>> > -Add missing 'continue' for case USB_ENDPOINT_XFER_INT.
>>
>> Don't do multiple things in the same patch, please make these multiple
>> patches.  And do the "add missing continue" first, so it can be
>> backported to other kernels easier please.
>

OK, I will send a patchset shortly.

> Also, make sure your patch does not contain gratuitous whitespace
> changes.
>

Does it have any?
I ran it through checkpatch.pl before sending it and didn't see any.

Thanks
--
Gustavo A. R. Silva

[PATCH 1/2] usb: misc: add missing continue in switch

[Gustavo A. R. Silva]

Add missing continue in switch.

Addresses-Coverity-ID: 1248733
Cc: Alan Stern <stern@rowloand.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/usb/misc/usbtest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 3525626..7bfb6b78 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -159,6 +159,7 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 			case USB_ENDPOINT_XFER_INT:
 				if (dev->info->intr)
 					goto try_intr;
+				continue;
 			case USB_ENDPOINT_XFER_ISOC:
 				if (dev->info->iso)
 					goto try_iso;
-- 
2.5.0

[PATCH 2/2] usb: misc: refactor code

[Gustavo A. R. Silva]

Code refactoring to make the flow easier to follow.

Cc: Alan Stern <stern@rowloand.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/usb/misc/usbtest.c | 67 +++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 37 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 7bfb6b78..382491e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,18 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct usbtest_dev *test)
 
 /*-------------------------------------------------------------------------*/
 
+static inline void endpoint_update(int edi,
+				   struct usb_host_endpoint **in,
+				   struct usb_host_endpoint **out,
+				   struct usb_host_endpoint *e)
+{
+	if (edi) {
+		if (!*in)
+			*in = e;
+	} else {
+		if (!*out)
+			*out = e;
+	}
+}
+
 static int
 get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 {
-	int				tmp;
-	struct usb_host_interface	*alt;
-	struct usb_host_endpoint	*in, *out;
-	struct usb_host_endpoint	*iso_in, *iso_out;
-	struct usb_host_endpoint	*int_in, *int_out;
-	struct usb_device		*udev;
+	int                             tmp;
+	struct usb_host_interface       *alt;
+	struct usb_host_endpoint        *in, *out;
+	struct usb_host_endpoint        *iso_in, *iso_out;
+	struct usb_host_endpoint        *int_in, *int_out;
+	struct usb_device               *udev;
 
 	for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
-		unsigned	ep;
+		unsigned        ep;
 
 		in = out = NULL;
 		iso_in = iso_out = NULL;
@@ -150,48 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 		 * ignore other endpoints and altsettings.
 		 */
 		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
-			struct usb_host_endpoint	*e;
+			struct usb_host_endpoint        *e;
+			int edi;
 
 			e = alt->endpoint + ep;
+			edi = usb_endpoint_dir_in(&e->desc);
+
 			switch (usb_endpoint_type(&e->desc)) {
 			case USB_ENDPOINT_XFER_BULK:
-				break;
+				endpoint_update(edi, &in, &out, e);
+				continue;
 			case USB_ENDPOINT_XFER_INT:
 				if (dev->info->intr)
-					goto try_intr;
+					endpoint_update(edi, &int_in, &int_out, e);
 				continue;
 			case USB_ENDPOINT_XFER_ISOC:
 				if (dev->info->iso)
-					goto try_iso;
-				/* FALLTHROUGH */
+					endpoint_update(edi, &iso_in, &iso_out, e);
+				/* fall through */
 			default:
 				continue;
 			}
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!in)
-					in = e;
-			} else {
-				if (!out)
-					out = e;
-			}
-			continue;
-try_intr:
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!int_in)
-					int_in = e;
-			} else {
-				if (!int_out)
-					int_out = e;
-			}
-			continue;
-try_iso:
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!iso_in)
-					iso_in = e;
-			} else {
-				if (!iso_out)
-					iso_out = e;
-			}
 		}
 		if ((in && out)  ||  iso_in || iso_out || int_in || int_out)
 			goto found;
-- 
2.5.0

Re: [PATCH 2/2] usb: misc: refactor code

[Alan Stern]

On Mon, 3 Apr 2017, Gustavo A. R. Silva wrote:

> Code refactoring to make the flow easier to follow.
> 
> Cc: Alan Stern <stern@rowloand.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/usb/misc/usbtest.c | 67 +++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> index 7bfb6b78..382491e 100644
> --- a/drivers/usb/misc/usbtest.c
> +++ b/drivers/usb/misc/usbtest.c
> @@ -124,18 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct usbtest_dev *test)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static inline void endpoint_update(int edi,
> +				   struct usb_host_endpoint **in,
> +				   struct usb_host_endpoint **out,
> +				   struct usb_host_endpoint *e)
> +{
> +	if (edi) {
> +		if (!*in)
> +			*in = e;
> +	} else {
> +		if (!*out)
> +			*out = e;
> +	}
> +}
> +
>  static int
>  get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
>  {
> -	int				tmp;
> -	struct usb_host_interface	*alt;
> -	struct usb_host_endpoint	*in, *out;
> -	struct usb_host_endpoint	*iso_in, *iso_out;
> -	struct usb_host_endpoint	*int_in, *int_out;
> -	struct usb_device		*udev;
> +	int                             tmp;
> +	struct usb_host_interface       *alt;
> +	struct usb_host_endpoint        *in, *out;
> +	struct usb_host_endpoint        *iso_in, *iso_out;
> +	struct usb_host_endpoint        *int_in, *int_out;
> +	struct usb_device               *udev;

What's the difference between these 6 lines you added and the 6 lines 
that were there originally?

>  	for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
> -		unsigned	ep;
> +		unsigned        ep;

And here?

>  
>  		in = out = NULL;
>  		iso_in = iso_out = NULL;
> @@ -150,48 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
>  		 * ignore other endpoints and altsettings.
>  		 */
>  		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
> -			struct usb_host_endpoint	*e;
> +			struct usb_host_endpoint        *e;

And here?

> +			int edi;
>  
>  			e = alt->endpoint + ep;
> +			edi = usb_endpoint_dir_in(&e->desc);
> +
>  			switch (usb_endpoint_type(&e->desc)) {
>  			case USB_ENDPOINT_XFER_BULK:
> -				break;
> +				endpoint_update(edi, &in, &out, e);
> +				continue;
>  			case USB_ENDPOINT_XFER_INT:
>  				if (dev->info->intr)
> -					goto try_intr;
> +					endpoint_update(edi, &int_in, &int_out, e);
>  				continue;
>  			case USB_ENDPOINT_XFER_ISOC:
>  				if (dev->info->iso)
> -					goto try_iso;
> -				/* FALLTHROUGH */
> +					endpoint_update(edi, &iso_in, &iso_out, e);
> +				/* fall through */

Why change the comment?

Alan Stern

>  			default:
>  				continue;
>  			}
> -			if (usb_endpoint_dir_in(&e->desc)) {
> -				if (!in)
> -					in = e;
> -			} else {
> -				if (!out)
> -					out = e;
> -			}
> -			continue;
> -try_intr:
> -			if (usb_endpoint_dir_in(&e->desc)) {
> -				if (!int_in)
> -					int_in = e;
> -			} else {
> -				if (!int_out)
> -					int_out = e;
> -			}
> -			continue;
> -try_iso:
> -			if (usb_endpoint_dir_in(&e->desc)) {
> -				if (!iso_in)
> -					iso_in = e;
> -			} else {
> -				if (!iso_out)
> -					iso_out = e;
> -			}
>  		}
>  		if ((in && out)  ||  iso_in || iso_out || int_in || int_out)
>  			goto found;
> 

Re: [PATCH 2/2] usb: misc: refactor code

[Gustavo A. R. Silva]

Hi Alan,

Quoting Alan Stern <stern@rowland.harvard.edu>:

> On Mon, 3 Apr 2017, Gustavo A. R. Silva wrote:
>
>> Code refactoring to make the flow easier to follow.
>>
>> Cc: Alan Stern <stern@rowloand.harvard.edu>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>>  drivers/usb/misc/usbtest.c | 67  
>> +++++++++++++++++++++-------------------------
>>  1 file changed, 30 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
>> index 7bfb6b78..382491e 100644
>> --- a/drivers/usb/misc/usbtest.c
>> +++ b/drivers/usb/misc/usbtest.c
>> @@ -124,18 +124,32 @@ static struct usb_device  
>> *testdev_to_usbdev(struct usbtest_dev *test)
>>
>>   
>> /*-------------------------------------------------------------------------*/
>>
>> +static inline void endpoint_update(int edi,
>> +				   struct usb_host_endpoint **in,
>> +				   struct usb_host_endpoint **out,
>> +				   struct usb_host_endpoint *e)
>> +{
>> +	if (edi) {
>> +		if (!*in)
>> +			*in = e;
>> +	} else {
>> +		if (!*out)
>> +			*out = e;
>> +	}
>> +}
>> +
>>  static int
>>  get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
>>  {
>> -	int				tmp;
>> -	struct usb_host_interface	*alt;
>> -	struct usb_host_endpoint	*in, *out;
>> -	struct usb_host_endpoint	*iso_in, *iso_out;
>> -	struct usb_host_endpoint	*int_in, *int_out;
>> -	struct usb_device		*udev;
>> +	int                             tmp;
>> +	struct usb_host_interface       *alt;
>> +	struct usb_host_endpoint        *in, *out;
>> +	struct usb_host_endpoint        *iso_in, *iso_out;
>> +	struct usb_host_endpoint        *int_in, *int_out;
>> +	struct usb_device               *udev;
>
> What's the difference between these 6 lines you added and the 6 lines
> that were there originally?
>

Yeah, well, certainly none at all. This is what happened: I created a  
local copy of my changes(this piece of code included) and fixed some  
issues in a previous patch, then I did a git revert and moved my  
changes back to the original file. During this process the tabs were  
replaced with spaces in the original file, then I had to add the tabs  
again and this was the resulting patch.

>>  	for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
>> -		unsigned	ep;
>> +		unsigned        ep;
>
> And here?
>

Same as above.

>>
>>  		in = out = NULL;
>>  		iso_in = iso_out = NULL;
>> @@ -150,48 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct  
>> usb_interface *intf)
>>  		 * ignore other endpoints and altsettings.
>>  		 */
>>  		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
>> -			struct usb_host_endpoint	*e;
>> +			struct usb_host_endpoint        *e;
>
> And here?
>

Same as above.

>> +			int edi;
>>
>>  			e = alt->endpoint + ep;
>> +			edi = usb_endpoint_dir_in(&e->desc);
>> +
>>  			switch (usb_endpoint_type(&e->desc)) {
>>  			case USB_ENDPOINT_XFER_BULK:
>> -				break;
>> +				endpoint_update(edi, &in, &out, e);
>> +				continue;
>>  			case USB_ENDPOINT_XFER_INT:
>>  				if (dev->info->intr)
>> -					goto try_intr;
>> +					endpoint_update(edi, &int_in, &int_out, e);
>>  				continue;
>>  			case USB_ENDPOINT_XFER_ISOC:
>>  				if (dev->info->iso)
>> -					goto try_iso;
>> -				/* FALLTHROUGH */
>> +					endpoint_update(edi, &iso_in, &iso_out, e);
>> +				/* fall through */
>
> Why change the comment?
>

Oh, I based this change in the following comment to another patch I  
sent some weeks ago:
https://lkml.org/lkml/2017/2/10/293

It is due to Coding Style.

> Alan Stern
>
>>  			default:
>>  				continue;
>>  			}
>> -			if (usb_endpoint_dir_in(&e->desc)) {
>> -				if (!in)
>> -					in = e;
>> -			} else {
>> -				if (!out)
>> -					out = e;
>> -			}
>> -			continue;
>> -try_intr:
>> -			if (usb_endpoint_dir_in(&e->desc)) {
>> -				if (!int_in)
>> -					int_in = e;
>> -			} else {
>> -				if (!int_out)
>> -					int_out = e;
>> -			}
>> -			continue;
>> -try_iso:
>> -			if (usb_endpoint_dir_in(&e->desc)) {
>> -				if (!iso_in)
>> -					iso_in = e;
>> -			} else {
>> -				if (!iso_out)
>> -					iso_out = e;
>> -			}
>>  		}
>>  		if ((in && out)  ||  iso_in || iso_out || int_in || int_out)
>>  			goto found;
>>

Thanks for your comments.
--
Gustavo A. R. Silva

[PATCH v2 1/2] usb: misc: add missing continue in switch

[Gustavo A. R. Silva]

Add missing continue in switch.

Addresses-Coverity-ID: 1248733
Cc: Alan Stern <stern@rowloand.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
 None.

 drivers/usb/misc/usbtest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 3525626..7bfb6b78 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -159,6 +159,7 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 			case USB_ENDPOINT_XFER_INT:
 				if (dev->info->intr)
 					goto try_intr;
+				continue;
 			case USB_ENDPOINT_XFER_ISOC:
 				if (dev->info->iso)
 					goto try_iso;
-- 
2.5.0

[PATCH v2 2/2] usb: misc: refactor code

[Gustavo A. R. Silva]

Code refactoring to make the flow easier to follow.

Cc: Alan Stern <stern@rowloand.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
Changes in v2:
 Remove unfruitful changes in previous patch.
 Revert change to comment.

 drivers/usb/misc/usbtest.c | 49 ++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 7bfb6b78..88f627e 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -124,6 +124,20 @@ static struct usb_device *testdev_to_usbdev(struct usbtest_dev *test)
 
 /*-------------------------------------------------------------------------*/
 
+static inline void endpoint_update(int edi,
+				   struct usb_host_endpoint **in,
+				   struct usb_host_endpoint **out,
+				   struct usb_host_endpoint *e)
+{
+	if (edi) {
+		if (!*in)
+			*in = e;
+	} else {
+		if (!*out)
+			*out = e;
+	}
+}
+
 static int
 get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 {
@@ -151,47 +165,26 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
 		 */
 		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
 			struct usb_host_endpoint	*e;
+			int edi;
 
 			e = alt->endpoint + ep;
+			edi = usb_endpoint_dir_in(&e->desc);
+
 			switch (usb_endpoint_type(&e->desc)) {
 			case USB_ENDPOINT_XFER_BULK:
-				break;
+				endpoint_update(edi, &in, &out, e);
+				continue;
 			case USB_ENDPOINT_XFER_INT:
 				if (dev->info->intr)
-					goto try_intr;
+					endpoint_update(edi, &int_in, &int_out, e);
 				continue;
 			case USB_ENDPOINT_XFER_ISOC:
 				if (dev->info->iso)
-					goto try_iso;
+					endpoint_update(edi, &iso_in, &iso_out, e);
 				/* FALLTHROUGH */
 			default:
 				continue;
 			}
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!in)
-					in = e;
-			} else {
-				if (!out)
-					out = e;
-			}
-			continue;
-try_intr:
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!int_in)
-					int_in = e;
-			} else {
-				if (!int_out)
-					int_out = e;
-			}
-			continue;
-try_iso:
-			if (usb_endpoint_dir_in(&e->desc)) {
-				if (!iso_in)
-					iso_in = e;
-			} else {
-				if (!iso_out)
-					iso_out = e;
-			}
 		}
 		if ((in && out)  ||  iso_in || iso_out || int_in || int_out)
 			goto found;
-- 
2.5.0

Re: [PATCH v2 2/2] usb: misc: refactor code

[Alan Stern]

On Mon, 3 Apr 2017, Gustavo A. R. Silva wrote:

> Code refactoring to make the flow easier to follow.
> 
> Cc: Alan Stern <stern@rowloand.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> Changes in v2:
>  Remove unfruitful changes in previous patch.
>  Revert change to comment.

For both patches:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

>  drivers/usb/misc/usbtest.c | 49 ++++++++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> index 7bfb6b78..88f627e 100644
> --- a/drivers/usb/misc/usbtest.c
> +++ b/drivers/usb/misc/usbtest.c
> @@ -124,6 +124,20 @@ static struct usb_device *testdev_to_usbdev(struct usbtest_dev *test)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static inline void endpoint_update(int edi,
> +				   struct usb_host_endpoint **in,
> +				   struct usb_host_endpoint **out,
> +				   struct usb_host_endpoint *e)
> +{
> +	if (edi) {
> +		if (!*in)
> +			*in = e;
> +	} else {
> +		if (!*out)
> +			*out = e;
> +	}
> +}
> +
>  static int
>  get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
>  {
> @@ -151,47 +165,26 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
>  		 */
>  		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
>  			struct usb_host_endpoint	*e;
> +			int edi;
>  
>  			e = alt->endpoint + ep;
> +			edi = usb_endpoint_dir_in(&e->desc);
> +
>  			switch (usb_endpoint_type(&e->desc)) {
>  			case USB_ENDPOINT_XFER_BULK:
> -				break;
> +				endpoint_update(edi, &in, &out, e);
> +				continue;
>  			case USB_ENDPOINT_XFER_INT:
>  				if (dev->info->intr)
> -					goto try_intr;
> +					endpoint_update(edi, &int_in, &int_out, e);
>  				continue;
>  			case USB_ENDPOINT_XFER_ISOC:
>  				if (dev->info->iso)
> -					goto try_iso;
> +					endpoint_update(edi, &iso_in, &iso_out, e);
>  				/* FALLTHROUGH */
>  			default:
>  				continue;
>  			}
> -			if (usb_endpoint_dir_in(&e->desc)) {
> -				if (!in)
> -					in = e;
> -			} else {
> -				if (!out)
> -					out = e;
> -			}
> -			continue;
> -try_intr:
> -			if (usb_endpoint_dir_in(&e->desc)) {
> -				if (!int_in)
> -					int_in = e;
> -			} else {
> -				if (!int_out)
> -					int_out = e;
> -			}
> -			continue;
> -try_iso:
> -			if (usb_endpoint_dir_in(&e->desc)) {
> -				if (!iso_in)
> -					iso_in = e;
> -			} else {
> -				if (!iso_out)
> -					iso_out = e;
> -			}
>  		}
>  		if ((in && out)  ||  iso_in || iso_out || int_in || int_out)
>  			goto found;
> 

Re: [PATCH 2/2] usb: misc: refactor code

[Felipe Balbi]

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


Hi,

"Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
> Code refactoring to make the flow easier to follow.
>
> Cc: Alan Stern <stern@rowloand.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/usb/misc/usbtest.c | 67 +++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
> index 7bfb6b78..382491e 100644
> --- a/drivers/usb/misc/usbtest.c
> +++ b/drivers/usb/misc/usbtest.c
> @@ -124,18 +124,32 @@ static struct usb_device *testdev_to_usbdev(struct usbtest_dev *test)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +static inline void endpoint_update(int edi,
> +				   struct usb_host_endpoint **in,
> +				   struct usb_host_endpoint **out,
> +				   struct usb_host_endpoint *e)
> +{
> +	if (edi) {
> +		if (!*in)
> +			*in = e;
> +	} else {
> +		if (!*out)
> +			*out = e;
> +	}
> +}
> +
>  static int
>  get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
>  {
> -	int				tmp;
> -	struct usb_host_interface	*alt;
> -	struct usb_host_endpoint	*in, *out;
> -	struct usb_host_endpoint	*iso_in, *iso_out;
> -	struct usb_host_endpoint	*int_in, *int_out;
> -	struct usb_device		*udev;
> +	int                             tmp;
> +	struct usb_host_interface       *alt;
> +	struct usb_host_endpoint        *in, *out;
> +	struct usb_host_endpoint        *iso_in, *iso_out;
> +	struct usb_host_endpoint        *int_in, *int_out;
> +	struct usb_device               *udev;

unnecessary change

>  
>  	for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
> -		unsigned	ep;
> +		unsigned        ep;

unnecessary change

>  
>  		in = out = NULL;
>  		iso_in = iso_out = NULL;
> @@ -150,48 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
>  		 * ignore other endpoints and altsettings.
>  		 */
>  		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
> -			struct usb_host_endpoint	*e;
> +			struct usb_host_endpoint        *e;

unnecessary change

-- 
balbi

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

Re: [PATCH 2/2] usb: misc: refactor code

[Gustavo A. R. Silva]

Hello,

Quoting Felipe Balbi <felipe.balbi@linux.intel.com>:

> Hi,
>
> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>> Code refactoring to make the flow easier to follow.
>>
>> Cc: Alan Stern <stern@rowloand.harvard.edu>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>>  drivers/usb/misc/usbtest.c | 67  
>> +++++++++++++++++++++-------------------------
>>  1 file changed, 30 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
>> index 7bfb6b78..382491e 100644
>> --- a/drivers/usb/misc/usbtest.c
>> +++ b/drivers/usb/misc/usbtest.c
>> @@ -124,18 +124,32 @@ static struct usb_device  
>> *testdev_to_usbdev(struct usbtest_dev *test)
>>
>>   
>> /*-------------------------------------------------------------------------*/
>>
>> +static inline void endpoint_update(int edi,
>> +				   struct usb_host_endpoint **in,
>> +				   struct usb_host_endpoint **out,
>> +				   struct usb_host_endpoint *e)
>> +{
>> +	if (edi) {
>> +		if (!*in)
>> +			*in = e;
>> +	} else {
>> +		if (!*out)
>> +			*out = e;
>> +	}
>> +}
>> +
>>  static int
>>  get_endpoints(struct usbtest_dev *dev, struct usb_interface *intf)
>>  {
>> -	int				tmp;
>> -	struct usb_host_interface	*alt;
>> -	struct usb_host_endpoint	*in, *out;
>> -	struct usb_host_endpoint	*iso_in, *iso_out;
>> -	struct usb_host_endpoint	*int_in, *int_out;
>> -	struct usb_device		*udev;
>> +	int                             tmp;
>> +	struct usb_host_interface       *alt;
>> +	struct usb_host_endpoint        *in, *out;
>> +	struct usb_host_endpoint        *iso_in, *iso_out;
>> +	struct usb_host_endpoint        *int_in, *int_out;
>> +	struct usb_device               *udev;
>
> unnecessary change
>
>>
>>  	for (tmp = 0; tmp < intf->num_altsetting; tmp++) {
>> -		unsigned	ep;
>> +		unsigned        ep;
>
> unnecessary change
>
>>
>>  		in = out = NULL;
>>  		iso_in = iso_out = NULL;
>> @@ -150,48 +164,27 @@ get_endpoints(struct usbtest_dev *dev, struct  
>> usb_interface *intf)
>>  		 * ignore other endpoints and altsettings.
>>  		 */
>>  		for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
>> -			struct usb_host_endpoint	*e;
>> +			struct usb_host_endpoint        *e;
>
> unnecessary change
>

I already sent the version 2 of this patch: https://lkml.org/lkml/2017/4/3/856

Thanks
--
Gustavo A. R. Silva