[PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN).

[PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN).

[Sandhya Bankar]

Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN).

This correction is made using below coccinelle script:

@@
struct usb_endpoint_descriptor *endpoint;
expression E;
@@
(
- endpoint->bEndpointAddress & USB_DIR_IN
+ usb_endpoint_dir_in(endpoint)
|
- endpoint->bEndpointAddress == USB_DIR_IN
+ usb_endpoint_dir_in(endpoint)
|
- (endpoint->bEndpointAddress & E) == USB_DIR_IN
+ usb_endpoint_dir_in(endpoint)
)


Signed-off-by: Sandhya Bankar <bankarsandhya512@gmail.com>
---
 drivers/staging/comedi/drivers/dt9812.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt9812.c b/drivers/staging/comedi/drivers/dt9812.c
index 7ebca86..4007c45 100644
--- a/drivers/staging/comedi/drivers/dt9812.c
+++ b/drivers/staging/comedi/drivers/dt9812.c
@@ -676,7 +676,7 @@ static int dt9812_find_endpoints(struct comedi_device *dev)
 			dir = USB_DIR_IN;
 			break;
 		}
-		if ((ep->bEndpointAddress & USB_DIR_IN) != dir) {
+		if ((usb_endpoint_dir_in(ep)) != dir) {
 			dev_err(dev->class_dev,
 				"Endpoint has wrong direction\n");
 			return -ENODEV;
-- 
1.7.1

Re: [Outreachy kernel] [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN).

[Julia Lawall]

On Sun, 18 Sep 2016, Sandhya Bankar wrote:

> Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN).
>
> This correction is made using below coccinelle script:
>
> @@
> struct usb_endpoint_descriptor *endpoint;
> expression E;
> @@
> (
> - endpoint->bEndpointAddress & USB_DIR_IN
> + usb_endpoint_dir_in(endpoint)
> |
> - endpoint->bEndpointAddress == USB_DIR_IN
> + usb_endpoint_dir_in(endpoint)
> |
> - (endpoint->bEndpointAddress & E) == USB_DIR_IN
> + usb_endpoint_dir_in(endpoint)
> )
>
>
> Signed-off-by: Sandhya Bankar <bankarsandhya512@gmail.com>
> ---
>  drivers/staging/comedi/drivers/dt9812.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/dt9812.c b/drivers/staging/comedi/drivers/dt9812.c
> index 7ebca86..4007c45 100644
> --- a/drivers/staging/comedi/drivers/dt9812.c
> +++ b/drivers/staging/comedi/drivers/dt9812.c
> @@ -676,7 +676,7 @@ static int dt9812_find_endpoints(struct comedi_device *dev)
>  			dir = USB_DIR_IN;
>  			break;
>  		}
> -		if ((ep->bEndpointAddress & USB_DIR_IN) != dir) {
> +		if ((usb_endpoint_dir_in(ep)) != dir) {

Small problem:  The parentheses around the function call are not needed.

Big problem: The transformation is not correct in this context.  If you
look at the definition of usb_endpoint_dir_in, you will see that, like in
most of the rules of the semantic patch, the main operator in the
computation of the return value is ==.  So the function returns a boolean,
ie 0 or 1.  The first rule of the semantic patch is only valid when the
result is being tested for being true or false, not when it is compared to
something.  Maybe there is some other way to reorganize the code to use
the endpoint functions.

julia

>  			dev_err(dev->class_dev,
>  				"Endpoint has wrong direction\n");
>  			return -ENODEV;
> --
> 1.7.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160918144757.GA3786%40sandhya.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN).

[Julia Lawall]

On Mon, 19 Sep 2016, Ian Abbott wrote:

> On 18/09/16 16:26, Julia Lawall wrote:
> > On Sun, 18 Sep 2016, Sandhya Bankar wrote:
> >
> > > Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress &
> > > USB_DIR_IN).
> > >
> > > This correction is made using below coccinelle script:
> > >
> > > @@
> > > struct usb_endpoint_descriptor *endpoint;
> > > expression E;
> > > @@
> > > (
> > > - endpoint->bEndpointAddress & USB_DIR_IN
> > > + usb_endpoint_dir_in(endpoint)
> > > |
> > > - endpoint->bEndpointAddress == USB_DIR_IN
> > > + usb_endpoint_dir_in(endpoint)
> > > |
> > > - (endpoint->bEndpointAddress & E) == USB_DIR_IN
> > > + usb_endpoint_dir_in(endpoint)
> > > )
> > >
> > >
> > > Signed-off-by: Sandhya Bankar <bankarsandhya512@gmail.com>
> > > ---
> > >  drivers/staging/comedi/drivers/dt9812.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/staging/comedi/drivers/dt9812.c
> > > b/drivers/staging/comedi/drivers/dt9812.c
> > > index 7ebca86..4007c45 100644
> > > --- a/drivers/staging/comedi/drivers/dt9812.c
> > > +++ b/drivers/staging/comedi/drivers/dt9812.c
> > > @@ -676,7 +676,7 @@ static int dt9812_find_endpoints(struct comedi_device
> > > *dev)
> > >  			dir = USB_DIR_IN;
> > >  			break;
> > >  		}
> > > -		if ((ep->bEndpointAddress & USB_DIR_IN) != dir) {
> > > +		if ((usb_endpoint_dir_in(ep)) != dir) {
> >
> > Small problem:  The parentheses around the function call are not needed.
> >
> > Big problem: The transformation is not correct in this context.  If you
> > look at the definition of usb_endpoint_dir_in, you will see that, like in
> > most of the rules of the semantic patch, the main operator in the
> > computation of the return value is ==.  So the function returns a boolean,
> > ie 0 or 1.  The first rule of the semantic patch is only valid when the
> > result is being tested for being true or false, not when it is compared to
> > something.  Maybe there is some other way to reorganize the code to use
> > the endpoint functions.
>
> There doesn't appear to be a function such as usb_endpoint_dir(ep) to return
> the endpoint direction as one of the values USB_DIR_IN or USB_DIR_OUT, which
> would have been nice.
>
> I think the original code is using USB_DIR_IN incorrectly on that line. It
> should really be replaced with USB_ENDPOINT_DIR_MASK like this:
>
> 		if ((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != dir) {
>
> (USB_ENDPOINT_DIR_MASK and USB_DIR_IN have the same value, but
> USB_ENDPOINT_DIR_MASK is more "correct" for use as a mask.)

That looks better to me.

julia

Re: [Outreachy kernel] [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN).

[sandhya bankar]

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

Thanks Ian and Julia ! I will send a patch as per your suggestion.

On Mon, Sep 19, 2016 at 5:34 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:

>
>
> On Mon, 19 Sep 2016, Ian Abbott wrote:
>
> > On 18/09/16 16:26, Julia Lawall wrote:
> > > On Sun, 18 Sep 2016, Sandhya Bankar wrote:
> > >
> > > > Use usb_endpoint_dir_in(endpoint) instead of
> (endpoint->bEndpointAddress &
> > > > USB_DIR_IN).
> > > >
> > > > This correction is made using below coccinelle script:
> > > >
> > > > @@
> > > > struct usb_endpoint_descriptor *endpoint;
> > > > expression E;
> > > > @@
> > > > (
> > > > - endpoint->bEndpointAddress & USB_DIR_IN
> > > > + usb_endpoint_dir_in(endpoint)
> > > > |
> > > > - endpoint->bEndpointAddress == USB_DIR_IN
> > > > + usb_endpoint_dir_in(endpoint)
> > > > |
> > > > - (endpoint->bEndpointAddress & E) == USB_DIR_IN
> > > > + usb_endpoint_dir_in(endpoint)
> > > > )
> > > >
> > > >
> > > > Signed-off-by: Sandhya Bankar <bankarsandhya512@gmail.com>
> > > > ---
> > > >  drivers/staging/comedi/drivers/dt9812.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/comedi/drivers/dt9812.c
> > > > b/drivers/staging/comedi/drivers/dt9812.c
> > > > index 7ebca86..4007c45 100644
> > > > --- a/drivers/staging/comedi/drivers/dt9812.c
> > > > +++ b/drivers/staging/comedi/drivers/dt9812.c
> > > > @@ -676,7 +676,7 @@ static int dt9812_find_endpoints(struct
> comedi_device
> > > > *dev)
> > > >                   dir = USB_DIR_IN;
> > > >                   break;
> > > >           }
> > > > -         if ((ep->bEndpointAddress & USB_DIR_IN) != dir) {
> > > > +         if ((usb_endpoint_dir_in(ep)) != dir) {
> > >
> > > Small problem:  The parentheses around the function call are not
> needed.
> > >
> > > Big problem: The transformation is not correct in this context.  If you
> > > look at the definition of usb_endpoint_dir_in, you will see that, like
> in
> > > most of the rules of the semantic patch, the main operator in the
> > > computation of the return value is ==.  So the function returns a
> boolean,
> > > ie 0 or 1.  The first rule of the semantic patch is only valid when the
> > > result is being tested for being true or false, not when it is
> compared to
> > > something.  Maybe there is some other way to reorganize the code to use
> > > the endpoint functions.
> >
> > There doesn't appear to be a function such as usb_endpoint_dir(ep) to
> return
> > the endpoint direction as one of the values USB_DIR_IN or USB_DIR_OUT,
> which
> > would have been nice.
> >
> > I think the original code is using USB_DIR_IN incorrectly on that line.
> It
> > should really be replaced with USB_ENDPOINT_DIR_MASK like this:
> >
> >               if ((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != dir)
> {
> >
> > (USB_ENDPOINT_DIR_MASK and USB_DIR_IN have the same value, but
> > USB_ENDPOINT_DIR_MASK is more "correct" for use as a mask.)
>
> That looks better to me.
>
> julia
>

[-- Attachment #2: Type: text/html, Size: 4218 bytes --]