Hi David
On Tue, Jul 19, 2022 at 9:35 PM Shengjiu Wang shengjiu.wang@gmail.com wrote:
On Tue, Jul 19, 2022 at 8:39 PM David Laight David.Laight@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@gmail.com *Sent:* 19 July 2022 12:07 *To:* David Laight David.Laight@ACULAB.COM *Cc:* Mark Brown broonie@kernel.org; Shengjiu Wang < shengjiu.wang@nxp.com>; Xiubo.Lee@gmail.com; festevam@gmail.com; nicoleotsuka@gmail.com; lgirdwood@gmail.com; perex@perex.cz; tiwai@suse.com; alsa-devel@alsa-project.org; linuxppc-dev@lists.ozlabs.org; linux-kernel@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@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*