[PATCH -next 2/5] ASoC: fsl_asrc: force cast the asrc_format type
Shengjiu Wang
shengjiu.wang at gmail.com
Wed Jul 20 07:42:28 CEST 2022
Hi David
On Tue, Jul 19, 2022 at 9:35 PM Shengjiu Wang <shengjiu.wang at gmail.com>
wrote:
>
>
> On Tue, Jul 19, 2022 at 8:39 PM David Laight <David.Laight at aculab.com>
> wrote:
>
>> grrr... top-posting because outluck is really stupid :-(
>>
>>
>>
>> The definition seems to be:
>>
>> typedef int __bitwise
>> <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/__bitwise>
>> snd_pcm_format_t
>> <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/snd_pcm_format_t>;
>>
>> #define SNDRV_PCM_FORMAT_S8
>> <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/SNDRV_PCM_FORMAT_S8>
>> ((__force <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/__force>
>> snd_pcm_format_t
>> <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/snd_pcm_format_t>) 0)
>>
>> #define SNDRV_PCM_FORMAT_U8
>> <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/SNDRV_PCM_FORMAT_U8>
>> ((__force <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/__force>
>> snd_pcm_format_t
>> <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/snd_pcm_format_t>) 1)
>>
>> #define SNDRV_PCM_FORMAT_S16_LE
>> <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/SNDRV_PCM_FORMAT_S16_LE>
>> ((__force <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/__force>
>> snd_pcm_format_t
>> <https://elixir.bootlin.com/linux/v5.19-rc7/C/ident/snd_pcm_format_t>) 2)
>>
>> ...
>>
>> (goes away and looks up __bitwIse)
>>
>>
>>
>> I think I’d add:
>>
>> #define snd_pcm_format(val) ((__force snd_pcm_format_t)(val))
>>
>
> Where is this definition? Which header file?
> Thanks.
>
>
Here is the change based on your proposal.
Not sure if there is misunderstanding.
Not sure if the definition can be put in pcm.h.
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 26523cfe428d..93e53b195ef9 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -1477,6 +1477,8 @@ static inline u64 pcm_format_to_bits(snd_pcm_format_t
pcm_format)
return 1ULL << (__force int) pcm_format;
}
+#define snd_pcm_format(val) ((__force snd_pcm_format_t)(val))
+
/**
* pcm_for_each_format - helper to iterate for each format type
* @f: the iterator variable in snd_pcm_format_t type
diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 544395efd605..dcfdfb6b3472 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -1066,6 +1066,7 @@ static int fsl_asrc_probe(struct platform_device
*pdev)
struct resource *res;
void __iomem *regs;
int irq, ret, i;
+ u32 asrc_fmt = 0;
u32 map_idx;
char tmp[16];
u32 width;
@@ -1174,7 +1175,8 @@ static int fsl_asrc_probe(struct platform_device
*pdev)
return ret;
}
- ret = of_property_read_u32(np, "fsl,asrc-format", (u32
*)&asrc->asrc_format);
+ ret = of_property_read_u32(np, "fsl,asrc-format", &asrc_fmt);
+ asrc->asrc_format = snd_pcm_format(asrc_fmt);
if (ret) {
ret = of_property_read_u32(np, "fsl,asrc-width", &width);
if (ret) {
@@ -1197,7 +1199,7 @@ static int fsl_asrc_probe(struct platform_device
*pdev)
}
}
- if (!(FSL_ASRC_FORMATS & (1ULL << (__force u32)asrc->asrc_format)))
{
+ if (!(FSL_ASRC_FORMATS & pcm_format_to_bits(asrc->asrc_format))) {
dev_warn(&pdev->dev, "unsupported width, use default
S24_LE\n");
asrc->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
best regards
wang shengjiu
>
> and use that to remove most of the casts.
>>
> But the ones where you have (u32 *)&xxx are only valid because u32 and int
>> are the same size.
>>
>> That does sort of happen to be true, but someone might look at all the
>> values and
>>
>> decide that u8 is big enough.
>>
>> After which the code will still compile, but the data areas get corrupted.
>>
>> So you really need to use a u32 ‘temp’ variable.
>>
>>
>>
>> It would all be slightly less problematic if the ‘force’ casts could be
>> sparse only
>>
>> (ie not seen by the compiler) – so the compiler would do the type
>> checking.
>>
>>
>>
>> David
>>
>>
>>
>> *From:* Shengjiu Wang <shengjiu.wang at gmail.com>
>> *Sent:* 19 July 2022 12:07
>> *To:* David Laight <David.Laight at ACULAB.COM>
>> *Cc:* Mark Brown <broonie at kernel.org>; Shengjiu Wang <
>> shengjiu.wang at nxp.com>; Xiubo.Lee at gmail.com; festevam at gmail.com;
>> nicoleotsuka at gmail.com; lgirdwood at gmail.com; perex at perex.cz;
>> tiwai at suse.com; alsa-devel at alsa-project.org;
>> linuxppc-dev at lists.ozlabs.org; linux-kernel at vger.kernel.org
>> *Subject:* Re: [PATCH -next 2/5] ASoC: fsl_asrc: force cast the
>> asrc_format type
>>
>>
>>
>>
>>
>>
>>
>> On Tue, Jul 19, 2022 at 6:34 PM David Laight <David.Laight at aculab.com>
>> wrote:
>>
>> From: Mark Brown
>> > Sent: 19 July 2022 11:17
>> >
>> > On Tue, Jul 19, 2022 at 10:01:54AM +0000, David Laight wrote:
>> > > From: Shengjiu Wang
>> >
>> > > > - ret = of_property_read_u32(np, "fsl,asrc-format",
>> &asrc->asrc_format);
>> > > > + ret = of_property_read_u32(np, "fsl,asrc-format", (u32
>> *)&asrc->asrc_format);
>> >
>> > > Ugg, you really shouldn't need to do that.
>> > > It means that something is badly wrong somewhere.
>> > > Casting pointers to integer types is just asking for a bug.
>> >
>> > That's casting one pointer type to another pointer type.
>>
>> It is casting the address of some type to a 'u32 *'.
>> This will then be dereferenced by the called function.
>> So the original type better be 32 bits.
>>
>> I'm also guessing that sparse was complaining about endianness?
>> It isn't at all clear that these casts actually fix it.
>>
>> The sparse is complaining about the snd_pcm_format_t cast to u32/int type.
>>
>>
>>
>> The code in include/sound/pcm.h also does such __force cast.
>>
>> #define _SNDRV_PCM_FMTBIT(fmt) (1ULL << (__force
>> int)SNDRV_PCM_FORMAT_##fmt)
>>
>>
>>
>> The change I have made does not cause an issue.
>>
>>
>>
>> Best regards
>>
>> Wang shengjiu
>>
>>
>>
>> (Mark: You'll be glad to hear that the office aircon is
>> broken again - two weeks lead time on the spare part.)
>>
>> David
>>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1
>> 1PT, UK
>> Registration No: 1397386 (Wales)
>>
>>
>>
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1
>> 1PT, UK
>> Registration No: 1397386 (Wales)
>>
>> P *Please consider the environment and don't print this e-mail unless
>> you really need to*
>>
>
More information about the Alsa-devel
mailing list