Re: [ALSA STABLE 3/3] a few more -- unregister platform device againif probe was unsuccessful

From: Rene Herman
Date: Thu Apr 13 2006 - 10:03:53 EST


Ingo Oeser wrote:

Why did you remove alsa-devel from the CC?

This is inserting lots of duplicate and control heavy code. It looks like the same pattern and is using just platform_xxx funxtions.

Any counter-examples with a different pattern, which you converted in
a different manner?

Not lots really... for all of them it's basically:

if (!platform_get_drvdata(device)) {
platform_device_unregister(device);
continue;
}

in the driver's main init loop. In this batch there were three drivers that both did not support more than one device and passed the error on up meaning it looks a bit a bit more clumsy than that but when moving these drivers from the platform_device_register_simple() interface this code will be revisited again; making them support more devices can also happen longer term.

Wouldn't it be more useful to do these checks in platform_register_simple() and return the proper error value there?

That interface is going away. I checked where ALSA used them due to that in fact (the same patches for sound/isa are already in ALSA and have been submitted for -stable as well).

Yes, it would be better if the driver model supplied this functionality itself. The problem is that an error return from the platform probe() method is not being honoured/passed up by the driver model so that we don't get a chance to say "no, the hardware's not here, go away!".

Not honouring/passing up probe() method error returns, not even -ENODEV, makes some sense for discoverable busses such as PCI where you at least have a driver independent bus_id sitting in /sys/devices/pci* that you can later echo into /sys/bus/pci/drivers/*/bind to make the driver bind to a device, but not much sense for the platform bus. Platform devices only "exist" (in /sys/devices/platform) due to the driver creating them itself and keeping them after failing a probe means that directory becomes an enumeration of the drivers we loaded, rather than a view of what's present in the system.

The driver model crowd did not seem exceedingly interested in the problem though:

http://marc.theaimsgroup.com/?l=linux-kernel&m=114417829014332&w=2

It _is_ ofcourse an option to not care about /sys/devices/platform, and ALSA could do that as well longer term. This was discussed:

http://marc.theaimsgroup.com/?l=linux-kernel&m=114442720631596&w=2

(marc seems to be not so good at keeping threads intact. reply at: http://marc.theaimsgroup.com/?l=linux-kernel&m=114469016131522&w=2)

and for now, we'll keep the old behaviour of not loading on probe failure, using drvdata() as a private success flag set from probe(). Longer term, we can revisit this.

Most importantly though, you replied to the one submitted for -stable. For -stable, this is certainly the way to go. ALSA drivers always failed to load on probe() failure before they were even using the platform driver interface (which was new to 2.6.16) and things like the alsaconf script rely on this. For -stable, it's a simple bugfix therefore.

Or just create a small helper, if this have to be done seperate.

That would be going way overboard for the three lines as stated in the beginning. Certainly when they might/will go again in the future...

Rene.

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