2006-11-29 22:51:05

by David Brownell

[permalink] [raw]
Subject: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Monday 27 November 2006 11:03 am, Greg KH wrote:

> David, any chance you can fix this up, as people are seeing this bug
> today.

Here's my fix. Unlike the fix Kay suggested, it causes at worst only
a minor loss of functionality, so that tradeoff seems fair for a late
merge. I audited all the drivers using the relevant APIs, and I can't
see many (if any!) folk hitting problems from this.

- Dave


============== CUT HERE
We've had recent reports of some legacy "probe the hardware" style platform
drivers having nasty problems with hotplug support.

The core issue is that those drivers don't fully conform to the driver model.
They assume a role that's the responsibility of infrastructure code: they
create their own device nodes. But hotplugging relies on drivers to have split
those roles into different modules, hence the problems. If the driver creates
nodes for devices that don't exist, the "modprobe $MODALIAS" step of hotplugging
can become an endless loop *when driver load aborts* (it's safe otherwise).

This fix adds a per-device flag saying whether its driver can't be hotplugged,
which is set by APIs used for "probe the hardware" style drivers. The flag
is used to bypass relevant hotplug logic (setting the $MODALIAS environment
variable) for those problematic drivers.

The minor glitch is that in a few cases those APIs are used by platform code,
instead of by drivers. Examples include most places where a "pcspkr" device is
created (although a recent un-merged patch fixed that for X86_PC), or the way
platform devices are created for OpenFirmware nodes ... all those cases seem
to kick in before userspace (and hotplug) is active though.

So the net of this patch is removing some nasty failures with legacy drivers,
while retaining hotplug capability for those platform drivers which are well
behaved (the majority).

Signed-off-by: David Brownell <[email protected]>


Index: g26/include/linux/device.h
===================================================================
--- g26.orig/include/linux/device.h 2006-11-27 21:01:30.000000000 -0800
+++ g26/include/linux/device.h 2006-11-27 21:03:48.000000000 -0800
@@ -330,6 +330,7 @@ struct device {
struct kobject kobj;
char bus_id[BUS_ID_SIZE]; /* position on parent bus */
unsigned is_registered:1;
+ unsigned is_driver_not_hotpluggable:1;
struct device_attribute uevent_attr;
struct device_attribute *devt_attr;

Index: g26/drivers/base/platform.c
===================================================================
--- g26.orig/drivers/base/platform.c 2006-11-27 21:03:46.000000000 -0800
+++ g26/drivers/base/platform.c 2006-11-29 11:02:04.000000000 -0800
@@ -160,6 +160,11 @@ static void platform_device_release(stru
*
* Create a platform device object which can have other objects attached
* to it, and which will have attached objects freed when it is released.
+ *
+ * This device will be marked as not supporting hotpluggable drivers. In
+ * the unusual case that the device isn't being dynamically allocated as
+ * of a legacy "probe the hardware" driver, infrastructure code should
+ * consider reversing this marking.
*/
struct platform_device *platform_device_alloc(const char *name, unsigned int id)
{
@@ -172,6 +177,7 @@ struct platform_device *platform_device_
pa->pdev.id = id;
device_initialize(&pa->pdev.dev);
pa->pdev.dev.release = platform_device_release;
+ pa->pdev.dev.is_driver_not_hotpluggable = 1;
}

return pa ? &pa->pdev : NULL;
@@ -349,6 +355,13 @@ EXPORT_SYMBOL_GPL(platform_device_unregi
* memory allocated for the device allows drivers using such devices
* to be unloaded iwithout waiting for the last reference to the device
* to be dropped.
+ *
+ * This interface is primarily intended for use with legacy drivers
+ * which probe hardware directly. Because such drivers create device
+ * nodes themselves, rather than letting system infrastructure handle
+ * such device enumeration tasks, they don't fully conform to the Linux
+ * driver model. In particular, when such drivers are built as modules,
+ * they can't be "hotplugged".
*/
struct platform_device *platform_device_register_simple(char *name, unsigned int id,
struct resource *res, unsigned int num)
@@ -525,6 +538,13 @@ static int platform_uevent(struct device
{
struct platform_device *pdev = to_platform_device(dev);

+ /* prevent hotplug "modprobe $(MODALIAS)" from causing trouble in
+ * legacy probe-the-hardware drivers, which don't properly split
+ * out enumeration logic from drivers.
+ */
+ if (dev->is_driver_not_hotpluggable)
+ return 0;
+
envp[0] = buffer;
snprintf(buffer, buffer_size, "MODALIAS=%s", pdev->name);
return 0;


2006-11-29 23:02:42

by Greg KH

[permalink] [raw]
Subject: Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Wed, Nov 29, 2006 at 02:50:58PM -0800, David Brownell wrote:
> On Monday 27 November 2006 11:03 am, Greg KH wrote:
>
> > David, any chance you can fix this up, as people are seeing this bug
> > today.
>
> Here's my fix. Unlike the fix Kay suggested, it causes at worst only
> a minor loss of functionality, so that tradeoff seems fair for a late
> merge. I audited all the drivers using the relevant APIs, and I can't
> see many (if any!) folk hitting problems from this.

But this still can cause the problem that your 'modalias' file in sysfs
contains exactly the same name as the module itself, right?

That's not good, it should be an alias, not the "real name".

I'd even settle for 'driver:MODULE_NAME' and have that automatically
generated by the driver core logic somehow (note, Kay came up with that
idea in the past, it's not new...)

So, I don't like this patch, can't we come up with a way for the alias
to be just that, an alias?

That will ensure that userspace tools do not get confused, _and_ it be
possible to properly use the 'blacklist' option to modprobe (which only
works on aliases right now, as it should.)

thanks,

greg k-h

2006-11-30 01:27:37

by David Brownell

[permalink] [raw]
Subject: Re: [Bulk] Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Wednesday 29 November 2006 3:02 pm, Greg KH wrote:
> >
> > Here's my fix. ...
> > ... I audited all the drivers using the relevant APIs, and I can't
> > see many (if any!) folk hitting problems from this.
>
> But this still can cause the problem that your 'modalias' file in sysfs
> contains exactly the same name as the module itself, right?

Not a problem if folk stick to the original design. Hotplug will at
most "modprobe $MODALIAS" (iff the device needs a driver) before doing
a udevsend ... and only coldplug uses "modprobe $(cat modalias)".

The two were provided to address distinct problems. And the issue that
was described to me was _only_ relevant on the hotplug paths; coldplug
scripts, using /sys/devices/.../modalias files, see no problems.


I could update the patch so that attribute turns into a null string,
but that would have a **negative effect** since it would break coldplug
for all the platform init code which doesn't use platform_add_devices()
or maybe platform_device_register().


> That's not good, it should be an alias, not the "real name".

Well, adding unjustified complexity _after the fact_ isn't good either,
and that's what I see going on here.

How many years has KMOD been around? It's worked just fine without that
sort of bizarre (and un-needed) rule. Aliases were provided just to give
*additional* names to modules ... not to make one needlessly "special".
Kernel request_module() calls make no distinction between which type of
name they use ... and when the filesystem name changes, they still work
when the old name is properly aliased.

That whole "give a module an alias of itself" model just seems ludicrous
to me. We _know_ that "A" means "A", so there's no point in aliasing it
as itself.


... plus it's a distraction from the real problem, namely that certain
"legacy" drivers, primarily stuffed onto the "platform" bus, can't ever
hotplug. (While normal platform drivers do so just fine, without needing
strange rules to make some names "more equal than others".)


> That will ensure that userspace tools do not get confused,

I don't observe any confusion on my systems. Platform device hotplug
works just fine. Udev creates their /dev nodes. If there are some
tools getting confused, that seems best characterized as tool bugs.

- Dave

2006-12-01 07:46:24

by Greg KH

[permalink] [raw]
Subject: Re: [Bulk] Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Wed, Nov 29, 2006 at 05:27:31PM -0800, David Brownell wrote:
> On Wednesday 29 November 2006 3:02 pm, Greg KH wrote:
> > >
> > > Here's my fix. ...
> > > ... I audited all the drivers using the relevant APIs, and I can't
> > > see many (if any!) folk hitting problems from this.
> >
> > But this still can cause the problem that your 'modalias' file in sysfs
> > contains exactly the same name as the module itself, right?
>
> Not a problem if folk stick to the original design. Hotplug will at
> most "modprobe $MODALIAS" (iff the device needs a driver) before doing
> a udevsend ... and only coldplug uses "modprobe $(cat modalias)".

I'm not saying that the design does not have problems, but having a
modalias with no real alias does not make much sense to me.

> I could update the patch so that attribute turns into a null string,
> but that would have a **negative effect** since it would break coldplug
> for all the platform init code which doesn't use platform_add_devices()
> or maybe platform_device_register().

Ok, I'm being thick here, but why do we need a modalias with the same
name as the module?

> > That's not good, it should be an alias, not the "real name".
>
> Well, adding unjustified complexity _after the fact_ isn't good either,
> and that's what I see going on here.
>
> How many years has KMOD been around? It's worked just fine without that
> sort of bizarre (and un-needed) rule.

What rule? KMOD worked off of char device aliases for the most part.

> Aliases were provided just to give *additional* names to modules ...
> not to make one needlessly "special". Kernel request_module() calls
> make no distinction between which type of name they use ... and when
> the filesystem name changes, they still work when the old name is
> properly aliased.
>
> That whole "give a module an alias of itself" model just seems ludicrous
> to me. We _know_ that "A" means "A", so there's no point in aliasing it
> as itself.

I thought that was my point here :)

So what's the issue?

> ... plus it's a distraction from the real problem, namely that certain
> "legacy" drivers, primarily stuffed onto the "platform" bus, can't ever
> hotplug. (While normal platform drivers do so just fine, without needing
> strange rules to make some names "more equal than others".)

But does this solve that problem?

ugh, I'm confused...

greg k-h

2006-12-05 02:28:13

by David Brownell

[permalink] [raw]
Subject: Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Thursday 30 November 2006 11:04 pm, Greg KH wrote:
> On Wed, Nov 29, 2006 at 05:27:31PM -0800, David Brownell wrote:
> > On Wednesday 29 November 2006 3:02 pm, Greg KH wrote:
> > > >
> > > > Here's my fix. ...
> > > > ... I audited all the drivers using the relevant APIs, and I can't
> > > > see many (if any!) folk hitting problems from this.
> > >
> > > But this still can cause the problem that your 'modalias' file in sysfs
> > > contains exactly the same name as the module itself, right?
> >
> > Not a problem if folk stick to the original design. Hotplug will at
> > most "modprobe $MODALIAS" (iff the device needs a driver) before doing
> > a udevsend ... and only coldplug uses "modprobe $(cat modalias)".
>
> I'm not saying that the design does not have problems, but having a

No design is problem free, but I was just saying that any problems
in that context are because of someone needlessly fighting that design.

Like "of course you get into accidents when you drive on the wrong
side of the road", or "of course zeroing /dev/kmem as root crashes".


> modalias with no real alias does not make much sense to me.
>
> > I could update the patch so that attribute turns into a null string,
> > but that would have a **negative effect** since it would break coldplug
> > for all the platform init code which doesn't use platform_add_devices()
> > or maybe platform_device_register().
>
> Ok, I'm being thick here, but why do we need a modalias with the same
> name as the module?

We don't, today. But that's the direct consequence of the change Kay has
promoted ... that is, the pushback on the $SUBJECT patch, trying to cast
the issue as related to how drivers are identified, not than how they act.

It's either create such pointless aliases for quite a few drivers *OR* break
what is currently _working_ hotplug on many busses ... nontrivial regression,
requiring a lot of work to resolve that would mean deployment delays on the
(usual/optimistic) order of 12 months or so for tool updates.

Or, more directly and simply, merge the $SUBJECT patch; no regression, no
deployment delays in userspace tools, etc.


In reality, "modalias" is the misnomer ... it's the kind of implementation
detail that's bad to put into an interfaces. The role of that information
is "driver identifier". Which for many busses is the module name.

For some expansion busses, like PCI and USB, the identifier supports
some bus-specific wildcard match rules. Those busses were architected
around such driver match rules. And modprobe was modified to understand
those wildcard rules; and kbuild was modified to generate module aliases
that hotplug could feed to "modprobe". Fancy busses, fancy infrastructure;
it took a long time to get to the point where /sbin/hotplug can be as
simple as "modprobe $MODALIAS" (if needed!) plus "udevsend".

Busses like that are the exception though. Most don't have any kind
of dynamic identification/enumeration infrastructure, or multi-vendor
driver match algorithm ... those have both design and implementation
costs, which are not always justifiable. (Once you know the chip/board,
you know every device on it; so there's no point in adding any support
for auto-enumeration. There are also power management issues, since
devices' interface clocks likely default to "off". Linux must explicitly
enable those clocks, defeating the purpose of auto-enumeration...)

So the best that most busses can have is a Linux-specific driver ID;
although if you were judging by PC expansion busses, you might think
otherwise. To Linux, "platform_bus" covers many dozens of different
hardware implementations. On one SOC series, I can easily count a
dozen such busses ... sometimes just within one chip.

And in the context of platform_device and platform_driver, the driver
ID has always been the driver name ... which in all but a few cases
is also the module name. There's been a trivial workaround for those
few cases: MODULE_ALIAS(driver_name).


> > > That's not good, it should be an alias, not the "real name".
> >
> > Well, adding unjustified complexity _after the fact_ isn't good either,
> > and that's what I see going on here.
> >
> > How many years has KMOD been around? It's worked just fine without that
> > sort of bizarre (and un-needed) rule.
>
> What rule?

The new rule that's being promoted, with effect of breaking hotplug
and coldplug for platform drivers. (And SPI drivers, and any other
bus that doesn't want to create needless complexity by requiring
updates to module-init-tools and kbuild.) And some cases of KMOD.

That is, the new rule that requires the driver identifier never to be
the same as its module name ... and (a different rule?) requiring that
the driver identifer always be an alias, since (courtesy of the other
rule) the identifier couldn't be the module name. The one requiring
that $MODALIAS become an actual alias, instead of a driver ID.

(You can tell those are new rules by the fact that applying them
would immediately break hotplug for some busses. As well as KMOD
in various cases. And that they literally have nothing to do with
the root cause of the problem addressed by $SUBJECT patch.)


> KMOD worked off of char device aliases for the most part.

Not always. I did a grep. Things will (needlessly) break if anyone
starts to apply those new rules. Platform bus is not the only example.
Notice how ata_probe() gets "ide-cd.ko", etc.


> > Aliases were provided just to give *additional* names to modules ...
> > not to make one needlessly "special". Kernel request_module() calls
> > make no distinction between which type of name they use ... and when
> > the filesystem name changes, they still work when the old name is
> > properly aliased.
> >
> > That whole "give a module an alias of itself" model just seems ludicrous
> > to me. We _know_ that "A" means "A", so there's no point in aliasing it
> > as itself.
>
> I thought that was my point here :)
>
> So what's the issue?

The pushback on $SUBJECT patch. Which amounts to wanting to break hotplug
for several busses, unless someone (NOT the folk promoting the breakage!)
updates (a) every driver on those busses, to add some new annotations for
hotplug, (b) kbuild, to handle those annotations so that (c) some new and
as-yet-undesigned/undeployed module-init-tool modifications can return those
systems back to **TODAY'S LEVEL OF FUNCTIONALITY** ... it'd likely be sometime
late next year before very many systems get back to working as well as they
work right now.

Just because of someone wanting to create a _new kernel policy_ about driver
identification ... requiring it to only be done using aliases, making real
names become, in some sense, second class citizens. (Which is a bizarre notion
on so many levels I have a hard time starting to describe them all...)


> > ... plus it's a distraction from the real problem, namely that certain
> > "legacy" drivers, primarily stuffed onto the "platform" bus, can't ever
> > hotplug. (While normal platform drivers do so just fine, without needing
> > strange rules to make some names "more equal than others".)
>
> But does this solve that problem?
>
> ugh, I'm confused...

There are really two issues here:

- The "real one", as (yes!) fixed by the $SUBJECT patch. Troublesome legacy
drivers, like "i82365", written so they can't hotplug ... but the kernel
hasn't previously known that.

- The confusion, caused by a false identification of the "i82365" issue
being a problem related to module aliasing ... instead of being rooted in
the fact that it's a "legacy style" non-hotpluggable driver, since it
creates its own device node.

The only _problem_ ever identified to me is the one where "modprobe i82365"
on systems without that hardware could make trouble. That should be fixed
by $SUBJECT patch; a few dozen other ISA-era drivers have the same issue.

The _confusion_ is all this noise about wanting to change the rules associated
with module naming instead ... while still letting non-hotpluggable drivers try
to hotplug. Most of it could have been avoided by not dodging the "legacy driver"
issue each time it was highlighted, or the way that half-formed rule change
proposal would cause significant regressions.

- Dave

2006-12-05 10:01:59

by Marco d'Itri

[permalink] [raw]
Subject: Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Dec 05, David Brownell <[email protected]> wrote:

> The pushback on $SUBJECT patch. Which amounts to wanting to break hotplug
> for several busses, unless someone (NOT the folk promoting the breakage!)
Please explain in more details how hotplugging would be broken, possibly
with examples.

> There are really two issues here:
>
> - The "real one", as (yes!) fixed by the $SUBJECT patch. Troublesome legacy
> drivers, like "i82365", written so they can't hotplug ... but the kernel
> hasn't previously known that.
>
> - The confusion, caused by a false identification of the "i82365" issue
> being a problem related to module aliasing ... instead of being rooted in
> the fact that it's a "legacy style" non-hotpluggable driver, since it
> creates its own device node.

Nonsense. The purpose of $MODALIAS is to allow automatically loading
modules using the information provided by the bus driver.
Because of this reason there is no point for a driver to provide a
$MODALIAS referring to itself. It will only waste resources causing udev
to try loading it again.

--
ciao,
Marco

2006-12-06 00:03:13

by David Brownell

[permalink] [raw]
Subject: Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Tuesday 05 December 2006 2:01 am, Marco d'Itri wrote:
> On Dec 05, David Brownell <[email protected]> wrote:
>
> > The pushback on $SUBJECT patch. Which amounts to wanting to break hotplug
> > for several busses, unless someone (NOT the folk promoting the breakage!)
>
> Please explain in more details how hotplugging would be broken, possibly
> with examples.

First, for reference, I refer to hotplugging using the trivial ASH scripts
from [1], updated by removing no-longer-needed special cases for platform_bus
(that original logic didn't work sometimes) and pcmcia. See the (short)
contemporary thread [2] discussing platform_bus support, and some of issues
related to why its hotplug support works the way it does now. (Looks like
some messages are not archived, but the key points are there.)

Those scripts have supported hotplug and coldplug on several embedded boards
for some time now. The ancient "runs on 2.4" scripts would have been way
too slow to justify using hotplug and udev to replace devfs, and also needed
all sorts of extra crap that's regularly not found with embedded Linuxes.
Plus of course they never understood platform_bus, which is the main way
those PCI-less systems are hooked together.


Second, note that you're asking me to construct a straw man for you and
then break it down, since nobody arguing with the $SUBJECT patch has ever
provided a complete counter-proposal (much less respond to the points
I've made about legacy driver bugginess -- which is suggestive).

That said, the common thread in that pushback is that $MODALIAS (and also
modalias files) "should" have some value other than platform_device.name ...
which name is conventionaly also the name of the driver's module. That goes
along with a (weak) rationale that requires spi_bus and KMOD to change, plus
(presumably, pending a code audit) other kernel subsystems.


That should make it clear how accepting that pushback would break hotplug:
"modprobe $MODALIAS" would no longer load the right module. Likewise
the more significant case of coldplug; "modprobe $(cat modalias)" would
likewise no longer work.


> > There are really two issues here:
> >
> > - The "real one", as (yes!) fixed by the $SUBJECT patch. Troublesome legacy
> > drivers, like "i82365", written so they can't hotplug ... but the kernel
> > hasn't previously known that.
> >
> > - The confusion, caused by a false identification of the "i82365" issue
> > being a problem related to module aliasing ... instead of being rooted in
> > the fact that it's a "legacy style" non-hotpluggable driver, since it
> > creates its own device node.
>
> Nonsense. The purpose of $MODALIAS is to allow automatically loading
> modules using the information provided by the bus driver.

Without using sysfs. And that's exactly what it does _today_ for the
platform_bus.

Modulo the real issue, which is that legacy/ISA style drivers like i82365 will
never support hotplugging ("automatically loading modules ..." plus consequent
device configuration), without first being split into separate bus support
(making the device nodes) and driver module (the thing $MODALIAS supports).
Hotplug depends on that split in a fundamental manner.

So ... on what point were you disagreeing with me??


> Because of this reason there is no point for a driver to provide a
> $MODALIAS referring to itself. It will only waste resources causing udev
> to try loading it again.

The $SUBJECT patch makes those legacy drivers NOT use the $MODALIAS
mechanism ... you seem to be overlooking that.

And "udev" was from day one supposed to solve a different problem
than "loading modules". There was to be a clear and strong separation
of roles between hotplug (load modules, don't rely on sysfs, use other
components for the rest of device setup) and udev (make /dev nodes,
ok to rely on sysfs). That is, "udev" wouldn't do any loading.

- Dave

[1] http://marc.theaimsgroup.com/?l=linux-hotplug-devel&m=111903647518255&w=2
[2] http://marc.theaimsgroup.com/?t=111895889800001&r=1&w=2

2006-12-06 06:08:30

by Greg KH

[permalink] [raw]
Subject: Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Tue, Dec 05, 2006 at 04:03:08PM -0800, David Brownell wrote:
> On Tuesday 05 December 2006 2:01 am, Marco d'Itri wrote:
> > On Dec 05, David Brownell <[email protected]> wrote:
> >
> > > The pushback on $SUBJECT patch. Which amounts to wanting to break hotplug
> > > for several busses, unless someone (NOT the folk promoting the breakage!)
> >
> > Please explain in more details how hotplugging would be broken, possibly
> > with examples.
>
> First, for reference, I refer to hotplugging using the trivial ASH scripts
> from [1], updated by removing no-longer-needed special cases for platform_bus
> (that original logic didn't work sometimes) and pcmcia. See the (short)
> contemporary thread [2] discussing platform_bus support, and some of issues
> related to why its hotplug support works the way it does now. (Looks like
> some messages are not archived, but the key points are there.)
>
> Those scripts have supported hotplug and coldplug on several embedded boards
> for some time now. The ancient "runs on 2.4" scripts would have been way
> too slow to justify using hotplug and udev to replace devfs, and also needed
> all sorts of extra crap that's regularly not found with embedded Linuxes.
> Plus of course they never understood platform_bus, which is the main way
> those PCI-less systems are hooked together.

Ah, so for the platform devices, doing a
modprobe /sys/devices/platform/*
would load all of the proper modules for the specific platform devices
that are already present due to the MODULE_ALIAS() stuff?

Interesting, I didn't know that.

> Second, note that you're asking me to construct a straw man for you and
> then break it down, since nobody arguing with the $SUBJECT patch has ever
> provided a complete counter-proposal (much less respond to the points
> I've made about legacy driver bugginess -- which is suggestive).

Well, no, I just thought the patch in $SUBJECT was very ugly, and hence,
didn't like it :)

> That said, the common thread in that pushback is that $MODALIAS (and also
> modalias files) "should" have some value other than platform_device.name ...
> which name is conventionaly also the name of the driver's module. That goes
> along with a (weak) rationale that requires spi_bus and KMOD to change, plus
> (presumably, pending a code audit) other kernel subsystems.
>
>
> That should make it clear how accepting that pushback would break hotplug:
> "modprobe $MODALIAS" would no longer load the right module. Likewise
> the more significant case of coldplug; "modprobe $(cat modalias)" would
> likewise no longer work.

But, I don't understand why a module would have an alias with the same
name as itself? What is that achieving here? Shouldn't redundancy like
that be eliminated?

> The $SUBJECT patch makes those legacy drivers NOT use the $MODALIAS
> mechanism ... you seem to be overlooking that.

No, I'm not overlooking that, I think it's a good thing. I'm just
wondering if it could be done a different way. Perhaps in the platform
device itself instead of the driver core code?

> And "udev" was from day one supposed to solve a different problem
> than "loading modules". There was to be a clear and strong separation
> of roles between hotplug (load modules, don't rely on sysfs, use other
> components for the rest of device setup) and udev (make /dev nodes,
> ok to rely on sysfs). That is, "udev" wouldn't do any loading.

Well, things evolve and change over time. In the beginning, Linux was
only supposed to run on one CPU type and merely display ABABABAB on a
console properly :)

thanks,

greg k-h

2006-12-07 00:22:18

by Marco d'Itri

[permalink] [raw]
Subject: Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Dec 06, David Brownell <[email protected]> wrote:

> > Please explain in more details how hotplugging would be broken, possibly
> > with examples.
> First, for reference, I refer to hotplugging using the trivial ASH scripts
> from [1], updated by removing no-longer-needed special cases for platform_bus
> (that original logic didn't work sometimes) and pcmcia. See the (short)
I.e. a quick hack which has never been used by any distribution.
And anyway some kernel component is supposed to provide the aliases
pointing from the $MODALIAS values to the drivers, so modprobe $MODALIAS
would still work.

> Those scripts have supported hotplug and coldplug on several embedded boards
> for some time now.
Can you mention some?

> Second, note that you're asking me to construct a straw man for you and
> then break it down, since nobody arguing with the $SUBJECT patch has ever
> provided a complete counter-proposal (much less respond to the points
> I've made about legacy driver bugginess -- which is suggestive).
I am asking what is the point for a module to provide its own name in
$MODALIAS. You say that not doing this would break your custom hotplug
subsystem, but you still have not explained how this would happen since
$MODALIAS is available only *after* the module has been loaded by
something else.

> And "udev" was from day one supposed to solve a different problem
> than "loading modules". There was to be a clear and strong separation
> of roles between hotplug (load modules, don't rely on sysfs, use other
> components for the rest of device setup) and udev (make /dev nodes,
> ok to rely on sysfs). That is, "udev" wouldn't do any loading.
Who cares? It does now, and for many good reasons.

--
ciao,
Marco

2006-12-07 01:47:54

by David Brownell

[permalink] [raw]
Subject: Re: [Bulk] Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

> > First, for reference, I refer to hotplugging using the trivial ASH scripts
> > from [1], updated by removing no-longer-needed special cases for platform_bus
> > (that original logic didn't work sometimes) and pcmcia. ...
>
> Ah, so for the platform devices, doing a
> modprobe /sys/devices/platform/*
> would load all of the proper modules for the specific platform devices
> that are already present due to the MODULE_ALIAS() stuff?

That's sort of how that original "coldplug" script worked, but it didn't
work except in some trivial cases. For example, it fails in a common case
when platform_device.id != -1; and for platform devices that are children
of other devices. And of course there's the syntax issue ... only one
module name at a time (so modprobe in a loop).

The MODULE_ALIAS() stuff only kicks in when the driver name isn't the
same as its module name. Normally, developers just stick to one name.


> > That should make it clear how accepting that pushback would break hotplug:
> > "modprobe $MODALIAS" would no longer load the right module. Likewise
> > the more significant case of coldplug; "modprobe $(cat modalias)" would
> > likewise no longer work.
>
> But, I don't understand why a module would have an alias with the same
> name as itself? What is that achieving here? Shouldn't redundancy like
> that be eliminated?

To repeat, I am _not_ the one who has made that proposal. I'm the one
pointing out that all names for a module (aliases vs. what "ls" shows)
should be treated the same ... introducing a new rule about how hotplug
(or coldplug) must only refer to aliases promotes fragility.


> > The $SUBJECT patch makes those legacy drivers NOT use the $MODALIAS
> > mechanism ... you seem to be overlooking that.
>
> No, I'm not overlooking that, I think it's a good thing. I'm just
> wondering if it could be done a different way. Perhaps in the platform
> device itself instead of the driver core code?

Marco was overlooking it.

I thought about moving that bit elsewhere, but three things came to mind:

* Space-wise, there are already unused bits there, so this is free;
but there are no such bits in platform_device.

* Given that this is a "legacy style" issue, not all such driver
code is (or will be) on the platform bus.

* Hey, not all devices and busses support hotplugging, and it'd be
worth having discussion on that. The flag is explicitly about
the _driver_ not supporting hotplug ... a device node creation
problem. When the _device_ is physically not hotpluggable, a
different approach might help rid the kernel of probe()/remove()
infrastructure.

Given those points, I thought this was probably the best place to
put it; at least as an initial proposal.

Another proposal, which I dislike, is just not to have platform_bus
do hotplug (via $MODALIAS). That'd be OK for some current embedded
systems, since the devices get created during board startup and are
not added/removed later, but that's exactly the sort of idiosyncratic
restriction I've observed will invariably cause pain later on. It's
too easy to think of counterexamples, like devices appearing when a
board gets powered up.

- Dave

2006-12-09 06:53:31

by David Brownell

[permalink] [raw]
Subject: Re: [patch 2.6.19-rc6] fix hotplug for legacy platform drivers

On Wednesday 06 December 2006 3:56 pm, Marco d'Itri wrote:
> On Dec 06, David Brownell <[email protected]> wrote:
>
> > > Please explain in more details how hotplugging would be broken, possibly
> > > with examples.
> >
> > First, for reference, I refer to hotplugging using the trivial ASH scripts
> > from [1], updated by removing no-longer-needed special cases for platform_bus
> > (that original logic didn't work sometimes) and pcmcia. See the (short)
>
> I.e. a quick hack which has never been used by any distribution.

Not a "quick hack" at all; boiling down hotplug + coldplug to something
that tight got a fair degree of thought back then. It takes work to get
things to be small enough to use on very small Linux-based systems.

As for "never been used" ... you can't know that, given that embedded
distros are normally custom built. I'm certain that I submitted it to
buildroot back then. So a lot of folk building custom distros have had
access to that, and I'd be surprised if it was "never" used.


> And anyway some kernel component is supposed to provide the aliases
> pointing from the $MODALIAS values to the drivers, so modprobe $MODALIAS
> would still work.

A driver named "foo" is usually named "foo.ko"; aliases not needed, or
even desirable. The kernel source tree has done that for years.


> > Second, note that you're asking me to construct a straw man for you and
> > then break it down, since nobody arguing with the $SUBJECT patch has ever
> > provided a complete counter-proposal (much less respond to the points
> > I've made about legacy driver bugginess -- which is suggestive).
>
> I am asking what is the point for a module to provide its own name in
> $MODALIAS.

As I pointed out previously, more than once: it's providing a driver
identifier there, all it needs is to be understood by "modprobe".


> $MODALIAS is available only *after* the module has been loaded by
> something else.

You seem like you're being intentionally dense here ... that's rarely
true, which is what makes hotplug-driven driver loading work.

The $MODALIAS thing is passed to userspace after creation of driver
model device nodes. (Contrary to what you said above. It's related
to add_device calls, not module loading.)

Normal driver modules do not create those nodes ... and $MODALIAS is used
as a driver identifier so that /sbin/hotplug can "modprobe $MODALIAS" to
load the right driver module.

But *legacy* driver modules are not well behaved; they create those nodes
themselves, rather than letting bus infrastructure do so. Which is why
they are not hotpluggable. The $SUBJECT patch just turns off $MODALIAS
based hotplugging for those ill-mannered drivers.

- Dave