[PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Atsushi Nemoto]

Add an MODULE_ALIAS() to make this platform driver hotplug-aware.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
diff --git a/drivers/rtc/rtc-ds1742.c b/drivers/rtc/rtc-ds1742.c
index b2e5481..4bd22dc 100644
--- a/drivers/rtc/rtc-ds1742.c
+++ b/drivers/rtc/rtc-ds1742.c
@@ -273,3 +273,4 @@ MODULE_AUTHOR("Atsushi Nemoto <anemo@mba.ocn.ne.jp>");
 MODULE_DESCRIPTION("Dallas DS1742 RTC driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
+MODULE_ALIAS("ds1742");

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Jan Engelhardt]

On Aug 17 2007 01:06, Atsushi Nemoto wrote:
>Add an MODULE_ALIAS() to make this platform driver hotplug-aware.
>
>Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
>---
>diff --git a/drivers/rtc/rtc-ds1742.c b/drivers/rtc/rtc-ds1742.c
>index b2e5481..4bd22dc 100644
>--- a/drivers/rtc/rtc-ds1742.c
>+++ b/drivers/rtc/rtc-ds1742.c
>@@ -273,3 +273,4 @@ MODULE_AUTHOR("Atsushi Nemoto <anemo@mba.ocn.ne.jp>");
> MODULE_DESCRIPTION("Dallas DS1742 RTC driver");
> MODULE_LICENSE("GPL");
> MODULE_VERSION(DRV_VERSION);
>+MODULE_ALIAS("ds1742");

Why exactly is this needed? What script refers to the module as ds1742 instead
of rtc-ds1742? Regular hotplug (e.g. udev/modprobe) also go by PCI ID or
whatever is applicable and load the module which provides support for said ID.


	Jan
-- 

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Friday 17 August 2007, Jan Engelhardt wrote:
> On Aug 17 2007 01:06, Atsushi Nemoto wrote:
> >Add an MODULE_ALIAS() to make this platform driver hotplug-aware.
> >
> >...
> >+MODULE_ALIAS("ds1742");
> 
> Why exactly is this needed? What script refers to the module as ds1742 instead
> of rtc-ds1742? Regular hotplug (e.g. udev/modprobe) also go by PCI ID or
> whatever is applicable and load the module which provides support for said ID.

Most busses don't have managed device IDs like PCI, USB, or PNP.

The platform, spi, and i2c busses use the driver name, which is
obviously managed within the scope of all Linux drivers.

- Dave

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Jan Engelhardt]

On Aug 17 2007 08:23, David Brownell wrote:
>On Friday 17 August 2007, Jan Engelhardt wrote:
>> On Aug 17 2007 01:06, Atsushi Nemoto wrote:
>> >Add an MODULE_ALIAS() to make this platform driver hotplug-aware.
>> >
>> >...
>> >+MODULE_ALIAS("ds1742");
>> 
>> Why exactly is this needed? What script refers to the module as ds1742 instead
>> of rtc-ds1742? Regular hotplug (e.g. udev/modprobe) also go by PCI ID or
>> whatever is applicable and load the module which provides support for said ID.
>
>Most busses don't have managed device IDs like PCI, USB, or PNP.
>
>The platform, spi, and i2c busses use the driver name, which is
>obviously managed within the scope of all Linux drivers.
>
Yeah but that does not tell me why it needs the ds1742 alias
if scripts (whatever they use to discover it) could modprobe rtc-ds1742
instead.


	Jan
-- 

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Friday 17 August 2007, Jan Engelhardt wrote:
> >Most busses don't have managed device IDs like PCI, USB, or PNP.
> >
> >The platform, spi, and i2c busses use the driver name, which is
> >obviously managed within the scope of all Linux drivers.
> >
> Yeah but that does not tell me why it needs the ds1742 alias
> if scripts (whatever they use to discover it) could modprobe rtc-ds1742
> instead.

Because for some reason the driver name isn't rtc-ds1742 ...

My preferred style for such patches puts the MODULE_ALIAS up
near the strange driver name, so it's more clear what's going
on.  Putting all the MODULE_*() stuff at the end of the file
gets confusing in this case.

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Atsushi Nemoto]

On Fri, 17 Aug 2007 09:02:00 -0700, David Brownell <david-b@pacbell.net> wrote:
> Because for some reason the driver name isn't rtc-ds1742 ...
> 
> My preferred style for such patches puts the MODULE_ALIAS up
> near the strange driver name, so it's more clear what's going
> on.  Putting all the MODULE_*() stuff at the end of the file
> gets confusing in this case.

OK, then I should update my patches anyway ;)

Which do you prefer, MODULE_ALIAS or change some strings?

I guess there are some out-of-tree users of this driver, but fixing
them is really trivial, so I don't think this is a big compatibility
problem.

---
Atsushi Nemoto

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Friday 17 August 2007, Atsushi Nemoto wrote:
> 
> > My preferred style for such patches puts the MODULE_ALIAS up
> > near the strange driver name, so it's more clear what's going
> > on.  Putting all the MODULE_*() stuff at the end of the file
> > gets confusing in this case.
> 
> OK, then I should update my patches anyway ;)

I wasn't going to be too picky here, but if you volunteer
to make things even better, I'll be happy!  :)


> Which do you prefer, MODULE_ALIAS or change some strings?
> 
> I guess there are some out-of-tree users of this driver, but fixing
> them is really trivial, so I don't think this is a big compatibility
> problem.

If you're willing to fix all the in-tree users, I'd think
not needing MODULE_ALIAS is best in the long term.

For these I2C drivers, the conversion to support "new style"
driver binding is new in 2.6.23, yes?  If so, I don't see any
real downside to making the driver and module names match.

_ Dave

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Kay Sievers]

On 8/17/07, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> On Fri, 17 Aug 2007 09:02:00 -0700, David Brownell <david-b@pacbell.net> wrote:
> > Because for some reason the driver name isn't rtc-ds1742 ...
> >
> > My preferred style for such patches puts the MODULE_ALIAS up
> > near the strange driver name, so it's more clear what's going
> > on.  Putting all the MODULE_*() stuff at the end of the file
> > gets confusing in this case.
>
> OK, then I should update my patches anyway ;)
>
> Which do you prefer, MODULE_ALIAS or change some strings?
>
> I guess there are some out-of-tree users of this driver, but fixing
> them is really trivial, so I don't think this is a big compatibility
> problem.

Again, the only sane solution is to provide MODALIAS="platform:<name>"
from the platform bus, and adding the aliases to drivers who support
autoloading. Modalias strings are not free-text strings, they are
required to be prefixed by the subsystem.

I still fail to see why platform must be different from everything
else in the kernel. Please change that stuff, and the bugs which we
are fighting magically go away, because module-init-tools alias
resolving works like it does for every other subsystem in the kernel.

Thanks for considering,
Kay

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Friday 17 August 2007, Kay Sievers wrote:
> Again,

"Again"?


> the only sane solution is to provide MODALIAS="platform:<name>" 
> from the platform bus, and adding the aliases to drivers who support
> autoloading. Modalias strings are not free-text strings, they are
> required to be prefixed by the subsystem.

If so, then whoever tried to change the usage of module aliases
in that way goofed in several ways.  First, by not changing all
the in-kernel uses.  Second, by not changing all the out-of-tree
uses in various distros, toolchains, etc.  Third, by not even
documenting that change...

Module aliases have been around since very early versions of
stuff like "modprobe" ... without that new rule.  Aliases have
always been 	


> I still fail to see why platform must be different from everything
> else in the kernel.

It isn't.  See above.


> Please change that stuff, and the bugs which we 
> are fighting magically go away, because module-init-tools alias
> resolving works like it does for every other subsystem in the kernel.

Those bugs would be ... what?  "We" haven't observed anything
attributable to this usage, which has been common for years now.

- Dave

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Kay Sievers]

On Fri, 2007-08-17 at 09:55 -0700, David Brownell wrote:
> On Friday 17 August 2007, Kay Sievers wrote:
> > Again,
> 
> "Again"?

We exchanges several mails a few weeks ago after the Debian bug caused
by a modprobe loop. 

> > the only sane solution is to provide MODALIAS="platform:<name>" 
> > from the platform bus, and adding the aliases to drivers who support
> > autoloading. Modalias strings are not free-text strings, they are
> > required to be prefixed by the subsystem.
> 
> If so, then whoever tried to change the usage of module aliases
> in that way goofed in several ways.  First, by not changing all
> the in-kernel uses.  Second, by not changing all the out-of-tree
> uses in various distros, toolchains, etc.  Third, by not even
> documenting that change...

Yes, please fix the misuse of MODALIAS.

> Module aliases have been around since very early versions of
> stuff like "modprobe" ... without that new rule.  Aliases have
> always been 	

The environment key MODALIAS= and the sysfs file 'modalias" are part of
the driver core, and have a well defined meaning. And it's a much
younger construct, and _not_ free-text.

> > I still fail to see why platform must be different from everything
> > else in the kernel.
> 
> It isn't.  See above.

Sure, it is. Please prefix it with "platform:".

> > Please change that stuff, and the bugs which we 
> > are fighting magically go away, because module-init-tools alias
> > resolving works like it does for every other subsystem in the kernel.
> 
> Those bugs would be ... what?  "We" haven't observed anything
> attributable to this usage, which has been common for years now.

Loops of modprobe by modules requesting itself, you worked around that
in the recent kernel, but that broke other setups. And module-init-tools
"blacklist" does not work, because platform uses no alias, but a plain
driver name. That should be fixed, please, please, please...

Every distro has problems with platform, and believe me, I as the udev
maintainer, get all the questions and the trouble. Some blacklist the
whole platform event, some only a few devices. All just because platform
thinks it must not follow the rules everybody else plays. Please,
please, please do what everybody else is doing with the driver core.

Thanks again,
Kay

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Friday 17 August 2007, Kay Sievers wrote:
> On Fri, 2007-08-17 at 09:55 -0700, David Brownell wrote:
> > On Friday 17 August 2007, Kay Sievers wrote:
> > > Again,
> > 
> > "Again"?
> 
> We exchanges several mails a few weeks ago after the Debian bug caused
> by a modprobe loop. 

Which has been fixed for some time now; it was caused by legacy
drivers, which are incapable of hotplugging.


> > > the only sane solution is to provide MODALIAS="platform:<name>" 
> > > from the platform bus, and adding the aliases to drivers who support
> > > autoloading. Modalias strings are not free-text strings, they are
> > > required to be prefixed by the subsystem.
> > 
> > If so, then whoever tried to change the usage of module aliases
> > in that way goofed in several ways.  First, by not changing all
> > the in-kernel uses.  Second, by not changing all the out-of-tree
> > uses in various distros, toolchains, etc.  Third, by not even
> > documenting that change...
> 
> Yes, please fix the misuse of MODALIAS.

I'm not the one who's advocating a change here.  If you want to
first change/break and then fix things, all of that is up to you.


> > > Please change that stuff, and the bugs which we 
> > > are fighting magically go away, because module-init-tools alias
> > > resolving works like it does for every other subsystem in the kernel.
> > 
> > Those bugs would be ... what?  "We" haven't observed anything
> > attributable to this usage, which has been common for years now.
> 
> Loops of modprobe by modules requesting itself, you worked around that
> in the recent kernel,

Legacy drivers of the "create my own device node" ilk will never
be able to hotplug ... since it's the /sys/device/... node creation
which causes any "find my driver" hotplug event.  (Like "i82365".)

That bug has been *FIXED* for some time now, as far as I hear.

And that's based on having looked at several hundred platform
drivers to make sure of it... mostly that's not legacy code, so
it can hotplug just fine.


> but that broke other setups.

That's news to me.  Of course, the breakage could also be in
those setups ... legacy drivers with legacy setup shouldn't
break, but they don't necessarily obey enough of the rules
that newer tools expect.


> And module-init-tools  
> "blacklist" does not work, because platform uses no alias, but a plain
> driver name. That should be fixed, please, please, please...

Module init tools "blacklist" is squirrely, yes ... it
ignores *most* of the names that "modprobe" accepts,
so it can't actually "blacklist" modules from loading.

The ancient "alias modname none" trick still has life.

- Dave

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Jan Engelhardt]

On Aug 17 2007 12:50, David Brownell wrote:
>On Friday 17 August 2007, Kay Sievers wrote:
>> On Fri, 2007-08-17 at 09:55 -0700, David Brownell wrote:
>> > On Friday 17 August 2007, Kay Sievers wrote:
>> > > Again,
>> > 
>> > "Again"?
>> 
>> We exchanges several mails a few weeks ago after the Debian bug caused
>> by a modprobe loop. 
>
>Which has been fixed for some time now; it was caused by legacy
>drivers, which are incapable of hotplugging.

Speaking of which ... does not platform: thing affect pcspkr and psmouse for
x86 too?


	Jan
-- 

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Friday 17 August 2007, Jan Engelhardt wrote:
> >> We exchanges several mails a few weeks ago after the Debian bug caused
> >> by a modprobe loop. 
> >
> >Which has been fixed for some time now; it was caused by legacy
> >drivers, which are incapable of hotplugging.
> 
> Speaking of which ... does not platform: thing affect pcspkr and psmouse for
> x86 too?

psmouse is a serio driver, not a platform driver.

pcspkr is a platform driver, but not a legacy driver.
It could be made to hotplug.

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Kay Sievers]

On Fri, 2007-08-17 at 12:50 -0700, David Brownell wrote:
> On Friday 17 August 2007, Kay Sievers wrote:
> > On Fri, 2007-08-17 at 09:55 -0700, David Brownell wrote:

> > > 
> > > If so, then whoever tried to change the usage of module aliases
> > > in that way goofed in several ways.  First, by not changing all
> > > the in-kernel uses.  Second, by not changing all the out-of-tree
> > > uses in various distros, toolchains, etc.  Third, by not even
> > > documenting that change...
> > 
> > Yes, please fix the misuse of MODALIAS.
> 
> I'm not the one who's advocating a change here.  If you want to
> first change/break and then fix things, all of that is up to you.

I'm happy to do that. Patch is attached.

Thanks,
Kay


From: Kay Sievers <kay.sievers@vrfy.org>
Subject: platform: prefix MODALIAS with "platform:"

Prefix platform modalias strings with "platform:", which
modprobe config to blacklist alias resolving if userspace
configures it.

Send uevents for all platform devices.

Add MODULE_ALIAS's to: pxa2xx_pcmcia, ds1742 and pcspkr to trigger
module autoloading by userspace.

  $ modinfo pcspkr
  alias:          platform:pcspkr
  license:        GPL
  description:    PC Speaker beeper driver
  ...

  $ modprobe -n -v platform:pcspkr
  insmod /lib/modules/2.6.23-rc3-g28e8351a-dirty/kernel/drivers/input/misc/pcspkr.ko

Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
---

 base/platform.c           |   15 ++-------------
 input/misc/pcspkr.c       |    1 +
 pcmcia/pxa2xx_mainstone.c |    2 +-
 pcmcia/pxa2xx_sharpsl.c   |    2 +-
 rtc/rtc-ds1742.c          |    1 +
 5 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 869ff8c..9bfc434 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -160,11 +160,6 @@ static void platform_device_release(struct device *dev)
  *
  *	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; no
- *	device add/remove uevents will be generated.  In the unusual case that
- *	the device isn't being dynamically allocated as a legacy "probe the
- *	hardware" driver, infrastructure code should reverse this marking.
  */
 struct platform_device *platform_device_alloc(const char *name, unsigned int id)
 {
@@ -177,12 +172,6 @@ struct platform_device *platform_device_alloc(const char *name, unsigned int id)
 		pa->pdev.id = id;
 		device_initialize(&pa->pdev.dev);
 		pa->pdev.dev.release = platform_device_release;
-
-		/* prevent hotplug "modprobe $(MODALIAS)" from causing trouble in
-		 * legacy probe-the-hardware drivers, which don't properly split
-		 * out device enumeration logic from drivers.
-		 */
-		pa->pdev.dev.uevent_suppress = 1;
 	}
 
 	return pa ? &pa->pdev : NULL;
@@ -530,7 +519,7 @@ static ssize_t
 modalias_show(struct device *dev, struct device_attribute *a, char *buf)
 {
 	struct platform_device	*pdev = to_platform_device(dev);
-	int len = snprintf(buf, PAGE_SIZE, "%s\n", pdev->name);
+	int len = snprintf(buf, PAGE_SIZE, "platform:%s\n", pdev->name);
 
 	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
 }
@@ -546,7 +535,7 @@ static int platform_uevent(struct device *dev, char **envp, int num_envp,
 	struct platform_device	*pdev = to_platform_device(dev);
 
 	envp[0] = buffer;
-	snprintf(buffer, buffer_size, "MODALIAS=%s", pdev->name);
+	snprintf(buffer, buffer_size, "MODALIAS=platform:%s", pdev->name);
 	return 0;
 }
 
diff --git a/drivers/input/misc/pcspkr.c b/drivers/input/misc/pcspkr.c
index 906bf5e..e1a4402 100644
--- a/drivers/input/misc/pcspkr.c
+++ b/drivers/input/misc/pcspkr.c
@@ -23,6 +23,7 @@
 MODULE_AUTHOR("Vojtech Pavlik <vojtech@ucw.cz>");
 MODULE_DESCRIPTION("PC Speaker beeper driver");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pcspkr");
 
 #ifdef CONFIG_X86
 /* Use the global PIT lock ! */
diff --git a/drivers/pcmcia/pxa2xx_mainstone.c b/drivers/pcmcia/pxa2xx_mainstone.c
index 383107b..f6722ba 100644
--- a/drivers/pcmcia/pxa2xx_mainstone.c
+++ b/drivers/pcmcia/pxa2xx_mainstone.c
@@ -175,7 +175,6 @@ static int __init mst_pcmcia_init(void)
 	if (!mst_pcmcia_device)
 		return -ENOMEM;
 
-	mst_pcmcia_device->dev.uevent_suppress = 0;
 	mst_pcmcia_device->dev.platform_data = &mst_pcmcia_ops;
 
 	ret = platform_device_add(mst_pcmcia_device);
@@ -195,3 +194,4 @@ fs_initcall(mst_pcmcia_init);
 module_exit(mst_pcmcia_exit);
 
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pxa2xx-pcmcia");
diff --git a/drivers/pcmcia/pxa2xx_sharpsl.c b/drivers/pcmcia/pxa2xx_sharpsl.c
index a2daa3f..d5c33bd 100644
--- a/drivers/pcmcia/pxa2xx_sharpsl.c
+++ b/drivers/pcmcia/pxa2xx_sharpsl.c
@@ -261,7 +261,6 @@ static int __init sharpsl_pcmcia_init(void)
 	if (!sharpsl_pcmcia_device)
 		return -ENOMEM;
 
-	sharpsl_pcmcia_device->dev.uevent_suppress = 0;
 	sharpsl_pcmcia_device->dev.platform_data = &sharpsl_pcmcia_ops;
 	sharpsl_pcmcia_device->dev.parent = platform_scoop_config->devs[0].dev;
 
@@ -284,3 +283,4 @@ module_exit(sharpsl_pcmcia_exit);
 
 MODULE_DESCRIPTION("Sharp SL Series PCMCIA Support");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pxa2xx-pcmcia");
diff --git a/drivers/rtc/rtc-ds1742.c b/drivers/rtc/rtc-ds1742.c
index b2e5481..f5af09a 100644
--- a/drivers/rtc/rtc-ds1742.c
+++ b/drivers/rtc/rtc-ds1742.c
@@ -273,3 +273,4 @@ MODULE_AUTHOR("Atsushi Nemoto <anemo@mba.ocn.ne.jp>");
 MODULE_DESCRIPTION("Dallas DS1742 RTC driver");
 MODULE_LICENSE("GPL");
 MODULE_VERSION(DRV_VERSION);
+MODULE_ALIAS("platform:ds1742");

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Friday 17 August 2007, Kay Sievers wrote:
> 
> > I'm not the one who's advocating a change here.  If you want to
> > first change/break and then fix things, all of that is up to you.
> 
> I'm happy to do that. Patch is attached.

NAK.  That wasn't even a serious attempt at the "fix" part,
though it does the "break" part well enough to cause severe
regressions.

(As well as leaving all my technical points about your pushback
un-addressed.  As I noted before, the evident fact that you don't
have technical responses to them says to me that your pushback
has no real technical basis ...)


Out of around 300 platform drivers in the tree (and many more
not yet merged upstream), this makes it so that only *THREE* of
them can hotplug ... versus in the current tree, essentially
everything that's not a legacy driver is hotplugging just fine.

That's one heck of a regression.  Just shy of 100% ...

Plus it treats rtc-ds1742 as if it's a platform_driver not
an i2c_driver.

- Dave

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Kay Sievers]

On Fri, 2007-08-17 at 21:06 -0700, David Brownell wrote:
> On Friday 17 August 2007, Kay Sievers wrote:
> > 
> > > I'm not the one who's advocating a change here.  If you want to
> > > first change/break and then fix things, all of that is up to you.
> > 
> > I'm happy to do that. Patch is attached.
> 
> NAK.  That wasn't even a serious attempt at the "fix" part,
> though it does the "break" part well enough to cause severe
> regressions.

You disabled uevents which breaks udev and HAL setups, because we can't
track the existence of the devices, You can't just disable device events
to disable module loading. Uevents have are by far not only about module
loading.

> (As well as leaving all my technical points about your pushback
> un-addressed.  As I noted before, the evident fact that you don't
> have technical responses to them says to me that your pushback
> has no real technical basis ...)

It has. Disabling uevents to control module loading is just the totally
wrong thing to do. Uevents are there to let userspace know that a device
exists. Only the existence of MODALIAS and the "modalias" file controls
module loading, not the enabling and disabling of uevents, which is a
completely broken hack.

My patch fixes tons of issues, and that is "technical basis" enough. It
makes platform play nice with userspace, by behaving like the rest of 
the kernel.

Userspace udev/HAL relies entirely on uevents, for hot- and for
coldplug. Coldplug is done by writing "add" to all "uevent" files during
early boot, that does not work anymore with platform, and needs to be
fixed.

> Out of around 300 platform drivers in the tree (and many more
> not yet merged upstream), this makes it so that only *THREE* of
> them can hotplug ... versus in the current tree, essentially
> everything that's not a legacy driver is hotplugging just fine.
> 
> That's one heck of a regression.  Just shy of 100% ...

So where are the 100's of drivers that send an uevent? I'll go and fix them.

> Plus it treats rtc-ds1742 as if it's a platform_driver not
> an i2c_driver.

Wrong, the whole model is the other way around. MODALIAS does not tie
other drivers to a device. A driver matches on a device, and the ds1742
driver triggers on the existence of a platform driver. The alias strings
don't "treat" anything.
The whole kernel works that way, a lot of drivers get loaded by matching
on "dmi:*" or "acpi:*" strings by not letting the driver be a dmi driver
or acpi driver.

Please stop doing adding weird hacks to the kernel to fit your personal
taste and breaking all assumptions existing tools rely on. Fix the stuff
you broke and enable uevent for _all_ devices again. And let userspace
make the decision what to do with the event, like all other subsystems
in the kernel do.

Thanks,
Kay

Re: legacy platform drivers and hotplugging

[David Brownell]

Notice I fixed $SUBJECT to not mention rtc-ds1742, which has
never been a platform device and thus is offtopic to your
specific complaints about platform bus hotplugging.


On Saturday 18 August 2007, Kay Sievers wrote:
> On Fri, 2007-08-17 at 21:06 -0700, David Brownell wrote:
> > On Friday 17 August 2007, Kay Sievers wrote:
> > > 
> > > > I'm not the one who's advocating a change here.  If you want to
> > > > first change/break and then fix things, all of that is up to you.
> > > 
> > > I'm happy to do that. Patch is attached.
> > 
> > NAK.  That wasn't even a serious attempt at the "fix" part,
> > though it does the "break" part well enough to cause severe
> > regressions.
> 
> You disabled uevents which breaks udev and HAL setups, because we can't
> track the existence of the devices.

Only certain code paths -- ones primarily used by legacy
drivers -- disable those events.

The basic problem with such legacy drivers is that they
violate core driver model assumptions ... notably, by
creating device nodes for non-existent hardware.

Which means that for these legacy drivers, userspace can't
track device existence in *that* way.

If said driver creates some other device node after it's
verified the hardware exists -- maybe one that's coupled
to a character device -- it'd be fine to track *those*
events.  ISTR that most such drivers *do* create such nodes,
since back in the prehistoric ooze from which they crawled,
that was the only way to be seen by userspace.


> You can't just disable device events 
> to disable module loading. Uevents have are by far not only about module
> loading.

Only for legacy drivers, because of the modprobe loop thing.

So your proposal for how to handle such broken/legacy drivers
is ... exactly what, then?

Near as I can tell, you propose that the platform bus never
support automatic module loading.  Which seems to me like a
nonstarting proposal:  a significant and needless regression,
inflicting harm on what for most platforms (by type, and also
maybe by number-in-the-field) is the primary system bus.


> > (As well as leaving all my technical points about your pushback
> > un-addressed.  As I noted before, the evident fact that you don't
> > have technical responses to them says to me that your pushback
> > has no real technical basis ...)
> 
> It has. Disabling uevents to control module loading is just the totally
> wrong thing to do. Uevents are there to let userspace know that a device
> exists.

See above.  By your definition, legacy drivers would be issuing
bogus events.  So one key issue here is how to handle them; the
patch you object to won't issue those buggy events.

>From you, I've not seen even a proposal for a viable workaround
to that looping bug.  Not even that udev configs should handle it,
for example, although that would appear to be the most natural
conclusion given your premises and the fact that ISTR you're
still maintaining it ...

Whatever happens, I believe that legacy drivers will necessarily
involve special casing to cope with the fact that they don't
follow the current set of rules for drivers (though some folk
seem to want to make them act as if they do).


> Only the existence of MODALIAS and the "modalias" file controls 
> module loading, not the enabling and disabling of uevents, which is a
> completely broken hack.

Well, ISTR my original patch for the "looping" bug only affected
those values, but you objected to that too...

Which is why the code now just uses a pre-existing "no uevents"
hook (on code paths used primarily by legacy drivers) instead of
only addressing the "this driver can't be hotplugged" issue.

It looks like you're maybe coming around to my original position,
at least that key part of it.


> My patch fixes tons of issues, and that is "technical basis" enough. It
> makes platform play nice with userspace, by behaving like the rest of 
> the kernel.

Yet as I noted: it causes a regression of just shy of 100% for
platform device hotplug.  Ergo a well-founded "NAK" ... two steps
forward versus hundreds of steps backward, rarely a good idea.

(You still haven't actually described any case where the current
code is a problem.  Handwaving "tons of issues" doesn't count.)


> Userspace udev/HAL relies entirely on uevents, for hot- and for
> coldplug. Coldplug is done by writing "add" to all "uevent" files during
> early boot, that does not work anymore with platform, and needs to be
> fixed.

Like I said:  "technical basis" enough not to merge your patch;
it breaks coldplug too, as well as hotplug.

 
> > Out of around 300 platform drivers in the tree (and many more
> > not yet merged upstream), this makes it so that only *THREE* of
> > them can hotplug ... versus in the current tree, essentially
> > everything that's not a legacy driver is hotplugging just fine.
> > 
> > That's one heck of a regression.  Just shy of 100% ...
> 
> So where are the 100's of drivers that send an uevent? I'll go and fix them.

Device creation sends those uevents, as you know, not drivers.
(Modulo the issue of broken/legacy platform drivers, which are
wrongly creating devices themselves.  When I counted before, I
recall thinking that only a few dozen drivers were broken in
that way.  I mentioned i82365, there are others.)

What you broke is the connection between the device "add" events
and the "modprobe" parameter which loads the relevant driver.  That
currently (i.e. without your patch) works for most platform drivers.

 
> > Plus it treats rtc-ds1742 as if it's a platform_driver not
> > an i2c_driver.
> 
> Wrong, the whole model is the other way around.

Please strike yourself repeatedly over the head with a clue-by-four.
I2C != platform, and ds1742 is an i2c device.  So your patch:

> > > +MODULE_ALIAS("platform:ds1742");

violates the (new/undocumented) rule *YOU* previously proposed
(which is violated by most MODULE_ALIAS declarations I found in
the source tree) whereby the syntax of those aliases requires
that they start with a subsystem prefix.

Wrong subsystem.  strcmp("i2c", "platform") != 0.



> Please stop doing adding weird hacks to the kernel to fit your personal
> taste and breaking all assumptions existing tools rely on.

The only "personal taste" I've observed here is yours: that
in regards to this issue, you've avoided constructive responses
to most issues I've raised ... more than once giving unfounded
ad hominem "responses" instead of providing a counter-argument
(such as evidence backing some of your assertions).


> Fix the stuff  
> you broke and enable uevent for _all_ devices again. And let userspace
> make the decision what to do with the event, like all other subsystems
> in the kernel do.

You know, all I was trying to do was fix a bug reported to me
with the legacy i82365 driver.  (The modprobe loop thing.)
That's what started your flamage.

Root cause was that it's a legacy driver, can't hotplug, and
shouldn't even try.  There are other legacy platform drivers
in that same boat.  So the fix couldn't apply to just that
one driver.

Now, if you had ever come up with a viable alternative fix to
that issue (patch or proposal) rather than just arguing against
actually fixing that nasty bug ... well, such "what-ifs" can't
matter a lot here.  Except that constructive discussions don't
feel like "talking to a brick wall" time-wastage.

- Dave

Re: legacy platform drivers and hotplugging

[Atsushi Nemoto]

On Sun, 19 Aug 2007 18:44:43 -0700, David Brownell <david-b@pacbell.net> wrote:
> Notice I fixed $SUBJECT to not mention rtc-ds1742, which has
> never been a platform device and thus is offtopic to your
> specific complaints about platform bus hotplugging.

Well, rtc-ds1742 *IS* a platform driver.  But it seems the discussion
is not DS1742 specific at all, and I guess ds1742 is not a good
example for hotplugging, so I welcome your $SUBJECT fix :-)

---
Atsushi Nemoto

Re: legacy platform drivers and hotplugging

[Kay Sievers]

David,
your change to the platfor bus disables uevents for almost all platform
devices. These events are needed and expected by userspace today, just
like they are for all other devices of the kernel too. The device event
sequence now has holes in the chain of events, which breaks setups that
depend on that.

Udev coldplug is also broken:
  udevtrigger --subsystem-match=platform
leads to nothing after your change, while it worked as expected before.

Userspace will not code around your very personal idea of hotplug. You
can not disable driver core events globally because _you_ don't need it.
The rest of the world needs them and wants them back. Upstream hotplug
works differently than you expect, and your change broke that.

My patch fixes all these issues, and make platform behave like a nice
citizen of the kernel, make it look like the rest of this small world,
without any special treatment.

Please take care of that or I will file a regression report with a
revert request for your change, to get userspace back into a working
state as it was before.

Thanks again,
Kay


On Sun, 2007-08-19 at 18:44 -0700, David Brownell wrote:
> Notice I fixed $SUBJECT to not mention rtc-ds1742, which has
> never been a platform device and thus is offtopic to your
> specific complaints about platform bus hotplugging.
> 
> 
> On Saturday 18 August 2007, Kay Sievers wrote:
> > On Fri, 2007-08-17 at 21:06 -0700, David Brownell wrote:
> > > On Friday 17 August 2007, Kay Sievers wrote:
> > > > 
> > > > > I'm not the one who's advocating a change here.  If you want to
> > > > > first change/break and then fix things, all of that is up to you.
> > > > 
> > > > I'm happy to do that. Patch is attached.
> > > 
> > > NAK.  That wasn't even a serious attempt at the "fix" part,
> > > though it does the "break" part well enough to cause severe
> > > regressions.
> > 
> > You disabled uevents which breaks udev and HAL setups, because we can't
> > track the existence of the devices.
> 
> Only certain code paths -- ones primarily used by legacy
> drivers -- disable those events.
> 
> The basic problem with such legacy drivers is that they
> violate core driver model assumptions ... notably, by
> creating device nodes for non-existent hardware.
> 
> Which means that for these legacy drivers, userspace can't
> track device existence in *that* way.
> 
> If said driver creates some other device node after it's
> verified the hardware exists -- maybe one that's coupled
> to a character device -- it'd be fine to track *those*
> events.  ISTR that most such drivers *do* create such nodes,
> since back in the prehistoric ooze from which they crawled,
> that was the only way to be seen by userspace.
> 
> 
> > You can't just disable device events 
> > to disable module loading. Uevents have are by far not only about module
> > loading.
> 
> Only for legacy drivers, because of the modprobe loop thing.
> 
> So your proposal for how to handle such broken/legacy drivers
> is ... exactly what, then?
> 
> Near as I can tell, you propose that the platform bus never
> support automatic module loading.  Which seems to me like a
> nonstarting proposal:  a significant and needless regression,
> inflicting harm on what for most platforms (by type, and also
> maybe by number-in-the-field) is the primary system bus.
> 
> 
> > > (As well as leaving all my technical points about your pushback
> > > un-addressed.  As I noted before, the evident fact that you don't
> > > have technical responses to them says to me that your pushback
> > > has no real technical basis ...)
> > 
> > It has. Disabling uevents to control module loading is just the totally
> > wrong thing to do. Uevents are there to let userspace know that a device
> > exists.
> 
> See above.  By your definition, legacy drivers would be issuing
> bogus events.  So one key issue here is how to handle them; the
> patch you object to won't issue those buggy events.
> 
> From you, I've not seen even a proposal for a viable workaround
> to that looping bug.  Not even that udev configs should handle it,
> for example, although that would appear to be the most natural
> conclusion given your premises and the fact that ISTR you're
> still maintaining it ...
> 
> Whatever happens, I believe that legacy drivers will necessarily
> involve special casing to cope with the fact that they don't
> follow the current set of rules for drivers (though some folk
> seem to want to make them act as if they do).
> 
> 
> > Only the existence of MODALIAS and the "modalias" file controls 
> > module loading, not the enabling and disabling of uevents, which is a
> > completely broken hack.
> 
> Well, ISTR my original patch for the "looping" bug only affected
> those values, but you objected to that too...
> 
> Which is why the code now just uses a pre-existing "no uevents"
> hook (on code paths used primarily by legacy drivers) instead of
> only addressing the "this driver can't be hotplugged" issue.
> 
> It looks like you're maybe coming around to my original position,
> at least that key part of it.
> 
> 
> > My patch fixes tons of issues, and that is "technical basis" enough. It
> > makes platform play nice with userspace, by behaving like the rest of 
> > the kernel.
> 
> Yet as I noted: it causes a regression of just shy of 100% for
> platform device hotplug.  Ergo a well-founded "NAK" ... two steps
> forward versus hundreds of steps backward, rarely a good idea.
> 
> (You still haven't actually described any case where the current
> code is a problem.  Handwaving "tons of issues" doesn't count.)
> 
> 
> > Userspace udev/HAL relies entirely on uevents, for hot- and for
> > coldplug. Coldplug is done by writing "add" to all "uevent" files during
> > early boot, that does not work anymore with platform, and needs to be
> > fixed.
> 
> Like I said:  "technical basis" enough not to merge your patch;
> it breaks coldplug too, as well as hotplug.
> 
>  
> > > Out of around 300 platform drivers in the tree (and many more
> > > not yet merged upstream), this makes it so that only *THREE* of
> > > them can hotplug ... versus in the current tree, essentially
> > > everything that's not a legacy driver is hotplugging just fine.
> > > 
> > > That's one heck of a regression.  Just shy of 100% ...
> > 
> > So where are the 100's of drivers that send an uevent? I'll go and fix them.
> 
> Device creation sends those uevents, as you know, not drivers.
> (Modulo the issue of broken/legacy platform drivers, which are
> wrongly creating devices themselves.  When I counted before, I
> recall thinking that only a few dozen drivers were broken in
> that way.  I mentioned i82365, there are others.)
> 
> What you broke is the connection between the device "add" events
> and the "modprobe" parameter which loads the relevant driver.  That
> currently (i.e. without your patch) works for most platform drivers.
> 
>  
> > > Plus it treats rtc-ds1742 as if it's a platform_driver not
> > > an i2c_driver.
> > 
> > Wrong, the whole model is the other way around.
> 
> Please strike yourself repeatedly over the head with a clue-by-four.
> I2C != platform, and ds1742 is an i2c device.  So your patch:
> 
> > > > +MODULE_ALIAS("platform:ds1742");
> 
> violates the (new/undocumented) rule *YOU* previously proposed
> (which is violated by most MODULE_ALIAS declarations I found in
> the source tree) whereby the syntax of those aliases requires
> that they start with a subsystem prefix.
> 
> Wrong subsystem.  strcmp("i2c", "platform") != 0.
> 
> 
> 
> > Please stop doing adding weird hacks to the kernel to fit your personal
> > taste and breaking all assumptions existing tools rely on.
> 
> The only "personal taste" I've observed here is yours: that
> in regards to this issue, you've avoided constructive responses
> to most issues I've raised ... more than once giving unfounded
> ad hominem "responses" instead of providing a counter-argument
> (such as evidence backing some of your assertions).
> 
> 
> > Fix the stuff  
> > you broke and enable uevent for _all_ devices again. And let userspace
> > make the decision what to do with the event, like all other subsystems
> > in the kernel do.
> 
> You know, all I was trying to do was fix a bug reported to me
> with the legacy i82365 driver.  (The modprobe loop thing.)
> That's what started your flamage.
> 
> Root cause was that it's a legacy driver, can't hotplug, and
> shouldn't even try.  There are other legacy platform drivers
> in that same boat.  So the fix couldn't apply to just that
> one driver.
> 
> Now, if you had ever come up with a viable alternative fix to
> that issue (patch or proposal) rather than just arguing against
> actually fixing that nasty bug ... well, such "what-ifs" can't
> matter a lot here.  Except that constructive discussions don't
> feel like "talking to a brick wall" time-wastage.
> 
> - Dave
> 

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Atsushi Nemoto]

On Fri, 17 Aug 2007 18:32:19 +0200, "Kay Sievers" <kay.sievers@vrfy.org> wrote:
> > I guess there are some out-of-tree users of this driver, but fixing
> > them is really trivial, so I don't think this is a big compatibility
> > problem.
> 
> Again, the only sane solution is to provide MODALIAS="platform:<name>"
> from the platform bus, and adding the aliases to drivers who support
> autoloading. Modalias strings are not free-text strings, they are
> required to be prefixed by the subsystem.

I guess your objection is to my usage of MODULE_ALIAS, right?

I will send a patch to change the driver name string, because I did
not use the different name intentionally, I just had not thought about
hotplugging at all.  I think there is no serious reason to stick with
current driver name anyway.

---
Atsushi Nemoto

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Kay Sievers]

On Sun, 2007-08-19 at 21:57 +0900, Atsushi Nemoto wrote:
> On Fri, 17 Aug 2007 18:32:19 +0200, "Kay Sievers" <kay.sievers@vrfy.org> wrote:
> > > I guess there are some out-of-tree users of this driver, but fixing
> > > them is really trivial, so I don't think this is a big compatibility
> > > problem.
> > 
> > Again, the only sane solution is to provide MODALIAS="platform:<name>"
> > from the platform bus, and adding the aliases to drivers who support
> > autoloading. Modalias strings are not free-text strings, they are
> > required to be prefixed by the subsystem.
> 
> I guess your objection is to my usage of MODULE_ALIAS, right?

No, I object to the concept of "platform" to disable all uevents by
default, just to work around its MODALIAS misuse. The rest of the kernel
works properly, and userspace has a unified way to handle events and
module loading.
There is absolutely no reason for "platform" to be different and break
all reasonable assumptions of module-init-tools, udev and HAL.

Thanks,
Kay

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Sunday 19 August 2007, Kay Sievers wrote:
> No, I object to the concept of "platform" to disable all uevents by
> default, 

Which it certainly doesn't do.

Since the $SUBJECT patch doesn't affect a platform driver in any case,
all those comments are well off-topic for $SUBJECT.

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[David Brownell]

On Sunday 19 August 2007, David Brownell wrote:
> On Sunday 19 August 2007, Kay Sievers wrote:
> > No, I object to the concept of "platform" to disable all uevents by
> > default, 
> 
> Which it certainly doesn't do.
>
> Since the $SUBJECT patch doesn't affect a platform driver in any case,
> all those comments are well off-topic for $SUBJECT.

Sorry, my bad -- I was looking at the wrong files.
(Kay -- apply cluebat to *my* head, on this point.)

This is indeed a platform driver.  And v2 of this patch
doesn't resolve its "won't hotplug" problem.  The simplest
way to resolve that would be switching to the more widely
used platform_device_register().

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Atsushi Nemoto]

On Sun, 19 Aug 2007 19:02:04 -0700, David Brownell <david-b@pacbell.net> wrote:
> This is indeed a platform driver.  And v2 of this patch
> doesn't resolve its "won't hotplug" problem.  The simplest
> way to resolve that would be switching to the more widely
> used platform_device_register().

Yes, if the current in-tree users (JMR3927 and RBTX4927 board) really
wanted to hotplug this driver.  And I believe they won't :-)

I just want to fix a potential problem in rtc-ds1742 driver for
someone who want to hotplug it.  That's all.

---
Atsushi Nemoto

Re: [PATCH] rtc: Make rtc-ds1742 driver hotplug-aware

[Atsushi Nemoto]

On Fri, 17 Aug 2007 17:37:01 +0200 (CEST), Jan Engelhardt <jengelh@computergmbh.de> wrote:
> >Most busses don't have managed device IDs like PCI, USB, or PNP.
> >
> >The platform, spi, and i2c busses use the driver name, which is
> >obviously managed within the scope of all Linux drivers.
> >
> Yeah but that does not tell me why it needs the ds1742 alias
> if scripts (whatever they use to discover it) could modprobe rtc-ds1742
> instead.

Well, the rtc-ds1742 platform driver uses a string "ds1742" for its
platform_driver definition.

static struct platform_driver ds1742_rtc_driver = {
	.probe		= ds1742_rtc_probe,
	.remove		= __devexit_p(ds1742_rtc_remove),
	.driver		= {
		.name	= "ds1742",
		.owner	= THIS_MODULE,
	},
};

We can change the name string and all definitions of "ds1742" platform
device instead of adding MODULE_ALIAS.  It is easy since there are
only a few users in kernel.  If that was preferred, I can rewrite this
patch.

---
Atsushi Nemoto