[PATCH v2] gpiolib: sysfs: Fix error handling on failed export

[PATCH v2] gpiolib: sysfs: Fix error handling on failed export

[Boerge Struempfel]

If gpio_set_transitory() fails, we should free the gpio again. Most
notably, the flag FLAG_REQUESTED has previously been set in
gpiod_request_commit(), and should be reset on failure.

To my knowledge, this does not affect any current users, since the
gpio_set_transitory() mainly returns 0 and -ENOTSUPP, which is converted
to 0. However the gpio_set_transitory() function calles the .set_config()
function of the corresponding gpio chip and there are some gpio drivers in
which some (unlikely) branches return other values like -EPROBE_DEFER,
and EINVAL. In these cases, the above mentioned FLAG_REQUESTED would not
be reset, which results in the pin being blocked until the next reboot.

Signed-off-by: Boerge Struempfel <boerge.struempfel@gmail.com>

---

V1: https://lore.kernel.org/linux-gpio/CAEktqcuxS1sPfkGVCgSy1ki8fmUDmuUsHrdAT+zFKy5vGSoKPw@mail.gmail.com/T/#t

Changes from V1:
	- Marked all functions in commit message with parenthesis
	- Elaborated in commit message

 drivers/gpio/gpiolib-sysfs.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 6f309a3b2d9a..12d853845bb8 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -474,14 +474,17 @@ static ssize_t export_store(const struct class *class,
 		goto done;
 
 	status = gpiod_set_transitory(desc, false);
-	if (!status) {
-		status = gpiod_export(desc, true);
-		if (status < 0)
-			gpiod_free(desc);
-		else
-			set_bit(FLAG_SYSFS, &desc->flags);
+	if (status) {
+		gpiod_free(desc);
+		goto done;
 	}
 
+	status = gpiod_export(desc, true);
+	if (status < 0)
+		gpiod_free(desc);
+	else
+		set_bit(FLAG_SYSFS, &desc->flags);
+
 done:
 	if (status)
 		pr_debug("%s: status %d\n", __func__, status);
-- 
2.42.0

Re: [PATCH v2] gpiolib: sysfs: Fix error handling on failed export

[Andy Shevchenko]

On Tue, Nov 28, 2023 at 03:13:21PM +0100, Boerge Struempfel wrote:
> If gpio_set_transitory() fails, we should free the gpio again. Most

gpio --> GPIO descriptor
(I already mentioned capitalization in v1 review)

> notably, the flag FLAG_REQUESTED has previously been set in
> gpiod_request_commit(), and should be reset on failure.
> 
> To my knowledge, this does not affect any current users, since the
> gpio_set_transitory() mainly returns 0 and -ENOTSUPP, which is converted
> to 0. However the gpio_set_transitory() function calles the .set_config()
> function of the corresponding gpio chip and there are some gpio drivers in

gpio --> GPIO

> which some (unlikely) branches return other values like -EPROBE_DEFER,
> and EINVAL. In these cases, the above mentioned FLAG_REQUESTED would not

-EINVAL

> be reset, which results in the pin being blocked until the next reboot.

Fixes tag?
(`git log --no-merges --grep "Fixes:" will show you examples)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] gpiolib: sysfs: Fix error handling on failed export

[Börge Strümpfel]

Hello Andy

Thanks again for your feedback.

On Tue, Nov 28, 2023 at 5:29 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Tue, Nov 28, 2023 at 03:13:21PM +0100, Boerge Struempfel wrote:
> > If gpio_set_transitory() fails, we should free the gpio again. Most
>
> gpio --> GPIO descriptor
> (I already mentioned capitalization in v1 review)
>

I'm sorry, I misunderstood your comment "GPIO" in the v1 review. I fixed it for
the next version.

> > notably, the flag FLAG_REQUESTED has previously been set in
> > gpiod_request_commit(), and should be reset on failure.
> >
> > To my knowledge, this does not affect any current users, since the
> > gpio_set_transitory() mainly returns 0 and -ENOTSUPP, which is converted
> > to 0. However the gpio_set_transitory() function calles the .set_config()
> > function of the corresponding gpio chip and there are some gpio drivers in
>
> gpio --> GPIO
>

thanks

> > which some (unlikely) branches return other values like -EPROBE_DEFER,
> > and EINVAL. In these cases, the above mentioned FLAG_REQUESTED would not
>
> -EINVAL
>

thanks, I missed that, when I added the minus to all the other Error codes.

> > be reset, which results in the pin being blocked until the next reboot.
>
> Fixes tag?
> (`git log --no-merges --grep "Fixes:" will show you examples)
>

I thought it was optional. But I have added it for the next version.

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v2] gpiolib: sysfs: Fix error handling on failed export

[Andy Shevchenko]

On Wed, Nov 29, 2023 at 04:13:44PM +0100, Börge Strümpfel wrote:
> On Tue, Nov 28, 2023 at 5:29 PM Andy Shevchenko <andy@kernel.org> wrote:
> > On Tue, Nov 28, 2023 at 03:13:21PM +0100, Boerge Struempfel wrote:

...

> > Fixes tag?
> > (`git log --no-merges --grep "Fixes:" will show you examples)
> 
> I thought it was optional. But I have added it for the next version.

It's optional. We want to have this only when it's real fix at the table,
and to me this one fits. Otherwise, put into the comment area (after
the '---' cutter line) why it shouldn't be considered as a such.

-- 
With Best Regards,
Andy Shevchenko