drivers: usb: Removes use of assignment in if condition

PATCH In review Atul Kumar Pant 2 participants 6 messages Aug 15, 2023 – Aug 16, 2023 USB

lore

Share & tools

lore

Atul Kumar Pant

author

2023-08-15 20:44 UTC #0 lore raw

PATCH 1/1 drivers: usb: Removes use of assignment in if condition

This patch fixes following checkpatch.pl issue:
ERROR: do not use assignment in if condition

Signed-off-by Atul Kumar Pant <[email protected]>

---
 drivers/usb/core/devio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/usb/core/devio.c +2 -1

index e501a03d6c70..56899fed6bd4 100644
@@ -2333,9 +2333,10 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
 		}
 	}
 
+	intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
 	if (ps->dev->state != USB_STATE_CONFIGURED)
 		retval = -EHOSTUNREACH;
-	else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
+	else if (!intf)
 		retval = -EINVAL;
 	else switch (ctl->ioctl_code) {

Signature · 1 line

2.25.1

Greg KH

maintainer

2023-08-15 21:08 UTC #1 lore raw

Quoted 13 lines · Atul Kumar Pant

This patch fixes following checkpatch.pl issue:
ERROR: do not use assignment in if condition
Signed-off-by Atul Kumar Pant <[email protected]>
---
 drivers/usb/core/devio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/core/devio.c +0 -0
index e501a03d6c70..56899fed6bd4 100644

drivers/usb/core/devio.c static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl) · line 2339

Quoted 8 lines

drivers/usb/core/devio.c +2 -1

@@ -2333,9 +2333,10 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
 		}
 	}
 
+	intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
 	if (ps->dev->state != USB_STATE_CONFIGURED)
 		retval = -EHOSTUNREACH;
-	else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
+	else if (!intf)

Did you mean to change the logic here by doing the calculation always? Does that change functionality?

The existing code is fine, running checkpatch on code outside of drivers/staging/ or on new patches you are writing, is generally discouraged as the code usually is older than checkpatch is :)

thanks,

greg k-h

Atul Kumar Pant

author

2023-08-16 02:53 UTC #2 lore raw

Quoted 26 lines · Greg KH

Quoted 13 lines · Atul Kumar Pant
This patch fixes following checkpatch.pl issue:
ERROR: do not use assignment in if condition
Signed-off-by Atul Kumar Pant <[email protected]>
---
 drivers/usb/core/devio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/core/devio.c +0 -0
index e501a03d6c70..56899fed6bd4 100644
drivers/usb/core/devio.c static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl) · line 2339
Quoted 8 lines
drivers/usb/core/devio.c +2 -1
@@ -2333,9 +2333,10 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
 		}
 	}
 
+	intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
 	if (ps->dev->state != USB_STATE_CONFIGURED)
 		retval = -EHOSTUNREACH;
-	else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
+	else if (!intf)
Did you mean to change the logic here by doing the calculation always? Does that change functionality?

Quoted 3 lines

The existing code is fine, running checkpatch on code outside of drivers/staging/ or on new patches you are writing, is generally discouraged as the code usually is older than checkpatch is :)

Understood. One question though, should we fix issues like no space before open parenthesis (e.g. if(val)) in the code that is outside of /drivers/staging/ ?

Quoted 4 lines

thanks,
greg k-h

Greg KH

maintainer

2023-08-16 14:44 UTC #3 lore raw

Quoted 27 lines · Atul Kumar Pant

Quoted 26 lines · Greg KH
Quoted 13 lines · Atul Kumar Pant
This patch fixes following checkpatch.pl issue:
ERROR: do not use assignment in if condition
Signed-off-by Atul Kumar Pant <[email protected]>
---
 drivers/usb/core/devio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/core/devio.c +0 -0
index e501a03d6c70..56899fed6bd4 100644
drivers/usb/core/devio.c static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl) · line 2339
Quoted 8 lines
drivers/usb/core/devio.c +2 -1
@@ -2333,9 +2333,10 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
 		}
 	}
 
+	intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
 	if (ps->dev->state != USB_STATE_CONFIGURED)
 		retval = -EHOSTUNREACH;
-	else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
+	else if (!intf)
Did you mean to change the logic here by doing the calculation always? Does that change functionality?

Quoted 7 lines

Quoted 3 lines
The existing code is fine, running checkpatch on code outside of drivers/staging/ or on new patches you are writing, is generally discouraged as the code usually is older than checkpatch is :)
Understood. One question though, should we fix issues like no space before open parenthesis (e.g. if(val)) in the code that is outside of /drivers/staging/ ?

Only if the maintainer of that subsystem says it is ok to do so.

good luck!

greg k-h

Atul Kumar Pant

author

2023-08-16 03:44 UTC #4 lore raw

In reply to Greg KH

Quoted 17 lines · Greg KH

Quoted 16 lines · Atul Kumar Pant
This patch fixes following checkpatch.pl issue:
ERROR: do not use assignment in if condition
Signed-off-by Atul Kumar Pant <[email protected]>
---
 drivers/usb/core/devio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/core/devio.c +0 -0
index e501a03d6c70..56899fed6bd4 100644
@@ -2333,9 +2333,10 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
 		}
 	}
 

Quoted 9 lines

drivers/usb/core/devio.c static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl) · line 2339
Quoted 5 lines
drivers/usb/core/devio.c +2 -1
+	intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
 	if (ps->dev->state != USB_STATE_CONFIGURED)
 		retval = -EHOSTUNREACH;
-	else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
+	else if (!intf)
Did you mean to change the logic here by doing the calculation always? Does that change functionality?

Now the calculation of interface will be done always, which can be redundant. But this would not change the functionality.

Quoted 7 lines

The existing code is fine, running checkpatch on code outside of drivers/staging/ or on new patches you are writing, is generally discouraged as the code usually is older than checkpatch is :)
thanks,
greg k-h

Greg KH

maintainer

2023-08-16 14:43 UTC #5 lore raw

Quoted 27 lines · Atul Kumar Pant

Quoted 17 lines · Greg KH
Quoted 16 lines · Atul Kumar Pant
This patch fixes following checkpatch.pl issue:
ERROR: do not use assignment in if condition
Signed-off-by Atul Kumar Pant <[email protected]>
---
 drivers/usb/core/devio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/core/devio.c +0 -0
index e501a03d6c70..56899fed6bd4 100644
@@ -2333,9 +2333,10 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
 		}
 	}
 
Quoted 9 lines
drivers/usb/core/devio.c static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl) · line 2339
Quoted 5 lines
drivers/usb/core/devio.c +2 -1
+	intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
 	if (ps->dev->state != USB_STATE_CONFIGURED)
 		retval = -EHOSTUNREACH;
-	else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
+	else if (!intf)
Did you mean to change the logic here by doing the calculation always? Does that change functionality?

Quoted 2 lines

Now the calculation of interface will be done always, which can be redundant. But this would not change the functionality.

Yes, but it is now redundant, so this change isn't really needed, the original code is "more correct", please leave it as-is.

thanks,

greg k-h