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

[Kent Gibson]

On Tue, May 23, 2023 at 09:17:26PM +0000, Chris Packham wrote:
> 
> On 24/05/23 04:38, andy.shevchenko@xxxxxxxxx wrote:
> > Wed, May 17, 2023 at 09:30:51PM +0000, Chris Packham kirjoitti:
> >> On 17/05/23 20:54, Andy Shevchenko wrote:
> >>> On Wed, May 17, 2023 at 2:50 AM Chris Packham
> >>> <Chris.Packham@xxxxxxxxxxxxxxxxxxx> wrote:
> >>>> On 17/05/23 10:47, Kent Gibson wrote:
> > ...
> >
> >> 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.
> > That user space driver is using what from the kernel? GPIO sysfs?
> 
> Yes GPIO sysfs and exported links with known names, which allows things 
> to be done per-port but also wildcarded from shell scripts if necessary. 
> I think the key point here is that it doesn't care about the GPIO chips 
> just the individual GPIO lines. Anything involving libgpiod currently 
> has to start caring about GPIO chips (or I'm misreading the docs).
> 

As previously mentioned, the libgpiod tools now support identification of
lines by name.
As long as your line names are unique at system scope you should be
good.  Otherwise you have no option but to identify by (chip,offset).

Wrt the library itself, I was thinking about relocating the line name
resolution logic from the tools into the library itself, so it would be
more generally accessible, but haven't gotten there yet.

I'm also of the opinion that libgpiod is too low level for common
tasks.  That is necessary to access all the features of the uAPI, but
for basic tasks it would be nice to have a higher level abstraction to
reduce the barrier to entry.

e.g. in Rust I can do this:

    let led0 = gpiocdev::find_named_line("LED0").unwrap();
    let req = Request::builder()
        .with_found_line(&led0)
        .as_output(Value::Active)
        .request()?;

    // change value later
    req.set_value(led0.offset, Value::Inactive)

which is the equivalent of the sysfs

echo 1 > /some/sysfs/line
...
echo 0 > /some/sysfs/line

That is bad enough. It pains me to see how complex the equivalent is using
the libgpiod v2 API (or v1), and that is not putting any shade on Bart or
anyone else who worked on it - there are a lot of constraints on how it
is designed.  It just doesn't feel complete yet, particularly from a
casual user's perspective.

One of the things I would like to see added to libgpiod is a set of working
examples of simple use cases.  Formerly the tools took double duty to
fill that role, but they've now grown too complex.
Those examples would highlight where we could provide simplified
higher level APIs.
Then rinse and repeat until the simple use cases are simple.

> >>>> 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.
> > Sorry for being unhelpful here. But without understanding the issue we can't
> > propose better solutions.
> No problem, this is probably the most engagement I've had out of a Linux 
> patch submission. Hopefully it's not too annoying for those on the Cc list.

Well, now you mention it....

Along the same lines as Andy, it is always useful to get feedback about
problems people are facing, and how the available solutions aren't
working for them.
If we don't know the problem exists then we can't fix it - except by
accident.

Cheers,
Kent.