Mailing List Archive

At Thu, 06 Apr 2006 04:08:34 +0200,
Rene Herman wrote:
>
> Rene Herman wrote:
>
> > 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)
>
> If you do agree, here's both patches generated against 2.6.17-rc1-mm1.
> First, the continue on IS_ERR one again.

Well, I'm not so confident that it's the way to go.

The problem is that currently the ALSA ISA driver wants to refuse
loading (and modprobe returns an error) when no devices are found at
loading time. On 2.2/2.4 kernels, PCI drivers also show the same
behavior, but it was changed for a good reason.

Then, shouldn't be the ISA drivers changed to follow the same style?
That is, we keep pnp_driver and platform_driver regardless probe calls
succeeded or not. They can be, in theory, later bound/activated over
sysfs.

At least it would make the code more simpler, I guess.


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

Takashi Iwai wrote:

[ unregistering platform devices again when no device found ]

> Well, I'm not so confident that it's the way to go.

Okay... Added Greg back to the CC. Don't know if he wants to comment or not.

> The problem is that currently the ALSA ISA driver wants to refuse
> loading (and modprobe returns an error) when no devices are found at
> loading time. On 2.2/2.4 kernels, PCI drivers also show the same
> behavior, but it was changed for a good reason.
>
> Then, shouldn't be the ISA drivers changed to follow the same style?
> That is, we keep pnp_driver and platform_driver regardless probe
> calls succeeded or not. They can be, in theory, later
> bound/activated over sysfs.

The thing I'm stumbling over is the non (generic) discoverability of ISA
versus busses such as ISA-PnP and PCI which makes for a big conceptual
difference.

A PnP/PCI device has a life all by itself by virtue of its bus knowing
that it's present. For one, the device will be present in /sys/devices/
without any specific driver for the device loaded yet. A platform device
on the other hand only "exists" by virtue of a driver creating it
because it might want to drive it. If we keep it registered even after
failing a probe, then /sys/devices/platform turns into a view of "what
drivers did we load" rather then "what's present in this system". As far
as I'm aware, that latter view of /sys/devices was at one time the idea.
Just imagine loading all ISA drivers for an appreciation of the amount
of pollution to this view always keeping the devices registered does.

However!

I must say I wasn't aware that ALSA PCI devices at the moment load
without devices. Wasn't there even an oft used ALSA configuration script
out there that worked by loading all drivers and checking which ones stuck?

I just checked the behaviour and yes, the drivers load. Most importantly
to the analogy, even without any of the supported IDs present.

I have a 125d:1978 (ESS Canyon3D) at PCI 0000\:00\:08\:0. If I remove
the 125d:1978 device ID from snd-es1968, then yes, snd-es1968 still
loads. With the ID still not present, a:

echo -n 0000\:00\:08.0 >"/sys/bus/pci/drivers/ES1968 (ESS Maestro)/bind"

runs into a bug somewhere it seems. The xterm I'm doing that in now
hangs (but is killable). Doing:

echo "125d 1978" >"/sys/bus/pci/drivers/ES1968 (ESS Maestro)/new_id"

instead does work fine, and after this binding and unbinding work fine
as well again, which means there at least seems to be some point.

In the analogy, given that the PCI driver loads even without any of its
IDs present means a platform_driver loading without anything present as
well isn't, from the view of the driver model, much of a difference
after all. PnP doesn't have a "new_id" field, but could have, and I
therefore in fact agree by now that it's best to follow PCI's lead here.

> At least it would make the code more simpler, I guess.

Not significantly. We'd need to seperate the probe() method a bit, so
that the things we need before a bind _could_ ever succeed were in the
main loop and would make the driver skip device registration if not
fullfilled. For example, if with the snd-adlib driver no port= is given,
then attempted binds would simply tell you "please specify port" over
and over again, something which can only be solved by unloading and
reloading the driver, this time specifying the port. This is still a
difference due to non-discoverability, and I feel this should still make
the driver fail to load.

For snd-adlib, this would look like the attached patch. I'm also being
more verbose about which bus_id is failing. The !cards printk() is gone,
as it is no longer a matter of "device not found or busy" (and there
will be a printk() from the actual error spot anyway) and changed the
-ENODEV there to -EINVAL.

It works. "modprobe snd-adlib" fails to load, and "modprobe snd-adlib
port=0x388" succeeds, with or without probe failures. I can for example
now do:

# modprobe snd-cs4236 // which claims 0x388
# modprobe snd-adlib port=0x388 // which has it load
# modprobe -r snd-cs4236 && modprobe snd-cs4236 fm_port=0

which enables the OPL at 0x388, but leaves it alone, so that I can then:

# echo -n snd_adlib.0 >/sys/bus/platform/drivers/snd_adlib/bind

to bind the snd_adlib driver to it. Which is... erm... great fun I guess.

(I am by the way being so specific about what I'm doing with these sysfs
things because until Dmitry pointed them out to me in the precursor
discussion, I was't even aware of bind/unbind. Want to make sure I
understand how to operate it all).

How do you feel about this one?

Rene.

At Fri, 07 Apr 2006 18:26:26 +0200,
Rene Herman wrote:
>
> I must say I wasn't aware that ALSA PCI devices at the moment load
> without devices. Wasn't there even an oft used ALSA configuration script
> out there that worked by loading all drivers and checking which ones stuck?

Not for PCI. alsaconf checks the return status of modprobe for ISA
non-PnP devices, though, so we would fix it once if the probe model
were changed.

(snip)
> In the analogy, given that the PCI driver loads even without any of its
> IDs present means a platform_driver loading without anything present as
> well isn't, from the view of the driver model, much of a difference
> after all. PnP doesn't have a "new_id" field, but could have, and I
> therefore in fact agree by now that it's best to follow PCI's lead here.

Exactly, that's the reason I suggested. To keep the behavior of
driver consistent.

OTOH, I'm not 100% for that change, because I know that it will break
the old behavior as mentioned in the above. Such a breakage isn't
always a good buy against just-for-cleanness or consistency...

> > At least it would make the code more simpler, I guess.
>
> Not significantly.

Well, you can remove the whole

if (!cards)
... unregister...

in the probe callback if we follow the pci driver model, i.e. modprobe
doesn't return an error if no device is found at the moment.

> We'd need to seperate the probe() method a bit, so
> that the things we need before a bind _could_ ever succeed were in the
> main loop and would make the driver skip device registration if not
> fullfilled. For example, if with the snd-adlib driver no port= is given,
> then attempted binds would simply tell you "please specify port" over
> and over again, something which can only be solved by unloading and
> reloading the driver, this time specifying the port. This is still a
> difference due to non-discoverability, and I feel this should still make
> the driver fail to load.

Hm, surely it's not so intuitive in the case of ISA devices. Maybe
it'd be better to keep the current behavior: probe() returns an error
if no device is found at loading...


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

Takashi Iwai wrote:

> Hm, surely it's not so intuitive in the case of ISA devices. Maybe
> it'd be better to keep the current behavior: probe() returns an
> error if no device is found at loading...

Okay. It's always possible to revisit the isssue later. Keeping the old
behaviour is what the already submitted patches did. probe() returning
an error is not enough; the issue was/is that that error is not
being passed up. Using drvdata as a private success flag as submitted
works fine fortunately; all drivers do a platform_set_drvdata(device,
card) just before returning success from probe().

I'll repost them following this message. Have been generated against
2.6.17-rc1-mm2.

Will also post them (and the !enable[i] patch) against 2.6.16.2 for
-stable. You already acked the !enable[i] and continue-on-is_err patches
for -stable, the third is the same unregister patch, restoring the old
fail-to-load behaviour also for -stable.

Considering that one of the rules for -stable is that fixes also need to
be present in upstream trees, could you relay the three patches for
-stable yourself?

Rene.


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

Hi Takashi.

Continue with the next one on error from device registration. Against
2.6.17-rc1-mm2.

This would seem the correct thing to do, even if it's not the probe()
error that we're getting.

sound/isa/ad1848/ad1848.c | 14 ++++----------
sound/isa/cmi8330.c | 14 ++++----------
sound/isa/cs423x/cs4231.c | 14 ++++----------
sound/isa/cs423x/cs4236.c | 14 ++++----------
sound/isa/es1688/es1688.c | 14 ++++----------
sound/isa/es18xx.c | 14 ++++----------
sound/isa/gus/gusclassic.c | 14 ++++----------
sound/isa/gus/gusextreme.c | 14 ++++----------
sound/isa/gus/gusmax.c | 14 ++++----------
sound/isa/gus/interwave.c | 14 ++++----------
sound/isa/opl3sa2.c | 14 ++++----------
sound/isa/sb/sb16.c | 14 ++++----------
sound/isa/sb/sb8.c | 14 ++++----------
sound/isa/sgalaxy.c | 14 ++++----------
sound/isa/sscape.c | 14 ++++----------
sound/isa/wavefront/wavefront.c | 14 ++++----------
16 files changed, 64 insertions(+), 160 deletions(-)

Rene

At Tue, 11 Apr 2006 01:10:59 +0200,
Rene Herman wrote:
>
> Will also post them (and the !enable[i] patch) against 2.6.16.2 for
> -stable. You already acked the !enable[i] and continue-on-is_err patches
> for -stable, the third is the same unregister patch, restoring the old
> fail-to-load behaviour also for -stable.
>
> Considering that one of the rules for -stable is that fixes also need to
> be present in upstream trees, could you relay the three patches for
> -stable yourself?

Yes, but only if you give a valid signed-off-by line at this time ;)


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