Hi David On Tue, Jul 19, 2022 at 9:35 PM Shengjiu Wang <shengjiu.wang@xxxxxxxxx> wrote: > > > On Tue, Jul 19, 2022 at 8:39 PM David Laight <David.Laight@xxxxxxxxxx> > 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@xxxxxxxxx> >> *Sent:* 19 July 2022 12:07 >> *To:* David Laight <David.Laight@xxxxxxxxxx> >> *Cc:* Mark Brown <broonie@xxxxxxxxxx>; Shengjiu Wang < >> shengjiu.wang@xxxxxxx>; Xiubo.Lee@xxxxxxxxx; festevam@xxxxxxxxx; >> nicoleotsuka@xxxxxxxxx; lgirdwood@xxxxxxxxx; perex@xxxxxxxx; >> tiwai@xxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; >> linuxppc-dev@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx >> *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@xxxxxxxxxx> >> 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* >> >
