[PATCH] gpiolib: don't allow setting values on input lines

[PATCH] gpiolib: don't allow setting values on input lines

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Some drivers as well as the character device and sysfs code check
whether the line actually is in output mode before allowing the user to
set a value.

However, GPIO value setters now return integer values and can indicate
failures. This allows us to move these checks into the core code.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c  |  3 ---
 drivers/gpio/gpiolib-sysfs.c | 12 +++++-------
 drivers/gpio/gpiolib.c       | 12 ++++++++++++
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 40f76a90fd7d..8da9c28d57f6 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1366,9 +1366,6 @@ static long linereq_set_values(struct linereq *lr, void __user *ip)
 	/* scan requested lines to determine the subset to be set */
 	for (num_set = 0, i = 0; i < lr->num_lines; i++) {
 		if (lv.mask & BIT_ULL(i)) {
-			/* setting inputs is not allowed */
-			if (!test_bit(FLAG_IS_OUT, &lr->lines[i].desc->flags))
-				return -EPERM;
 			/* add to compacted values */
 			if (lv.bits & BIT_ULL(i))
 				__set_bit(num_set, vals);
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 1acfa43bf1ab..4a3aa09dad9d 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -134,16 +134,14 @@ static ssize_t value_store(struct device *dev,
 	long value;
 
 	status = kstrtol(buf, 0, &value);
-
-	guard(mutex)(&data->mutex);
-
-	if (!test_bit(FLAG_IS_OUT, &desc->flags))
-		return -EPERM;
-
 	if (status)
 		return status;
 
-	gpiod_set_value_cansleep(desc, value);
+	guard(mutex)(&data->mutex);
+
+	status = gpiod_set_value_cansleep(desc, value);
+	if (status)
+		return status;
 
 	return size;
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e5eb3f0ee071..a4b746e80e57 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3592,6 +3592,9 @@ static int gpio_set_open_source_value_commit(struct gpio_desc *desc, bool value)
 
 static int gpiod_set_raw_value_commit(struct gpio_desc *desc, bool value)
 {
+	if (unlikely(!test_bit(FLAG_IS_OUT, &desc->flags)))
+		return -EPERM;
+
 	CLASS(gpio_chip_guard, guard)(desc);
 	if (!guard.gc)
 		return -ENODEV;
@@ -3663,6 +3666,12 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 		if (!can_sleep)
 			WARN_ON(array_info->gdev->can_sleep);
 
+		for (i = 0; i < array_size; i++) {
+			if (unlikely(!test_bit(FLAG_IS_OUT,
+					       &desc_array[i]->flags)))
+				return -EPERM;
+		}
+
 		guard(srcu)(&array_info->gdev->srcu);
 		gc = srcu_dereference(array_info->gdev->chip,
 				      &array_info->gdev->srcu);
@@ -3722,6 +3731,9 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 			int hwgpio = gpio_chip_hwgpio(desc);
 			int value = test_bit(i, value_bitmap);
 
+			if (unlikely(!test_bit(FLAG_IS_OUT, &desc->flags)))
+				return -EPERM;
+
 			/*
 			 * Pins applicable for fast input but not for
 			 * fast output processing may have been already

---
base-commit: 0a2f889128969dab41861b6e40111aa03dc57014
change-id: 20250311-gpio-set-check-output-8321c1859ae3

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH] gpiolib: don't allow setting values on input lines

[Linus Walleij]

On Tue, Mar 11, 2025 at 3:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Some drivers as well as the character device and sysfs code check
> whether the line actually is in output mode before allowing the user to
> set a value.
>
> However, GPIO value setters now return integer values and can indicate
> failures. This allows us to move these checks into the core code.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Makes sense, if there are regressions let's smoke them out
in linux-next.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] gpiolib: don't allow setting values on input lines

[Bartosz Golaszewski]

On Fri, Mar 14, 2025 at 11:33 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Mar 11, 2025 at 3:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Some drivers as well as the character device and sysfs code check
> > whether the line actually is in output mode before allowing the user to
> > set a value.
> >
> > However, GPIO value setters now return integer values and can indicate
> > failures. This allows us to move these checks into the core code.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Makes sense, if there are regressions let's smoke them out
> in linux-next.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks. I decided not to queue it for v6.15 for exactly that reason,
I'll pick it up early into the v6.16 cycle and let it sit in next for
several weeks.

Bart

Re: [PATCH] gpiolib: don't allow setting values on input lines

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Tue, 11 Mar 2025 15:19:51 +0100, Bartosz Golaszewski wrote:
> Some drivers as well as the character device and sysfs code check
> whether the line actually is in output mode before allowing the user to
> set a value.
> 
> However, GPIO value setters now return integer values and can indicate
> failures. This allows us to move these checks into the core code.
> 
> [...]

Applied, thanks!

[1/1] gpiolib: don't allow setting values on input lines
      commit: 92ac7de3175e3c17cbb76ae752b598cfb9dadf49

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH] gpiolib: don't allow setting values on input lines

[Andy Shevchenko]

On Fri, Mar 14, 2025 at 11:35:21AM +0100, Bartosz Golaszewski wrote:
> On Fri, Mar 14, 2025 at 11:33 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Mar 11, 2025 at 3:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Some drivers as well as the character device and sysfs code check
> > > whether the line actually is in output mode before allowing the user to
> > > set a value.
> > >
> > > However, GPIO value setters now return integer values and can indicate
> > > failures. This allows us to move these checks into the core code.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Makes sense, if there are regressions let's smoke them out
> > in linux-next.
> >
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Thanks. I decided not to queue it for v6.15 for exactly that reason,
> I'll pick it up early into the v6.16 cycle and let it sit in next for
> several weeks.

As far as I can tell from the reading of the code, this will break the open
drain emulation. Am I mistaken?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: don't allow setting values on input lines

[Bartosz Golaszewski]

On Wed, Apr 9, 2025 at 4:38 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Fri, Mar 14, 2025 at 11:35:21AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Mar 14, 2025 at 11:33 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Tue, Mar 11, 2025 at 3:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Some drivers as well as the character device and sysfs code check
> > > > whether the line actually is in output mode before allowing the user to
> > > > set a value.
> > > >
> > > > However, GPIO value setters now return integer values and can indicate
> > > > failures. This allows us to move these checks into the core code.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Makes sense, if there are regressions let's smoke them out
> > > in linux-next.
> > >
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >
> > Thanks. I decided not to queue it for v6.15 for exactly that reason,
> > I'll pick it up early into the v6.16 cycle and let it sit in next for
> > several weeks.
>
> As far as I can tell from the reading of the code, this will break the open
> drain emulation. Am I mistaken?
>

Could you produce a call trace where this could result in a breakage?
I tested open-drain and open-source emulation but maybe I'm missing
something.

Bartosz

Re: [PATCH] gpiolib: don't allow setting values on input lines

[Andy Shevchenko]

On Wed, Apr 09, 2025 at 06:43:47PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 9, 2025 at 4:38 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Fri, Mar 14, 2025 at 11:35:21AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Mar 14, 2025 at 11:33 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > On Tue, Mar 11, 2025 at 3:20 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > Some drivers as well as the character device and sysfs code check
> > > > > whether the line actually is in output mode before allowing the user to
> > > > > set a value.
> > > > >
> > > > > However, GPIO value setters now return integer values and can indicate
> > > > > failures. This allows us to move these checks into the core code.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Makes sense, if there are regressions let's smoke them out
> > > > in linux-next.
> > > >
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > >
> > > Thanks. I decided not to queue it for v6.15 for exactly that reason,
> > > I'll pick it up early into the v6.16 cycle and let it sit in next for
> > > several weeks.
> >
> > As far as I can tell from the reading of the code, this will break the open
> > drain emulation. Am I mistaken?
> 
> Could you produce a call trace where this could result in a breakage?

I can't right now, my comment was based on the my (mis)understanding of
the code flow.

> I tested open-drain and open-source emulation but maybe I'm missing
> something.

I;m glad to know that you tested this! So, it means that I misunderstood
something.

-- 
With Best Regards,
Andy Shevchenko