[PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Andy Shevchenko]

When non-optional GPIO is requested and failed, the variable that holds
the (invalid) descriptor can contain an error pointer. However, gpiod_put()
ignores that fact and tries to cleanup never requested descriptor.
Make sure gpiod_put() ignores that as well.

While at it, do the same for the gpiod_put_array().

Note, it arguable needs to be present in the stubs as those are usually
called when CONFIG_GPIOLIB=n and GPIOs are requested using gpiod_get_optional()
or similar APIs.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eda4f51d6bb8..1e96b83d2670 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -5111,8 +5111,10 @@ EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
  */
 void gpiod_put(struct gpio_desc *desc)
 {
-	if (desc)
-		gpiod_free(desc);
+	if (IS_ERR_OR_NULL(desc))
+		return;
+
+	gpiod_free(desc);
 }
 EXPORT_SYMBOL_GPL(gpiod_put);
 
@@ -5124,6 +5126,9 @@ void gpiod_put_array(struct gpio_descs *descs)
 {
 	unsigned int i;
 
+	if (IS_ERR_OR_NULL(descs))
+		return;
+
 	for (i = 0; i < descs->ndescs; i++)
 		gpiod_put(descs->desc[i]);
 
-- 
2.47.2

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Bartosz Golaszewski]

On Wed, Apr 2, 2025 at 5:20 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> When non-optional GPIO is requested and failed, the variable that holds
> the (invalid) descriptor can contain an error pointer. However, gpiod_put()
> ignores that fact and tries to cleanup never requested descriptor.
> Make sure gpiod_put() ignores that as well.
>
> While at it, do the same for the gpiod_put_array().
>
> Note, it arguable needs to be present in the stubs as those are usually
> called when CONFIG_GPIOLIB=n and GPIOs are requested using gpiod_get_optional()
> or similar APIs.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

I'm not a fan of this. Silently ignoring NULL makes sense in the
context of _optional() calls where we want to do nothing on GPIOs that
aren't there. But this encourages people to get sloppy and just ignore
error pointers returned from gpiod_get()? Also: all other calls error
out on IS_ERR(desc) so why would we make it an exception? If anything,
the broadcom SPI driver this is about should store the return value of
gpiod_get() in a local variable, check it and then assign NULL to the
actual descriptor stored in the driver data.

We return errors for a reason, I don't like the idea of just ignoring
them in gpiod_put().

Bartosz

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 08:58:09AM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 2, 2025 at 5:20 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > When non-optional GPIO is requested and failed, the variable that holds
> > the (invalid) descriptor can contain an error pointer. However, gpiod_put()
> > ignores that fact and tries to cleanup never requested descriptor.
> > Make sure gpiod_put() ignores that as well.
> >
> > While at it, do the same for the gpiod_put_array().
> >
> > Note, it arguable needs to be present in the stubs as those are usually
> > called when CONFIG_GPIOLIB=n and GPIOs are requested using gpiod_get_optional()
> > or similar APIs.

> I'm not a fan of this. Silently ignoring NULL makes sense in the
> context of _optional() calls where we want to do nothing on GPIOs that
> aren't there.

> But this encourages people to get sloppy and just ignore
> error pointers returned from gpiod_get()?

From where did you come to this conclusion, please? We have many subsystems
that ignore invalid resource on the release stage, starting from platform
device driver core.

> Also: all other calls error out on IS_ERR(desc) so why would we make it an
> exception?

Because it's _release_ stage that participates in the cleaning up of
the allocated resources in error paths. It's a common approach in
the kernel. I would rather ask what makes GPIOLIB so special about it?

> If anything, the broadcom SPI driver this is about should store the return
> value of gpiod_get() in a local variable, check it and then assign NULL to
> the actual descriptor stored in the driver data.

Broadcom SPI driver just reveals this disadvantage in GPIOLIB.

> We return errors for a reason, I don't like the idea of just ignoring
> them in gpiod_put().

Yes, how does one links to the other, please?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Bartosz Golaszewski]

On Thu, Apr 3, 2025 at 10:04 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 03, 2025 at 08:58:09AM +0200, Bartosz Golaszewski wrote:
> > On Wed, Apr 2, 2025 at 5:20 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > When non-optional GPIO is requested and failed, the variable that holds
> > > the (invalid) descriptor can contain an error pointer. However, gpiod_put()
> > > ignores that fact and tries to cleanup never requested descriptor.
> > > Make sure gpiod_put() ignores that as well.
> > >
> > > While at it, do the same for the gpiod_put_array().
> > >
> > > Note, it arguable needs to be present in the stubs as those are usually
> > > called when CONFIG_GPIOLIB=n and GPIOs are requested using gpiod_get_optional()
> > > or similar APIs.
>
> > I'm not a fan of this. Silently ignoring NULL makes sense in the
> > context of _optional() calls where we want to do nothing on GPIOs that
> > aren't there.
>
> > But this encourages people to get sloppy and just ignore
> > error pointers returned from gpiod_get()?
>
> From where did you come to this conclusion, please? We have many subsystems
> that ignore invalid resource on the release stage, starting from platform
> device driver core.
>

The fact that many people do something does not mean it's correct.
Many other subsystem scream loudly when that happens, so I would be ok
with adding a big WARN_ON(IS_ERR(desc)).

> > Also: all other calls error out on IS_ERR(desc) so why would we make it an
> > exception?
>
> Because it's _release_ stage that participates in the cleaning up of
> the allocated resources in error paths. It's a common approach in
> the kernel. I would rather ask what makes GPIOLIB so special about it?
>

Just because it's the release stage, does not mean you shouldn't care
about the correctness of the consumer code. Passing an IS_ERR(descr)
to any of the GPIO APIs can happen if the user ignores an error
returned by gpiod_get(). That's not alright.

Bart

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 10:20:08AM +0200, Bartosz Golaszewski wrote:
> On Thu, Apr 3, 2025 at 10:04 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Apr 03, 2025 at 08:58:09AM +0200, Bartosz Golaszewski wrote:
> > > On Wed, Apr 2, 2025 at 5:20 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

> > > > When non-optional GPIO is requested and failed, the variable that holds
> > > > the (invalid) descriptor can contain an error pointer. However, gpiod_put()
> > > > ignores that fact and tries to cleanup never requested descriptor.
> > > > Make sure gpiod_put() ignores that as well.
> > > >
> > > > While at it, do the same for the gpiod_put_array().
> > > >
> > > > Note, it arguable needs to be present in the stubs as those are usually
> > > > called when CONFIG_GPIOLIB=n and GPIOs are requested using gpiod_get_optional()
> > > > or similar APIs.
> >
> > > I'm not a fan of this. Silently ignoring NULL makes sense in the
> > > context of _optional() calls where we want to do nothing on GPIOs that
> > > aren't there.
> >
> > > But this encourages people to get sloppy and just ignore
> > > error pointers returned from gpiod_get()?
> >
> > From where did you come to this conclusion, please? We have many subsystems
> > that ignore invalid resource on the release stage, starting from platform
> > device driver core.
> 
> The fact that many people do something does not mean it's correct.

And it doesn't tell it is incorrect either. We are going to conclude that there
are pros and cons on each of the approaches, but I don't see much a point in
yours, sorry.

> Many other subsystem scream loudly when that happens, so I would be ok
> with adding a big WARN_ON(IS_ERR(desc)).

I disagree. This is not that case where passing an error pointer should be
an issue.

> > > Also: all other calls error out on IS_ERR(desc) so why would we make it an
> > > exception?
> >
> > Because it's _release_ stage that participates in the cleaning up of
> > the allocated resources in error paths. It's a common approach in
> > the kernel. I would rather ask what makes GPIOLIB so special about it?
> 
> Just because it's the release stage, does not mean you shouldn't care
> about the correctness of the consumer code. Passing an IS_ERR(descr)
> to any of the GPIO APIs can happen if the user ignores an error
> returned by gpiod_get(). That's not alright.

Have you ever seen such a code in the cases when it's okay (like in platform
device driver users)? I do not. So, the above is based on the hypothetical
assumption that somebody will make silly things. If you _really_ care about
checking the error, add __must_check to the respective functions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Bartosz Golaszewski]

On Thu, Apr 3, 2025 at 1:17 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > > But this encourages people to get sloppy and just ignore
> > > > error pointers returned from gpiod_get()?
> > >
> > > From where did you come to this conclusion, please? We have many subsystems
> > > that ignore invalid resource on the release stage, starting from platform
> > > device driver core.
> >
> > The fact that many people do something does not mean it's correct.
>
> And it doesn't tell it is incorrect either. We are going to conclude that there
> are pros and cons on each of the approaches, but I don't see much a point in
> yours, sorry.
>

My point is:

You get a descriptor with gpiod_get_optional(). You can get three
types of a pointer back:

1. Valid descriptor: you can use it in all GPIO consumer functions.
2. NULL-pointer: you can still use it in all GPIO consumer functions.
They will act as if they succeeded. This is expected as this is how
the "optional" functionality is implemented. Had it been written in
rust, we'd do it better but we use the tools we have. It's very much a
"valid" value to pass to gpiod routines.
3. IS_ERR() value. If you try to pass it to any of the GPIO consumer
functions, they will return it back to you and print a warning. Why?
Because it's an invalid object. And there's no reason to make
gpiod_put() an exemption. You never could have used an IS_ERR()
correctly. Look at what devres does - if it got an IS_ERR(), it never
schedules a release action.

> > Many other subsystem scream loudly when that happens, so I would be ok
> > with adding a big WARN_ON(IS_ERR(desc)).
>
> I disagree. This is not that case where passing an error pointer should be
> an issue.
>
> > > > Also: all other calls error out on IS_ERR(desc) so why would we make it an
> > > > exception?
> > >
> > > Because it's _release_ stage that participates in the cleaning up of
> > > the allocated resources in error paths. It's a common approach in
> > > the kernel. I would rather ask what makes GPIOLIB so special about it?
> >
> > Just because it's the release stage, does not mean you shouldn't care
> > about the correctness of the consumer code. Passing an IS_ERR(descr)
> > to any of the GPIO APIs can happen if the user ignores an error
> > returned by gpiod_get(). That's not alright.
>
> Have you ever seen such a code in the cases when it's okay (like in platform
> device driver users)? I do not. So, the above is based on the hypothetical
> assumption that somebody will make silly things. If you _really_ care about
> checking the error, add __must_check to the respective functions.
>

They already have but people do the following (like in the affected SPI driver):

struct driver_data *data = kzalloc();

Then elsewhere:

data->gpiod = gpiod_get();
if (IS_ERR(data->gpiod))
    return PTR_ERR(data->gpiod);

The data struct now contains the IS_ERR() value. I don't think it
makes any sense for it to carry it around and I don't want to enable
it.

Bart

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 02:23:24PM +0200, Bartosz Golaszewski wrote:
> On Thu, Apr 3, 2025 at 1:17 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > > But this encourages people to get sloppy and just ignore
> > > > > error pointers returned from gpiod_get()?
> > > >
> > > > From where did you come to this conclusion, please? We have many subsystems
> > > > that ignore invalid resource on the release stage, starting from platform
> > > > device driver core.
> > >
> > > The fact that many people do something does not mean it's correct.
> >
> > And it doesn't tell it is incorrect either. We are going to conclude that there
> > are pros and cons on each of the approaches, but I don't see much a point in
> > yours, sorry.
> 
> My point is:

I understand your point, but I disagree with the omni appliance of 3.

> You get a descriptor with gpiod_get_optional(). You can get three
> types of a pointer back:
> 
> 1. Valid descriptor: you can use it in all GPIO consumer functions.
> 2. NULL-pointer: you can still use it in all GPIO consumer functions.
> They will act as if they succeeded. This is expected as this is how
> the "optional" functionality is implemented. Had it been written in
> rust, we'd do it better but we use the tools we have. It's very much a
> "valid" value to pass to gpiod routines.
> 3. IS_ERR() value. If you try to pass it to any of the GPIO consumer
> functions, they will return it back to you and print a warning. Why?
> Because it's an invalid object. And there's no reason to make
> gpiod_put() an exemption.

No, the release is special as it's used in error paths and there often
it is simpler for all just to ignore invalid pointers rather then put
a burden on the driver developers. The gpiod_set_*() and gpiod_get_*() over
_assumed requested_ descriptor of course should fail any attempt to be supplied
with an invalid pointer.

> You never could have used an IS_ERR()
> correctly. Look at what devres does - if it got an IS_ERR(), it never
> schedules a release action.

This is again unrelated. devres is an upper layer and we don't care about its
implementation which is logically correct since we assume to put on the list
only _valid_ resources.


> > > Many other subsystem scream loudly when that happens, so I would be ok
> > > with adding a big WARN_ON(IS_ERR(desc)).
> >
> > I disagree. This is not that case where passing an error pointer should be
> > an issue.
> >
> > > > > Also: all other calls error out on IS_ERR(desc) so why would we make it an
> > > > > exception?
> > > >
> > > > Because it's _release_ stage that participates in the cleaning up of
> > > > the allocated resources in error paths. It's a common approach in
> > > > the kernel. I would rather ask what makes GPIOLIB so special about it?
> > >
> > > Just because it's the release stage, does not mean you shouldn't care
> > > about the correctness of the consumer code. Passing an IS_ERR(descr)
> > > to any of the GPIO APIs can happen if the user ignores an error
> > > returned by gpiod_get(). That's not alright.
> >
> > Have you ever seen such a code in the cases when it's okay (like in platform
> > device driver users)? I do not. So, the above is based on the hypothetical
> > assumption that somebody will make silly things. If you _really_ care about
> > checking the error, add __must_check to the respective functions.
> 
> They already have but people do the following (like in the affected SPI driver):
> 
> struct driver_data *data = kzalloc();
> 
> Then elsewhere:
> 
> data->gpiod = gpiod_get();
> if (IS_ERR(data->gpiod))
>     return PTR_ERR(data->gpiod);
> 
> The data struct now contains the IS_ERR() value. I don't think it
> makes any sense for it to carry it around and I don't want to enable
> it.

It's up to the driver developer.

Again, if you want the result of gpiod_get() to be tested, mark it so.
That's why I think your initial point is mistargetting.

Ask yourself, why we don't fail kfree() on a NULL object? (from kfree() p.o.v.
it's an invalid one) Or if you want to be more precise in analogue with the
gpiod_get_optional(), consider

	p = kmalloc(0);
	kfree(p);

and this is valid code despite p being not NULL!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Bartosz Golaszewski]

On Thu, Apr 3, 2025 at 3:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 03, 2025 at 02:23:24PM +0200, Bartosz Golaszewski wrote:
> > On Thu, Apr 3, 2025 at 1:17 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > > But this encourages people to get sloppy and just ignore
> > > > > > error pointers returned from gpiod_get()?
> > > > >
> > > > > From where did you come to this conclusion, please? We have many subsystems
> > > > > that ignore invalid resource on the release stage, starting from platform
> > > > > device driver core.
> > > >
> > > > The fact that many people do something does not mean it's correct.
> > >
> > > And it doesn't tell it is incorrect either. We are going to conclude that there
> > > are pros and cons on each of the approaches, but I don't see much a point in
> > > yours, sorry.
> >
> > My point is:
>
> I understand your point, but I disagree with the omni appliance of 3.
>
> > You get a descriptor with gpiod_get_optional(). You can get three
> > types of a pointer back:
> >
> > 1. Valid descriptor: you can use it in all GPIO consumer functions.
> > 2. NULL-pointer: you can still use it in all GPIO consumer functions.
> > They will act as if they succeeded. This is expected as this is how
> > the "optional" functionality is implemented. Had it been written in
> > rust, we'd do it better but we use the tools we have. It's very much a
> > "valid" value to pass to gpiod routines.
> > 3. IS_ERR() value. If you try to pass it to any of the GPIO consumer
> > functions, they will return it back to you and print a warning. Why?
> > Because it's an invalid object. And there's no reason to make
> > gpiod_put() an exemption.
>
> No, the release is special as it's used in error paths and there often
> it is simpler for all just to ignore invalid pointers rather then put
> a burden on the driver developers. The gpiod_set_*() and gpiod_get_*() over
> _assumed requested_ descriptor of course should fail any attempt to be supplied
> with an invalid pointer.
>
> > You never could have used an IS_ERR()
> > correctly. Look at what devres does - if it got an IS_ERR(), it never
> > schedules a release action.
>
> This is again unrelated. devres is an upper layer and we don't care about its
> implementation which is logically correct since we assume to put on the list
> only _valid_ resources.
>
>
> > > > Many other subsystem scream loudly when that happens, so I would be ok
> > > > with adding a big WARN_ON(IS_ERR(desc)).
> > >
> > > I disagree. This is not that case where passing an error pointer should be
> > > an issue.
> > >
> > > > > > Also: all other calls error out on IS_ERR(desc) so why would we make it an
> > > > > > exception?
> > > > >
> > > > > Because it's _release_ stage that participates in the cleaning up of
> > > > > the allocated resources in error paths. It's a common approach in
> > > > > the kernel. I would rather ask what makes GPIOLIB so special about it?
> > > >
> > > > Just because it's the release stage, does not mean you shouldn't care
> > > > about the correctness of the consumer code. Passing an IS_ERR(descr)
> > > > to any of the GPIO APIs can happen if the user ignores an error
> > > > returned by gpiod_get(). That's not alright.
> > >
> > > Have you ever seen such a code in the cases when it's okay (like in platform
> > > device driver users)? I do not. So, the above is based on the hypothetical
> > > assumption that somebody will make silly things. If you _really_ care about
> > > checking the error, add __must_check to the respective functions.
> >
> > They already have but people do the following (like in the affected SPI driver):
> >
> > struct driver_data *data = kzalloc();
> >
> > Then elsewhere:
> >
> > data->gpiod = gpiod_get();
> > if (IS_ERR(data->gpiod))
> >     return PTR_ERR(data->gpiod);
> >
> > The data struct now contains the IS_ERR() value. I don't think it
> > makes any sense for it to carry it around and I don't want to enable
> > it.
>
> It's up to the driver developer.
>
> Again, if you want the result of gpiod_get() to be tested, mark it so.
> That's why I think your initial point is mistargetting.
>

It's already annotated, I said it before.

> Ask yourself, why we don't fail kfree() on a NULL object? (from kfree() p.o.v.
> it's an invalid one) Or if you want to be more precise in analogue with the
> gpiod_get_optional(), consider
>
>         p = kmalloc(0);
>         kfree(p);
>
> and this is valid code despite p being not NULL!

We're discussing an interface returning an object, not a simple buffer
so it's a bad example. Try feeding an IS_ERR() to regmap_exit() or
numerous other deinit functions and tell me if that works.

I explained why I believe this change is wrong and I will allow myself
to not accept it unless Linus is very positively in favor.

Bartosz

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Andy Shevchenko]

On Mon, Apr 07, 2025 at 02:49:27PM +0200, Bartosz Golaszewski wrote:
> On Thu, Apr 3, 2025 at 3:22 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:

...

> I explained why I believe this change is wrong and I will allow myself
> to not accept it unless Linus is very positively in favor.

I am on the opposite site. With all burden on the users' shoulders...

And your _any deinit_ function example is wrong. This is not a deinit case,
this is resource allocation / reservation / etc and freeing it. Freeing
invalid resource is bad, freeing an optional resource, or resource which
has never been allocated — is okay. It makes device driver developer's life
easier. And many kernel APIs are written in that form. Since you again
pointed out that gpiod_get() is annotated, there is very unlikely somebody
deliberately will ignore those errors.

P.S.
I will continue insisting that this is an inconvenience (or bug) in gpiod_put().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

[Linus Walleij]

On Mon, Apr 7, 2025 at 2:49 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> I explained why I believe this change is wrong and I will allow myself
> to not accept it unless Linus is very positively in favor.

I am neutral on this unfortunately so my opinion doesn't help...
I'm simply fine either way :/

Yours,
Linus Walleij