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

PATCH In review Sandhya Bankar 2 participants 4 messages Sep 18, 2016 – Sep 19, 2016 1 message not in the archive Staging

lore

Share & tools

lore

Sandhya Bankar

author

2016-09-18 14:47 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/staging/comedi/drivers/dt9812.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/comedi/drivers/dt9812.c +1 -1

index 7ebca86..4007c45 100644
@@ -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;

Signature · 1 line

1.7.1

Julia Lawall

2016-09-18 15:26 UTC #1 lore raw

Quoted 30 lines · 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 <[email protected]>
---
 drivers/staging/comedi/drivers/dt9812.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/staging/comedi/drivers/dt9812.c +0 -0
index 7ebca86..4007c45 100644

drivers/staging/comedi/drivers/dt9812.c static int dt9812_find_endpoints(struct comedi_device *dev) · line 679

Quoted 5 lines

drivers/staging/comedi/drivers/dt9812.c +1 -1

@@ -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

Quoted 13 lines

drivers/staging/comedi/drivers/dt9812.c +0 -1
 			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 [email protected]. To post to this group, send email to [email protected].
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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Julia Lawall

2016-09-19 12:04 UTC #3 lore raw

Quoted 57 lines · Ian Abbott

Quoted 43 lines · Julia Lawall
Quoted 32 lines · 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 <[email protected]>
---
 drivers/staging/comedi/drivers/dt9812.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/staging/comedi/drivers/dt9812.c +0 -0
index 7ebca86..4007c45 100644
drivers/staging/comedi/drivers/dt9812.c static int dt9812_find_endpoints(struct comedi_device · line 680
Quoted 6 lines
drivers/staging/comedi/drivers/dt9812.c +1 -1
@@ -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.
Quoted 8 lines
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.

Quoted 7 lines

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

sandhya bankar

author

2016-09-19 15:27 UTC #4 lore raw

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

Quoted 83 lines · Julia Lawall

Quoted 4 lines · Ian Abbott
Quoted 3 lines · Julia Lawall
Quoted 1 line · Sandhya Bankar
Use usb_endpoint_dir_in(endpoint) instead of
(endpoint->bEndpointAddress &
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
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 <[email protected]>
---
 drivers/staging/comedi/drivers/dt9812.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Quoted 6 lines
Quoted 6 lines
drivers/staging/comedi/drivers/dt9812.c static int dt9812_find_endpoints(struct
Quoted 6 lines
drivers/staging/comedi/drivers/dt9812.c +0 -0
index 7ebca86..4007c45 100644
@@ -676,7 +676,7 @@ static int dt9812_find_endpoints(struct
comedi_device
Quoted 8 lines
Quoted 8 lines
drivers/staging/comedi/drivers/dt9812.c static int dt9812_find_endpoints(struct comedi_device *dev) · line 679
Quoted 6 lines
drivers/staging/comedi/drivers/dt9812.c +1 -1
 *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.
Quoted 3 lines
Quoted 3 lines
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
Quoted 2 lines
Quoted 2 lines
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,
Quoted 2 lines
Quoted 2 lines
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
Quoted 4 lines
Quoted 2 lines
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
Quoted 1 line
the endpoint direction as one of the values USB_DIR_IN or USB_DIR_OUT,
which
Quoted 3 lines
would have been nice.
I think the original code is using USB_DIR_IN incorrectly on that line.
It
Quoted 3 lines
should really be replaced with USB_ENDPOINT_DIR_MASK like this:
              if ((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != dir)
{
Quoted 3 lines
(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