Skip to contentSkip to conversation

usb: class: usblp: replace conditional statement with min()

PATCH In review Salah Triki 3 participants 3 messages Aug 3, 2021

Subsystems (2)
USB USB printer driver (usblp)
lore
Share & tools
lore

Salah Triki

author
#0 lore raw

PATCH 1/1 usb: class: usblp: replace conditional statement with min()

Replace conditional statement with min() in order to make code cleaner. Issue found by coccinelle.

Signed-off-by Salah Triki <[email protected]>
---
 drivers/usb/class/usblp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/class/usblp.c +1 -1
index f27b4aecff3d..fd779a989526 100644
@@ -264,7 +264,7 @@ static int usblp_ctrl_msg(struct usblp *usblp, int request, int type, int dir, i
 	dev_dbg(&usblp->intf->dev,
 		"usblp_control_msg: rq: 0x%02x dir: %d recip: %d value: %d idx: %d len: %#x result: %d\n",
 		request, !!dir, recip, value, index, len, retval);
-	return retval < 0 ? retval : 0;
+	return min(retval, 0);
 }
 
 #define usblp_read_status(usblp, status)\
Signature · 1 line
2.25.1
maintainer
#1 lore raw

Review

Quoted 8 lines · On Tue, 3 Aug 2021 01:28:06 +0100 Salah Triki <[email protected]> wrote:

Replace conditional statement with min() in order to make code cleaner. Issue found by coccinelle.

+++ b/drivers/usb/class/usblp.c

(quoted diff) +1 -1
 		request, !!dir, recip, value, index, len, retval);
-	return retval < 0 ? retval : 0;
+	return min(retval, 0);
 }

I'm very much against this change. The function min() is there for numeric values. But here we have a situation where we do one thing if there was an error, and another thing if there was no error.

This sort of abuse is exactly why blindly clicking heels whenever a tool tells you is not optimal.

If the objective is to shut the tool up, please consider the following instead:

drivers/usb/class/usblp.c +3 -1
index 9596e4279294..bbcbcf199fa9 100644
@@ -264,7 +264,9 @@ static int usblp_ctrl_msg(struct usblp *usblp, int request, int type, int dir, i
        dev_dbg(&usblp->intf->dev,
                "usblp_control_msg: rq: 0x%02x dir: %d recip: %d value: %d idx: %d len: %#x result: %d\n",
                request, !!dir, recip, value, index, len, retval);
-       return retval < 0 ? retval : 0;
+       if (retval < 0)
+               return retval;
+       return 0;
 }
 
 #define usblp_read_status(usblp, status)\

-- Pete

maintainer
#2 lore raw
Quoted 28 lines · Pete Zaitcev

drivers/usb/class/usblp.c static int usblp_ctrl_msg(struct usblp *usblp, int request, int type, int dir, i · line 270

Quoted 8 lines · On Tue, 3 Aug 2021 01:28:06 +0100 Salah Triki <[email protected]> wrote:

Replace conditional statement with min() in order to make code cleaner. Issue found by coccinelle.

+++ b/drivers/usb/class/usblp.c

drivers/usb/class/usblp.c +1 -1
 		request, !!dir, recip, value, index, len, retval);
-	return retval < 0 ? retval : 0;
+	return min(retval, 0);
 }

I'm very much against this change. The function min() is there for numeric values. But here we have a situation where we do one thing if there was an error, and another thing if there was no error.

This sort of abuse is exactly why blindly clicking heels whenever a tool tells you is not optimal.

If the objective is to shut the tool up, please consider the following instead:

drivers/usb/class/usblp.c +0 -0
index 9596e4279294..bbcbcf199fa9 100644

drivers/usb/class/usblp.c static int usblp_ctrl_msg(struct usblp *usblp, int request, int type, int dir, i · line 270

Quoted 8 lines
drivers/usb/class/usblp.c +3 -1
@@ -264,7 +264,9 @@ static int usblp_ctrl_msg(struct usblp *usblp, int request, int type, int dir, i
        dev_dbg(&usblp->intf->dev,
                "usblp_control_msg: rq: 0x%02x dir: %d recip: %d value: %d idx: %d len: %#x result: %d\n",
                request, !!dir, recip, value, index, len, retval);
-       return retval < 0 ? retval : 0;
+       if (retval < 0)
+               return retval;
+       return 0;
 }

I agree with Pete here, this is the "correct" fix for this, using min() is not ok here.

thanks,

greg k-h