Re: [PATCH v5 2/3] usb: serial: xr_serial: Add gpiochip support

[Johan Hovold]

On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:
> On Wed, Dec 9, 2020 at 4:24 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> > On Tue, Dec 08, 2020 at 01:41:52PM +0100, Linus Walleij wrote:
> 
> > > depends on !GPIO_SYSFS
> > >
> > > so it can't even be compiled in if someone is using the sysfs.
> > >
> > > That should solve the situation where people are (ab)using
> > > the sysfs and getting name collisions as a result.
> >
> > Would it possible to set a flag to suppress just the sysfs entry
> > renaming instead?
> 
> Hm you mean that when a GPIO is "exported" in sysfs
> it should not get a symbolic name from the names but instead
> just the number?

Right.

> I bet someone has written their scripts to take advantage of
> the symbolic names so I suspect the task becomes bigger
> like suppress the sysfs entry renaming if and only if there is
> a namespace collision.
> 
> But I think we can do that, doesn't seem too hard?
> 
> I just hacked up this:
> https://lore.kernel.org/linux-gpio/20201209161821.92931-1-linus.walleij@xxxxxxxxxx/T/#u

I just replied to that thread, but to summarize, you can't rely on
having the sysfs code detect collisions since that will trigger a bunch
of nasty warnings and backtraces. We also don't want the sysfs interface
for a specific USB device to depend on probe order (only the first one
plugged in gets to use the line names). And adding line names now could
in fact be what breaks currently working scripts.

> > Despite its flaws the sysfs interface is still very convenient and I'd
> > prefer not to disable it just because of the line names.
> 
> Would these conveniences be identical to those listed
> in my recent TODO entry?
> https://lore.kernel.org/linux-gpio/20201204083533.65830-1-linus.walleij@xxxxxxxxxx/

Indeed.

> There are several other issues with the sysfs, so making it conflict
> with other drivers is almost  plus in the direction of discouragement
> from the GPIO submaintainer point of view, but I do see that
> people like it for the reasons in the TODO. :/
> 
> I am strongly encouraging any developer with a few spare cycles
> on their hands to go and implement the debugfs facility because
> we can make it so much better than the sysfs, easier and
> more convenient for testing etc.

Don't you run the risk of having people enable debugfs in production
systems now just so they can use the old-style interface?

Side note: if you skip the "export" part of the interface, how would you
indicate that a line is already in use or not available (e.g.
gpio-range-reserved)?

> > > Then it should be fine for any driver to provide a names array
> > > provided all the names are unique on that gpiochip.
> >
> > So it sounds like there's nothing preventing per-chip-unique names in
> > the rest of gpiolib and the new chardev interface then? Are the
> > user-space libraries able to cope with it, etc?
> 
> Yes the documentation refers to libgpiod a very well maintained
> library:
> https://www.kernel.org/doc/html/latest/driver-api/gpio/using-gpio.html
> https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/

Just did a super quick check and it seems libgpiod still assumes a flat
name space. For example, gpiod_chip_find_line() returns only the first
line found that matches a name. Shouldn't be impossible to extend, but
just want to make sure this flat namespace assumption hasn't been to
heavily relied upon.

Johan