Re: [PATCH -next 2/5] ASoC: fsl_asrc: force cast the asrc_format type

[Shengjiu Wang <shengjiu.wang@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6670 bytes --]

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*
>>
>

[-- Attachment #2: Type: text/html, Size: 12856 bytes --]