[PATCH] ALSA: emu10k1: fix multi-channel playback device class

[PATCH] ALSA: emu10k1: fix multi-channel playback device class

[Oswald Buddenhagen]

It's multi, not mono/stereo.

AFAICT, this doesn't do anything in the kernel.
Also, I think the subclass is meaningless for devices with just one
stream, but whatever.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emupcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index 87c3b19c6b2d..c04ef6ea188e 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -1377,7 +1377,8 @@ int snd_emu10k1_pcm_multi(struct snd_emu10k1 *emu, int device)
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &snd_emu10k1_efx_playback_ops);
 
 	pcm->info_flags = 0;
-	pcm->dev_subclass = SNDRV_PCM_SUBCLASS_GENERIC_MIX;
+	pcm->dev_class = SNDRV_PCM_CLASS_MULTI;
+	pcm->dev_subclass = SNDRV_PCM_SUBCLASS_MULTI_MIX;
 	strcpy(pcm->name, "Multichannel Playback");
 	emu->pcm_multi = pcm;
 
-- 
2.40.0.152.g15d061e6df

Re: [PATCH] ALSA: emu10k1: fix multi-channel playback device class

[Takashi Iwai]

On Sat, 22 Apr 2023 18:10:21 +0200,
Oswald Buddenhagen wrote:
> 
> It's multi, not mono/stereo.
> 
> AFAICT, this doesn't do anything in the kernel.

... but those values are read by user-space.

> Also, I think the subclass is meaningless for devices with just one
> stream, but whatever.

Again, the value is read by user-space.

So changing both have clear influence on the user-space program, and
unless you have to change this for fixing a real bug (and there is no
other way), this is too risky.  IOW, too late to change, we have to
accept those values.


thanks,

Takashi

Re: [PATCH] ALSA: emu10k1: fix multi-channel playback device class

[Oswald Buddenhagen]

On Sun, Apr 23, 2023 at 09:30:11AM +0200, Takashi Iwai wrote:
>On Sat, 22 Apr 2023 18:10:21 +0200,
>Oswald Buddenhagen wrote:
>> 
>> It's multi, not mono/stereo.
>> 
>> AFAICT, this doesn't do anything in the kernel.
>
>... but those values are read by user-space.
>
>> Also, I think the subclass is meaningless for devices with just one
>> stream, but whatever.
>
>Again, the value is read by user-space.
>
i assumed that much.
but these are another thing that appears to have exactly zero useful 
documentation.

>So changing both have clear influence on the user-space program, and
>unless you have to change this for fixing a real bug (and there is no
>other way), this is too risky.  IOW, too late to change, we have to
>accept those values.
>
there aren't any precedents for use of SNDRV_PCM_CLASS_MULTI and 
SNDRV_PCM_SUBCLASS_MULTI_MIX in the kernel tree.
there don't appear to be relevant hits outside the kernel, either.
it's conceivable that some code would check for the *_GENERIC enums, but 
i didn't find such code.
so i'd postulate that these enums are effectively dead, and both the 
risk and the gain of this change are about zero.
i suggest to initiate a formal deprecation procedure for the MULTI enum 
values, however that's supposed to look like.

regards

Re: [PATCH] ALSA: emu10k1: fix multi-channel playback device class

[Oswald Buddenhagen]

On Mon, Apr 24, 2023 at 12:32:58PM +0200, Oswald Buddenhagen wrote:
>so i'd postulate that these enums are effectively dead, and both the 
>risk and the gain of this change are about zero.

speaking of dead code, lots of drivers explicitly assign info_flags = 0, 
dev_class = SNDRV_PCM_CLASS_GENERIC, and dev_subclass = 
SNDRV_PCM_SUBCLASS_GENERIC_MIX, which are all technically pointless, as 
the pcm struct is kzalloc'd anyway. and of course it's entirely 
inconsistent (with just about every combination present), because it 
obviously works without this. how would you feel about a patch that just 
removes all these?

regards

Re: [PATCH] ALSA: emu10k1: fix multi-channel playback device class

[Takashi Iwai]

On Tue, 25 Apr 2023 10:00:45 +0200,
Oswald Buddenhagen wrote:
> 
> On Mon, Apr 24, 2023 at 12:32:58PM +0200, Oswald Buddenhagen wrote:
> > so i'd postulate that these enums are effectively dead, and both the
> > risk and the gain of this change are about zero.
> 
> speaking of dead code, lots of drivers explicitly assign info_flags =
> 0, dev_class = SNDRV_PCM_CLASS_GENERIC, and dev_subclass =
> SNDRV_PCM_SUBCLASS_GENERIC_MIX, which are all technically pointless,
> as the pcm struct is kzalloc'd anyway. and of course it's entirely
> inconsistent (with just about every combination present), because it
> obviously works without this. how would you feel about a patch that
> just removes all these?

The dev_class and info_flags initializations to zero aren't entirely
pointless, IMO.  It explicitly shows that the default value is used.
OTOH, dev_sub_class is always 0 for the existing drivers, hence
dropping the explicit initialization would be an acceptable cleanup.


thanks,

Takashi

Re: [PATCH] ALSA: emu10k1: fix multi-channel playback device class

[Oswald Buddenhagen]

On Tue, Apr 25, 2023 at 10:15:36AM +0200, Takashi Iwai wrote:
>On Tue, 25 Apr 2023 10:00:45 +0200,
>Oswald Buddenhagen wrote:
>> speaking of dead code, lots of drivers explicitly assign info_flags =
>> 0, dev_class = SNDRV_PCM_CLASS_GENERIC, and dev_subclass =
>> SNDRV_PCM_SUBCLASS_GENERIC_MIX, which are all technically pointless,
>> as the pcm struct is kzalloc'd anyway.
>
>The dev_class and info_flags initializations to zero aren't entirely
>pointless, IMO.  It explicitly shows that the default value is used.
>
that can be said about many other fields as well. but defaults are there 
for a reason, to reduce the noise. not even the example in the docu 
includes it. and if we were consistent about the "be explicit" approach, 
we should add it to a lot of drivers which lack it - which feels just 
wrong.

regards