Is consumer documentation correct?

Is consumer documentation correct?

[Andy Shevchenko]

Hi!

It _seems_ between
  79a9becda894 ("gpiolib: export descriptor-based GPIO interface")
and
  14e85c0e69d5 ("gpio: remove gpio_descs global array")
the "Interacting With the Legacy GPIO Subsystem" of GPIO consumer
interface is correct.

But is it the case afterwards?

Details of the question are here [1].

[1]: https://stackoverflow.com/q/64455505/2511795

-- 
With Best Regards,
Andy Shevchenko

Re: Is consumer documentation correct?

[Linus Walleij]

On Thu, Oct 22, 2020 at 6:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> It _seems_ between
>   79a9becda894 ("gpiolib: export descriptor-based GPIO interface")
> and
>   14e85c0e69d5 ("gpio: remove gpio_descs global array")
> the "Interacting With the Legacy GPIO Subsystem" of GPIO consumer
> interface is correct.
>
> But is it the case afterwards?
>
> Details of the question are here [1].
>
> [1]: https://stackoverflow.com/q/64455505/2511795

I don't know if I even understand the question right, but gpio_to_desc()
can be called at any time as long as there is a struct gpio_chip with
the global GPIO number assigned to it. It will just index into the
array of descriptors stored inside the chip and return a pointer.
The allocation of the memory lives with the struct gpio_chip.

If for example that chip is on USB and you unplug that USB
cable, naturally referencing the descriptor after that point
will cause disasters.

The gpio_request()/free() doesn't really allocate or free
any memory at all. This is why we call gpio_to_desc() first
thing in the legacy functions.

Yours,
Linus Walleij

Re: Is consumer documentation correct?

[Andy Shevchenko]

On Wed, Oct 28, 2020 at 6:15 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Oct 22, 2020 at 6:26 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
> > It _seems_ between
> >   79a9becda894 ("gpiolib: export descriptor-based GPIO interface")
> > and
> >   14e85c0e69d5 ("gpio: remove gpio_descs global array")
> > the "Interacting With the Legacy GPIO Subsystem" of GPIO consumer
> > interface is correct.
> >
> > But is it the case afterwards?
> >
> > Details of the question are here [1].
> >
> > [1]: https://stackoverflow.com/q/64455505/2511795
>
> I don't know if I even understand the question right, but gpio_to_desc()
> can be called at any time as long as there is a struct gpio_chip with
> the global GPIO number assigned to it. It will just index into the
> array of descriptors stored inside the chip and return a pointer.
> The allocation of the memory lives with the struct gpio_chip.
>
> If for example that chip is on USB and you unplug that USB
> cable, naturally referencing the descriptor after that point
> will cause disasters.
>
> The gpio_request()/free() doesn't really allocate or free
> any memory at all. This is why we call gpio_to_desc() first
> thing in the legacy functions.

The section titled "Interacting With the Legacy GPIO Subsystem"
describes as far as I got it the interaction of gpio_to_desc() and
desc_to_gpio() with new / legacy APIs along with their scope. But is
that description correct after the above mentioned commit, i.e.
14e85c0e69d5 ("gpio: remove gpio_descs global array") when we lose the
always-present data structure (if I'm not mistaken)?

-- 
With Best Regards,
Andy Shevchenko

Re: Is consumer documentation correct?

[Linus Walleij]

On Wed, Oct 28, 2020 at 9:29 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> The section titled "Interacting With the Legacy GPIO Subsystem"
> describes as far as I got it the interaction of gpio_to_desc() and
> desc_to_gpio() with new / legacy APIs along with their scope. But is
> that description correct after the above mentioned commit, i.e.
> 14e85c0e69d5 ("gpio: remove gpio_descs global array") when we lose the
> always-present data structure (if I'm not mistaken)?

Yes I think it needs to be updated...

Yours,
Linus Walleij

Re: Is consumer documentation correct?

[Alexandre Courbot]

Hi! Sorry for the delayed reply.

On Thu, Oct 29, 2020 at 10:50 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Oct 28, 2020 at 9:29 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
> > The section titled "Interacting With the Legacy GPIO Subsystem"
> > describes as far as I got it the interaction of gpio_to_desc() and
> > desc_to_gpio() with new / legacy APIs along with their scope. But is
> > that description correct after the above mentioned commit, i.e.
> > 14e85c0e69d5 ("gpio: remove gpio_descs global array") when we lose the
> > always-present data structure (if I'm not mistaken)?
>
> Yes I think it needs to be updated...

Although there is no global array anymore, gpio_to_desc() should still
perform as expected after 14e85c0e69d5 ("gpio: remove gpio_descs
global array"), since it parses all the registered gpio chips and
looks for the one which range includes the requested number.

desc_to_gpio() is a bit simpler, as it simply returns the GPIO number
corresponding to a valid descriptor. The descriptor must remain
acquired as long as the GPIO is in use through its number. I think the
misleading sentence is

"The GPIO number returned by desc_to_gpio() can be safely used as long
as the GPIO descriptor has not been freed."

This has been written a long time ago, so maybe I am mistaken, but I
suspect the intent was to say something along the lines of "... as the
GPIO descriptor has not been *released* (using gpiod_put()).

For gpio_to_desc(), the intent of "a GPIO number passed to
gpio_to_desc() must have been properly acquired", is to say "...
properly acquired using gpio_request()".

As for "and usage of the returned GPIO descriptor is only possible
after the GPIO number has been released", I am a bit puzzled. My
understanding is that the descriptor is only guaranteed to be valid
between calls to gpio_request() and gpio_free(), so that's probably a
mistake here?

Re: Is consumer documentation correct?

[Andy Shevchenko]

On Fri, Oct 30, 2020 at 4:16 PM Alexandre Courbot <gnurou@gmail.com> wrote:
>
> Hi! Sorry for the delayed reply.
>
> On Thu, Oct 29, 2020 at 10:50 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Wed, Oct 28, 2020 at 9:29 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >
> > > The section titled "Interacting With the Legacy GPIO Subsystem"
> > > describes as far as I got it the interaction of gpio_to_desc() and
> > > desc_to_gpio() with new / legacy APIs along with their scope. But is
> > > that description correct after the above mentioned commit, i.e.
> > > 14e85c0e69d5 ("gpio: remove gpio_descs global array") when we lose the
> > > always-present data structure (if I'm not mistaken)?
> >
> > Yes I think it needs to be updated...
>
> Although there is no global array anymore, gpio_to_desc() should still
> perform as expected after 14e85c0e69d5 ("gpio: remove gpio_descs
> global array"), since it parses all the registered gpio chips and
> looks for the one which range includes the requested number.
>
> desc_to_gpio() is a bit simpler, as it simply returns the GPIO number
> corresponding to a valid descriptor. The descriptor must remain
> acquired as long as the GPIO is in use through its number. I think the
> misleading sentence is
>
> "The GPIO number returned by desc_to_gpio() can be safely used as long
> as the GPIO descriptor has not been freed."
>
> This has been written a long time ago, so maybe I am mistaken, but I
> suspect the intent was to say something along the lines of "... as the
> GPIO descriptor has not been *released* (using gpiod_put()).
>
> For gpio_to_desc(), the intent of "a GPIO number passed to
> gpio_to_desc() must have been properly acquired", is to say "...
> properly acquired using gpio_request()".
>
> As for "and usage of the returned GPIO descriptor is only possible
> after the GPIO number has been released", I am a bit puzzled. My
> understanding is that the descriptor is only guaranteed to be valid
> between calls to gpio_request() and gpio_free(), so that's probably a
> mistake here?

Funny thing is that it is you who is the author of fd8e198cfcaa
("Documentation: gpiolib: document new interface"). Or am I mistaken?

Perhaps you can send a follow up to amend that chapter?

-- 
With Best Regards,
Andy Shevchenko

Re: Is consumer documentation correct?

[Andy Shevchenko]

On Fri, Oct 30, 2020 at 4:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Oct 30, 2020 at 4:16 PM Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Thu, Oct 29, 2020 at 10:50 PM Linus Walleij <linus.walleij@linaro.org> wrote:

...

> > As for "and usage of the returned GPIO descriptor is only possible
> > after the GPIO number has been released", I am a bit puzzled.

So does the OP of [1].
Have you had a chance to read that?

> My
> > understanding is that the descriptor is only guaranteed to be valid
> > between calls to gpio_request() and gpio_free(), so that's probably a
> > mistake here?

Sounds like this and my investigation tells me that this mistake is a
result of the global array removal w/o updating documentation part.

> Funny thing is that it is you who is the author of fd8e198cfcaa
> ("Documentation: gpiolib: document new interface"). Or am I mistaken?
>
> Perhaps you can send a follow up to amend that chapter?

[1]: https://stackoverflow.com/q/64455505/2511795

-- 
With Best Regards,
Andy Shevchenko

Re: Is consumer documentation correct?

[Alexandre Courbot]

On Fri, Oct 30, 2020 at 11:29 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Oct 30, 2020 at 4:26 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Oct 30, 2020 at 4:16 PM Alexandre Courbot <gnurou@gmail.com> wrote:
> > > On Thu, Oct 29, 2020 at 10:50 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> ...
>
> > > As for "and usage of the returned GPIO descriptor is only possible
> > > after the GPIO number has been released", I am a bit puzzled.
>
> So does the OP of [1].
> Have you had a chance to read that?
>
> > My
> > > understanding is that the descriptor is only guaranteed to be valid
> > > between calls to gpio_request() and gpio_free(), so that's probably a
> > > mistake here?
>
> Sounds like this and my investigation tells me that this mistake is a
> result of the global array removal w/o updating documentation part.

I have written a reply to the SO post:
https://stackoverflow.com/a/64610316/251248

The important thing here is that gpio_acquire() and gpio_free() call
gpiod_request() and gpiod_free() under the hood. So all the lifetimes
are ultimately managed through GPIO descriptors. This means that when
gpio_acquire() is called, the descriptor's refcount is increased by
one, and thus it is safe to use it until the refcount is decreased by
the corresponding call to gpio_free() (which will call gpiod_free()).

For the opposite operation, desc_to_gpio(), the same logic applies:
the descriptor has been acquired using gpiod_get(), and so legacy
gpio_*() functions, which ultimately use that descriptor, can be
called safely as long as you know the magic number and until
gpiod_put() is called on the descriptor.

So yeah the documentation (which I have written as far as I can
remember) looks a bit clumsy at best and I probably should amend it a
bit. Now I don't want to give the impression that these functions
should be used as they absolutely should not. :)

>
> > Funny thing is that it is you who is the author of fd8e198cfcaa
> > ("Documentation: gpiolib: document new interface"). Or am I mistaken?
> >
> > Perhaps you can send a follow up to amend that chapter?
>
> [1]: https://stackoverflow.com/q/64455505/2511795
>
> --
> With Best Regards,
> Andy Shevchenko

Re: Is consumer documentation correct?

[Linus Walleij]

Hi Alexandre,

long time no C! :D

On Fri, Oct 30, 2020 at 3:36 PM Alexandre Courbot <gnurou@gmail.com> wrote:

> So yeah the documentation (which I have written as far as I can
> remember) looks a bit clumsy at best and I probably should amend it a
> bit. Now I don't want to give the impression that these functions
> should be used as they absolutely should not. :)

Yes pretty please with sugar on top can you send a patch to clear
this up once and for all so I don't have to!

FYI when it comes to GPIO descriptor refactoring we are churning
along with it, my tentative plan is to finish it before I retire.
I still have ~22 years until regular retirement age in Sweden so
I think I might be able to pull it off!
If you would feel an urge to jump in and help out here is the TODO:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/TODO

Thanks in advance,
Linus Walleij

Re: Is consumer documentation correct?

[Alexandre Courbot]

On Thu, Nov 5, 2020 at 11:49 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Alexandre,
>
> long time no C! :D

Hi Linus! Indeed! :D

>
> On Fri, Oct 30, 2020 at 3:36 PM Alexandre Courbot <gnurou@gmail.com> wrote:
>
> > So yeah the documentation (which I have written as far as I can
> > remember) looks a bit clumsy at best and I probably should amend it a
> > bit. Now I don't want to give the impression that these functions
> > should be used as they absolutely should not. :)
>
> Yes pretty please with sugar on top can you send a patch to clear
> this up once and for all so I don't have to!

Sorry for the time it took to get back to this, but you should have a
patch in your Inbox.

>
> FYI when it comes to GPIO descriptor refactoring we are churning
> along with it, my tentative plan is to finish it before I retire.
> I still have ~22 years until regular retirement age in Sweden so
> I think I might be able to pull it off!
> If you would feel an urge to jump in and help out here is the TODO:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/TODO

$ git grep gpio_set |wc -l
2649

Yikes. ^_^; but it looks like we are gaining ground nonetheless:

$ git grep gpiod_set |wc -l
1548

Re: Is consumer documentation correct?

[Linus Walleij]

On Sun, Nov 22, 2020 at 10:28 AM Alexandre Courbot <gnurou@gmail.com> wrote:
> On Thu, Nov 5, 2020 at 11:49 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> Sorry for the time it took to get back to this, but you should have a
> patch in your Inbox.

Thanks!

> > FYI when it comes to GPIO descriptor refactoring we are churning
> > along with it, my tentative plan is to finish it before I retire.
> > I still have ~22 years until regular retirement age in Sweden so
> > I think I might be able to pull it off!
> > If you would feel an urge to jump in and help out here is the TODO:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/TODO
>
> $ git grep gpio_set |wc -l
> 2649
>
> Yikes. ^_^; but it looks like we are gaining ground nonetheless:
>
> $ git grep gpiod_set |wc -l
> 1548

There are ways to make it look better!

$ git grep 'linux/gpio\.h' |wc -l
632
$ git grep 'linux/gpio/consumer\.h' |wc -l
644

Makes it look like more than halfway there, hey!

Yours,
Linus Walleij