bus_add_device() losing an error return from the probe() method

bus_add_device() losing an error return from the probe() method

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]

Hi Greg, Takashi.

ALSA moved all ISA drivers over to the platform_driver interface in
2.6.16, using this code structure in the module_inits:

    cards = 0;
    for (i = 0; i < SNDRV_CARDS; i++) {
	  struct platform_device *device;
	  device = platform_device_register_simple(
			SND_FOO_DRIVER, i, NULL, 0);
	  if (IS_ERR(device)) {
		  err = PTR_ERR(device);
		  goto errout;
	  }
	  devices[i] = device;
	  cards++;
    }
    if (!cards) {
	  printk(KERN_ERR "FOO soundcard not found or device busy\n");
	  err = -ENODEV;
	  goto errout;
    }
    return 0;
errout:
    snd_foo_unregister_all();
    return err;

Unfortunately, the snd_foo_unregister_all() part here is unreachable
under normal circumstances, since platform_device_register_simple()
returns !IS_ERR, regardless of what the driver probe method returned.
The driver then never fails to load, even when no cards were found.

An error return from the driver probe() method is carried up through
device_attach, but is then dropped on the floor in bus_add_device(). If
I apply the attached patch, things work as I (and ALSA it seems) expect.
Is it correct?

(the printk still isn't reached, but see next message for that).

Rene.



[-- Attachment #2: bus_add_device.diff --]
[-- Type: text/plain, Size: 1055 bytes --]

Index: local/drivers/base/bus.c
===================================================================
--- local.orig/drivers/base/bus.c	2006-02-27 19:22:08.000000000 +0100
+++ local/drivers/base/bus.c	2006-03-24 04:27:02.000000000 +0100
@@ -363,19 +363,21 @@ static void device_remove_attrs(struct b
 int bus_add_device(struct device * dev)
 {
 	struct bus_type * bus = get_bus(dev->bus);
-	int error = 0;
+	int error;
 
 	if (bus) {
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
-		device_attach(dev);
+		error = device_attach(dev);
+		if (error < 0)
+			return error;
 		klist_add_tail(&dev->knode_bus, &bus->klist_devices);
 		error = device_add_attrs(bus, dev);
-		if (!error) {
-			sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
-			sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
-		}
+		if (error)
+			return error;
+		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
+		sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
 	}
-	return error;
+	return 0;
 }
 
 /**

Re: bus_add_device() losing an error return from the probe() method

[Andrew Morton]

Rene Herman <rene.herman@keyaccess.nl> wrote:
>
>  ===================================================================
>  --- local.orig/drivers/base/bus.c	2006-02-27 19:22:08.000000000 +0100
>  +++ local/drivers/base/bus.c	2006-03-24 04:27:02.000000000 +0100
>  @@ -363,19 +363,21 @@ static void device_remove_attrs(struct b
>   int bus_add_device(struct device * dev)
>   {
>   	struct bus_type * bus = get_bus(dev->bus);
>  -	int error = 0;
>  +	int error;
>   
>   	if (bus) {
>   		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
>  -		device_attach(dev);
>  +		error = device_attach(dev);
>  +		if (error < 0)
>  +			return error;
>   		klist_add_tail(&dev->knode_bus, &bus->klist_devices);
>   		error = device_add_attrs(bus, dev);
>  -		if (!error) {
>  -			sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
>  -			sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
>  -		}
>  +		if (error)
>  +			return error;
>  +		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
>  +		sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
>   	}
>  -	return error;
>  +	return 0;
>   }
>   

Looks sane, but please don't sprinkle `return' statements all over a
function in this manner.


--- devel/drivers/base/bus.c~bus_add_device-losing-an-error-return-from-the-probe-method	2006-03-25 17:46:34.000000000 -0800
+++ devel-akpm/drivers/base/bus.c	2006-03-25 17:49:45.000000000 -0800
@@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
 
 	if (bus) {
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
-		device_attach(dev);
+		error = device_attach(dev);
+		if (error < 0)
+			goto out;
 		klist_add_tail(&dev->knode_bus, &bus->klist_devices);
 		error = device_add_attrs(bus, dev);
-		if (!error) {
-			sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
-			sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
-		}
+		if (error)
+			goto out;
+		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
+		sysfs_create_link(&dev->kobj,&dev->bus->subsys.kset.kobj,"bus");
 	}
+out:
 	return error;
 }
 

It's a little surprising that this function returns "OK" if bus==NULL.

Note that sysfs_create_link() can fail too.  This was one optimistic
function.

Re: bus_add_device() losing an error return from the probe() method

[Rene Herman]

Andrew Morton wrote:

> Looks sane, but please don't sprinkle `return' statements all over a 
> function in this manner.

I actually prefer the multiple returns. You then don't have to "visually 
scroll down" to the label to see what would happen when reading the 
code. Even when there's common code before the return, I've never seen 
GCC not optimise that to the goto form itself. You obviously the boss 
though.

> It's a little surprising that this function returns "OK" if bus==NULL.
> 
> Note that sysfs_create_link() can fail too.  This was one optimistic
> function.

I assume that Greg hasn't commented yet since he's busy rewriting it 
all, so that'll be okay :-)

Rene.

Re: bus_add_device() losing an error return from the probe() method

[Rene Herman]

Andrew Morton wrote:

> Looks sane, but please don't sprinkle `return' statements all over a 
> function in this manner.

I actually prefer the multiple returns. You then don't have to "visually 
scroll down" to the label to see what would happen when reading the 
code. Even when there's common code before the return, I've never seen 
GCC not optimise that to the goto form itself. You obviously the boss 
though.

> It's a little surprising that this function returns "OK" if bus==NULL.
> 
> Note that sysfs_create_link() can fail too.  This was one optimistic
> function.

I assume that Greg hasn't commented yet since he's busy rewriting it 
all, so that'll be okay :-)

Rene.



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: bus_add_device() losing an error return from the probe() method

[Andrew Morton]

Rene Herman <rene.herman@keyaccess.nl> wrote:
>
>  ===================================================================
>  --- local.orig/drivers/base/bus.c	2006-02-27 19:22:08.000000000 +0100
>  +++ local/drivers/base/bus.c	2006-03-24 04:27:02.000000000 +0100
>  @@ -363,19 +363,21 @@ static void device_remove_attrs(struct b
>   int bus_add_device(struct device * dev)
>   {
>   	struct bus_type * bus = get_bus(dev->bus);
>  -	int error = 0;
>  +	int error;
>   
>   	if (bus) {
>   		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
>  -		device_attach(dev);
>  +		error = device_attach(dev);
>  +		if (error < 0)
>  +			return error;
>   		klist_add_tail(&dev->knode_bus, &bus->klist_devices);
>   		error = device_add_attrs(bus, dev);
>  -		if (!error) {
>  -			sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
>  -			sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
>  -		}
>  +		if (error)
>  +			return error;
>  +		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
>  +		sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
>   	}
>  -	return error;
>  +	return 0;
>   }
>   

Looks sane, but please don't sprinkle `return' statements all over a
function in this manner.


--- devel/drivers/base/bus.c~bus_add_device-losing-an-error-return-from-the-probe-method	2006-03-25 17:46:34.000000000 -0800
+++ devel-akpm/drivers/base/bus.c	2006-03-25 17:49:45.000000000 -0800
@@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
 
 	if (bus) {
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
-		device_attach(dev);
+		error = device_attach(dev);
+		if (error < 0)
+			goto out;
 		klist_add_tail(&dev->knode_bus, &bus->klist_devices);
 		error = device_add_attrs(bus, dev);
-		if (!error) {
-			sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
-			sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
-		}
+		if (error)
+			goto out;
+		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
+		sysfs_create_link(&dev->kobj,&dev->bus->subsys.kset.kobj,"bus");
 	}
+out:
 	return error;
 }
 

It's a little surprising that this function returns "OK" if bus==NULL.

Note that sysfs_create_link() can fail too.  This was one optimistic
function.



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: bus_add_device() losing an error return from the probe() method

to my gregkh-2.6 tree.  Its filename is

     bus_add_device-losing-an-error-return-from-the-probe-method.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/

Patches currently in gregkh-2.6 which might be from rene.herman@keyaccess.nl are

driver/bus_add_device-losing-an-error-return-from-the-probe-method.patch


>From rene.herman@keyaccess.nl Thu Mar 23 21:32:00 2006
Message-ID: <44238489.8090402@keyaccess.nl>
Date: Fri, 24 Mar 2006 06:32:57 +0100
From: Rene Herman <rene.herman@keyaccess.nl>
To: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Takashi Iwai <tiwai@suse.de>, ALSA devel <alsa-devel@alsa-project.org>, Linux Kernel <linux-kernel@vger.kernel.org>
Subject: bus_add_device() losing an error return from the probe() method

ALSA moved all ISA drivers over to the platform_driver interface in
2.6.16, using this code structure in the module_inits:

    cards = 0;
    for (i = 0; i < SNDRV_CARDS; i++) {
	  struct platform_device *device;
	  device = platform_device_register_simple(
			SND_FOO_DRIVER, i, NULL, 0);
	  if (IS_ERR(device)) {
		  err = PTR_ERR(device);
		  goto errout;
	  }
	  devices[i] = device;
	  cards++;
    }
    if (!cards) {
	  printk(KERN_ERR "FOO soundcard not found or device busy\n");
	  err = -ENODEV;
	  goto errout;
    }
    return 0;
errout:
    snd_foo_unregister_all();
    return err;

Unfortunately, the snd_foo_unregister_all() part here is unreachable
under normal circumstances, since platform_device_register_simple()
returns !IS_ERR, regardless of what the driver probe method returned.
The driver then never fails to load, even when no cards were found.

An error return from the driver probe() method is carried up through
device_attach, but is then dropped on the floor in bus_add_device(). If
I apply the attached patch, things work as I (and ALSA it seems) expect.

From: Rene Herman <rene.herman@keyaccess.nl>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/base/bus.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

--- gregkh-2.6.orig/drivers/base/bus.c
+++ gregkh-2.6/drivers/base/bus.c
@@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
 
 	if (bus) {
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
-		device_attach(dev);
+		error = device_attach(dev);
+		if (error < 0)
+			goto exit;
 		klist_add_tail(&dev->knode_bus, &bus->klist_devices);
 		error = device_add_attrs(bus, dev);
-		if (!error) {
-			sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
-			sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
-		}
+		if (error)
+			goto exit;
+		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
+		sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
 	}
+exit:
 	return error;
 }
 

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On 4/4/06, gregkh@suse.de <gregkh@suse.de> wrote:
>
> --- gregkh-2.6.orig/drivers/base/bus.c
> +++ gregkh-2.6/drivers/base/bus.c
> @@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
>
>        if (bus) {
>                pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
> -               device_attach(dev);
> +               error = device_attach(dev);
> +               if (error < 0)
> +                       goto exit;

I do not believe that this is correct. The fact that _some_ driver
failed to attach to a device does not necessarily mean that device
itself does not exist. While this assuption might work for platform
devices it won't work for other busses.

As fasr as ALSA goes, if they want to abort loading module if device
is not present they will have to separate device probing from final
driver binding.

--
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x110944&bid$1720&dat\x121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On 4/4/06, gregkh@suse.de <gregkh@suse.de> wrote:
>
> --- gregkh-2.6.orig/drivers/base/bus.c
> +++ gregkh-2.6/drivers/base/bus.c
> @@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
>
>        if (bus) {
>                pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
> -               device_attach(dev);
> +               error = device_attach(dev);
> +               if (error < 0)
> +                       goto exit;

I do not believe that this is correct. The fact that _some_ driver
failed to attach to a device does not necessarily mean that device
itself does not exist. While this assuption might work for platform
devices it won't work for other busses.

As fasr as ALSA goes, if they want to abort loading module if device
is not present they will have to separate device probing from final
driver binding.

--
Dmitry

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Greg KH]

On Tue, Apr 04, 2006 at 04:23:43PM -0400, Dmitry Torokhov wrote:
> On 4/4/06, gregkh@suse.de <gregkh@suse.de> wrote:
> >
> > --- gregkh-2.6.orig/drivers/base/bus.c
> > +++ gregkh-2.6/drivers/base/bus.c
> > @@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
> >
> >        if (bus) {
> >                pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
> > -               device_attach(dev);
> > +               error = device_attach(dev);
> > +               if (error < 0)
> > +                       goto exit;
> 
> I do not believe that this is correct. The fact that _some_ driver
> failed to attach to a device does not necessarily mean that device
> itself does not exist. While this assuption might work for platform
> devices it won't work for other busses.

Hm, no, I unwound this mess, and found the following:

 - bus_add_device() calls device_attach()
 - device_attach() calls bus_for_each_drv() for every driver on the bus
 - bus_for_each_drv() walks all drivers on the bus and calls
   __device_attach() for every individual driver
 - __device_attach() calls driver_probe_device() for that driver and device
 - driver_probe_device() calls down to the probe() function for the
   driver, passing it that driver, if match() for the bus matches this
   device.
 - if that probe() function returns -ENODEV or -ENXIO[1] then the error
   is ignored and 0 is returned, causing the loop to continue to try
   more drivers
 - if the probe() function returns any other error code, it is
   propagated up, all the way back to bus_add_device.
 - if the probe() function returns 0, the device is bound to the driver,
   and it returns 0.  Hm, looks like we continue to loop here too, we
   could probably stop now that we have bound a driver to the device.

So, I'm pretty sure that this is safe and should work just fine.  To be
sure, let me go reboot my box with this change on it after I finish this
email :)

Does that help?

thanks,

greg k-h

[1] - stupid agp drivers (or some other video drivers) require this.  I
    need to go fix them up so they don't do this, if they haven't been
    fixed already...

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Greg KH]

On Tue, Apr 04, 2006 at 02:00:48PM -0700, Greg KH wrote:
> On Tue, Apr 04, 2006 at 04:23:43PM -0400, Dmitry Torokhov wrote:
> > On 4/4/06, gregkh@suse.de <gregkh@suse.de> wrote:
> > >
> > > --- gregkh-2.6.orig/drivers/base/bus.c
> > > +++ gregkh-2.6/drivers/base/bus.c
> > > @@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
> > >
> > >        if (bus) {
> > >                pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
> > > -               device_attach(dev);
> > > +               error = device_attach(dev);
> > > +               if (error < 0)
> > > +                       goto exit;
> > 
> > I do not believe that this is correct. The fact that _some_ driver
> > failed to attach to a device does not necessarily mean that device
> > itself does not exist. While this assuption might work for platform
> > devices it won't work for other busses.
> 
> Hm, no, I unwound this mess, and found the following:
> 
>  - bus_add_device() calls device_attach()
>  - device_attach() calls bus_for_each_drv() for every driver on the bus
>  - bus_for_each_drv() walks all drivers on the bus and calls
>    __device_attach() for every individual driver
>  - __device_attach() calls driver_probe_device() for that driver and device
>  - driver_probe_device() calls down to the probe() function for the
>    driver, passing it that driver, if match() for the bus matches this
>    device.
>  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
>    is ignored and 0 is returned, causing the loop to continue to try
>    more drivers
>  - if the probe() function returns any other error code, it is
>    propagated up, all the way back to bus_add_device.
>  - if the probe() function returns 0, the device is bound to the driver,
>    and it returns 0.  Hm, looks like we continue to loop here too, we
>    could probably stop now that we have bound a driver to the device.
> 
> So, I'm pretty sure that this is safe and should work just fine.  To be
> sure, let me go reboot my box with this change on it after I finish this
> email :)

Yup, things still seem to work properly for me.  The patch will show up
in the next -mm for others to beat on...

thanks,

greg k-h


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Greg KH]

On Tue, Apr 04, 2006 at 02:00:48PM -0700, Greg KH wrote:
> On Tue, Apr 04, 2006 at 04:23:43PM -0400, Dmitry Torokhov wrote:
> > On 4/4/06, gregkh@suse.de <gregkh@suse.de> wrote:
> > >
> > > --- gregkh-2.6.orig/drivers/base/bus.c
> > > +++ gregkh-2.6/drivers/base/bus.c
> > > @@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
> > >
> > >        if (bus) {
> > >                pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
> > > -               device_attach(dev);
> > > +               error = device_attach(dev);
> > > +               if (error < 0)
> > > +                       goto exit;
> > 
> > I do not believe that this is correct. The fact that _some_ driver
> > failed to attach to a device does not necessarily mean that device
> > itself does not exist. While this assuption might work for platform
> > devices it won't work for other busses.
> 
> Hm, no, I unwound this mess, and found the following:
> 
>  - bus_add_device() calls device_attach()
>  - device_attach() calls bus_for_each_drv() for every driver on the bus
>  - bus_for_each_drv() walks all drivers on the bus and calls
>    __device_attach() for every individual driver
>  - __device_attach() calls driver_probe_device() for that driver and device
>  - driver_probe_device() calls down to the probe() function for the
>    driver, passing it that driver, if match() for the bus matches this
>    device.
>  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
>    is ignored and 0 is returned, causing the loop to continue to try
>    more drivers
>  - if the probe() function returns any other error code, it is
>    propagated up, all the way back to bus_add_device.
>  - if the probe() function returns 0, the device is bound to the driver,
>    and it returns 0.  Hm, looks like we continue to loop here too, we
>    could probably stop now that we have bound a driver to the device.
> 
> So, I'm pretty sure that this is safe and should work just fine.  To be
> sure, let me go reboot my box with this change on it after I finish this
> email :)

Yup, things still seem to work properly for me.  The patch will show up
in the next -mm for others to beat on...

thanks,

greg k-h

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Andrew Morton]

Greg KH <gregkh@suse.de> wrote:
>
> > So, I'm pretty sure that this is safe and should work just fine.  To be
>  > sure, let me go reboot my box with this change on it after I finish this
>  > email :)
> 
>  Yup, things still seem to work properly for me.  The patch will show up
>  in the next -mm for others to beat on...

It was in 2.6.16-mm2 and 2.6.17-rc1-mm1.

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Andrew Morton]

Greg KH <gregkh@suse.de> wrote:
>
> > So, I'm pretty sure that this is safe and should work just fine.  To be
>  > sure, let me go reboot my box with this change on it after I finish this
>  > email :)
> 
>  Yup, things still seem to work properly for me.  The patch will show up
>  in the next -mm for others to beat on...

It was in 2.6.16-mm2 and 2.6.17-rc1-mm1.


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On 4/4/06, Greg KH <gregkh@suse.de> wrote:
>
> Hm, no, I unwound this mess, and found the following:
>
>  - bus_add_device() calls device_attach()
>  - device_attach() calls bus_for_each_drv() for every driver on the bus
>  - bus_for_each_drv() walks all drivers on the bus and calls
>   __device_attach() for every individual driver
>  - __device_attach() calls driver_probe_device() for that driver and device
>  - driver_probe_device() calls down to the probe() function for the
>   driver, passing it that driver, if match() for the bus matches this
>   device.
>  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
>   is ignored and 0 is returned, causing the loop to continue to try
>   more drivers
>  - if the probe() function returns any other error code, it is
>   propagated up, all the way back to bus_add_device.

But why do we do that? probe() failing is driver's problem. The device
is still there and should still be presented in sysfs. I don't think
that we should stop if probe() fails - maybe next driver manages to
bind itself.

>  - if the probe() function returns 0, the device is bound to the driver,
>   and it returns 0.  Hm, looks like we continue to loop here too, we
>   could probably stop now that we have bound a driver to the device.
>

Could it be that logic is simply reversed?

> So, I'm pretty sure that this is safe and should work just fine.  To be
> sure, let me go reboot my box with this change on it after I finish this
> email :)
>
> Does that help?
>
> thanks,
>
> greg k-h
>
> [1] - stupid agp drivers (or some other video drivers) require this.  I
>    need to go fix them up so they don't do this, if they haven't been
>    fixed already...
>


--
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x110944&bid$1720&dat\x121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On 4/4/06, Greg KH <gregkh@suse.de> wrote:
>
> Hm, no, I unwound this mess, and found the following:
>
>  - bus_add_device() calls device_attach()
>  - device_attach() calls bus_for_each_drv() for every driver on the bus
>  - bus_for_each_drv() walks all drivers on the bus and calls
>   __device_attach() for every individual driver
>  - __device_attach() calls driver_probe_device() for that driver and device
>  - driver_probe_device() calls down to the probe() function for the
>   driver, passing it that driver, if match() for the bus matches this
>   device.
>  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
>   is ignored and 0 is returned, causing the loop to continue to try
>   more drivers
>  - if the probe() function returns any other error code, it is
>   propagated up, all the way back to bus_add_device.

But why do we do that? probe() failing is driver's problem. The device
is still there and should still be presented in sysfs. I don't think
that we should stop if probe() fails - maybe next driver manages to
bind itself.

>  - if the probe() function returns 0, the device is bound to the driver,
>   and it returns 0.  Hm, looks like we continue to loop here too, we
>   could probably stop now that we have bound a driver to the device.
>

Could it be that logic is simply reversed?

> So, I'm pretty sure that this is safe and should work just fine.  To be
> sure, let me go reboot my box with this change on it after I finish this
> email :)
>
> Does that help?
>
> thanks,
>
> greg k-h
>
> [1] - stupid agp drivers (or some other video drivers) require this.  I
>    need to go fix them up so they don't do this, if they haven't been
>    fixed already...
>


--
Dmitry

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Greg KH]

On Tue, Apr 04, 2006 at 05:28:48PM -0400, Dmitry Torokhov wrote:
> On 4/4/06, Greg KH <gregkh@suse.de> wrote:
> >
> > Hm, no, I unwound this mess, and found the following:
> >
> >  - bus_add_device() calls device_attach()
> >  - device_attach() calls bus_for_each_drv() for every driver on the bus
> >  - bus_for_each_drv() walks all drivers on the bus and calls
> >   __device_attach() for every individual driver
> >  - __device_attach() calls driver_probe_device() for that driver and device
> >  - driver_probe_device() calls down to the probe() function for the
> >   driver, passing it that driver, if match() for the bus matches this
> >   device.
> >  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
> >   is ignored and 0 is returned, causing the loop to continue to try
> >   more drivers
> >  - if the probe() function returns any other error code, it is
> >   propagated up, all the way back to bus_add_device.
> 
> But why do we do that? probe() failing is driver's problem. The device
> is still there and should still be presented in sysfs. I don't think
> that we should stop if probe() fails - maybe next driver manages to
> bind itself.

The device is still there.

Ah, I see what you are saying now.  Yeah, we should still add the
default attributes for the bus and create the bus link even if some
random driver had problems.  But then, we should still propagate the
error back up, right?

If I changed the patch to do that, would it be acceptable to you?

thanks,

greg k-h


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Greg KH]

On Tue, Apr 04, 2006 at 05:28:48PM -0400, Dmitry Torokhov wrote:
> On 4/4/06, Greg KH <gregkh@suse.de> wrote:
> >
> > Hm, no, I unwound this mess, and found the following:
> >
> >  - bus_add_device() calls device_attach()
> >  - device_attach() calls bus_for_each_drv() for every driver on the bus
> >  - bus_for_each_drv() walks all drivers on the bus and calls
> >   __device_attach() for every individual driver
> >  - __device_attach() calls driver_probe_device() for that driver and device
> >  - driver_probe_device() calls down to the probe() function for the
> >   driver, passing it that driver, if match() for the bus matches this
> >   device.
> >  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
> >   is ignored and 0 is returned, causing the loop to continue to try
> >   more drivers
> >  - if the probe() function returns any other error code, it is
> >   propagated up, all the way back to bus_add_device.
> 
> But why do we do that? probe() failing is driver's problem. The device
> is still there and should still be presented in sysfs. I don't think
> that we should stop if probe() fails - maybe next driver manages to
> bind itself.

The device is still there.

Ah, I see what you are saying now.  Yeah, we should still add the
default attributes for the bus and create the bus link even if some
random driver had problems.  But then, we should still propagate the
error back up, right?

If I changed the patch to do that, would it be acceptable to you?

thanks,

greg k-h

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On Tuesday 04 April 2006 17:45, Greg KH wrote:
> On Tue, Apr 04, 2006 at 05:28:48PM -0400, Dmitry Torokhov wrote:
> > On 4/4/06, Greg KH <gregkh@suse.de> wrote:
> > >
> > > Hm, no, I unwound this mess, and found the following:
> > >
> > >  - bus_add_device() calls device_attach()
> > >  - device_attach() calls bus_for_each_drv() for every driver on the bus
> > >  - bus_for_each_drv() walks all drivers on the bus and calls
> > >   __device_attach() for every individual driver
> > >  - __device_attach() calls driver_probe_device() for that driver and device
> > >  - driver_probe_device() calls down to the probe() function for the
> > >   driver, passing it that driver, if match() for the bus matches this
> > >   device.
> > >  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
> > >   is ignored and 0 is returned, causing the loop to continue to try
> > >   more drivers
> > >  - if the probe() function returns any other error code, it is
> > >   propagated up, all the way back to bus_add_device.
> > 
> > But why do we do that? probe() failing is driver's problem. The device
> > is still there and should still be presented in sysfs. I don't think
> > that we should stop if probe() fails - maybe next driver manages to
> > bind itself.
> 
> The device is still there.
> 
> Ah, I see what you are saying now.  Yeah, we should still add the
> default attributes for the bus and create the bus link even if some
> random driver had problems.  But then, we should still propagate the
> error back up, right?
> 

I don't think so because device creation did not fail. Otherwise how
would you as a caller of device_register() distinguish between the
following 2 scenarios:

 - you got -ENOMEM (or other error code) because device creation
   indeed failed;
 - you got -ENOMEM because some odd driver could not allocate 4MB
   of memory.

IOW you trying to propagate driver error to device creation code...

Also result of device_register() should not depend on whether
driver_register() was called earlier or not.

-- 
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On Tuesday 04 April 2006 17:45, Greg KH wrote:
> On Tue, Apr 04, 2006 at 05:28:48PM -0400, Dmitry Torokhov wrote:
> > On 4/4/06, Greg KH <gregkh@suse.de> wrote:
> > >
> > > Hm, no, I unwound this mess, and found the following:
> > >
> > >  - bus_add_device() calls device_attach()
> > >  - device_attach() calls bus_for_each_drv() for every driver on the bus
> > >  - bus_for_each_drv() walks all drivers on the bus and calls
> > >   __device_attach() for every individual driver
> > >  - __device_attach() calls driver_probe_device() for that driver and device
> > >  - driver_probe_device() calls down to the probe() function for the
> > >   driver, passing it that driver, if match() for the bus matches this
> > >   device.
> > >  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
> > >   is ignored and 0 is returned, causing the loop to continue to try
> > >   more drivers
> > >  - if the probe() function returns any other error code, it is
> > >   propagated up, all the way back to bus_add_device.
> > 
> > But why do we do that? probe() failing is driver's problem. The device
> > is still there and should still be presented in sysfs. I don't think
> > that we should stop if probe() fails - maybe next driver manages to
> > bind itself.
> 
> The device is still there.
> 
> Ah, I see what you are saying now.  Yeah, we should still add the
> default attributes for the bus and create the bus link even if some
> random driver had problems.  But then, we should still propagate the
> error back up, right?
> 

I don't think so because device creation did not fail. Otherwise how
would you as a caller of device_register() distinguish between the
following 2 scenarios:

 - you got -ENOMEM (or other error code) because device creation
   indeed failed;
 - you got -ENOMEM because some odd driver could not allocate 4MB
   of memory.

IOW you trying to propagate driver error to device creation code...

Also result of device_register() should not depend on whether
driver_register() was called earlier or not.

-- 
Dmitry

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Russell King]

On Tue, Apr 04, 2006 at 09:35:40PM -0400, Dmitry Torokhov wrote:
> On Tuesday 04 April 2006 17:45, Greg KH wrote:
> > Ah, I see what you are saying now.  Yeah, we should still add the
> > default attributes for the bus and create the bus link even if some
> > random driver had problems.  But then, we should still propagate the
> > error back up, right?
> 
> I don't think so because device creation did not fail. Otherwise how
> would you as a caller of device_register() distinguish between the
> following 2 scenarios:
> 
>  - you got -ENOMEM (or other error code) because device creation
>    indeed failed;
>  - you got -ENOMEM because some odd driver could not allocate 4MB
>    of memory.
> 
> IOW you trying to propagate driver error to device creation code...
> 
> Also result of device_register() should not depend on whether
> driver_register() was called earlier or not.

Indeed.  Greg - this patch is bogus.

When device_register() returns a negative number, many device drivers
assume (correctly) that the device was _not_ registered and they will
take whatever cleanup action is necessary to free the memory associated
with that device, believing that sysfs never used it.

If we start propagating driver probe error codes up through device_register()
we then have a big headache.  Is this -ENOMEM error a result of not being
able to register the struct device, or is it because something failed in
the driver.

This will also impact hotplug and PCMCIA.  For example, you have a device
driver loaded, but are short on memory, and you insert a PCMCIA device.
That creates a struct device.  Let's say that the driver fails to allocate
enough memory, and returns -ENOMEM.  With this patch, that'll kill off
the struct device.  Now, when the device is pulled, we could end up calling
driver_unregister on an unregistered struct device.

The expectations by current subsystems is that struct device registration
does _NOT_ depend on the currently loaded set of device drivers not
returning errors.

The thing is that this is all corner case stuff - you can apply this patch
and apparantly have a working system.  It's the corner cases where some
driver returns a failure which are the problem case.

Note also that this patch will not do what the ALSA folk want - they want
to know if the device was found or whether the probe returned -ENODEV.
We knock -ENODEV and -ENXIO to zero in driver_probe_device(), so they
won't even see that.

There is also no 1:1 relationship between platform device drivers and
platform devices.  You can have multiple platform devices driven by one
platform device driver.  See the plethora of platform devices in the
ARM sub-tree.  Or serial sub-tree.

So, all round this patch seems wrong.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Greg KH]

On Wed, Apr 05, 2006 at 08:36:02AM +0100, Russell King wrote:
> On Tue, Apr 04, 2006 at 09:35:40PM -0400, Dmitry Torokhov wrote:
> > On Tuesday 04 April 2006 17:45, Greg KH wrote:
> > > Ah, I see what you are saying now.  Yeah, we should still add the
> > > default attributes for the bus and create the bus link even if some
> > > random driver had problems.  But then, we should still propagate the
> > > error back up, right?
> > 
> > I don't think so because device creation did not fail. Otherwise how
> > would you as a caller of device_register() distinguish between the
> > following 2 scenarios:
> > 
> >  - you got -ENOMEM (or other error code) because device creation
> >    indeed failed;
> >  - you got -ENOMEM because some odd driver could not allocate 4MB
> >    of memory.
> > 
> > IOW you trying to propagate driver error to device creation code...
> > 
> > Also result of device_register() should not depend on whether
> > driver_register() was called earlier or not.
> 
> Indeed.  Greg - this patch is bogus.

You and Dmitry are correct.  I'm dropping this patch.  The ISA drivers
just need to get used to the proper way to use the driver model (they do
not know if they have been bound to a device when the driver is loaded,
no big deal).

If there's other issues with platform devices still, without this patch
applied, please let me know.

thanks to you all for pointing out the real issues here.

greg k-h

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Greg KH]

On Wed, Apr 05, 2006 at 08:36:02AM +0100, Russell King wrote:
> On Tue, Apr 04, 2006 at 09:35:40PM -0400, Dmitry Torokhov wrote:
> > On Tuesday 04 April 2006 17:45, Greg KH wrote:
> > > Ah, I see what you are saying now.  Yeah, we should still add the
> > > default attributes for the bus and create the bus link even if some
> > > random driver had problems.  But then, we should still propagate the
> > > error back up, right?
> > 
> > I don't think so because device creation did not fail. Otherwise how
> > would you as a caller of device_register() distinguish between the
> > following 2 scenarios:
> > 
> >  - you got -ENOMEM (or other error code) because device creation
> >    indeed failed;
> >  - you got -ENOMEM because some odd driver could not allocate 4MB
> >    of memory.
> > 
> > IOW you trying to propagate driver error to device creation code...
> > 
> > Also result of device_register() should not depend on whether
> > driver_register() was called earlier or not.
> 
> Indeed.  Greg - this patch is bogus.

You and Dmitry are correct.  I'm dropping this patch.  The ISA drivers
just need to get used to the proper way to use the driver model (they do
not know if they have been bound to a device when the driver is loaded,
no big deal).

If there's other issues with platform devices still, without this patch
applied, please let me know.

thanks to you all for pointing out the real issues here.

greg k-h


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Russell King]

On Tue, Apr 04, 2006 at 09:35:40PM -0400, Dmitry Torokhov wrote:
> On Tuesday 04 April 2006 17:45, Greg KH wrote:
> > Ah, I see what you are saying now.  Yeah, we should still add the
> > default attributes for the bus and create the bus link even if some
> > random driver had problems.  But then, we should still propagate the
> > error back up, right?
> 
> I don't think so because device creation did not fail. Otherwise how
> would you as a caller of device_register() distinguish between the
> following 2 scenarios:
> 
>  - you got -ENOMEM (or other error code) because device creation
>    indeed failed;
>  - you got -ENOMEM because some odd driver could not allocate 4MB
>    of memory.
> 
> IOW you trying to propagate driver error to device creation code...
> 
> Also result of device_register() should not depend on whether
> driver_register() was called earlier or not.

Indeed.  Greg - this patch is bogus.

When device_register() returns a negative number, many device drivers
assume (correctly) that the device was _not_ registered and they will
take whatever cleanup action is necessary to free the memory associated
with that device, believing that sysfs never used it.

If we start propagating driver probe error codes up through device_register()
we then have a big headache.  Is this -ENOMEM error a result of not being
able to register the struct device, or is it because something failed in
the driver.

This will also impact hotplug and PCMCIA.  For example, you have a device
driver loaded, but are short on memory, and you insert a PCMCIA device.
That creates a struct device.  Let's say that the driver fails to allocate
enough memory, and returns -ENOMEM.  With this patch, that'll kill off
the struct device.  Now, when the device is pulled, we could end up calling
driver_unregister on an unregistered struct device.

The expectations by current subsystems is that struct device registration
does _NOT_ depend on the currently loaded set of device drivers not
returning errors.

The thing is that this is all corner case stuff - you can apply this patch
and apparantly have a working system.  It's the corner cases where some
driver returns a failure which are the problem case.

Note also that this patch will not do what the ALSA folk want - they want
to know if the device was found or whether the probe returned -ENODEV.
We knock -ENODEV and -ENXIO to zero in driver_probe_device(), so they
won't even see that.

There is also no 1:1 relationship between platform device drivers and
platform devices.  You can have multiple platform devices driven by one
platform device driver.  See the plethora of platform devices in the
ARM sub-tree.  Or serial sub-tree.

So, all round this patch seems wrong.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On Tuesday 04 April 2006 17:45, Greg KH wrote:
> > But why do we do that? probe() failing is driver's problem. The device
> > is still there and should still be presented in sysfs. I don't think
> > that we should stop if probe() fails - maybe next driver manages to
> > bind itself.
> 
> The device is still there.
> 
> Ah, I see what you are saying now.  Yeah, we should still add the
> default attributes for the bus and create the bus link even if some
> random driver had problems. 

Not only that, but device will be killed as well - if bus_add_device()
fails device_add() branches into BusError which leads to kobject_del().

-- 
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x110944&bid$1720&dat\x121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On Tuesday 04 April 2006 17:45, Greg KH wrote:
> > But why do we do that? probe() failing is driver's problem. The device
> > is still there and should still be presented in sysfs. I don't think
> > that we should stop if probe() fails - maybe next driver manages to
> > bind itself.
> 
> The device is still there.
> 
> Ah, I see what you are saying now.  Yeah, we should still add the
> default attributes for the bus and create the bus link even if some
> random driver had problems. 

Not only that, but device will be killed as well - if bus_add_device()
fails device_add() branches into BusError which leads to kobject_del().

-- 
Dmitry

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

Greg KH wrote:

>  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
>    is ignored and 0 is returned, causing the loop to continue to try
>    more drivers

> [1] - stupid agp drivers (or some other video drivers) require this.  I
>     need to go fix them up so they don't do this, if they haven't been
>     fixed already...

Is the "this" in "require this" referring to (-ENODEV or -ENXIO) or to 
-ENXIO alone and do you consider the -ENODEV behaviour correct?

ALSA wants platform_device_register_simple() to return an IS_ERR() on 
driver probe error and the submitted patch makes it do so for all the 
other errors but ALSA likes to propagate errors up as far as possible, 
and currently its probe() methods can return either...

To Dmitry, I see you saying "probe() failing is driver's problem. The 
device is still there and should still be presented in sysfs.". No, at 
least in the case of these platform drivers (or at least these old ISA 
cards using the platform driver interface), a -ENODEV return from the 
probe() method would mean the device is _not_ present (or not found at 
least). NODEV.

As said before, if the behaviour makes sense for other busses, maybe 
propagating errors up should be dependent on a flags value somewhere 
that a platform-driver sets?

If platform_device_register_simple() never returns an IS_ERR() when the 
device is not found that means it's not a useful interface for hardware 
that needs to be probed for at the very least. ALSA would need to do 
something like, just before returning a succesfull return from the 
probe() method, set a global flag that the platform_device that is about 
to be registered is actually representing something, and freeing all 
platform_devices for which the flag is _not_ set again after this.

Which ofcourse means this is not at all useful. It's just working around 
the driver model then...

Rene.

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

Rene Herman wrote:

> As said before, if the behaviour makes sense for other busses, maybe 
> propagating errors up should be dependent on a flags value somewhere 
> that a platform-driver sets?
> 
> If platform_device_register_simple() never returns an IS_ERR() when the 
> device is not found that means it's not a useful interface for hardware 
> that needs to be probed for at the very least. ALSA would need to do 
> something like, just before returning a succesfull return from the 
> probe() method, set a global flag that the platform_device that is about 
> to be registered is actually representing something, and freeing all 
> platform_devices for which the flag is _not_ set again after this.
> 
> Which ofcourse means this is not at all useful. It's just working around 
> the driver model then...

Well, we could in fact hang an unregister off device->private_data as 
per attached example. Wouldn't be _excessively_ ugly. Still sucks 
though. Having a flag somewhere in struct device_driver that a 
platform_driver would set and which would tell the driver model to 
honour the driver probe() method return seems the cleanest approach for 
all hardware where only the driver can now if the device is really 
present or not.

Ofcourse, I'm also still fine with just propagating the error up always 
(and if can be doing something about the ignore of -ENODEV / -ENOXIO)...

Rene.

[-- Attachment #2: adlib-unregister.diff --]
[-- Type: text/plain, Size: 1033 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-05 02:00:55.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-05 02:05:45.000000000 +0200
@@ -43,8 +43,7 @@ static int __devinit snd_adlib_probe(str
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
-	int error;
-	int i = device->id;
+	int error, i = device->id;
 
 	if (port[i] == SNDRV_AUTO_PORT) {
 		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
@@ -95,8 +94,7 @@ static int __devinit snd_adlib_probe(str
 	return 0;
 
 out1:	snd_card_free(card);
- out0:	error = -EINVAL; /* FIXME: should be the original error code */
-	return error;
+out0:	return error;
 }
 
 static int __devexit snd_adlib_remove(struct platform_device *device)
@@ -134,6 +132,11 @@ static int __init alsa_card_adlib_init(v
 		if (IS_ERR(device))
 			continue;
 
+		if (!platform_get_drvdata(device)) {
+			platform_device_unregister(device);
+			continue;
+		}
+
 		devices[i] = device;
 		cards++;
 	}

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

Rene Herman wrote:

> As said before, if the behaviour makes sense for other busses, maybe 
> propagating errors up should be dependent on a flags value somewhere 
> that a platform-driver sets?
> 
> If platform_device_register_simple() never returns an IS_ERR() when the 
> device is not found that means it's not a useful interface for hardware 
> that needs to be probed for at the very least. ALSA would need to do 
> something like, just before returning a succesfull return from the 
> probe() method, set a global flag that the platform_device that is about 
> to be registered is actually representing something, and freeing all 
> platform_devices for which the flag is _not_ set again after this.
> 
> Which ofcourse means this is not at all useful. It's just working around 
> the driver model then...

Well, we could in fact hang an unregister off device->private_data as 
per attached example. Wouldn't be _excessively_ ugly. Still sucks 
though. Having a flag somewhere in struct device_driver that a 
platform_driver would set and which would tell the driver model to 
honour the driver probe() method return seems the cleanest approach for 
all hardware where only the driver can now if the device is really 
present or not.

Ofcourse, I'm also still fine with just propagating the error up always 
(and if can be doing something about the ignore of -ENODEV / -ENOXIO)...

Rene.

[-- Attachment #2: adlib-unregister.diff --]
[-- Type: text/plain, Size: 1033 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-05 02:00:55.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-05 02:05:45.000000000 +0200
@@ -43,8 +43,7 @@ static int __devinit snd_adlib_probe(str
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
-	int error;
-	int i = device->id;
+	int error, i = device->id;
 
 	if (port[i] == SNDRV_AUTO_PORT) {
 		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
@@ -95,8 +94,7 @@ static int __devinit snd_adlib_probe(str
 	return 0;
 
 out1:	snd_card_free(card);
- out0:	error = -EINVAL; /* FIXME: should be the original error code */
-	return error;
+out0:	return error;
 }
 
 static int __devexit snd_adlib_remove(struct platform_device *device)
@@ -134,6 +132,11 @@ static int __init alsa_card_adlib_init(v
 		if (IS_ERR(device))
 			continue;
 
+		if (!platform_get_drvdata(device)) {
+			platform_device_unregister(device);
+			continue;
+		}
+
 		devices[i] = device;
 		cards++;
 	}

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On Tuesday 04 April 2006 20:23, Rene Herman wrote:
> Rene Herman wrote:
> 
> > As said before, if the behaviour makes sense for other busses, maybe 
> > propagating errors up should be dependent on a flags value somewhere 
> > that a platform-driver sets?
> > 
> > If platform_device_register_simple() never returns an IS_ERR() when the 
> > device is not found that means it's not a useful interface for hardware 
> > that needs to be probed for at the very least. ALSA would need to do 
> > something like, just before returning a succesfull return from the 
> > probe() method, set a global flag that the platform_device that is about 
> > to be registered is actually representing something, and freeing all 
> > platform_devices for which the flag is _not_ set again after this.
> > 
> > Which ofcourse means this is not at all useful. It's just working around 
> > the driver model then...
> 
> Well, we could in fact hang an unregister off device->private_data as 
> per attached example. Wouldn't be _excessively_ ugly. Still sucks 
> though.

Plus it broke all the drivers that create platform devices before
registering drivers or the ones simply not using private data. Given
that some arches have a means to separate device creation from driver
probing (see pcspkr on PPC for exaple) I don;t think this is acceptable.

-- 
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On Tuesday 04 April 2006 20:23, Rene Herman wrote:
> Rene Herman wrote:
> 
> > As said before, if the behaviour makes sense for other busses, maybe 
> > propagating errors up should be dependent on a flags value somewhere 
> > that a platform-driver sets?
> > 
> > If platform_device_register_simple() never returns an IS_ERR() when the 
> > device is not found that means it's not a useful interface for hardware 
> > that needs to be probed for at the very least. ALSA would need to do 
> > something like, just before returning a succesfull return from the 
> > probe() method, set a global flag that the platform_device that is about 
> > to be registered is actually representing something, and freeing all 
> > platform_devices for which the flag is _not_ set again after this.
> > 
> > Which ofcourse means this is not at all useful. It's just working around 
> > the driver model then...
> 
> Well, we could in fact hang an unregister off device->private_data as 
> per attached example. Wouldn't be _excessively_ ugly. Still sucks 
> though.

Plus it broke all the drivers that create platform devices before
registering drivers or the ones simply not using private data. Given
that some arches have a means to separate device creation from driver
probing (see pcspkr on PPC for exaple) I don;t think this is acceptable.

-- 
Dmitry

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

Dmitry Torokhov wrote:

>> Well, we could in fact hang an unregister off device->private_data as 
>> per attached example. Wouldn't be _excessively_ ugly. Still sucks 
>> though.
> 
> Plus it broke all the drivers that create platform devices before
> registering drivers or the ones simply not using private data.

No, this was just a suggestion for an ALSA specific workaround. I was 
suggesting ALSA drivers could do this.

Rene.

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On 4/5/06, Rene Herman <rene.herman@keyaccess.nl> wrote:
> Dmitry Torokhov wrote:
>
> >> Well, we could in fact hang an unregister off device->private_data as
> >> per attached example. Wouldn't be _excessively_ ugly. Still sucks
> >> though.
> >
> > Plus it broke all the drivers that create platform devices before
> > registering drivers or the ones simply not using private data.
>
> No, this was just a suggestion for an ALSA specific workaround. I was
> suggesting ALSA drivers could do this.
>

Yes, I am sorry - I misread the code snipped as if it was in driver
core, not in ALSA itself.

--
Dmitry

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On 4/5/06, Rene Herman <rene.herman@keyaccess.nl> wrote:
> Dmitry Torokhov wrote:
>
> >> Well, we could in fact hang an unregister off device->private_data as
> >> per attached example. Wouldn't be _excessively_ ugly. Still sucks
> >> though.
> >
> > Plus it broke all the drivers that create platform devices before
> > registering drivers or the ones simply not using private data.
>
> No, this was just a suggestion for an ALSA specific workaround. I was
> suggesting ALSA drivers could do this.
>

Yes, I am sorry - I misread the code snipped as if it was in driver
core, not in ALSA itself.

--
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x110944&bid$1720&dat\x121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

Dmitry Torokhov wrote:

>> Well, we could in fact hang an unregister off device->private_data as 
>> per attached example. Wouldn't be _excessively_ ugly. Still sucks 
>> though.
> 
> Plus it broke all the drivers that create platform devices before
> registering drivers or the ones simply not using private data.

No, this was just a suggestion for an ALSA specific workaround. I was 
suggesting ALSA drivers could do this.

Rene.


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On Tuesday 04 April 2006 18:12, Rene Herman wrote:
> To Dmitry, I see you saying "probe() failing is driver's problem. The 
> device is still there and should still be presented in sysfs.". No, at 
> least in the case of these platform drivers (or at least these old ISA 
> cards using the platform driver interface), a -ENODEV return from the 
> probe() method would mean the device is _not_ present (or not found at 
> least). NODEV.

Or you could separate device probing code from driver->probe(). BTW I think
that ->probe() is not the best name for that method. It really is supposed
to allocate resources and initialize the device so that it is ready to be
used, not to verify that device is present. The code that created device
shoudl've done that.

-- 
Dmitry

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 4509 bytes --]

Dmitry Torokhov wrote:

> On Tuesday 04 April 2006 18:12, Rene Herman wrote:

>> To Dmitry, I see you saying "probe() failing is driver's problem.
>> The device is still there and should still be presented in sysfs.".
>> No, at least in the case of these platform drivers (or at least
>> these old ISA cards using the platform driver interface), a -ENODEV
>> return from the probe() method would mean the device is _not_
>> present (or not found at least). NODEV.
> 
> Or you could separate device probing code from driver->probe(). BTW I
> think that ->probe() is not the best name for that method.

Right...

> It really is supposed to allocate resources and initialize the device
> so that it is ready to be used, not to verify that device is present.
> The code that created device shoudl've done that.

How do you feel about the flag that I've been proposing that a driver 
that needs to probe for its hardware could set and that says "if we 
return an error (or specifically -ENODEV I guess) the hardware's 
reallyreally not present and the device should not register"?

Greg, and how do you feel about such a flag?

As an alternative to the flag, how would either of you feel about either

1) adding a .discover method to struct device_driver that noone other 
than drivers for this old non generically discoverable hardware would 
set but which would make a registration fail if set and returning < 0?

2) adding that method only to platform_driver?

3) ... and after a few months when people aren't paying attention 
anymore rename .probe to .init and .discover back to .probe? ;-)

Russel, you wrote:

> Note also that this patch will not do what the ALSA folk want - they
> want to know if the device was found or whether the probe returned
> -ENODEV. We knock -ENODEV and -ENXIO to zero in
> driver_probe_device(), so they won't even see that.

Yes, I know about the -ENODEV / -ENXIO thing. I asked for comment on 
that as well, but haven't gotten any.

Anyways, the additional method would, I feel, be the conceptually 
cleanest approach. Practically speaking though, simply doing a manual 
probe and only calling platform_register() after everything is found to 
be present and accounted for is not much of a problem either.

Takashi, how would you feel about a setup for ALSA going like:

static struct platform_device *devices[SNDRV_CARDS];

static int __devinit snd_foo_probe(int i)
{
	struct snd_card *card;

         if (port[i] == SNDRV_AUTO_PORT) {
                 snd_printk(KERN_ERR "specify port\n");
		return -EINVAL;
         }

	card = snd_card_new(index[i], id[i], THIS_MODULE, 0);
         if (!card) {
                 snd_printk(KERN_ERR "could not create card\n");
                 return -EWHATEVER;
         }

	/* all the other things the probe method does */

         if (snd_card_register(card) < 0) {
                 snd_printk(KERN_ERR "could not register card\n");
                 return -EWHATEVERSNDCARDREGISTERRETURNED;
         }

         devices[i] = platform_device_register_simple(NAME, i, NULL, 0);
         if (IS_ERR(devices[i])) {
                 snd_printk(KERN_ERR "could not register device\n");
		snd_card_free(card);
                 return PTR_ERR(devices[i]);
         }

         platform_set_drvdata(devices[i], card);
         return 0;
}

Then deleting the .probe method from the foo_platform_driver struct and 
in the module init simply do a manual probe, using:

         if (platform_driver_register(&snd_foo_driver) < 0) {
                 snd_printk(KERN_ERR "could not register driver\n");
                 return -EARGHH;
         }

         for (cards = 0, i = 0; i < SNDRV_CARDS; i++)
                 if (enable[i] && snd_foo_probe(i) >= 0)
                         cards++

         if (!cards) {
                 printk(KERN_ER "foo not found or device busy\n");
                 platform_driver_unregister(&snd_foo_driver);
                 return -ENODEV;
         }

Also attached as a patch against adlib.c (it's the simplest driver by 
far, so using that one as an example. it's present in 2.6.17-rc1).

The platform_device registration could also stay in the init loop, but 
since we want the free the card again if it fails same as with all the 
other failures it's best off at the end of probe() I feel. You 
introduced all these nonpnp_probe() methods for platform devices so we 
do have a nice place to put this...

Would you feel this would be an okay setup, assuming we're not getting 
that .discover method?

Rene.

[-- Attachment #2: adlib-unregister.diff --]
[-- Type: text/plain, Size: 1033 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-05 02:00:55.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-05 02:05:45.000000000 +0200
@@ -43,8 +43,7 @@ static int __devinit snd_adlib_probe(str
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
-	int error;
-	int i = device->id;
+	int error, i = device->id;
 
 	if (port[i] == SNDRV_AUTO_PORT) {
 		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
@@ -95,8 +94,7 @@ static int __devinit snd_adlib_probe(str
 	return 0;
 
 out1:	snd_card_free(card);
- out0:	error = -EINVAL; /* FIXME: should be the original error code */
-	return error;
+out0:	return error;
 }
 
 static int __devexit snd_adlib_remove(struct platform_device *device)
@@ -134,6 +132,11 @@ static int __init alsa_card_adlib_init(v
 		if (IS_ERR(device))
 			continue;
 
+		if (!platform_get_drvdata(device)) {
+			platform_device_unregister(device);
+			continue;
+		}
+
 		devices[i] = device;
 		cards++;
 	}

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 4509 bytes --]

Dmitry Torokhov wrote:

> On Tuesday 04 April 2006 18:12, Rene Herman wrote:

>> To Dmitry, I see you saying "probe() failing is driver's problem.
>> The device is still there and should still be presented in sysfs.".
>> No, at least in the case of these platform drivers (or at least
>> these old ISA cards using the platform driver interface), a -ENODEV
>> return from the probe() method would mean the device is _not_
>> present (or not found at least). NODEV.
> 
> Or you could separate device probing code from driver->probe(). BTW I
> think that ->probe() is not the best name for that method.

Right...

> It really is supposed to allocate resources and initialize the device
> so that it is ready to be used, not to verify that device is present.
> The code that created device shoudl've done that.

How do you feel about the flag that I've been proposing that a driver 
that needs to probe for its hardware could set and that says "if we 
return an error (or specifically -ENODEV I guess) the hardware's 
reallyreally not present and the device should not register"?

Greg, and how do you feel about such a flag?

As an alternative to the flag, how would either of you feel about either

1) adding a .discover method to struct device_driver that noone other 
than drivers for this old non generically discoverable hardware would 
set but which would make a registration fail if set and returning < 0?

2) adding that method only to platform_driver?

3) ... and after a few months when people aren't paying attention 
anymore rename .probe to .init and .discover back to .probe? ;-)

Russel, you wrote:

> Note also that this patch will not do what the ALSA folk want - they
> want to know if the device was found or whether the probe returned
> -ENODEV. We knock -ENODEV and -ENXIO to zero in
> driver_probe_device(), so they won't even see that.

Yes, I know about the -ENODEV / -ENXIO thing. I asked for comment on 
that as well, but haven't gotten any.

Anyways, the additional method would, I feel, be the conceptually 
cleanest approach. Practically speaking though, simply doing a manual 
probe and only calling platform_register() after everything is found to 
be present and accounted for is not much of a problem either.

Takashi, how would you feel about a setup for ALSA going like:

static struct platform_device *devices[SNDRV_CARDS];

static int __devinit snd_foo_probe(int i)
{
	struct snd_card *card;

         if (port[i] == SNDRV_AUTO_PORT) {
                 snd_printk(KERN_ERR "specify port\n");
		return -EINVAL;
         }

	card = snd_card_new(index[i], id[i], THIS_MODULE, 0);
         if (!card) {
                 snd_printk(KERN_ERR "could not create card\n");
                 return -EWHATEVER;
         }

	/* all the other things the probe method does */

         if (snd_card_register(card) < 0) {
                 snd_printk(KERN_ERR "could not register card\n");
                 return -EWHATEVERSNDCARDREGISTERRETURNED;
         }

         devices[i] = platform_device_register_simple(NAME, i, NULL, 0);
         if (IS_ERR(devices[i])) {
                 snd_printk(KERN_ERR "could not register device\n");
		snd_card_free(card);
                 return PTR_ERR(devices[i]);
         }

         platform_set_drvdata(devices[i], card);
         return 0;
}

Then deleting the .probe method from the foo_platform_driver struct and 
in the module init simply do a manual probe, using:

         if (platform_driver_register(&snd_foo_driver) < 0) {
                 snd_printk(KERN_ERR "could not register driver\n");
                 return -EARGHH;
         }

         for (cards = 0, i = 0; i < SNDRV_CARDS; i++)
                 if (enable[i] && snd_foo_probe(i) >= 0)
                         cards++

         if (!cards) {
                 printk(KERN_ER "foo not found or device busy\n");
                 platform_driver_unregister(&snd_foo_driver);
                 return -ENODEV;
         }

Also attached as a patch against adlib.c (it's the simplest driver by 
far, so using that one as an example. it's present in 2.6.17-rc1).

The platform_device registration could also stay in the init loop, but 
since we want the free the card again if it fails same as with all the 
other failures it's best off at the end of probe() I feel. You 
introduced all these nonpnp_probe() methods for platform devices so we 
do have a nice place to put this...

Would you feel this would be an okay setup, assuming we're not getting 
that .discover method?

Rene.

[-- Attachment #2: adlib-unregister.diff --]
[-- Type: text/plain, Size: 1033 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-05 02:00:55.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-05 02:05:45.000000000 +0200
@@ -43,8 +43,7 @@ static int __devinit snd_adlib_probe(str
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
-	int error;
-	int i = device->id;
+	int error, i = device->id;
 
 	if (port[i] == SNDRV_AUTO_PORT) {
 		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
@@ -95,8 +94,7 @@ static int __devinit snd_adlib_probe(str
 	return 0;
 
 out1:	snd_card_free(card);
- out0:	error = -EINVAL; /* FIXME: should be the original error code */
-	return error;
+out0:	return error;
 }
 
 static int __devexit snd_adlib_remove(struct platform_device *device)
@@ -134,6 +132,11 @@ static int __init alsa_card_adlib_init(v
 		if (IS_ERR(device))
 			continue;
 
+		if (!platform_get_drvdata(device)) {
+			platform_device_unregister(device);
+			continue;
+		}
+
 		devices[i] = device;
 		cards++;
 	}

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On 4/5/06, Rene Herman <rene.herman@keyaccess.nl> wrote:
> Dmitry Torokhov wrote:
>
> > On Tuesday 04 April 2006 18:12, Rene Herman wrote:
>
> >> To Dmitry, I see you saying "probe() failing is driver's problem.
> >> The device is still there and should still be presented in sysfs.".
> >> No, at least in the case of these platform drivers (or at least
> >> these old ISA cards using the platform driver interface), a -ENODEV
> >> return from the probe() method would mean the device is _not_
> >> present (or not found at least). NODEV.
> >
> > Or you could separate device probing code from driver->probe(). BTW I
> > think that ->probe() is not the best name for that method.
>
> Right...
>
> > It really is supposed to allocate resources and initialize the device
> > so that it is ready to be used, not to verify that device is present.
> > The code that created device shoudl've done that.
>
> How do you feel about the flag that I've been proposing that a driver
> that needs to probe for its hardware could set and that says "if we
> return an error (or specifically -ENODEV I guess) the hardware's
> reallyreally not present and the device should not register"?
>
> Greg, and how do you feel about such a flag?
>
> As an alternative to the flag, how would either of you feel about either
>
> 1) adding a .discover method to struct device_driver that noone other
> than drivers for this old non generically discoverable hardware would
> set but which would make a registration fail if set and returning < 0?
>
> 2) adding that method only to platform_driver?
>
> 3) ... and after a few months when people aren't paying attention
> anymore rename .probe to .init and .discover back to .probe? ;-)
>

You need to let go of the model that driver that drives hardware also
do the device discovery and it will all fall into place. While it may
be contained in the same source module it is 2 different code chunks
(for the lack of the better word) and we should not mix them together.

> Russel, you wrote:
>
> > Note also that this patch will not do what the ALSA folk want - they
> > want to know if the device was found or whether the probe returned
> > -ENODEV. We knock -ENODEV and -ENXIO to zero in
> > driver_probe_device(), so they won't even see that.
>
> Yes, I know about the -ENODEV / -ENXIO thing. I asked for comment on
> that as well, but haven't gotten any.
>
> Anyways, the additional method would, I feel, be the conceptually
> cleanest approach. Practically speaking though, simply doing a manual
> probe and only calling platform_register() after everything is found to
> be present and accounted for is not much of a problem either.
>

Unfortunately it breaks manual driver binding/unbinding through sysfs
so I don't think it is a good long-term solution.

--
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x110944&bid$1720&dat\x121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On 4/5/06, Rene Herman <rene.herman@keyaccess.nl> wrote:
> Dmitry Torokhov wrote:
>
> > On Tuesday 04 April 2006 18:12, Rene Herman wrote:
>
> >> To Dmitry, I see you saying "probe() failing is driver's problem.
> >> The device is still there and should still be presented in sysfs.".
> >> No, at least in the case of these platform drivers (or at least
> >> these old ISA cards using the platform driver interface), a -ENODEV
> >> return from the probe() method would mean the device is _not_
> >> present (or not found at least). NODEV.
> >
> > Or you could separate device probing code from driver->probe(). BTW I
> > think that ->probe() is not the best name for that method.
>
> Right...
>
> > It really is supposed to allocate resources and initialize the device
> > so that it is ready to be used, not to verify that device is present.
> > The code that created device shoudl've done that.
>
> How do you feel about the flag that I've been proposing that a driver
> that needs to probe for its hardware could set and that says "if we
> return an error (or specifically -ENODEV I guess) the hardware's
> reallyreally not present and the device should not register"?
>
> Greg, and how do you feel about such a flag?
>
> As an alternative to the flag, how would either of you feel about either
>
> 1) adding a .discover method to struct device_driver that noone other
> than drivers for this old non generically discoverable hardware would
> set but which would make a registration fail if set and returning < 0?
>
> 2) adding that method only to platform_driver?
>
> 3) ... and after a few months when people aren't paying attention
> anymore rename .probe to .init and .discover back to .probe? ;-)
>

You need to let go of the model that driver that drives hardware also
do the device discovery and it will all fall into place. While it may
be contained in the same source module it is 2 different code chunks
(for the lack of the better word) and we should not mix them together.

> Russel, you wrote:
>
> > Note also that this patch will not do what the ALSA folk want - they
> > want to know if the device was found or whether the probe returned
> > -ENODEV. We knock -ENODEV and -ENXIO to zero in
> > driver_probe_device(), so they won't even see that.
>
> Yes, I know about the -ENODEV / -ENXIO thing. I asked for comment on
> that as well, but haven't gotten any.
>
> Anyways, the additional method would, I feel, be the conceptually
> cleanest approach. Practically speaking though, simply doing a manual
> probe and only calling platform_register() after everything is found to
> be present and accounted for is not much of a problem either.
>

Unfortunately it breaks manual driver binding/unbinding through sysfs
so I don't think it is a good long-term solution.

--
Dmitry

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

Dmitry Torokhov wrote:

> You need to let go of the model that driver that drives hardware also
> do the device discovery and it will all fall into place.

It's not possible to sanely let go of that model. ISA discovery involves 
(as an example) poking at the same I/O ports as using does, meaning you 
will have to share a request_region() over discovery and use for one -- 
you can't decouple that due to obvious races.

>> Anyways, the additional method would, I feel, be the conceptually
>> cleanest approach. Practically speaking though, simply doing a manual
>> probe and only calling platform_register() after everything is found to
>> be present and accounted for is not much of a problem either.
>>
> 
> Unfortunately it breaks manual driver binding/unbinding through sysfs
> so I don't think it is a good long-term solution.

Yes. I don't see a significantly cleaner solution then than the slightly 
hackish "using drvdata as a private success flag" that I posted before. 
Example patch versus snd_adlib attached again. This seems to work well.

Takashi: do you agree? If the probe() method return is not going to be 
propagated up, there are few other options.

(Continuing the loop on IS_ERR(device) is then also a bit questionable 
again as the IS_ERR then signifies an eror in the bowels of the device 
model, but I feel it's still the correct thing to do)

Rene.


[-- Attachment #2: adlib_unregister.diff --]
[-- Type: text/plain, Size: 1033 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-05 02:00:55.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-05 02:05:45.000000000 +0200
@@ -43,8 +43,7 @@ static int __devinit snd_adlib_probe(str
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
-	int error;
-	int i = device->id;
+	int error, i = device->id;
 
 	if (port[i] == SNDRV_AUTO_PORT) {
 		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
@@ -95,8 +94,7 @@ static int __devinit snd_adlib_probe(str
 	return 0;
 
 out1:	snd_card_free(card);
- out0:	error = -EINVAL; /* FIXME: should be the original error code */
-	return error;
+out0:	return error;
 }
 
 static int __devexit snd_adlib_remove(struct platform_device *device)
@@ -134,6 +132,11 @@ static int __init alsa_card_adlib_init(v
 		if (IS_ERR(device))
 			continue;
 
+		if (!platform_get_drvdata(device)) {
+			platform_device_unregister(device);
+			continue;
+		}
+
 		devices[i] = device;
 		cards++;
 	}

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]

Dmitry Torokhov wrote:

> You need to let go of the model that driver that drives hardware also
> do the device discovery and it will all fall into place.

It's not possible to sanely let go of that model. ISA discovery involves 
(as an example) poking at the same I/O ports as using does, meaning you 
will have to share a request_region() over discovery and use for one -- 
you can't decouple that due to obvious races.

>> Anyways, the additional method would, I feel, be the conceptually
>> cleanest approach. Practically speaking though, simply doing a manual
>> probe and only calling platform_register() after everything is found to
>> be present and accounted for is not much of a problem either.
>>
> 
> Unfortunately it breaks manual driver binding/unbinding through sysfs
> so I don't think it is a good long-term solution.

Yes. I don't see a significantly cleaner solution then than the slightly 
hackish "using drvdata as a private success flag" that I posted before. 
Example patch versus snd_adlib attached again. This seems to work well.

Takashi: do you agree? If the probe() method return is not going to be 
propagated up, there are few other options.

(Continuing the loop on IS_ERR(device) is then also a bit questionable 
again as the IS_ERR then signifies an eror in the bowels of the device 
model, but I feel it's still the correct thing to do)

Rene.


[-- Attachment #2: adlib_unregister.diff --]
[-- Type: text/plain, Size: 1033 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-05 02:00:55.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-05 02:05:45.000000000 +0200
@@ -43,8 +43,7 @@ static int __devinit snd_adlib_probe(str
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
-	int error;
-	int i = device->id;
+	int error, i = device->id;
 
 	if (port[i] == SNDRV_AUTO_PORT) {
 		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
@@ -95,8 +94,7 @@ static int __devinit snd_adlib_probe(str
 	return 0;
 
 out1:	snd_card_free(card);
- out0:	error = -EINVAL; /* FIXME: should be the original error code */
-	return error;
+out0:	return error;
 }
 
 static int __devexit snd_adlib_remove(struct platform_device *device)
@@ -134,6 +132,11 @@ static int __init alsa_card_adlib_init(v
 		if (IS_ERR(device))
 			continue;
 
+		if (!platform_get_drvdata(device)) {
+			platform_device_unregister(device);
+			continue;
+		}
+
 		devices[i] = device;
 		cards++;
 	}

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 4596 bytes --]

Dmitry Torokhov wrote:

[ crap, attached the wrong patch, this time with correct attachment ]

> On Tuesday 04 April 2006 18:12, Rene Herman wrote:

>> To Dmitry, I see you saying "probe() failing is driver's problem.
>> The device is still there and should still be presented in sysfs.".
>> No, at least in the case of these platform drivers (or at least
>> these old ISA cards using the platform driver interface), a -ENODEV
>> return from the probe() method would mean the device is _not_
>> present (or not found at least). NODEV.
> 
> Or you could separate device probing code from driver->probe(). BTW I
> think that ->probe() is not the best name for that method.

Right...

> It really is supposed to allocate resources and initialize the device
> so that it is ready to be used, not to verify that device is present.
> The code that created device shoudl've done that.

How do you feel about the flag that I've been proposing that a driver
that needs to probe for its hardware could set and that says "if we
return an error (or specifically -ENODEV I guess) the hardware's
reallyreally not present and the device should not register"?

Greg, and how do you feel about such a flag?

As an alternative to the flag, how would either of you feel about either

1) adding a .discover method to struct device_driver that noone other
than drivers for this old non generically discoverable hardware would
set but which would make a registration fail if set and returning < 0?

2) adding that method only to platform_driver?

3) ... and after a few months when people aren't paying attention
anymore rename .probe to .init and .discover back to .probe? ;-)

Russel, you wrote:

> Note also that this patch will not do what the ALSA folk want - they
> want to know if the device was found or whether the probe returned
> -ENODEV. We knock -ENODEV and -ENXIO to zero in
> driver_probe_device(), so they won't even see that.

Yes, I know about the -ENODEV / -ENXIO thing. I asked for comment on
that as well, but haven't gotten any.

Anyways, the additional method would, I feel, be the conceptually
cleanest approach. Practically speaking though, simply doing a manual
probe and only calling platform_register() after everything is found to
be present and accounted for is not much of a problem either.

Takashi, how would you feel about a setup for ALSA going like:

static struct platform_device *devices[SNDRV_CARDS];

static int __devinit snd_foo_probe(int i)
{
	struct snd_card *card;

          if (port[i] == SNDRV_AUTO_PORT) {
                  snd_printk(KERN_ERR "specify port\n");
		return -EINVAL;
          }

	 card = snd_card_new(index[i], id[i], THIS_MODULE, 0);
          if (!card) {
                  snd_printk(KERN_ERR "could not create card\n");
                  return -EWHATEVER;
          }

	 /* all the other things the probe method does */

          if (snd_card_register(card) < 0) {
                  snd_printk(KERN_ERR "could not register card\n");
                  return -EWHATEVERSNDCARDREGISTERRETURNED;
          }

          devices[i] = platform_device_register_simple(NAME, i, NULL, 0);
          if (IS_ERR(devices[i])) {
                  snd_printk(KERN_ERR "could not register device\n");
		snd_card_free(card);
                  return PTR_ERR(devices[i]);
          }

          platform_set_drvdata(devices[i], card);
          return 0;
}

Then deleting the .probe method from the foo_platform_driver struct and
in the module init simply do a manual probe, using:

          if (platform_driver_register(&snd_foo_driver) < 0) {
                  snd_printk(KERN_ERR "could not register driver\n");
                  return -EARGHH;
          }

          for (cards = 0, i = 0; i < SNDRV_CARDS; i++)
                  if (enable[i] && snd_foo_probe(i) >= 0)
                          cards++

          if (!cards) {
                  printk(KERN_ER "foo not found or device busy\n");
                  platform_driver_unregister(&snd_foo_driver);
                  return -ENODEV;
          }

Also attached as a patch against adlib.c (it's the simplest driver by
far, so using that one as an example. it's present in 2.6.17-rc1).

The platform_device registration could also stay in the init loop, but
since we want the free the card again if it fails same as with all the
other failures it's best off at the end of probe() I feel. You
introduced all these nonpnp_probe() methods for platform devices so we
do have a nice place to put this...

Would you feel this would be an okay setup, assuming we're not getting
that .discover method?

Rene.


[-- Attachment #2: adlib-manual-probe.diff --]
[-- Type: text/plain, Size: 2284 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-05 13:41:27.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-05 14:31:32.000000000 +0200
@@ -38,13 +38,12 @@ static void snd_adlib_free(struct snd_ca
 	release_and_free_resource(card->private_data);
 }
 
-static int __devinit snd_adlib_probe(struct platform_device *device)
+static int __devinit snd_adlib_probe(int i)
 {
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
 	int error;
-	int i = device->id;
 
 	if (port[i] == SNDRV_AUTO_PORT) {
 		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
@@ -91,12 +90,18 @@ static int __devinit snd_adlib_probe(str
 		goto out1;
 	}
 
-	platform_set_drvdata(device, card);
+	devices[i] = platform_device_register_simple(DRV_NAME, i, NULL, 0);
+	if (IS_ERR(devices[i])) {
+		snd_printk(KERN_ERR DRV_NAME ": could not register device\n");
+		error = PTR_ERR(devices[i]);
+		goto out1;
+	}
+
+	platform_set_drvdata(devices[i], card);
 	return 0;
 
 out1:	snd_card_free(card);
- out0:	error = -EINVAL; /* FIXME: should be the original error code */
-	return error;
+out0:	return error;
 }
 
 static int __devexit snd_adlib_remove(struct platform_device *device)
@@ -107,9 +112,7 @@ static int __devexit snd_adlib_remove(st
 }
 
 static struct platform_driver snd_adlib_driver = {
-	.probe		= snd_adlib_probe,
 	.remove		= __devexit_p(snd_adlib_remove),
-
 	.driver		= {
 		.name	= DRV_NAME
 	}
@@ -117,26 +120,17 @@ static struct platform_driver snd_adlib_
 
 static int __init alsa_card_adlib_init(void)
 {
-	int i, cards;
+	int error, cards, i;
 
-	if (platform_driver_register(&snd_adlib_driver) < 0) {
+	error = platform_driver_register(&snd_adlib_driver);
+	if (error < 0) {
 		snd_printk(KERN_ERR DRV_NAME ": could not register driver\n");
-		return -ENODEV;
+		return error;
 	}
 
-	for (cards = 0, i = 0; i < SNDRV_CARDS; i++) {
-		struct platform_device *device;
-
-		if (!enable[i])
-			continue;
-
-		device = platform_device_register_simple(DRV_NAME, i, NULL, 0);
-		if (IS_ERR(device))
-			continue;
-
-		devices[i] = device;
-		cards++;
-	}
+	for (cards = 0, i = 0; i < SNDRV_CARDS; i++)
+		if (enable[i] && snd_adlib_probe(i) >= 0)
+			cards++
 
 	if (!cards) {
 #ifdef MODULE

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

[-- Attachment #1: Type: text/plain, Size: 4596 bytes --]

Dmitry Torokhov wrote:

[ crap, attached the wrong patch, this time with correct attachment ]

> On Tuesday 04 April 2006 18:12, Rene Herman wrote:

>> To Dmitry, I see you saying "probe() failing is driver's problem.
>> The device is still there and should still be presented in sysfs.".
>> No, at least in the case of these platform drivers (or at least
>> these old ISA cards using the platform driver interface), a -ENODEV
>> return from the probe() method would mean the device is _not_
>> present (or not found at least). NODEV.
> 
> Or you could separate device probing code from driver->probe(). BTW I
> think that ->probe() is not the best name for that method.

Right...

> It really is supposed to allocate resources and initialize the device
> so that it is ready to be used, not to verify that device is present.
> The code that created device shoudl've done that.

How do you feel about the flag that I've been proposing that a driver
that needs to probe for its hardware could set and that says "if we
return an error (or specifically -ENODEV I guess) the hardware's
reallyreally not present and the device should not register"?

Greg, and how do you feel about such a flag?

As an alternative to the flag, how would either of you feel about either

1) adding a .discover method to struct device_driver that noone other
than drivers for this old non generically discoverable hardware would
set but which would make a registration fail if set and returning < 0?

2) adding that method only to platform_driver?

3) ... and after a few months when people aren't paying attention
anymore rename .probe to .init and .discover back to .probe? ;-)

Russel, you wrote:

> Note also that this patch will not do what the ALSA folk want - they
> want to know if the device was found or whether the probe returned
> -ENODEV. We knock -ENODEV and -ENXIO to zero in
> driver_probe_device(), so they won't even see that.

Yes, I know about the -ENODEV / -ENXIO thing. I asked for comment on
that as well, but haven't gotten any.

Anyways, the additional method would, I feel, be the conceptually
cleanest approach. Practically speaking though, simply doing a manual
probe and only calling platform_register() after everything is found to
be present and accounted for is not much of a problem either.

Takashi, how would you feel about a setup for ALSA going like:

static struct platform_device *devices[SNDRV_CARDS];

static int __devinit snd_foo_probe(int i)
{
	struct snd_card *card;

          if (port[i] == SNDRV_AUTO_PORT) {
                  snd_printk(KERN_ERR "specify port\n");
		return -EINVAL;
          }

	 card = snd_card_new(index[i], id[i], THIS_MODULE, 0);
          if (!card) {
                  snd_printk(KERN_ERR "could not create card\n");
                  return -EWHATEVER;
          }

	 /* all the other things the probe method does */

          if (snd_card_register(card) < 0) {
                  snd_printk(KERN_ERR "could not register card\n");
                  return -EWHATEVERSNDCARDREGISTERRETURNED;
          }

          devices[i] = platform_device_register_simple(NAME, i, NULL, 0);
          if (IS_ERR(devices[i])) {
                  snd_printk(KERN_ERR "could not register device\n");
		snd_card_free(card);
                  return PTR_ERR(devices[i]);
          }

          platform_set_drvdata(devices[i], card);
          return 0;
}

Then deleting the .probe method from the foo_platform_driver struct and
in the module init simply do a manual probe, using:

          if (platform_driver_register(&snd_foo_driver) < 0) {
                  snd_printk(KERN_ERR "could not register driver\n");
                  return -EARGHH;
          }

          for (cards = 0, i = 0; i < SNDRV_CARDS; i++)
                  if (enable[i] && snd_foo_probe(i) >= 0)
                          cards++

          if (!cards) {
                  printk(KERN_ER "foo not found or device busy\n");
                  platform_driver_unregister(&snd_foo_driver);
                  return -ENODEV;
          }

Also attached as a patch against adlib.c (it's the simplest driver by
far, so using that one as an example. it's present in 2.6.17-rc1).

The platform_device registration could also stay in the init loop, but
since we want the free the card again if it fails same as with all the
other failures it's best off at the end of probe() I feel. You
introduced all these nonpnp_probe() methods for platform devices so we
do have a nice place to put this...

Would you feel this would be an okay setup, assuming we're not getting
that .discover method?

Rene.


[-- Attachment #2: adlib-manual-probe.diff --]
[-- Type: text/plain, Size: 2284 bytes --]

Index: local/sound/isa/adlib.c
===================================================================
--- local.orig/sound/isa/adlib.c	2006-04-05 13:41:27.000000000 +0200
+++ local/sound/isa/adlib.c	2006-04-05 14:31:32.000000000 +0200
@@ -38,13 +38,12 @@ static void snd_adlib_free(struct snd_ca
 	release_and_free_resource(card->private_data);
 }
 
-static int __devinit snd_adlib_probe(struct platform_device *device)
+static int __devinit snd_adlib_probe(int i)
 {
 	struct snd_card *card;
 	struct snd_opl3 *opl3;
 
 	int error;
-	int i = device->id;
 
 	if (port[i] == SNDRV_AUTO_PORT) {
 		snd_printk(KERN_ERR DRV_NAME ": please specify port\n");
@@ -91,12 +90,18 @@ static int __devinit snd_adlib_probe(str
 		goto out1;
 	}
 
-	platform_set_drvdata(device, card);
+	devices[i] = platform_device_register_simple(DRV_NAME, i, NULL, 0);
+	if (IS_ERR(devices[i])) {
+		snd_printk(KERN_ERR DRV_NAME ": could not register device\n");
+		error = PTR_ERR(devices[i]);
+		goto out1;
+	}
+
+	platform_set_drvdata(devices[i], card);
 	return 0;
 
 out1:	snd_card_free(card);
- out0:	error = -EINVAL; /* FIXME: should be the original error code */
-	return error;
+out0:	return error;
 }
 
 static int __devexit snd_adlib_remove(struct platform_device *device)
@@ -107,9 +112,7 @@ static int __devexit snd_adlib_remove(st
 }
 
 static struct platform_driver snd_adlib_driver = {
-	.probe		= snd_adlib_probe,
 	.remove		= __devexit_p(snd_adlib_remove),
-
 	.driver		= {
 		.name	= DRV_NAME
 	}
@@ -117,26 +120,17 @@ static struct platform_driver snd_adlib_
 
 static int __init alsa_card_adlib_init(void)
 {
-	int i, cards;
+	int error, cards, i;
 
-	if (platform_driver_register(&snd_adlib_driver) < 0) {
+	error = platform_driver_register(&snd_adlib_driver);
+	if (error < 0) {
 		snd_printk(KERN_ERR DRV_NAME ": could not register driver\n");
-		return -ENODEV;
+		return error;
 	}
 
-	for (cards = 0, i = 0; i < SNDRV_CARDS; i++) {
-		struct platform_device *device;
-
-		if (!enable[i])
-			continue;
-
-		device = platform_device_register_simple(DRV_NAME, i, NULL, 0);
-		if (IS_ERR(device))
-			continue;
-
-		devices[i] = device;
-		cards++;
-	}
+	for (cards = 0, i = 0; i < SNDRV_CARDS; i++)
+		if (enable[i] && snd_adlib_probe(i) >= 0)
+			cards++
 
 	if (!cards) {
 #ifdef MODULE

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Dmitry Torokhov]

On Tuesday 04 April 2006 18:12, Rene Herman wrote:
> To Dmitry, I see you saying "probe() failing is driver's problem. The 
> device is still there and should still be presented in sysfs.". No, at 
> least in the case of these platform drivers (or at least these old ISA 
> cards using the platform driver interface), a -ENODEV return from the 
> probe() method would mean the device is _not_ present (or not found at 
> least). NODEV.

Or you could separate device probing code from driver->probe(). BTW I think
that ->probe() is not the best name for that method. It really is supposed
to allocate resources and initialize the device so that it is ready to be
used, not to verify that device is present. The code that created device
shoudl've done that.

-- 
Dmitry


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Rene Herman]

Greg KH wrote:

>  - if that probe() function returns -ENODEV or -ENXIO[1] then the error
>    is ignored and 0 is returned, causing the loop to continue to try
>    more drivers

> [1] - stupid agp drivers (or some other video drivers) require this.  I
>     need to go fix them up so they don't do this, if they haven't been
>     fixed already...

Is the "this" in "require this" referring to (-ENODEV or -ENXIO) or to 
-ENXIO alone and do you consider the -ENODEV behaviour correct?

ALSA wants platform_device_register_simple() to return an IS_ERR() on 
driver probe error and the submitted patch makes it do so for all the 
other errors but ALSA likes to propagate errors up as far as possible, 
and currently its probe() methods can return either...

To Dmitry, I see you saying "probe() failing is driver's problem. The 
device is still there and should still be presented in sysfs.". No, at 
least in the case of these platform drivers (or at least these old ISA 
cards using the platform driver interface), a -ENODEV return from the 
probe() method would mean the device is _not_ present (or not found at 
least). NODEV.

As said before, if the behaviour makes sense for other busses, maybe 
propagating errors up should be dependent on a flags value somewhere 
that a platform-driver sets?

If platform_device_register_simple() never returns an IS_ERR() when the 
device is not found that means it's not a useful interface for hardware 
that needs to be probed for at the very least. ALSA would need to do 
something like, just before returning a succesfull return from the 
probe() method, set a global flag that the platform_device that is about 
to be registered is actually representing something, and freeing all 
platform_devices for which the flag is _not_ set again after this.

Which ofcourse means this is not at all useful. It's just working around 
the driver model then...

Rene.




-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

Re: patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[Greg KH]

On Tue, Apr 04, 2006 at 04:23:43PM -0400, Dmitry Torokhov wrote:
> On 4/4/06, gregkh@suse.de <gregkh@suse.de> wrote:
> >
> > --- gregkh-2.6.orig/drivers/base/bus.c
> > +++ gregkh-2.6/drivers/base/bus.c
> > @@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
> >
> >        if (bus) {
> >                pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
> > -               device_attach(dev);
> > +               error = device_attach(dev);
> > +               if (error < 0)
> > +                       goto exit;
> 
> I do not believe that this is correct. The fact that _some_ driver
> failed to attach to a device does not necessarily mean that device
> itself does not exist. While this assuption might work for platform
> devices it won't work for other busses.

Hm, no, I unwound this mess, and found the following:

 - bus_add_device() calls device_attach()
 - device_attach() calls bus_for_each_drv() for every driver on the bus
 - bus_for_each_drv() walks all drivers on the bus and calls
   __device_attach() for every individual driver
 - __device_attach() calls driver_probe_device() for that driver and device
 - driver_probe_device() calls down to the probe() function for the
   driver, passing it that driver, if match() for the bus matches this
   device.
 - if that probe() function returns -ENODEV or -ENXIO[1] then the error
   is ignored and 0 is returned, causing the loop to continue to try
   more drivers
 - if the probe() function returns any other error code, it is
   propagated up, all the way back to bus_add_device.
 - if the probe() function returns 0, the device is bound to the driver,
   and it returns 0.  Hm, looks like we continue to loop here too, we
   could probably stop now that we have bound a driver to the device.

So, I'm pretty sure that this is safe and should work just fine.  To be
sure, let me go reboot my box with this change on it after I finish this
email :)

Does that help?

thanks,

greg k-h

[1] - stupid agp drivers (or some other video drivers) require this.  I
    need to go fix them up so they don't do this, if they haven't been
    fixed already...


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642

patch bus_add_device-losing-an-error-return-from-the-probe-method.patch added to gregkh-2.6 tree

[gregkh]

This is a note to let you know that I've just added the patch titled

     Subject: bus_add_device() losing an error return from the probe() method

to my gregkh-2.6 tree.  Its filename is

     bus_add_device-losing-an-error-return-from-the-probe-method.patch

This tree can be found at 
    http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/

Patches currently in gregkh-2.6 which might be from rene.herman@keyaccess.nl are

driver/bus_add_device-losing-an-error-return-from-the-probe-method.patch


>From rene.herman@keyaccess.nl Thu Mar 23 21:32:00 2006
Message-ID: <44238489.8090402@keyaccess.nl>
Date: Fri, 24 Mar 2006 06:32:57 +0100
From: Rene Herman <rene.herman@keyaccess.nl>
To: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Takashi Iwai <tiwai@suse.de>, ALSA devel <alsa-devel@alsa-project.org>, Linux Kernel <linux-kernel@vger.kernel.org>
Subject: bus_add_device() losing an error return from the probe() method

ALSA moved all ISA drivers over to the platform_driver interface in
2.6.16, using this code structure in the module_inits:

    cards = 0;
    for (i = 0; i < SNDRV_CARDS; i++) {
	  struct platform_device *device;
	  device = platform_device_register_simple(
			SND_FOO_DRIVER, i, NULL, 0);
	  if (IS_ERR(device)) {
		  err = PTR_ERR(device);
		  goto errout;
	  }
	  devices[i] = device;
	  cards++;
    }
    if (!cards) {
	  printk(KERN_ERR "FOO soundcard not found or device busy\n");
	  err = -ENODEV;
	  goto errout;
    }
    return 0;
errout:
    snd_foo_unregister_all();
    return err;

Unfortunately, the snd_foo_unregister_all() part here is unreachable
under normal circumstances, since platform_device_register_simple()
returns !IS_ERR, regardless of what the driver probe method returned.
The driver then never fails to load, even when no cards were found.

An error return from the driver probe() method is carried up through
device_attach, but is then dropped on the floor in bus_add_device(). If
I apply the attached patch, things work as I (and ALSA it seems) expect.

From: Rene Herman <rene.herman@keyaccess.nl>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 drivers/base/bus.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

--- gregkh-2.6.orig/drivers/base/bus.c
+++ gregkh-2.6/drivers/base/bus.c
@@ -372,14 +372,17 @@ int bus_add_device(struct device * dev)
 
 	if (bus) {
 		pr_debug("bus %s: add device %s\n", bus->name, dev->bus_id);
-		device_attach(dev);
+		error = device_attach(dev);
+		if (error < 0)
+			goto exit;
 		klist_add_tail(&dev->knode_bus, &bus->klist_devices);
 		error = device_add_attrs(bus, dev);
-		if (!error) {
-			sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
-			sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
-		}
+		if (error)
+			goto exit;
+		sysfs_create_link(&bus->devices.kobj, &dev->kobj, dev->bus_id);
+		sysfs_create_link(&dev->kobj, &dev->bus->subsys.kset.kobj, "bus");
 	}
+exit:
 	return error;
 }
 


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642