[PATCH v1] gpiolib: Separate GPIO_GET_LINEINFO_WATCH_IOCTL conditional

[PATCH v1] gpiolib: Separate GPIO_GET_LINEINFO_WATCH_IOCTL conditional

[Andy Shevchenko]

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 <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eaa0e209188d..8e3945eb41e3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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;
-- 
2.26.2

Re: [PATCH v1] gpiolib: Separate GPIO_GET_LINEINFO_WATCH_IOCTL conditional

[Bartosz Golaszewski]

pon., 25 maj 2020 o 21:40 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> 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 <andriy.shevchenko@linux.intel.com>
> ---

Well... fair enough.

Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Re: [PATCH v1] gpiolib: Separate GPIO_GET_LINEINFO_WATCH_IOCTL conditional

[Linus Walleij]

On Mon, May 25, 2020 at 9:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

 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 <andriy.shevchenko@linux.intel.com>

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

Yours,
Linus Walleij