* [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN). @ 2016-09-18 14:47 Sandhya Bankar 2016-09-18 15:26 ` [Outreachy kernel] " Julia Lawall 0 siblings, 1 reply; 4+ messages in thread From: Sandhya Bankar @ 2016-09-18 14:47 UTC (permalink / raw) To: outreachy-kernel; +Cc: abbotti, hsweeten, gregkh 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN). 2016-09-18 14:47 [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN) Sandhya Bankar @ 2016-09-18 15:26 ` Julia Lawall [not found] ` <c273ac58-4efb-9abe-4ec8-3c818ed8a17d@mev.co.uk> 0 siblings, 1 reply; 4+ messages in thread From: Julia Lawall @ 2016-09-18 15:26 UTC (permalink / raw) To: Sandhya Bankar; +Cc: outreachy-kernel, abbotti, hsweeten, gregkh 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. > ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <c273ac58-4efb-9abe-4ec8-3c818ed8a17d@mev.co.uk>]
* Re: [Outreachy kernel] [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN). [not found] ` <c273ac58-4efb-9abe-4ec8-3c818ed8a17d@mev.co.uk> @ 2016-09-19 12:04 ` Julia Lawall 2016-09-19 15:27 ` sandhya bankar 0 siblings, 1 reply; 4+ messages in thread From: Julia Lawall @ 2016-09-19 12:04 UTC (permalink / raw) To: Ian Abbott Cc: Julia Lawall, Sandhya Bankar, outreachy-kernel, hsweeten, gregkh 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Outreachy kernel] [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN). 2016-09-19 12:04 ` Julia Lawall @ 2016-09-19 15:27 ` sandhya bankar 0 siblings, 0 replies; 4+ messages in thread From: sandhya bankar @ 2016-09-19 15:27 UTC (permalink / raw) To: Julia Lawall; +Cc: Ian Abbott, outreachy-kernel, hsweeten, Greg KH [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-19 15:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-18 14:47 [PATCH] Staging: comedi: Use usb_endpoint_dir_in(endpoint) instead of (endpoint->bEndpointAddress & USB_DIR_IN) Sandhya Bankar
2016-09-18 15:26 ` [Outreachy kernel] " Julia Lawall
[not found] ` <c273ac58-4efb-9abe-4ec8-3c818ed8a17d@mev.co.uk>
2016-09-19 12:04 ` Julia Lawall
2016-09-19 15:27 ` sandhya bankar
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.