Hi,
On Mon, Oct 21, 2024 at 1:48 AM Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
>
> Hi Greg,
>
> On Mon, Oct 21, 2024 at 10:23 AM Geert Uytterhoeven
> <ge...@linux-m68k.org> wrote:
> > On Mon, Oct 21, 2024 at 9:27 AM Greg KH <gre...@linuxfoundation.org> wrote:
> > > On Mon, Oct 21, 2024 at 08:58:30AM +0200, Geert Uytterhoeven wrote:
> > > > On Mon, Oct 21, 2024 at 8:39 AM Greg KH <gre...@linuxfoundation.org>
> > > > wrote:
> > > > > On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > > > > > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > > > > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > > > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven
> > > > > > > > > wrote:
> > > > > > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert
> > > > > > > > > > > Uytterhoeven wrote:
> > > > > > > > > > > > Each bridge instance creates up to four auxiliary
> > > > > > > > > > > > devices with different
> > > > > > > > > > > > names. However, their IDs are always zero, causing
> > > > > > > > > > > > duplicate filename
> > > > > > > > > > > > errors when a system has multiple bridges:
> > > > > > > > > > > >
> > > > > > > > > > > > sysfs: cannot create duplicate filename
> > > > > > > > > > > > '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > > > > > > >
> > > > > > > > > > > > Fix this by using a unique instance ID per bridge
> > > > > > > > > > > > instance.
> > > > > > > > > > >
> > > > > > > > > > > Isn't this something that should be handled by the AUX
> > > > > > > > > > > core ? The code
> > > > > > > > > > > below would otherwise need to be duplicated by all
> > > > > > > > > > > drivers, which seems
> > > > > > > > > > > a burden we should avoid.
> > > > > > > > > >
> > > > > > > > > > According to the documentation, this is the responsibility
> > > > > > > > > > of the caller
> > > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > > > > > > I believe this is the same for platform devices.
> > > > > > > > > > See also the example at
> > > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > > > > > >
> > > > > > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but
> > > > > > > > > > the auxiliary
> > > > > > > > > > bus does not.
> > > > > > > > >
> > > > > > > > > Yes, it does not as it's up to the caller to create a unique
> > > > > > > > > name, like
> > > > > > > > > your patch here does. I'd argue that platform should also
> > > > > > > > > not do
> > > > > > > > > automatic device ids, but that's a different argument :)
> > > > > > > >
> > > > > > > > __auxiliary_device_add() creates the device name with
> > > > > > > >
> > > > > > > > dev_set_name(dev, "%s.%s.%d", modname, auxdev->name,
> > > > > > > > auxdev->id);
> > > > > > > >
> > > > > > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > > > > > > shouldn't the first component of the device name use the
> > > > > > > > parent's name
> > > > > > > > instead of the module name ?
> > > > > > >
> > > > > > > Why would the parent's name not be the module name? That name is
> > > > > > > guaranteed unique in the system. If you want "uniqueness" within
> > > > > > > the
> > > > > > > driver/module, use the name and id field please.
> > > > > > >
> > > > > > > That's worked well so far, but to be fair, aux devices are pretty
> > > > > > > new.
> > > > > > > What problem is this naming scheme causing?
> > > > > >
> > > > > > Auxiliary devices are created as children of a parent device. When
> > > > > > multiple instances of the same parent type exist, this will be
> > > > > > reflected
> > > > > > in the /sys/devices/ devices tree hierarchy without any issue. The
> > > > > > problem comes from the fact the the auxiliary devices need a unique
> > > > > > name
> > > > > > for /sys/bus/auxialiary/devices/, where we somehow have to
> > > > > > differenciate
> > > > > > devices of identical types.
> > > > > >
> > > > > > Essentially, we're trying to summarize a whole hierarchy (path in
> > > > > > /sys/devices/) into a single string. There are different ways to
> > > > > > solve
> > > > > > this. For platform devices, we use a device ID. For I2C devices, we
> > > > > > use
> > > > > > the parent's bus number. Other buses use different schemes.
> > > > > >
> > > > > > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> > > > > > handle this, and assign an id managed by the parent. In a sense we
> > > > > > could
> > > > > > consider this to be similar to what is done for I2C, where the bus
> > > > > > number is also a property of the parent. However, the big
> > > > > > difference is
> > > > > > that the I2C bus number is managed by the I2C subsystem, while here
> > > > > > the
> > > > > > id is managed by the ti-sn65dsi86 driver, not by the auxiliary
> > > > > > device
> > > > > > core. This would require duplicating the same mechanism in every
> > > > > > single
> > > > > > driver creating auxiliary devices. This strikes me as a fairly bad
> > > > > > idea.
> > > > > > The problem should be solved by the core, not by individual drivers.
> > > > >
> > > > > The "id" is just a unique number, it is "managed" by the thing that is
> > > > > creating the devices themselves, not the aux core code. I don't see
> > > > > why
> > > > > the i2c bus number has to match the same number that the ti driver
> > > > > creates, it could be anything, as long as it doesn't match anything
> > > > > else
> > > > > currently created by that driver.
> > > >
> > > > Laurent does not say it has to match the i2c bus number.
> > > > He does think the auxilliary bus should provide a mechanism to
> > > > allocate these IDs (e.g. usin g AUX_DEVID_AUTO?).
> > >
> > > As this is the first subsystem to ask for such a thing, I didn't think
> > > it was needed, but the aux subsystem is new :)
> > >
> > > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > > in the TI driver does sound like a good idea to me...
> > >
> > > Great!
>
> > With the I2C adapter numbers, that becomes:
> >
> > /sys/bus/auxiliary/devices
> > ├── ti_sn65dsi86.gpio.1
> > ├── ti_sn65dsi86.pwm.1
> > ├── ti_sn65dsi86.aux.1
> > ├── ti_sn65dsi86.bridge.1
> > ├── ti_sn65dsi86.gpio.4
> > ├── ti_sn65dsi86.pwm.4
> > ├── ti_sn65dsi86.aux.4
> > └── ti_sn65dsi86.bridge.4
> >
> > > adapter->nr instead like other aux subsystems already do.
>
> Unfortunately the devil is in the details, as usual: there can be
> multiple instances of the sn65dsi86 bridge on a single I2C bus,
> so adapter->nr is not guaranteed to generate a unique name.
In the case of sn65dsi86 I think we'd actually be OK. The TI bridge
chip is always at bus address 0x2d so you can't have more than one on
the same bus. Unless you added something funky atop it (like a mux of
some sort) you might be OK.
> Changing the auxiliary bus to use the parent's name instead of the
> module name, as suggested by Laurent, would fix that.
Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If
we had a second on i2c bus 4, we'd have:
/sys/bus/auxiliary/devices
├── 2-002d.gpio.0
├── 2-002d.pwm.0
├── 2-002d.aux.0
├── 2-002d.bridge.0
├── 4-002d.gpio.0
├── 4-002d.pwm.0
├── 4-002d.aux.0
└── 4-002d.bridge.0
...and I think that's guaranteed to be unique because all the i2c
devices are flat in "/sys/bus/i2c/devices".
In any case, I've been standing on the sidelines because really any of
these ideas are fine to me.
Using the parent name is nice because it's completely automatic and
that's nice. It's a bigger change to paths / device names, though. I
could see it breaking someone somewhere.
The patch that Geert originally posted has the advantage of not
breaking things in case there's someone out there that wrote bad
userspace code and somehow hardcoded some old sysfs path. Others could
disagree, but IMO if it's easy it's nice to keep the sysfs paths
consistent even though it's not really an ABI promise. :-P I'm always
surprised how often people seem to hardcode paths like this. Sometimes
there's not a great alternative, but sometimes people just do it
because they don't know better...
The adapter->nr also seems OK to me in that it's a simple/small change.
Anyway, I'm happy to let others fight out for their favorite, but I
don't have a strong enough opinion to really argue one way or the
other...
-Doug
