Date:   Wed, 17 May 2023 21:30:51 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Kent Gibson <warthog618@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "brgl@...ev.pl" <brgl@...ev.pl>,
        "johan@...nel.org" <johan@...nel.org>,
        "maz@...nel.org" <maz@...nel.org>,
        "Ben Brown" <Ben.Brown@...iedtelesis.co.nz>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()


On 17/05/23 20:54, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 2:50 AM Chris Packham
> <Chris.Packham@...iedtelesis.co.nz> 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?

There isn't any PoE PSE infrastructure in the kernel. I'm not really 
sure what it'd look like either as the hardware designs are all highly 
customized and often have very specialized requirements. Even the vendor 
reference boards tend to use the i2c userspace interface and punt 
everything to a specialist application.

Of course if anyone is thinking about adding PoE PSE support in-kernel 
I'd be very keen to be involved.

>> 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?

Yes true. The main issue is that without total support for the class of 
device in the kernel there's little more that you can do other than 
exposing gpios (either as gpio_export_link() or some other bespoke 
interface).

>>   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?

Probably one of the primary things it's doing is bringing the chip out 
of reset by driving the GPIO (we don't want the PoE PSE supplying power 
if nothing is monitoring the temperature of the system). There's also 
some corner cases involving not resetting the PoE chipset on a hot restart.

>
>> 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.

Yes I'm aware of the switchdev work and I'm very enthusiastic about it 
(as an aside I do have a fairly functional switchdev driver for some of 
the older Marvell Poncat2 silicon, never quite got to submitting it 
upstream before we ran out of time on the project).

Again the problem boils down to the fact that we have a userspace switch 
driver (which uses a vendor supplied non-free SDK). So despite the 
kernel having quite good support for SFPs I can't use it without a 
netdev to attach it to.

>> 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.

It certainly doesn't help, but I do think that is all orthogonal to the 
fact that gpio_is_visible() changes things rather than just determining 
if an attribute should be exported or not.

Powered by blists - more mailing lists