| Message ID | 20230525135955.2108140-3-sbinding@opensource.cirrus.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes and cleanup for CS35L41 HDA | expand |
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
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; }
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(-)