Message-ID: <CAMRc=Md0gD=XPEkb=C6JJcRvDpBbcJb5Xv8fE-v94iT=COHw7A@mail.gmail.com>
Date: Mon, 7 Apr 2025 14:49:27 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, linux-gpio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>, 
	Florian Fainelli <florian.fainelli@...adcom.com>, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v1 1/1] gpiolib: Make gpiod_put() error pointer aware

On Thu, Apr 3, 2025 at 3:22 PM Andy Shevchenko
<andriy.shevchenko@...ux.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@...ux.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

Powered by blists - more mailing lists