[PATCH] ALSA: i2c: cs8427: Use common code in snd_cs8427_qsubcode_get()

[PATCH] ALSA: i2c: cs8427: Use common code in snd_cs8427_qsubcode_get()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 Sep 2024 13:38:29 +0200

Add two labels so that a bit of exception handling can be better reused
at the end of this function implementation.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 sound/i2c/cs8427.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/sound/i2c/cs8427.c b/sound/i2c/cs8427.c
index 29a1a7a0d050..ee650006cac4 100644
--- a/sound/i2c/cs8427.c
+++ b/sound/i2c/cs8427.c
@@ -397,18 +397,25 @@ static int snd_cs8427_qsubcode_get(struct snd_kcontrol *kcontrol,
 	if (err != 1) {
 		dev_err(device->bus->card->dev,
 			"unable to send register 0x%x byte to CS8427\n", reg);
-		snd_i2c_unlock(device->bus);
-		return err < 0 ? err : -EIO;
+		goto recheck_err;
 	}
 	err = snd_i2c_readbytes(device, ucontrol->value.bytes.data, 10);
 	if (err != 10) {
 		dev_err(device->bus->card->dev,
 			"unable to read Q-subcode bytes from CS8427\n");
-		snd_i2c_unlock(device->bus);
-		return err < 0 ? err : -EIO;
+		goto recheck_err;
 	}
+
+	err = 0;
+unlock:
 	snd_i2c_unlock(device->bus);
-	return 0;
+	return err;
+
+recheck_err:
+	if (err >= 0)
+		err = -EIO;
+
+	goto unlock;
 }

 static int snd_cs8427_spdif_info(struct snd_kcontrol *kcontrol,
--
2.46.0

Re: [PATCH] ALSA: i2c: cs8427: Use common code in snd_cs8427_qsubcode_get()

[Takashi Iwai]

On Sun, 15 Sep 2024 13:48:51 +0200,
Markus Elfring wrote:
> 
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Sep 2024 13:38:29 +0200
> 
> Add two labels so that a bit of exception handling can be better reused
> at the end of this function implementation.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

The double goto jumps aren't nice, either.
I guess we can do clean up better with cleanup.h stuff.


thanks,

Takashi

> ---
>  sound/i2c/cs8427.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/i2c/cs8427.c b/sound/i2c/cs8427.c
> index 29a1a7a0d050..ee650006cac4 100644
> --- a/sound/i2c/cs8427.c
> +++ b/sound/i2c/cs8427.c
> @@ -397,18 +397,25 @@ static int snd_cs8427_qsubcode_get(struct snd_kcontrol *kcontrol,
>  	if (err != 1) {
>  		dev_err(device->bus->card->dev,
>  			"unable to send register 0x%x byte to CS8427\n", reg);
> -		snd_i2c_unlock(device->bus);
> -		return err < 0 ? err : -EIO;
> +		goto recheck_err;
>  	}
>  	err = snd_i2c_readbytes(device, ucontrol->value.bytes.data, 10);
>  	if (err != 10) {
>  		dev_err(device->bus->card->dev,
>  			"unable to read Q-subcode bytes from CS8427\n");
> -		snd_i2c_unlock(device->bus);
> -		return err < 0 ? err : -EIO;
> +		goto recheck_err;
>  	}
> +
> +	err = 0;
> +unlock:
>  	snd_i2c_unlock(device->bus);
> -	return 0;
> +	return err;
> +
> +recheck_err:
> +	if (err >= 0)
> +		err = -EIO;
> +
> +	goto unlock;
>  }
> 
>  static int snd_cs8427_spdif_info(struct snd_kcontrol *kcontrol,
> --
> 2.46.0
> 

Re: ALSA: i2c: cs8427: Use common code in snd_cs8427_qsubcode_get()

[Markus Elfring]

>> Add two labels so that a bit of exception handling can be better reused
>> at the end of this function implementation.

The implementation of the function “snd_cs8427_qsubcode_get” was identified
as another update candidate.
https://elixir.bootlin.com/linux/v6.11/source/sound/i2c/cs8427.c#L385
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/sound/i2c/cs8427.c?h=v6.11


> The double goto jumps aren't nice, either.

Would you like to care to avoid a bit of duplicate code from repeated
return statements?
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources


> I guess we can do clean up better with cleanup.h stuff.

Scope-based resource management can eventually be applied also for
snd_i2c_unlock() calls.
https://elixir.bootlin.com/linux/v6.11/source/include/sound/i2c.h#L76

How do you think about to clarify any software extensions accordingly?

Regards,
Markus