gpiolib: Separate GPIO_GET_LINEINFO_WATCH_IOCTL conditional

PATCH In review Andy Shevchenko 3 participants 3 messages May 25, 2020 – May 27, 2020 GPIO

Share & tools

Andy Shevchenko

author

2020-05-25 19:40 UTC #0 lore raw

PATCH 1/1 gpiolib: Separate GPIO_GET_LINEINFO_WATCH_IOCTL conditional

We already have two conditionals inside the outer one to check if the command is GPIO_GET_LINEINFO_WATCH_IOCTL. I think it's time to actually do what I have proposed in the first place, i.e. to separate GPIO_GET_LINEINFO_WATCH_IOCTL from GPIO_GET_LINEINFO_IOCTL. It's +13 LOCs, and surprisingly only +13 bytes of binary on x86_32, but for the price of much better readability.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/gpio/gpiolib.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

drivers/gpio/gpiolib.c +23 ~~-10~~

index eaa0e209188d..8e3945eb41e3 100644
@@ -1272,8 +1272,7 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
 			return -EFAULT;
 		return 0;
-	} else if (cmd == GPIO_GET_LINEINFO_IOCTL ||
-		   cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
+	} else if (cmd == GPIO_GET_LINEINFO_IOCTL) {
 		struct gpioline_info lineinfo;
 
 		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))

@@ -1285,23 +1284,37 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 		hwgpio = gpio_chip_hwgpio(desc);
 
-		if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL &&
-		    test_bit(hwgpio, priv->watched_lines))
-			return -EBUSY;
-
 		gpio_desc_to_lineinfo(desc, &lineinfo);
 
 		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
 			return -EFAULT;
-
-		if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL)
-			set_bit(hwgpio, priv->watched_lines);
-
 		return 0;
 	} else if (cmd == GPIO_GET_LINEHANDLE_IOCTL) {
 		return linehandle_create(gdev, ip);
 	} else if (cmd == GPIO_GET_LINEEVENT_IOCTL) {
 		return lineevent_create(gdev, ip);
+	} else if (cmd == GPIO_GET_LINEINFO_WATCH_IOCTL) {
+		struct gpioline_info lineinfo;
+
+		if (copy_from_user(&lineinfo, ip, sizeof(lineinfo)))
+			return -EFAULT;
+
+		desc = gpiochip_get_desc(gc, lineinfo.line_offset);
+		if (IS_ERR(desc))
+			return PTR_ERR(desc);
+
+		hwgpio = gpio_chip_hwgpio(desc);
+
+		if (test_bit(hwgpio, priv->watched_lines))
+			return -EBUSY;
+
+		gpio_desc_to_lineinfo(desc, &lineinfo);
+
+		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
+			return -EFAULT;
+
+		set_bit(hwgpio, priv->watched_lines);
+		return 0;
 	} else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) {
 		if (copy_from_user(&offset, ip, sizeof(offset)))
 			return -EFAULT;

Signature · 1 line

2.26.2

Bartosz Golaszewski

Acked-by on v1 1/1

2020-05-27 12:24 UTC #1 lore raw

pon., 25 maj 2020 o 21:40 Andy Shevchenko
<[email protected]> napisał(a):

Quoted 10 lines

We already have two conditionals inside the outer one to check if the command is GPIO_GET_LINEINFO_WATCH_IOCTL. I think it's time to actually do what I have proposed in the first place, i.e. to separate GPIO_GET_LINEINFO_WATCH_IOCTL from GPIO_GET_LINEINFO_IOCTL. It's +13 LOCs, and surprisingly only +13 bytes of binary on x86_32, but for the price of much better readability.
Signed-off-by Andy Shevchenko <[email protected]>
---

Well... fair enough.

Acked-by Bartosz Golaszewski <[email protected]>

Linus Walleij

2020-05-27 14:16 UTC #2 lore raw

In reply to Andy Shevchenko

On Mon, May 25, 2020 at 9:40 PM Andy Shevchenko
<[email protected]> wrote:

We already have two conditionals inside the outer one to check

Quoted 7 lines

if the command is GPIO_GET_LINEINFO_WATCH_IOCTL. I think it's time to actually do what I have proposed in the first place, i.e. to separate GPIO_GET_LINEINFO_WATCH_IOCTL from GPIO_GET_LINEINFO_IOCTL. It's +13 LOCs, and surprisingly only +13 bytes of binary on x86_32, but for the price of much better readability.
Signed-off-by Andy Shevchenko <[email protected]>

Patch applied, had to merge in v5.7-rc7 for it to apply but I guess that was expected, thanks!

Yours,
Linus Walleij