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

[Rene Herman]

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/