LinuxLists.cc - [PATCH] alsa: correct nonsensical sysfs device symlinks

[permalink] [raw]

Subject: Re: [PATCH] alsa: correct nonsensical sysfs device symlinks

On Wed, Jan 24, 2007 at 08:50:06PM -0500, Christopher Monty Montgomery wrote:
> This patch was generated against 2.6.20-rc5; it fixes a bug that
> cropped up in a late 2.6.19-mm kernel.
>
> When ALSA's sysfs device creation was converted from using
> class_device_create() to device_create(), the fourth param from
> class_device_create() [dev] was simply plugged into arg 2 of
> device_create(). This causes the device symlinks under all the
> class/sound/[node] to point to nonsensical places. Among other
> problems, this breaks HAL and all audio software that depends on HAL.

I don't understand, where does the symlink currently point to? It looks
correct to me on my machines:

$ tree /sys/class/sound/
/sys/class/sound/
|-- card0 -> ../../devices/pci0000:00/0000:00:1b.0/card0
|-- controlC0 -> ../../devices/pci0000:00/0000:00:1b.0/card0/controlC0
|-- pcmC0D0c -> ../../devices/pci0000:00/0000:00:1b.0/card0/pcmC0D0c
|-- pcmC0D0p -> ../../devices/pci0000:00/0000:00:1b.0/card0/pcmC0D0p
|-- pcmC0D2c -> ../../devices/pci0000:00/0000:00:1b.0/card0/pcmC0D2c
|-- pcmC0D6c -> ../../devices/pci0000:00/0000:00:1b.0/card0/pcmC0D6c
|-- pcmC0D6p -> ../../devices/pci0000:00/0000:00:1b.0/card0/pcmC0D6p
`-- timer -> ../../devices/virtual/sound/timer

What do the symlinks look on your machine?

thanks,

greg k-h

2007-01-25 11:27:46

by Pierre Ossman

[permalink] [raw]

Subject: Re: [PATCH] alsa: correct nonsensical sysfs device symlinks

Christopher "Monty" Montgomery wrote:
> This patch was generated against 2.6.20-rc5; it fixes a bug that
> cropped up in a late 2.6.19-mm kernel.
>
> When ALSA's sysfs device creation was converted from using
> class_device_create() to device_create(), the fourth param from
> class_device_create() [dev] was simply plugged into arg 2 of
> device_create(). This causes the device symlinks under all the
> class/sound/[node] to point to nonsensical places. Among other
> problems, this breaks HAL and all audio software that depends on HAL.
>

There are no device symlinks anymore, so the current behaviour seems
correct. HAL should follow the symlink, then move up in the device tree
to find a suitable parent.

Rgds
Pierre

Attachments:

signature.asc (251.00 B)
OpenPGP digital signature

2007-01-25 14:11:34

[permalink] [raw]

Subject: Re: [PATCH] alsa: correct nonsensical sysfs device symlinks

2007-01-25 15:15:53

by Pierre Ossman

[permalink] [raw]

Subject: Re: [PATCH] alsa: correct nonsensical sysfs device symlinks

Christopher "Monty" Montgomery wrote:
>
> My machines and the fedora rawhide machines using the 2.6.20-rcX
> releases look nothing like what you pasted. If they did, hald would
> be working... (I'll note that bugs have also been logged against
> Pulse and HAL by users having trouble, so it's not just us.)

On my vanilla 2.6.20-rc5, I get the same structure as Greg. Yet hal
still doesn't pick stuff up as it should.

Rgds
Pierre

Attachments:

signature.asc (251.00 B)
OpenPGP digital signature

2007-01-25 15:36:57

[permalink] [raw]

Subject: Re: [PATCH] alsa: correct nonsensical sysfs device symlinks

On 1/25/07, Pierre Ossman <[email protected]> wrote:
> Christopher "Monty" Montgomery wrote:
> >
> > My machines and the fedora rawhide machines using the 2.6.20-rcX
> > releases look nothing like what you pasted. If they did, hald would
> > be working... (I'll note that bugs have also been logged against
> > Pulse and HAL by users having trouble, so it's not just us.)
>
> On my vanilla 2.6.20-rc5, I get the same structure as Greg. Yet hal
> still doesn't pick stuff up as it should.

Interesting to know. Looking more closely, it looks like machines
here are split between the messed up output I forwarded previously and
the output that is expected. All of my personal boxes are messed up.

Having looked at HAL earlier, it appeared to have code to parse the
new format but if it desn't work, it doesn't work. Updating HAL is
relatively easy but I can't test the changes on my own boxes because
they have the messed up /sys/class/sound structure.

Monty

2007-01-25 15:40:48

by Pierre Ossman

[permalink] [raw]

Subject: Re: [PATCH] alsa: correct nonsensical sysfs device symlinks

Christopher "Monty" Montgomery wrote:
>
> Interesting to know. Looking more closely, it looks like machines
> here are split between the messed up output I forwarded previously and
> the output that is expected. All of my personal boxes are messed up.
>

There is some option about deprecated sysfs stuff. Perhaps this is the
cause of your twisted tree. It's off here:

# CONFIG_SYSFS_DEPRECATED is not set

Rgds
Pierre

Attachments:

signature.asc (251.00 B)
OpenPGP digital signature

2007-01-25 15:57:26

[permalink] [raw]

Subject: Re: [PATCH] alsa: correct nonsensical sysfs device symlinks

On 1/25/07, Pierre Ossman <[email protected]> wrote:

> There is some option about deprecated sysfs stuff. Perhaps this is the
> cause of your twisted tree. It's off here:
>
> # CONFIG_SYSFS_DEPRECATED is not set

It is set. It is also set in the default config, so plently of people
are running with a broken sysfs tree.

I assume we agree that even if that's set, the tree should not be
outright broken like it is (the compatability mode should actually be
compatable, ie, work, right? :-) Given that, is my patch correct?
What was there (plugging the old 'dev' arg into the new call's
'parent' makes no sense) is clearly wrong.

And it's clear my pacth is incomplete, as it doesn't correct the
device entries for the other entries.

Monty

2007-01-25 16:54:51

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

At Thu, 25 Jan 2007 10:57:25 -0500,
Christopher "Monty" Montgomery wrote:
>
> On 1/25/07, Pierre Ossman <[email protected]> wrote:
>
> > There is some option about deprecated sysfs stuff. Perhaps this is the
> > cause of your twisted tree. It's off here:
> >
> > # CONFIG_SYSFS_DEPRECATED is not set
>
> It is set. It is also set in the default config, so plently of people
> are running with a broken sysfs tree.
>
> I assume we agree that even if that's set, the tree should not be
> outright broken like it is (the compatability mode should actually be
> compatable, ie, work, right? :-) Given that, is my patch correct?
> What was there (plugging the old 'dev' arg into the new call's
> 'parent' makes no sense) is clearly wrong.

It makes sense because the meaning of card->dev was changed, too.
Now it points the "card*" object that is the root of all belonging
devices. The former card->dev is stored in card->parent.

> And it's clear my pacth is incomplete, as it doesn't correct the
> device entries for the other entries.

Well, for older systems, we shouldn't have also "card*" objects, too.
An untested patch below...

Takashi

diff -r 64671853e8e2 core/init.c
--- a/core/init.c Thu Jan 25 13:15:05 2007 +0100
+++ b/core/init.c Thu Jan 25 17:50:06 2007 +0100
@@ -503,12 +503,14 @@ int snd_card_register(struct snd_card *c
int err;

snd_assert(card != NULL, return -EINVAL);
+#ifndef CONFIG_SYSFS_DEPRECATED
if (!card->dev) {
card->dev = device_create(sound_class, card->parent, 0,
"card%i", card->number);
if (IS_ERR(card->dev))
card->dev = NULL;
}
+#endif
if ((err = snd_device_register_all(card)) < 0)
return err;
mutex_lock(&snd_card_mutex);
diff -r 64671853e8e2 core/sound.c
--- a/core/sound.c Thu Jan 25 13:15:05 2007 +0100
+++ b/core/sound.c Thu Jan 25 17:50:40 2007 +0100
@@ -241,6 +241,9 @@ int snd_register_device_for_dev(int type
int minor;
struct snd_minor *preg;

+ if (!device && !card)
+ device = card->parent;
+
snd_assert(name, return -EINVAL);
preg = kmalloc(sizeof *preg, GFP_KERNEL);
if (preg == NULL)

2007-01-25 17:03:15

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

> > What was there (plugging the old 'dev' arg into the new call's
> > 'parent' makes no sense) is clearly wrong.
>
> It makes sense because the meaning of card->dev was changed, too.
> Now it points the "card*" object that is the root of all belonging
> devices. The former card->dev is stored in card->parent.

Unfortunately card->parent does not work... but card->dev->parent does...

> > And it's clear my pacth is incomplete, as it doesn't correct the
> > device entries for the other entries.
>
> Well, for older systems, we shouldn't have also "card*" objects, too.

True enough.

> An untested patch below...

This patch does not correct the problem, unfortunately. I had
originally tried something similar, but card->parent still results in
the device symlink pointing to ../../../class/cardN.

I'm working on this now and will doublecheck just in case my test was
flawed first time.

Monty

2007-01-25 17:30:46

by Monty

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

> I'm working on this now and will doublecheck just in case my test was
> flawed first time.

Doublechecking indicates my initial test was wrong somehow; both
card->dev->parent and card->parent passed as arg 2 to the
device_create call in snd_register_device result in correct device
symlinks. Are these two actually semantically different? I'm just
curious.

The call to device_create in cound_core.c:sound_insert_unit also needs
to be modified in order for the device symlinks in the other unit
types to be correct. Again, modifying the 'dev' argument to
'(dev?dev->parent:NULL)' appears to fix the problem. Is that
correct/appropriate there?

Monty

2007-01-25 17:47:33

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

At Thu, 25 Jan 2007 12:30:44 -0500,
[email protected] wrote:
>
> > I'm working on this now and will doublecheck just in case my test was
> > flawed first time.
>
> Doublechecking indicates my initial test was wrong somehow; both
> card->dev->parent and card->parent passed as arg 2 to the
> device_create call in snd_register_device result in correct device
> symlinks. Are these two actually semantically different? I'm just
> curious.

If card->dev is created, they should be identical.
But, again, card->dev should be NULL on the older systems.

> The call to device_create in cound_core.c:sound_insert_unit also needs
> to be modified in order for the device symlinks in the other unit
> types to be correct. Again, modifying the 'dev' argument to
> '(dev?dev->parent:NULL)' appears to fix the problem. Is that
> correct/appropriate there?

Your change breaks the expected heavior if CONFIG_SYSFS_DEPRECATED is
enabled. Then devices won't belong to card* objects any more...

Takashi

2007-01-25 17:48:14

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

Updated version of patch also taking into account feedback from
Takashi is attached.

I'm going to look at hald now.

Monty

Attachments:

(No filename) (124.00 B)
alsa-sysfs-fix-2.6.20-rc5-updated.txt (1.27 kB)
Download all attachments

2007-01-25 18:07:09

by Monty

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

On 1/25/07, Takashi Iwai <[email protected]> wrote:
> At Thu, 25 Jan 2007 12:30:44 -0500,
> [email protected] wrote:
> >
> > > I'm working on this now and will doublecheck just in case my test was
> > > flawed first time.
> >
> > Doublechecking indicates my initial test was wrong somehow; both
> > card->dev->parent and card->parent passed as arg 2 to the
> > device_create call in snd_register_device result in correct device
> > symlinks. Are these two actually semantically different? I'm just
> > curious.
>
> If card->dev is created, they should be identical.
> But, again, card->dev should be NULL on the older systems.
>
> > The call to device_create in cound_core.c:sound_insert_unit also needs
> > to be modified in order for the device symlinks in the other unit
> > types to be correct. Again, modifying the 'dev' argument to
> > '(dev?dev->parent:NULL)' appears to fix the problem. Is that
> > correct/appropriate there?
>
> Your change breaks the expected heavior if CONFIG_SYSFS_DEPRECATED is
> enabled. Then devices won't belong to card* objects any more...

FWIW, CONFIG_SYSFS_DEPRECATED is enabled on all my machines-- it's
exactly that behavior that is wrong in 2.6.20-rc5 and that I'm trying
to fix (and testing against here/now).

Having just checked a 2.6.18 machine, none of the device symlinks
point to card* objects; thay all point to /sys/devices/....

Monty

2007-01-25 18:23:30

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

At Thu, 25 Jan 2007 13:07:04 -0500,
[email protected] wrote:
>
> On 1/25/07, Takashi Iwai <[email protected]> wrote:
> > At Thu, 25 Jan 2007 12:30:44 -0500,
> > [email protected] wrote:
> > >
> > > > I'm working on this now and will doublecheck just in case my test was
> > > > flawed first time.
> > >
> > > Doublechecking indicates my initial test was wrong somehow; both
> > > card->dev->parent and card->parent passed as arg 2 to the
> > > device_create call in snd_register_device result in correct device
> > > symlinks. Are these two actually semantically different? I'm just
> > > curious.
> >
> > If card->dev is created, they should be identical.
> > But, again, card->dev should be NULL on the older systems.
> >
> > > The call to device_create in cound_core.c:sound_insert_unit also needs
> > > to be modified in order for the device symlinks in the other unit
> > > types to be correct. Again, modifying the 'dev' argument to
> > > '(dev?dev->parent:NULL)' appears to fix the problem. Is that
> > > correct/appropriate there?
> >
> > Your change breaks the expected heavior if CONFIG_SYSFS_DEPRECATED is
> > enabled. Then devices won't belong to card* objects any more...
>
> FWIW, CONFIG_SYSFS_DEPRECATED is enabled on all my machines-- it's
> exactly that behavior that is wrong in 2.6.20-rc5 and that I'm trying
> to fix (and testing against here/now).

Err, replace /enabled/disabled/ in my reply above.

The problem with your patch is that it breaks the structure newly
introduced. In the new tree, card* contains the whole belonging
devices, and each device points to the one in the card object.
Passing dev->parent there cuts the relation between the card object
and each device.

That's why my patch rules out setting card->dev with ifdef
CONFIG_SYSFS_DEPRECATED. As fallback, it takes the parent device to
build the old style tree.

> Having just checked a 2.6.18 machine, none of the device symlinks
> point to card* objects; thay all point to /sys/devices/....

Yes, it's a new stuff.

Takashi

2007-01-25 18:34:22

by Monty

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

On 1/25/07, Takashi Iwai <[email protected]> wrote:

> The problem with your patch is that it breaks the structure newly
> introduced. In the new tree, card* contains the whole belonging
> devices, and each device points to the one in the card object.
> Passing dev->parent there cuts the relation between the card object
> and each device.
>
> That's why my patch rules out setting card->dev with ifdef
> CONFIG_SYSFS_DEPRECATED. As fallback, it takes the parent device to
> build the old style tree.
>
> > Having just checked a 2.6.18 machine, none of the device symlinks
> > point to card* objects; thay all point to /sys/devices/....
>
> Yes, it's a new stuff.

[This does beg the question: Does the benefit of this complete
restructuring in a subminor release of an allegedly stable kernel
outweigh the fact that it breaks all audio for any user running a
gnome desktop?]

OK, so my patch should be passing card->parent or dev->parent only in
the case that CONFIG_SYSFS_DEPRECATED is set, and leaving the call the
way it was previously in the case CONFIG_SYSFS_DEPRECATED is not set.
Essentially, ifdef/else both cases?

Is this also going to have to be the case for every other class type
that converts from class_device_create to device_create?

Monty

>
>
> Takashi
>

2007-01-25 18:38:55

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

At Thu, 25 Jan 2007 13:34:19 -0500,
[email protected] wrote:
>
> On 1/25/07, Takashi Iwai <[email protected]> wrote:
>
> > The problem with your patch is that it breaks the structure newly
> > introduced. In the new tree, card* contains the whole belonging
> > devices, and each device points to the one in the card object.
> > Passing dev->parent there cuts the relation between the card object
> > and each device.
> >
> > That's why my patch rules out setting card->dev with ifdef
> > CONFIG_SYSFS_DEPRECATED. As fallback, it takes the parent device to
> > build the old style tree.
> >
> > > Having just checked a 2.6.18 machine, none of the device symlinks
> > > point to card* objects; thay all point to /sys/devices/....
> >
> > Yes, it's a new stuff.
>
> [This does beg the question: Does the benefit of this complete
> restructuring in a subminor release of an allegedly stable kernel
> outweigh the fact that it breaks all audio for any user running a
> gnome desktop?]

Well, that's not me who introduced that ;)

> OK, so my patch should be passing card->parent or dev->parent only in
> the case that CONFIG_SYSFS_DEPRECATED is set, and leaving the call the
> way it was previously in the case CONFIG_SYSFS_DEPRECATED is not set.
> Essentially, ifdef/else both cases?

Also, avoid creating card* instance. It makes no sense in that case.
(So... what's wrong with my patch?)

> Is this also going to have to be the case for every other class type
> that converts from class_device_create to device_create?

I don't think so. The case of sound class is more complicated, a
combination of API change and a new tree.

Takashi

2007-01-25 18:51:45

by Monty

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

On 1/25/07, Takashi Iwai <[email protected]> wrote:

> > [This does beg the question: Does the benefit of this complete
> > restructuring in a subminor release of an allegedly stable kernel
> > outweigh the fact that it breaks all audio for any user running a
> > gnome desktop?]
>
> Well, that's not me who introduced that ;)

Consider it addressed to those responsible.

> Also, avoid creating card* instance. It makes no sense in that case.

Well, the 'new' entries don't interfere with old-- and we inevitably
are going to end up with a situation where some software can only read
the 'new' entries and some software can only read the 'old', otherwise
why offer compatability at all? I interpreted the
CONFIG_SYSFS_DEPRECATED option to mean 'also offer the deprecated
entries in addition to the new structure', otherwise the name should
be CONFIG_SYSFS_OLDSTYLE or some such to imply they're mutually
exclusive....

> (So... what's wrong with my patch?)

As it turns out, nothing (I made some error somewhere initially when
testing it).
Apologies if I wasn't clear.

(Well, it's incomplete like my original patch was in that it only
changed the device symlinks for controlX and pcmX; the other entries
still had device entries pointing at card*).

Monty

2007-01-25 19:50:40

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

On Thu, Jan 25, 2007 at 01:51:42PM -0500, [email protected] wrote:
> On 1/25/07, Takashi Iwai <[email protected]> wrote:
>
> >> [This does beg the question: Does the benefit of this complete
> >> restructuring in a subminor release of an allegedly stable kernel
> >> outweigh the fact that it breaks all audio for any user running a
> >> gnome desktop?]
> >
> >Well, that's not me who introduced that ;)
>
> Consider it addressed to those responsible.

Which would be me. :)

And no, I don't want to break anything, hence that config option. If
it's enabled, sysfs should look just like before. But it looks like we
messed up somewhere and we need to fix it

> >Also, avoid creating card* instance. It makes no sense in that case.
>
> Well, the 'new' entries don't interfere with old-- and we inevitably
> are going to end up with a situation where some software can only read
> the 'new' entries and some software can only read the 'old', otherwise
> why offer compatability at all? I interpreted the
> CONFIG_SYSFS_DEPRECATED option to mean 'also offer the deprecated
> entries in addition to the new structure', otherwise the name should
> be CONFIG_SYSFS_OLDSTYLE or some such to imply they're mutually
> exclusive....

The name of the config option doesn't really matter as much as the help
text of the option does. Does reading that help out more?

Basically, new distros can disable that option if their userspace can
handle the new structure of sysfs with the symlinks. Users of older
distros with newer kernels can enable the option and (hopefully) not
break anything. So far, it's been working with this being the first
regression reported.

> >(So... what's wrong with my patch?)
>
> As it turns out, nothing (I made some error somewhere initially when
> testing it).
> Apologies if I wasn't clear.
>
> (Well, it's incomplete like my original patch was in that it only
> changed the device symlinks for controlX and pcmX; the other entries
> still had device entries pointing at card*).

So what does running with this patch make sysfs look like?

thanks,

greg k-h

2007-01-25 20:40:11

by Monty

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

> Basically, new distros can disable that option if their userspace can
> handle the new structure of sysfs with the symlinks. Users of older
> distros with newer kernels can enable the option and (hopefully) not
> break anything.

I would like to register a general objection to a change of this size
'hoping to not break anything' in a stable release update (seeing as
how something did break), especially as how no one here who was
maintaining the userspace was apparently even aware it was coming.

> So far, it's been working with this being the first
> regression reported.

Had it worked properly, it still would have been a regression. This
seems similar to the ALSA copout ("maintaining a kernel ABI is hard!
We require you have a userspace officially approved by us matching
your kernel, and that is your API.") except that in this case, the
userspace maintainence doesn't seem to be as tightly coupled (is hald
the only thing that broke?)

> > (Well, it's incomplete like my original patch was in that it only
> > changed the device symlinks for controlX and pcmX; the other entries
> > still had device entries pointing at card*).
>
> So what does running with this patch make sysfs look like?

A union of old and new, with old taking precedence over new when they
conflict. That is, the [eg] pcmCXdXx entries look like old sysfs, but
the new entries for cardX etc are there too. All device symlinks
point to /sys/device/pciXXXX:XX/XXXX:XX:XX.x entries as before.

Monty

2007-01-25 22:00:44

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

On Thu, Jan 25, 2007 at 03:40:06PM -0500, [email protected] wrote:
> >Basically, new distros can disable that option if their userspace can
> >handle the new structure of sysfs with the symlinks. Users of older
> >distros with newer kernels can enable the option and (hopefully) not
> >break anything.
>
> I would like to register a general objection to a change of this size
> 'hoping to not break anything' in a stable release update (seeing as
> how something did break), especially as how no one here who was
> maintaining the userspace was apparently even aware it was coming.

There is no such thing as a "stable release update" series anymore, you
should know that :)

It's incremental changes, over time, evolving into something new as you
look at the larger picture.

> >So far, it's been working with this being the first
> >regression reported.
>
> Had it worked properly, it still would have been a regression. This
> seems similar to the ALSA copout ("maintaining a kernel ABI is hard!
> We require you have a userspace officially approved by us matching
> your kernel, and that is your API.") except that in this case, the
> userspace maintainence doesn't seem to be as tightly coupled (is hald
> the only thing that broke?)

Well, as this HALD issue is the only thing that I have had reported, and
these patches have been in the -mm tree for quite some time, yes, I
think it is the only thing that has broken.

And yes, we have to have a way to evolve the api over time. I thought
that given a kernel config option to allow the api to change, if your
userspace has also been updated, is the perfict way. Then new distro
releases get to take advantage of the new stuff in the kernel, and we
don't break people who cling to Fedora Core 3 while wanting to run new
kernels (hi Andrew...)

In short, it's a bug, I messed up somehow. And I'm trying to fix it
now, I don't think there's more that I can do, do you?

> >> (Well, it's incomplete like my original patch was in that it only
> >> changed the device symlinks for controlX and pcmX; the other entries
> >> still had device entries pointing at card*).
> >
> >So what does running with this patch make sysfs look like?
>
> A union of old and new, with old taking precedence over new when they
> conflict. That is, the [eg] pcmCXdXx entries look like old sysfs, but
> the new entries for cardX etc are there too. All device symlinks
> point to /sys/device/pciXXXX:XX/XXXX:XX:XX.x entries as before.

Can you give me the 'tree /sys/class/sound' output?

Does it work properly with old versions of hald now?

Is there anything else left to fix?

thanks,

greg k-h

2007-01-26 10:53:40

by Monty

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

On 1/25/07, Greg KH <[email protected]> wrote:

I want to do just a little more testing here (Takashi reminded me I
have a bit more testing of my own to do).

> Can you give me the 'tree /sys/class/sound' output?
Once I verify it's 'all good' (actually, I'm stalling; not in front of
the machine with the patches right now.

> Does it work properly with old versions of hald now?

Yes. I still need to make doubly sure I didn't break the new sysfs tree though.

> Is there anything else left to fix?

Once that testing is done, no. But don't trust the two patches I sent
yet, I'll resumbit the patch resulting from more thorough testing in a
few hours (much thanks to Takashi for giving me the parent device
feedback I was trolling for).

Monty

2007-01-26 11:40:34

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

At Fri, 26 Jan 2007 05:53:36 -0500,
[email protected] wrote:
>
> On 1/25/07, Greg KH <[email protected]> wrote:
>
> > Is there anything else left to fix?
>
> Once that testing is done, no. But don't trust the two patches I sent
> yet, I'll resumbit the patch resulting from more thorough testing in a
> few hours (much thanks to Takashi for giving me the parent device
> feedback I was trolling for).

After rechecking the current code regarding this sysfs change at last
night, I found out that it's more broken for some devices like
sound/arm/*. They refer to card->dev to obtain the device for memory
allocation, etc, and passing card* object will screw them up.

The below is my current fix. Hoepfully all evils got away now... and
thanks for Monty for heading up this issue!

Takashi

====
[PATCH] ALSA: Fix sysfs breakage

The recent change for a new sysfs tree with card* object breaks the
/sys/class/sound tree if CONFIG_SYSFS_DEPRECATED is enabled.
The device in each entry doesn't point the correct device object:

/sys/class/sound
...
|-- pcmC0D0c
| |-- dev
| |-- device -> ../../../class/sound/card0
| |-- pcm_class
| |-- power
| | `-- wakeup
| |-- subsystem -> ../../../class/sound
| `-- uevent

Also, this change breaks some drivers (like sound/arm/*) referring
card->dev directly to obtain the device object for memory handling.

This patch reverts the semantics of card->dev to the former version,
which points to a real device object. The card* object is stored in a
new card->card_dev field, instead. The device parent is chosen either
card->dev or card->card_dev according to CONFIG_SYSFS_DEPRECATED to
keep the tree compatibility.
Also, card* isn't created if CONFIG_SYSFS_DEPRECATED is enabled. The
reason of card* object is a root of all beloing devices, and it makes
little sense if each sound device points to the real device object
directly.

Signed-off-by: Takashi Iwai <[email protected]>
---
include/sound/core.h | 18 +++++++++++++++---
sound/core/init.c | 18 +++++++++++-------
sound/core/sound.c | 4 +---
sound/core/sound_oss.c | 4 +---
4 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index a994bea..521f036 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -132,8 +132,10 @@ struct snd_card {
int shutdown; /* this card is going down */
int free_on_last_close; /* free in context of file_release */
wait_queue_head_t shutdown_sleep;
- struct device *parent;
- struct device *dev;
+ struct device *dev; /* device assigned to this card */
+#ifndef CONFIG_SYSFS_DEPRECATED
+ struct device *card_dev; /* cardX object for sysfs */
+#endif

#ifdef CONFIG_PM
unsigned int power_state; /* power state */
@@ -191,6 +193,16 @@ struct snd_minor {
struct device *dev; /* device for sysfs */
};

+/* return a device pointer linked to each sound device as a parent */
+static inline struct device *snd_card_get_device_link(struct snd_card *card)
+{
+#ifdef CONFIG_SYSFS_DEPRECATED
+ return card ? card->dev : NULL;
+#else
+ return card ? card->card_dev : NULL;
+#endif
+}
+
/* sound.c */

extern int snd_major;
@@ -257,7 +269,7 @@ int snd_card_file_add(struct snd_card *c
int snd_card_file_remove(struct snd_card *card, struct file *file);

#ifndef snd_card_set_dev
-#define snd_card_set_dev(card,devptr) ((card)->parent = (devptr))
+#define snd_card_set_dev(card,devptr) ((card)->dev = (devptr))
#endif

/* device.c */
diff --git a/sound/core/init.c b/sound/core/init.c
index 6152a75..a4cc6b1 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -361,8 +361,10 @@ static int snd_card_do_free(struct snd_c
snd_printk(KERN_WARNING "unable to free card info\n");
/* Not fatal error */
}
- if (card->dev)
- device_unregister(card->dev);
+#ifndef CONFIG_SYSFS_DEPRECATED
+ if (card->card_dev)
+ device_unregister(card->card_dev);
+#endif
kfree(card);
return 0;
}
@@ -497,12 +499,14 @@ int snd_card_register(struct snd_card *c
int err;

snd_assert(card != NULL, return -EINVAL);
- if (!card->dev) {
- card->dev = device_create(sound_class, card->parent, 0,
- "card%i", card->number);
- if (IS_ERR(card->dev))
- card->dev = NULL;
+#ifndef CONFIG_SYSFS_DEPRECATED
+ if (!card->card_dev) {
+ card->card_dev = device_create(sound_class, card->dev, 0,
+ "card%i", card->number);
+ if (IS_ERR(card->card_dev))
+ card->card_dev = NULL;
}
+#endif
if ((err = snd_device_register_all(card)) < 0)
return err;
mutex_lock(&snd_card_mutex);
diff --git a/sound/core/sound.c b/sound/core/sound.c
index 2827420..82a61c6 100644
--- a/sound/core/sound.c
+++ b/sound/core/sound.c
@@ -238,7 +238,7 @@ int snd_register_device(int type, struct
{
int minor;
struct snd_minor *preg;
- struct device *device = NULL;
+ struct device *device = snd_card_get_device_link(card);

snd_assert(name, return -EINVAL);
preg = kmalloc(sizeof *preg, GFP_KERNEL);
@@ -263,8 +263,6 @@ int snd_register_device(int type, struct
return minor;
}
snd_minors[minor] = preg;
- if (card)
- device = card->dev;
preg->dev = device_create(sound_class, device, MKDEV(major, minor),
"%s", name);
if (preg->dev)
diff --git a/sound/core/sound_oss.c b/sound/core/sound_oss.c
index b2fc40a..4566df4 100644
--- a/sound/core/sound_oss.c
+++ b/sound/core/sound_oss.c
@@ -106,7 +106,7 @@ int snd_register_oss_device(int type, st
int cidx = SNDRV_MINOR_OSS_CARD(minor);
int track2 = -1;
int register1 = -1, register2 = -1;
- struct device *carddev = NULL;
+ struct device *carddev = snd_card_get_device_link(card);

if (card && card->number >= 8)
return 0; /* ignore silently */
@@ -134,8 +134,6 @@ int snd_register_oss_device(int type, st
track2 = SNDRV_MINOR_OSS(cidx, SNDRV_MINOR_OSS_DMMIDI1);
break;
}
- if (card)
- carddev = card->dev;
register1 = register_sound_special_device(f_ops, minor, carddev);
if (register1 != minor)
goto __end;

2007-01-26 18:03:43

by Monty

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

Before I get to the less focused rant below, I wanted to mention that
the HELP text for 'Create deprecated sysfs files' does not make it
clear that one is opting for old in exclusion of new.

On 1/25/07, Greg KH <[email protected]> wrote:

> There is no such thing as a "stable release update" series anymore, you
> should know that :)

To the detriment of the users, yes. Subminor release update and
*bam*, audio doesn't work and it takes a kernel hacker to figure out
why, and because of a small but inevitable error (error is inevitable
in change). If this was a Windows service pack or a MacOS subminor
update, it would be front page news in industry trade mags. The fact
that it isn't is because it's Linux and we're frankly still the
retarded foster child adopted for tax purposes, at least as far as the
desktop goes.

I'm not saying change is bad, I'm saying this was a subtle, large
change and no one has explained the functional benefit beyond 'it's
cleaner' to outweigh 'many users want to strangle us'.

> Well, as this HALD issue is the only thing that I have had reported, and
> these patches have been in the -mm tree for quite some time, yes, I
> think it is the only thing that has broken.

Plenty of people spotted it, several bugs got filed, and the fact that
you're only hearing about it from me at least two months after
introduction is mildly worrisome. (I am already worried that *I*
didn't hear about it till recently as I attempted to push out Pulse in
fc rawhide and found it magically no longer worked. At all.)

If the hald developers were caught unprepared (so unprepared, in fact,
they've all ben inaccessable due to holiday + conference + vacation
for over a month and we can't push an update out), what is the average
user to do? Not to mention the fact that it's just a little
embarrassing that sysfs was invented for 2.6 and the original version
has already been declared deprecated, still in 2.6. There's change,
and then there's churn.

> In short, it's a bug, I messed up somehow. And I'm trying to fix it
> now, I don't think there's more that I can do, do you?

No. I'm not pushing back against you, I'm pushing back against the
budding 'stable APIs are for boring losers' culture. There's a reason
I refuse to update my personal machines more often than once every
year or two-- they spend the next few weeks completely dysfunctional
each time. To be fair, the kernel is usually far better than userland
[excepting ALSA, GRRRR], but let's not go pissing away that relative
lead :-)

Monty

2007-01-26 18:05:55

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

On Fri, Jan 26, 2007 at 12:40:31PM +0100, Takashi Iwai wrote:
> At Fri, 26 Jan 2007 05:53:36 -0500,
> [email protected] wrote:
> >
> > On 1/25/07, Greg KH <[email protected]> wrote:
> >
> > > Is there anything else left to fix?
> >
> > Once that testing is done, no. But don't trust the two patches I sent
> > yet, I'll resumbit the patch resulting from more thorough testing in a
> > few hours (much thanks to Takashi for giving me the parent device
> > feedback I was trolling for).
>
> After rechecking the current code regarding this sysfs change at last
> night, I found out that it's more broken for some devices like
> sound/arm/*. They refer to card->dev to obtain the device for memory
> allocation, etc, and passing card* object will screw them up.
>
> The below is my current fix. Hoepfully all evils got away now... and
> thanks for Monty for heading up this issue!
>
>
> Takashi
>
> ====
> [PATCH] ALSA: Fix sysfs breakage
>
> The recent change for a new sysfs tree with card* object breaks the
> /sys/class/sound tree if CONFIG_SYSFS_DEPRECATED is enabled.
> The device in each entry doesn't point the correct device object:
>
> /sys/class/sound
> ...
> |-- pcmC0D0c
> | |-- dev
> | |-- device -> ../../../class/sound/card0
> | |-- pcm_class
> | |-- power
> | | `-- wakeup
> | |-- subsystem -> ../../../class/sound
> | `-- uevent
>
> Also, this change breaks some drivers (like sound/arm/*) referring
> card->dev directly to obtain the device object for memory handling.
>
> This patch reverts the semantics of card->dev to the former version,
> which points to a real device object. The card* object is stored in a
> new card->card_dev field, instead. The device parent is chosen either
> card->dev or card->card_dev according to CONFIG_SYSFS_DEPRECATED to
> keep the tree compatibility.
> Also, card* isn't created if CONFIG_SYSFS_DEPRECATED is enabled. The
> reason of card* object is a root of all beloing devices, and it makes
> little sense if each sound device points to the real device object
> directly.
>
> Signed-off-by: Takashi Iwai <[email protected]>

Signed-off-by: Greg Kroah-Hartman <[email protected]>

Thanks for working on tracking this down, sorry I forgot all about this
when doing this original conversion.

greg k-h

2007-01-26 18:25:43

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

At Fri, 26 Jan 2007 10:04:57 -0800,
Greg KH wrote:
>
> On Fri, Jan 26, 2007 at 12:40:31PM +0100, Takashi Iwai wrote:
> > At Fri, 26 Jan 2007 05:53:36 -0500,
> > [email protected] wrote:
> > >
> > > On 1/25/07, Greg KH <[email protected]> wrote:
> > >
> > > > Is there anything else left to fix?
> > >
> > > Once that testing is done, no. But don't trust the two patches I sent
> > > yet, I'll resumbit the patch resulting from more thorough testing in a
> > > few hours (much thanks to Takashi for giving me the parent device
> > > feedback I was trolling for).
> >
> > After rechecking the current code regarding this sysfs change at last
> > night, I found out that it's more broken for some devices like
> > sound/arm/*. They refer to card->dev to obtain the device for memory
> > allocation, etc, and passing card* object will screw them up.
> >
> > The below is my current fix. Hoepfully all evils got away now... and
> > thanks for Monty for heading up this issue!
> >
> >
> > Takashi
> >
> > ====
> > [PATCH] ALSA: Fix sysfs breakage
> >
> > The recent change for a new sysfs tree with card* object breaks the
> > /sys/class/sound tree if CONFIG_SYSFS_DEPRECATED is enabled.
> > The device in each entry doesn't point the correct device object:
> >
> > /sys/class/sound
> > ...
> > |-- pcmC0D0c
> > | |-- dev
> > | |-- device -> ../../../class/sound/card0
> > | |-- pcm_class
> > | |-- power
> > | | `-- wakeup
> > | |-- subsystem -> ../../../class/sound
> > | `-- uevent
> >
> > Also, this change breaks some drivers (like sound/arm/*) referring
> > card->dev directly to obtain the device object for memory handling.
> >
> > This patch reverts the semantics of card->dev to the former version,
> > which points to a real device object. The card* object is stored in a
> > new card->card_dev field, instead. The device parent is chosen either
> > card->dev or card->card_dev according to CONFIG_SYSFS_DEPRECATED to
> > keep the tree compatibility.
> > Also, card* isn't created if CONFIG_SYSFS_DEPRECATED is enabled. The
> > reason of card* object is a root of all beloing devices, and it makes
> > little sense if each sound device points to the real device object
> > directly.
> >
> > Signed-off-by: Takashi Iwai <[email protected]>
>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> Thanks for working on tracking this down, sorry I forgot all about this
> when doing this original conversion.

Greg, feel free to pick it up to your tree for pushing. Since this
patch conflicts with the development tree of ALSA mm branch, it'd be
even harder to push from ALSA tree, unfortuantely...

Of course, it'd be safer after confirming that the patch works indeed.
(My brief tests show good results, though.)

thanks,

Takashi

2007-01-26 18:31:04

by Monty

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

Yes, working on testing all cases here, takes a while, kernel builds
are hella long on this machine.

Monty

2007-01-26 18:43:23

[permalink] [raw]

Subject: Re: [Alsa-devel] [PATCH] alsa: correct nonsensical sysfs device symlinks

On Fri, Jan 26, 2007 at 01:03:41PM -0500, [email protected] wrote:
> Before I get to the less focused rant below, I wanted to mention that
> the HELP text for 'Create deprecated sysfs files' does not make it
> clear that one is opting for old in exclusion of new.

Hm, can you think of some better wording that I might use for it?

> On 1/25/07, Greg KH <[email protected]> wrote:
>
> >There is no such thing as a "stable release update" series anymore, you
> >should know that :)
>
> To the detriment of the users, yes. Subminor release update and
> *bam*, audio doesn't work and it takes a kernel hacker to figure out
> why, and because of a small but inevitable error (error is inevitable
> in change). If this was a Windows service pack or a MacOS subminor
> update, it would be front page news in industry trade mags. The fact
> that it isn't is because it's Linux and we're frankly still the
> retarded foster child adopted for tax purposes, at least as far as the
> desktop goes.

Again, the kernel will only have "subminor release updates" as per the
number, but not as per what we are really doing here in the kernel.

And it only looks like it affected a subset of the distros out there, my
boxes all worked just fine, even with that config option turned off,
otherwise I would have caught it very early in testing.

> I'm not saying change is bad, I'm saying this was a subtle, large
> change and no one has explained the functional benefit beyond 'it's
> cleaner' to outweigh 'many users want to strangle us'.

Well, users should never want to strangle us, that means we messed up.
And yes, this was a bug, sorry.

As for why we are doing this kind of change, it makes sense from a
hierarchical point of view (one device tree, symlinks into the tree for
information that you need instead of many multiple device trees all
scattered around.) It also properly gives us the "card" abstraction for
ALSA devices, something people have been asking for quite some time (in
order to do persistent device names and other stuff). And it fixes a
number of suspend/resume issues by allowing the class core that controls
the device to be notified of a suspend/resume before the device is, so
that it can do common work without all of the individual drivers having
to be changed.

> >Well, as this HALD issue is the only thing that I have had reported, and
> >these patches have been in the -mm tree for quite some time, yes, I
> >think it is the only thing that has broken.
>
> Plenty of people spotted it, several bugs got filed, and the fact that
> you're only hearing about it from me at least two months after
> introduction is mildly worrisome. (I am already worried that *I*
> didn't hear about it till recently as I attempted to push out Pulse in
> fc rawhide and found it magically no longer worked. At all.)

Where were they filed? Should I be looking somewhere else for kernel
bug reports?

> If the hald developers were caught unprepared (so unprepared, in fact,
> they've all ben inaccessable due to holiday + conference + vacation
> for over a month and we can't push an update out), what is the average
> user to do?

Some of the HAL developers knew about this a long time ago, as they were
one of the main people who did the kernel changes. So I don't think
they were all unprepared :)

> Not to mention the fact that it's just a little embarrassing that
> sysfs was invented for 2.6 and the original version has already been
> declared deprecated, still in 2.6. There's change, and then there's
> churn.

How would you go about making changes like the above? I had two
choices:
- fix all broken userspace programs for all old, unsupported
distros before making the change. I actually started to do
this, but quickly got lost in a maze of old distro images...
- add a kernel option that kept the old structure of sysfs, so
that people can slowly migrate over as their userspace
programs catch up and get updated in the future.

I implemented the latter, and so far, this is the only hic-up that has
been reported in the mainline kernel with it so far (there were bug
reports about stuff in the -mm tree, but that is what it is there for,
and they were quickly fixed.)

> >In short, it's a bug, I messed up somehow. And I'm trying to fix it
> >now, I don't think there's more that I can do, do you?
>
> No. I'm not pushing back against you, I'm pushing back against the
> budding 'stable APIs are for boring losers' culture.

Note, I don't think that at all. We are changing these userspace things
because we have learned and realize that after a few years of using this
model, things need to be tweaked to properly reflect the needs of the
real world. It's almost impossible to get such a change right the first
time, and Linux relies on evolution of stuff in order to adapt properly.

Again, I'm providing a way to allow backward compatibility so that we do
not break anything that is currently out there. So far no one has
objected to this :)

> There's a reason I refuse to update my personal machines more often
> than once every year or two-- they spend the next few weeks completely
> dysfunctional each time. To be fair, the kernel is usually far better
> than userland [excepting ALSA, GRRRR], but let's not go pissing away
> that relative lead :-)

And there's a reason that I'm always using the latest experimental
userspace and use a distro that lets me do that :)

But I also have old machines that I don't update to ensure that things
still work properly. Unfortunatly in this case, they were too old such
that the problem was never seen (pre-HAL-relying-gnome-distro).

thanks,

greg k-h

2007-01-26 19:07:43