Message-ID: <20090427180036.269a40b3@hyperion.delvare>
Date:	Mon, 27 Apr 2009 18:00:36 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Kay Sievers <kay.sievers@...y.org>
Cc:	Michael E Brown <Michael_E_Brown@...l.com>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	linux-kernel@...r.kernel.org,
	Mauro Carvalho Chehab <mchehab@...radead.org>,
	Matt Domsch <Matt_Domsch@...l.com>
Subject: Re: Class device namespaces

Hi Kay,

On Sun, 26 Apr 2009 15:42:25 +0200, Kay Sievers wrote:
> On Sun, Apr 26, 2009 at 08:54, Jean Delvare <khali@...ux-fr.org> wrote:
> > But anyway, this isn't going to happen quickly, and I still believe we
> > should solve the namespace problem I reported.
> 
> I think the real namespace problem lives well inside the i2c devices,
> not in the core. :) Sure, the firmware class is pretty dumb with its
> naming, but the root case is the way i2c devices are created.
> 
> First, i2c stacks class devices, which it should not do, it should use
> a bus not a class, if it has device <-> children relations between
> different types.

Could as well be. I didn't invent it, it was that way when I took over
maintenance of the code. If i2c-adapters shouldn't be class devices,
let's change that... with all due carefulness for user-space tools that
may be relying on sysfs paths.

> Second, it names all the devices from different classes with the
> _same_ name, then puts them in a hierarchy,

Yes... but that shouldn't be (and isn't, really) a problem. We control
the hierarchy, and the fact that different class devices have the same
name doesn't matter at this point.

> and the tries to mix-in a
> third class, the firmware class, which also uses the _same_ name. That
> just asks for trouble. :)

Yes it is. But you will note that the naming of firmware class devices
is the root of the problem. It doesn't even allow for two different
firmware class devices off the same parent. Even the author of the code
put in a warning in the code saying that this would inevitably lead to
collisions. This really should have never be accepted in the kernel to
start with.

> > For firmware, it doesn't seem as legitimate, I can't really think of a
> > good reason to use the i2c-adapter as the parent for the firmware
> > device.
> 
> Oh, I may miss something, I thought that is what you already do and
> causes the problem? I though you use the "adapter" to request the
> firmware? Otherwise I don't see how this can conflict.

Err, yes, this is exactly what we do, and also what I said just above.
Was I not clear maybe?

> You have the "adapter" and the "device" stacked, right?
>   /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/
> 
> Then you use the adapter:
>    /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0
> to request the firmware, which will try to create:
>   /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/
> 
> I thought you could use the "device" and not the "adapter" to request
> the firmware? Then you would get a "nice" chain of devices with all
> the same names, like :)
>    /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/i2c-0/

Ah, that's what you mean by "use the device". I read it the other way
around, using the _parent_ (bus) device:

    /sys/devices/pci0000:00/0000:00:1f.3/firmware/0000:00:1f.3

Actually I have already sent a patch doing this change to dvb drivers:
  http://www.spinics.net/lists/linux-media/msg04926.html
This still needs testing though.

What you propose wouldn't work, loading i2c-dev is optional so the
device may or may not be there. We definitely don't want to make all
the v4l/dvb stack depend on i2c-dev.

> I see the following options:
>   - the i2c "device", not the "adapter" would request the firmware

See above.

>   - the i2c-adapter converts to a bus, and is no longer a class

You apparently think this should be done either way?

>   - i2c "adapters" and "devices" don't get the same duplicated name

I consider this a feature. Really, they are the same devices, the
device nodes are there for user-space access to the adapters.

>   - if "adapters" can only ever have a single "device", the
>     both could just be merged to a single instance, and maybe compat
>     symlinks created

As said before, I have been considering this already. This would
certainly make things a little easier, but also less flexible (i2c-dev
could only be disabled at build time; if enabled it would always be
loaded, together with i2c-core). I'll think about it some more, in the
light of other i2c-core cleanups.

> 
>   - the firmware class adds a firmware-% prefix in the case the
>     requesting device is a class device, and not the common case
>     which is a bus device

That works but adds inconsistency which may in turn cause trouble in
the future (e.g. converting a bus device to class device or back would
break user-space compatibility.)

> 
>   - change the "glue dir" rules, which I would try to avoid, because
>     we don't know who else might stack class devices of different
>     classes, and might get surprised by this

I understand your reluctance, but still believe this is something we
need to fix. Sure there are alternative fixes or workarounds for the
problem at hand, but there may be other cases in the future.

Paradox of the day, either there are no other stacks of class devices,
and the change is safe and needless. Or there are other stacks of class
devices, and the change is needed but potentially risky. Oh well.

> What do you think?

I'd like to give a try to converting i2c-adapter devices to bus devices
rather than class devices, if you think that's how things should be.
Can you please explain to me how this would be done?

Thanks,
-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists