Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <ZGzsD_HMbMGhGwcr@surfacebook> <72990baf-6964-01ad-d891-7090831d0310@alliedtelesis.co.nz>
 <ZG2jgwjK+CBmOk3G@sol>
In-Reply-To: <ZG2jgwjK+CBmOk3G@sol>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Fri, 26 May 2023 14:46:30 +0200
Message-ID: <CAMRc=Mf4+uYuwHACF3arkwhx2sXmTAJL-t1nVk-Xbg6tVy4WFQ@mail.gmail.com>
Subject: Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()
To:     Kent Gibson <warthog618@gmail.com>
Cc:     Chris Packham <Chris.Packham@alliedtelesis.co.nz>,
        "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        "johan@kernel.org" <johan@kernel.org>,
        "maz@kernel.org" <maz@kernel.org>,
        Ben Brown <Ben.Brown@alliedtelesis.co.nz>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 24, 2023 at 7:41=E2=80=AFAM Kent Gibson <warthog618@gmail.com> =
wrote:
>
> On Tue, May 23, 2023 at 09:17:26PM +0000, Chris Packham wrote:
> >
> > On 24/05/23 04:38, andy.shevchenko@gmail.com 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=E2=80=AFAM Chris Packham
> > >>> <Chris.Packham@alliedtelesis.co.nz> wrote:
> > >>>> On 17/05/23 10:47, Kent Gibson wrote:
> > > ...
> > >
> > >> Again the problem boils down to the fact that we have a userspace sw=
itch
> > >> 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 =3D gpiocdev::find_named_line("LED0").unwrap();
>     let req =3D Request::builder()
>         .with_found_line(&led0)
>         .as_output(Value::Active)
>         .request()?;
>

I would argue that existing high-level bindings for mainline libgpiod
(C++ and Python) allow similar functionality in a comparable number of
LOC. On the other hand - core C library should remain relatively
simple and limited in features.

Chris: are you forced to use C or could you use C++ for line lookup
and management?

I'm also in the process of designing the DBus API and the base for it
will be GLib/GObject bindings for the core C lib. Maybe this is the
place where we could place more advanced features in C as GLib already
makes C coding so much easier.

Bart

>     // 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 usin=
g
> 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 worki=
ng
> 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 lib=
gpiod
> > >>>> but that would incur a significant amount of regression testing on
> > >>>> existing platforms. And I still consider dealing with GPIO chips a=
n
> > >>>> 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 th=
e
> > >>> 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 determin=
ing
> > >> if an attribute should be exported or not.
> > > Sorry for being unhelpful here. But without understanding the issue w=
e can't
> > > propose better solutions.
> > No problem, this is probably the most engagement I've had out of a Linu=
x
> > patch submission. Hopefully it's not too annoying for those on the Cc l=
ist.
>
> 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.