[PATCH] ALSA: emu10k1: fix error codes

[PATCH] ALSA: emu10k1: fix error codes

[Oswald Buddenhagen]

One might argue that this potentially breaks user space, but a) this is
just one driver among many, so it seems unlikely that someone would
expect (only) the broken codes and b) it seems unlikely that someone
would check these syscalls for particular errors at all, rather than
just logging them (this might be debatable for the voice allocator
calls).

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/pci/emu10k1/emu10k1_callback.c | 2 +-
 sound/pci/emu10k1/emufx.c            | 4 ++--
 sound/pci/emu10k1/io.c               | 2 +-
 sound/pci/emu10k1/voice.c            | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/pci/emu10k1/emu10k1_callback.c b/sound/pci/emu10k1/emu10k1_callback.c
index dba1e9fc2eec..5943747eb7db 100644
--- a/sound/pci/emu10k1/emu10k1_callback.c
+++ b/sound/pci/emu10k1/emu10k1_callback.c
@@ -106,7 +106,7 @@ snd_emu10k1_synth_get_voice(struct snd_emu10k1 *hw)
 	}
 
 	/* not found */
-	return -ENOMEM;
+	return -EBUSY;
 }
 
 
diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
index 6cf7c8b1de47..267d1bab3ee4 100644
--- a/sound/pci/emu10k1/emufx.c
+++ b/sound/pci/emu10k1/emufx.c
@@ -360,15 +360,15 @@ static int snd_emu10k1_gpr_ctl_put(struct snd_kcontrol *kcontrol, struct snd_ctl
 			break;
 		case EMU10K1_GPR_TRANSLATION_BASS:
 			if ((ctl->count % 5) != 0 || (ctl->count / 5) != ctl->vcount) {
-				change = -EIO;
+				change = -EINVAL;
 				goto __error;
 			}
 			for (j = 0; j < 5; j++)
 				snd_emu10k1_ptr_write(emu, emu->gpr_base + ctl->gpr[j * ctl->vcount + i], 0, bass_table[val][j]);
 			break;
 		case EMU10K1_GPR_TRANSLATION_TREBLE:
 			if ((ctl->count % 5) != 0 || (ctl->count / 5) != ctl->vcount) {
-				change = -EIO;
+				change = -EINVAL;
 				goto __error;
 			}
 			for (j = 0; j < 5; j++)
diff --git a/sound/pci/emu10k1/io.c b/sound/pci/emu10k1/io.c
index e15092ce9848..cfcdb33992bf 100644
--- a/sound/pci/emu10k1/io.c
+++ b/sound/pci/emu10k1/io.c
@@ -226,7 +226,7 @@ int snd_emu10k1_i2c_write(struct snd_emu10k1 *emu,
 		dev_err(emu->card->dev, "status=0x%x, reg=%d, value=%d\n",
 			status, reg, value);
 		/* dump_stack(); */
-		err = -EINVAL;
+		err = -EIO;
 	}
     
 	spin_unlock(&emu->i2c_lock);
diff --git a/sound/pci/emu10k1/voice.c b/sound/pci/emu10k1/voice.c
index cbeb8443492c..a5cb932d525a 100644
--- a/sound/pci/emu10k1/voice.c
+++ b/sound/pci/emu10k1/voice.c
@@ -70,7 +70,7 @@ static int voice_alloc(struct snd_emu10k1 *emu, int type, int number,
 	}
 	
 	if (first_voice == last_voice)
-		return -ENOMEM;
+		return -EBUSY;
 	
 	for (i = 0; i < number; i++) {
 		voice = &emu->voices[(first_voice + i) % NUM_G];
-- 
2.40.0.152.g15d061e6df

Re: [PATCH] ALSA: emu10k1: fix error codes

[Takashi Iwai]

On Fri, 21 Apr 2023 19:26:23 +0200,
Oswald Buddenhagen wrote:
> 
> One might argue that this potentially breaks user space, but a) this is
> just one driver among many, so it seems unlikely that someone would
> expect (only) the broken codes and b) it seems unlikely that someone
> would check these syscalls for particular errors at all, rather than
> just logging them (this might be debatable for the voice allocator
> calls).
> 
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

I find this is too risky for really little win.  The error code is
returned to user space in quite many cases; e.g. the voice allocator
is called from PCM hw_params, too, and that's most of user-space
programs do actually check.  It could be a surprise if it's changed
without much reason, may trigger unexpected behavior changes.

Of course, if the error code must be corrected, we can fix it.
But I don't see it in this patch description.


thanks,

Takashi

Re: [PATCH] ALSA: emu10k1: fix error codes

[Oswald Buddenhagen]

On Sat, Apr 22, 2023 at 09:46:24AM +0200, Takashi Iwai wrote:
>On Fri, 21 Apr 2023 19:26:23 +0200, Oswald Buddenhagen wrote:
>> One might argue that this potentially breaks user space, but a) this 
>> is
>> just one driver among many, so it seems unlikely that someone would
>> expect (only) the broken codes and b) it seems unlikely that someone
>> would check these syscalls for particular errors at all, rather than
>> just logging them (this might be debatable for the voice allocator
>> calls).
>
>I find this is too risky for really little win.
>
yes, the gain is relatively low. it merely means applications displaying 
somewhat less confusing error messages.

>The error code is
>returned to user space in quite many cases; e.g. the voice allocator
>is called from PCM hw_params, too, and that's most of user-space
>programs do actually check.  It could be a surprise if it's changed
>without much reason, may trigger unexpected behavior changes.
>
of course. hypothetically.
but these aren't error codes around which specific error recovery would 
exist.
codes that actually _have_ error recovery built around them tend to be 
already correct, because people actually tried using them and noticed 
mistakes in time.

>Of course, if the error code must be corrected, we can fix it.
>But I don't see it in this patch description.
>
i can provide a rationale for each of the changes, even though i think 
that the patch context speaks for itself - the theme is always "return 
an error code whose description better reflects the situation being 
reported".
but none of that would change the fact that it would break code that was 
specifically designed to rely on this driver's bogus behavior. i just 
don't think such code exists, because that wouldn't make any sense.
so i don't know what your criteria for "must be corrected" are.

regards

Re: [PATCH] ALSA: emu10k1: fix error codes

[Takashi Iwai]

On Sat, 22 Apr 2023 14:04:36 +0200,
Oswald Buddenhagen wrote:
> 
> On Sat, Apr 22, 2023 at 09:46:24AM +0200, Takashi Iwai wrote:
> > On Fri, 21 Apr 2023 19:26:23 +0200, Oswald Buddenhagen wrote:
> >> One might argue that this potentially breaks user space, but a)
> >> this is
> >> just one driver among many, so it seems unlikely that someone would
> >> expect (only) the broken codes and b) it seems unlikely that someone
> >> would check these syscalls for particular errors at all, rather than
> >> just logging them (this might be debatable for the voice allocator
> >> calls).
> > 
> > I find this is too risky for really little win.
> > 
> yes, the gain is relatively low. it merely means applications
> displaying somewhat less confusing error messages.
> 
> > The error code is
> > returned to user space in quite many cases; e.g. the voice allocator
> > is called from PCM hw_params, too, and that's most of user-space
> > programs do actually check.  It could be a surprise if it's changed
> > without much reason, may trigger unexpected behavior changes.
> > 
> of course. hypothetically.
> but these aren't error codes around which specific error recovery
> would exist.
> codes that actually _have_ error recovery built around them tend to be
> already correct, because people actually tried using them and noticed
> mistakes in time.

Yes, but remember that they adapt how the existing code behaves; so
even if EBUSY might appears like a better fit, a different error code
may bring unexpected outcome.  User space programs can do every crazy
thing, so never underestimate a subtle change if it's visible --
that's what I've learned from the long development history.
Especially true, if the stuff is a very old one like this.

That said, I wouldn't mind changing if it's about a new driver code.
But this isn't such a case.

> > Of course, if the error code must be corrected, we can fix it.
> > But I don't see it in this patch description.
> > 
> i can provide a rationale for each of the changes, even though i think
> that the patch context speaks for itself - the theme is always "return
> an error code whose description better reflects the situation being
> reported".
> but none of that would change the fact that it would break code that
> was specifically designed to rely on this driver's bogus behavior. i
> just don't think such code exists, because that wouldn't make any
> sense.
> so i don't know what your criteria for "must be corrected" are.

So my take is rather to skip this.


thanks,

Takashi