Message-ID: <a61415db-fa3f-2fce-9c21-08d8dd026960@alliedtelesis.co.nz>
Date:   Tue, 16 May 2023 23:50:42 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Kent Gibson <warthog618@...il.com>
CC:     Linus Walleij <linus.walleij@...aro.org>,
        "brgl@...ev.pl" <brgl@...ev.pl>,
        "johan@...nel.org" <johan@...nel.org>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "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()

Hi Kent,

On 17/05/23 10:47, Kent Gibson wrote:
> On Tue, May 16, 2023 at 10:19:14PM +0000, Chris Packham wrote:
>> On 17/05/23 01:57, Linus Walleij wrote:
>>> On Mon, May 15, 2023 at 12:27 AM Chris Packham
>>> <Chris.Packham@...iedtelesis.co.nz> wrote:
>>>
>>>> In my original case which is a kernel module that exports a GPIO for
>>>> userspace using gpiod_export()
>>> We should not add new users for that API as it increase the usage
>>> of the sysfs ABI but if it's an existing in-tree usecase I buy it.
>>>
>>>> The crux of the problem is that the irq_desc is created when it hasn't
>>>> been requested.
>>> The right solution to me seems to be to not use gpiod_export()
>>> and not use sysfs TBH.
>> That's not really a feasible solution. I'm dealing with application code
>> that has been happily using the sysfs interface for many years.
>>
>> I actually did look at getting that code updated to use libgpio earlier
>> this year but found the API was in a state of flux and I wasn't going to
>> recommend re-writing the application code to use libgpio if I knew the
>> API was going to change and we'd have to re-write it again.
>>
> Its 'libgpiod'.
>
>> Even now with the 2.0.1 libgpio there doesn't seem to be a way of asking
>> about just GPIO lines in the system, application code would still need
>> to open every /dev/gpiochipN device and ask what lines are on the chip
>> and keep the fds open for the chips that have lines the application
>> cares about but make sure to close the fd for the ones that don't. So
>> now the application code has to care about GPIO chips in addition to the
>> GPIO lines.
>>
> Trying to better understand your use case - how does your application
> identify lines of interest - just whatever lines pop up in
> /sys/class/gpio?

Thanks for taking an interest. We actually have 2 applications that make 
use of this functionality

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

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

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

>
> Cheers,
> Kent.

Powered by blists - more mailing lists