Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, May 17, 2023 at 2:50 AM Chris Packham
<Chris.Packham@xxxxxxxxxxxxxxxxxxx> wrote:
> On 17/05/23 10:47, Kent Gibson wrote:

...

> The first is a userspace driver for a Power Over Ethernet Controller+PSE
> chipset (I'll refer to this as an MCU since the thing we talk to is
> really a micro controller with a vendor supplied firmware on it that
> does most of the PoE stuff). Communication to the MCU is based around
> commands sent via i2c. But there are a few extra GPIOs that are used to
> reset the MCU as well as provide a mechanism for quickly dropping power
> on certain events (e.g. if the temperature monitoring detects a problem).

Why does the MCU have no in-kernel driver?

> We do have a small kernel module that grabs the GPIOs based on the
> device tree and exports them with a known names (e.g. "poe-reset",
> "poe-dis") that the userspace driver can use.

So, besides that you repeat gpio-aggregator functionality, you already
have a "proxy" driver in the kernel. What prevents you from doing more
in-kernel?

>  Back when that code was
> written we did consider not exporting the GPIOs and instead having some
> other sysfs/ioctl interface into this kernel module but that seemed more
> work than just calling gpiod_export() for little gain. This is where
> adding the gpio-names property in our .dts would allow libgpiod to do
> something similar.
>
> Having the GPIOs in sysfs is also convenient as we can have a systemd
> ExecStopPost script that can drop power and/or reset the MCU if our
> application crashes.

I'm a bit lost. What your app is doing and how that is related to the
(userspace) drivers?

> I'm not sure if the GPIO chardev interface deals
> with releasing the GPIO lines if the process that requested them exits
> abnormally (I assume it does) and obviously our ExecStopPost script
> would need updating to use some of the libgpiod applications to do what
> it currently does with a simple 'echo 1 >.../poe-reset'
>
> The second application is a userspace driver for a L3 network switch
> (actually two of them for different silicon vendors). Again this needs
> to deal with resets for PHYs connected to the switch that the kernel has
> no visibility of as well as the GPIOs for the SFP cages. Again we have a
> slightly less simple kernel module that grabs all these GPIOs and
> exports them with known names. This time there are considerably more of
> these GPIOs (our largest system currently has 96 SFP+ ports with 4 GPIOs
> per port) so we're much more reliant on being able to do things like
> `for x in port*tx-dis; do echo 1 >$x; done`

Hmm... Have you talked to the net subsystem guys? I know that there is
a lot going on around SFP cage enumeration for some of the modules
(Marvell?) and perhaps they can advise something different.

> I'm sure both of these applications could be re-written around libgpiod
> but that would incur a significant amount of regression testing on
> existing platforms. And I still consider dealing with GPIO chips an
> extra headache that the applications don't need (particularly with the
> sheer number of them the SFP case).

It seems to me that having no in-kernel driver for your stuff is the
main point of all headache here. But I might be mistaken.

-- 
With Best Regards,
Andy Shevchenko