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

[Linus Walleij]

On Thu, Dec 10, 2020 at 9:53 AM Johan Hovold <johan@xxxxxxxxxx> wrote:
> On Wed, Dec 09, 2020 at 05:25:32PM +0100, Linus Walleij wrote:

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

Yes the sysfs ABI is very volatile and easy to break.

As pointed out in the other reply, sysfs base GPIO number is all
wibbly-wobbly on anything hot-pluggable so in a way I feel it
is the right thing to disallow sysfs altogether on hotpluggable
devices.

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

That risk always exist of course. For this and many other reasons.
I just have to trust developers to understand that debugfs is named
debugfs for a reason.

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

The idea is that if you poke around there you know what you're
doing or ready to face the consequences.

I am thinking if people want to toggle LEDs and switches from
debugfs for testing and hacking they'd be alright with corrupting
the SPI interface if they make mistakes.

The chardev ABI is the only thing which we really designed with
some users, multiple users, compatibility and security in mind,
yet we had to revamp it once from scratch...

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

The unique way to identify a GPIO is gpiochip instance (with
topology from sysfs) and then a line number on that chip.
This is done e.g. in the example tool
tools/gpio/gpio-hammer.c

As you can see the tool doesn't use these line names.

The line names are really like symbolic links or something.
But they are indeed in a flat namespace so we should try to
at least make them unique if it turns out people love to use
these.

As it is now system designers mostly use device tree to assign
line names and they try to make these unique because they don't
like the nasty warnings from gpiolib.

If I google for the phrase "Detected name collision for GPIO name"
I just find the code, our discussions and some USB serial devices
warning about this so far.

Maybe we should just make a patch to disallow it?

Yours,
Linus Walleij