[PATCH RESEND v1 0/3] Fixes and cleanup for CS35L41 HDA
Several minor issues were found during additional testing and static analysis. These patches fix these minor issues.
Stefan Binding (3): ALSA: hda: cs35l41: Clean up Firmware Load Controls ALSA: hda: cs35l41: Fix endian conversions ALSA: hda/realtek: Delete cs35l41 component master during free
sound/pci/hda/cs35l41_hda.c | 40 ++++++++++++++++------------------- sound/pci/hda/patch_realtek.c | 2 ++ 2 files changed, 20 insertions(+), 22 deletions(-)
Ensure Firmware Load control and Firmware Type control returns 1 when the value changes.
Remove fw_mutex from firmware load control put, since it is unnecessary, and prevents any possibility of mutex inversion.
Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- sound/pci/hda/cs35l41_hda.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index b5210abb5141f..d100189e15b83 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -835,34 +835,26 @@ static int cs35l41_fw_load_ctl_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct cs35l41_hda *cs35l41 = snd_kcontrol_chip(kcontrol); - unsigned int ret = 0; - - mutex_lock(&cs35l41->fw_mutex);
if (cs35l41->request_fw_load == ucontrol->value.integer.value[0]) - goto err; + return 0;
if (cs35l41->fw_request_ongoing) { dev_dbg(cs35l41->dev, "Existing request not complete\n"); - ret = -EBUSY; - goto err; + return -EBUSY; }
/* Check if playback is ongoing when initial request is made */ if (cs35l41->playback_started) { dev_err(cs35l41->dev, "Cannot Load/Unload firmware during Playback\n"); - ret = -EBUSY; - goto err; + return -EBUSY; }
cs35l41->fw_request_ongoing = true; cs35l41->request_fw_load = ucontrol->value.integer.value[0]; schedule_work(&cs35l41->fw_load_work);
-err: - mutex_unlock(&cs35l41->fw_mutex); - - return ret; + return 1; }
static int cs35l41_fw_type_ctl_get(struct snd_kcontrol *kcontrol, @@ -881,8 +873,12 @@ static int cs35l41_fw_type_ctl_put(struct snd_kcontrol *kcontrol, struct cs35l41_hda *cs35l41 = snd_kcontrol_chip(kcontrol);
if (ucontrol->value.enumerated.item[0] < HDA_CS_DSP_NUM_FW) { - cs35l41->firmware_type = ucontrol->value.enumerated.item[0]; - return 0; + if (cs35l41->firmware_type != ucontrol->value.enumerated.item[0]) { + cs35l41->firmware_type = ucontrol->value.enumerated.item[0]; + return 1; + } else { + return 0; + } }
return -EINVAL;
Found during static analysis, ensure variables are correct types before endian conversion.
Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- sound/pci/hda/cs35l41_hda.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index d100189e15b83..b02462ae21f04 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -308,8 +308,8 @@ static int cs35l41_request_firmware_files(struct cs35l41_hda *cs35l41, }
#if IS_ENABLED(CONFIG_EFI) -static int cs35l41_apply_calibration(struct cs35l41_hda *cs35l41, unsigned int ambient, - unsigned int r0, unsigned int status, unsigned int checksum) +static int cs35l41_apply_calibration(struct cs35l41_hda *cs35l41, __be32 ambient, __be32 r0, + __be32 status, __be32 checksum) { int ret;
@@ -379,10 +379,10 @@ static int cs35l41_save_calibration(struct cs35l41_hda *cs35l41)
/* Calibration can only be applied whilst the DSP is not running */ ret = cs35l41_apply_calibration(cs35l41, - cpu_to_be32(cl->calAmbient), - cpu_to_be32(cl->calR), - cpu_to_be32(cl->calStatus), - cpu_to_be32(cl->calR + 1)); + (__be32)cpu_to_be32(cl->calAmbient), + (__be32)cpu_to_be32(cl->calR), + (__be32)cpu_to_be32(cl->calStatus), + (__be32)cpu_to_be32(cl->calR + 1)); } } vfree(data); @@ -745,7 +745,7 @@ static int cs35l41_runtime_resume(struct device *dev)
static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41) { - int halo_sts; + __be32 halo_sts; int ret;
ret = cs35l41_init_dsp(cs35l41); @@ -773,7 +773,7 @@ static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41) &halo_sts, sizeof(halo_sts));
if (ret) { - dev_err(cs35l41->dev, "Timeout waiting for HALO Core to start. State: %d\n", + dev_err(cs35l41->dev, "Timeout waiting for HALO Core to start. State: %u\n", halo_sts); goto clean_dsp; }
On Thu, 25 May 2023 15:59:54 +0200, Stefan Binding wrote:
@@ -379,10 +379,10 @@ static int cs35l41_save_calibration(struct cs35l41_hda *cs35l41)
/* Calibration can only be applied whilst the DSP is not running */ ret = cs35l41_apply_calibration(cs35l41,
cpu_to_be32(cl->calAmbient),cpu_to_be32(cl->calR),cpu_to_be32(cl->calStatus),cpu_to_be32(cl->calR + 1));
(__be32)cpu_to_be32(cl->calAmbient),(__be32)cpu_to_be32(cl->calR),(__be32)cpu_to_be32(cl->calStatus),(__be32)cpu_to_be32(cl->calR + 1));
Do we really need those cast? Even if yes, it must be with __force prefix for the endian cast in general.
thanks,
Takashi
Hi Takashi,
On 05/06/2023 08:21, Takashi Iwai wrote:
On Thu, 25 May 2023 15:59:54 +0200, Stefan Binding wrote:
@@ -379,10 +379,10 @@ static int cs35l41_save_calibration(struct cs35l41_hda *cs35l41)
/* Calibration can only be applied whilst the DSP is not running */ ret = cs35l41_apply_calibration(cs35l41,
cpu_to_be32(cl->calAmbient),cpu_to_be32(cl->calR),cpu_to_be32(cl->calStatus),cpu_to_be32(cl->calR + 1));
(__be32)cpu_to_be32(cl->calAmbient),(__be32)cpu_to_be32(cl->calR),(__be32)cpu_to_be32(cl->calStatus),(__be32)cpu_to_be32(cl->calR + 1));Do we really need those cast? Even if yes, it must be with __force prefix for the endian cast in general.
These casts were added because we found some warnings when we ran the static analyzer sparse locally. I think these warnings are very minor, and we can drop this patch if you prefer?
Thanks,
Stefan
thanks,
Takashi
On Mon, 05 Jun 2023 14:50:54 +0200, Stefan Binding wrote:
Hi Takashi,
On 05/06/2023 08:21, Takashi Iwai wrote:
On Thu, 25 May 2023 15:59:54 +0200, Stefan Binding wrote:
@@ -379,10 +379,10 @@ static int cs35l41_save_calibration(struct cs35l41_hda *cs35l41) /* Calibration can only be applied whilst the DSP is not running */ ret = cs35l41_apply_calibration(cs35l41,
cpu_to_be32(cl->calAmbient),cpu_to_be32(cl->calR),cpu_to_be32(cl->calStatus),cpu_to_be32(cl->calR + 1));
(__be32)cpu_to_be32(cl->calAmbient),(__be32)cpu_to_be32(cl->calR),(__be32)cpu_to_be32(cl->calStatus),(__be32)cpu_to_be32(cl->calR + 1));Do we really need those cast? Even if yes, it must be with __force prefix for the endian cast in general.
These casts were added because we found some warnings when we ran the static analyzer sparse locally. I think these warnings are very minor, and we can drop this patch if you prefer?
The warnings must be bogus, or maybe pointing to other things? The cpu_to_be32() macro itself must return a __be32 value, hence it makes no sense to add an extra cast .
If the static analysis still shows such a warning, it should be fixed differently -- either fix the analyzer or fix the cpu_to_be32() macro itself.
The changes of the argument types to __be32 are fine. I'm arguing only about those unnecessary cast.
thanks,
Takashi
Hi Takashi,
On 05/06/2023 14:17, Takashi Iwai wrote:
On Mon, 05 Jun 2023 14:50:54 +0200, Stefan Binding wrote:
Hi Takashi,
On 05/06/2023 08:21, Takashi Iwai wrote:
On Thu, 25 May 2023 15:59:54 +0200, Stefan Binding wrote:
@@ -379,10 +379,10 @@ static int cs35l41_save_calibration(struct cs35l41_hda *cs35l41) /* Calibration can only be applied whilst the DSP is not running */ ret = cs35l41_apply_calibration(cs35l41,
cpu_to_be32(cl->calAmbient),cpu_to_be32(cl->calR),cpu_to_be32(cl->calStatus),cpu_to_be32(cl->calR + 1));
(__be32)cpu_to_be32(cl->calAmbient),(__be32)cpu_to_be32(cl->calR),(__be32)cpu_to_be32(cl->calStatus),(__be32)cpu_to_be32(cl->calR + 1));Do we really need those cast? Even if yes, it must be with __force prefix for the endian cast in general.
These casts were added because we found some warnings when we ran the static analyzer sparse locally. I think these warnings are very minor, and we can drop this patch if you prefer?
The warnings must be bogus, or maybe pointing to other things? The cpu_to_be32() macro itself must return a __be32 value, hence it makes no sense to add an extra cast .
If the static analysis still shows such a warning, it should be fixed differently -- either fix the analyzer or fix the cpu_to_be32() macro itself.
The changes of the argument types to __be32 are fine. I'm arguing only about those unnecessary cast.
You are correct, I double checked and the cast is not needed. I'll push up a v2.
Thanks,
Stefan
thanks,
Takashi
This ensures that the driver is properly cleaned up when freed.
Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- sound/pci/hda/patch_realtek.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 7b5f194513c7b..e3774903918fe 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -6757,6 +6757,8 @@ static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char else spec->gen.pcm_playback_hook = comp_generic_playback_hook; break; + case HDA_FIXUP_ACT_FREE: + component_master_del(dev, &comp_master_ops); } }
participants (2)
-
Stefan Binding -
Takashi Iwai