From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4F9A1C43334 for ; Wed, 20 Jul 2022 12:43:30 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4LnwNc2C2mz3d9f for ; Wed, 20 Jul 2022 22:43:28 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20210112 header.b=K2sf2zbZ; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gmail.com (client-ip=2a00:1450:4864:20::12e; helo=mail-lf1-x12e.google.com; envelope-from=shengjiu.wang@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20210112 header.b=K2sf2zbZ; dkim-atps=neutral Received: from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com [IPv6:2a00:1450:4864:20::12e]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4Lnl3C4CJmz2xmg for ; Wed, 20 Jul 2022 15:42:46 +1000 (AEST) Received: by mail-lf1-x12e.google.com with SMTP id n18so28502199lfq.1 for ; Tue, 19 Jul 2022 22:42:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=+aL53xeHszDzXfVsqRlkFlKt1uWxXV5MXK0jzsINeM8=; b=K2sf2zbZ6UbHQ8IMqoiIxBfw/z2F06npJu4TRP/EyTANSofa3VS/PwhDxSkek/fDU6 zN6txnGaWxWJj7AazYi4X8E6XhqZnKqwtuLJ7W7J9NQuMIN6F+S35ebOo69ThgvMI8ZL nDK9ERxiVUhg5T+Ll3IXVyJ66vSAlaz+hxBzBxKHMYrstFiof7N0AEsBuh/kFrIQlieh 59RFXtVYFxCt5FdA23dC8bnWqO2KUK90MepFV/g2QL0iPN2PNTYJW8sR6Pnyp/BixdbX skIfO+Q5GGCKVkof4WPFEhO3qDzMUzSmCFWe8QrL30ZiqqKdTqw7+Sp7GyUUtb62gWlg 81AA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+aL53xeHszDzXfVsqRlkFlKt1uWxXV5MXK0jzsINeM8=; b=UH8fsA3wSQHp1fS0Q+xRZ/AsamwfYzPRGF6UlvFtRsV7eksViahLeDZDPNd4UNqMXY 6WbBWGbrlcvGnRm9OM7tR4FrypGs6NmbgUujMFA61ZDA/XUJXWE1Ix5CDvkyQdVmNdk7 emQ+jF5FyKDumZUIQraurSHK2cPplIVE3WNWyXF+J1ny8aXm8TbYQfG0HzAyXmUy5bBI vEm52MdZdv9PUyb8Ki7pi1jTQIS/0Qv6/ZoXHpCaDJ3G6dhXUToj/zB/9/x5DOlC/jF3 qssYrgeQj5LbPVMrD1IRLBQrLbIOeCOSiEt8nlP72uF68DIjK/bJs2OVrpRxBvwM7Gr9 S0iA== X-Gm-Message-State: AJIora8H4c6RWuL871qxnUdV3v3ZNAAJoGUoZ8ksTKdnfQJPrGGHJA2t cPvrYLDi9lT4ybVvuFldDU2RsBs6jI6R5Kn8qi4= X-Google-Smtp-Source: AGRyM1t3Ie1nCgCu4TCjs+FvoSxysaFjDDcvKN0+JNwIZ0/8s7qHAPvs7mGVswvUY90fJtPXGawN5k8b0KxEl99PBNg= X-Received: by 2002:a05:6512:1153:b0:48a:201d:5f77 with SMTP id m19-20020a056512115300b0048a201d5f77mr13168824lfg.280.1658295760028; Tue, 19 Jul 2022 22:42:40 -0700 (PDT) MIME-Version: 1.0 References: <1658222864-25378-1-git-send-email-shengjiu.wang@nxp.com> <1658222864-25378-3-git-send-email-shengjiu.wang@nxp.com> <20cfcc8e59a74166846cff028cd2c4e8@AcuMS.aculab.com> In-Reply-To: From: Shengjiu Wang Date: Wed, 20 Jul 2022 13:42:28 +0800 Message-ID: Subject: Re: [PATCH -next 2/5] ASoC: fsl_asrc: force cast the asrc_format type To: David Laight Content-Type: multipart/alternative; boundary="000000000000b58c3a05e4361134" X-Mailman-Approved-At: Wed, 20 Jul 2022 22:42:51 +1000 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "alsa-devel@alsa-project.org" , "Xiubo.Lee@gmail.com" , "linuxppc-dev@lists.ozlabs.org" , Shengjiu Wang , "tiwai@suse.com" , "lgirdwood@gmail.com" , "perex@perex.cz" , "nicoleotsuka@gmail.com" , Mark Brown , "festevam@gmail.com" , "linux-kernel@vger.kernel.org" Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" --000000000000b58c3a05e4361134 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi David On Tue, Jul 19, 2022 at 9:35 PM Shengjiu Wang wrote: > > > On Tue, Jul 19, 2022 at 8:39 PM David Laight > wrote: > >> grrr... top-posting because outluck is really stupid :-( >> >> >> >> The definition seems to be: >> >> typedef int __bitwise >> >> snd_pcm_format_t >> ; >> >> #define SNDRV_PCM_FORMAT_S8 >> >> ((__force >> snd_pcm_format_t >> ) 0= ) >> >> #define SNDRV_PCM_FORMAT_U8 >> >> ((__force >> snd_pcm_format_t >> ) 1= ) >> >> #define SNDRV_PCM_FORMAT_S16_LE >> >> ((__force >> snd_pcm_format_t >> ) 2= ) >> >> ... >> >> (goes away and looks up __bitwIse) >> >> >> >> I think I=E2=80=99d 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 =3D 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 =3D of_property_read_u32(np, "fsl,asrc-format", (u32 *)&asrc->asrc_format); + ret =3D of_property_read_u32(np, "fsl,asrc-format", &asrc_fmt); + asrc->asrc_format =3D snd_pcm_format(asrc_fmt); if (ret) { ret =3D 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 =3D 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 in= t >> 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 corrupte= d. >> >> So you really need to use a u32 =E2=80=98temp=E2=80=99 variable. >> >> >> >> It would all be slightly less problematic if the =E2=80=98force=E2=80=99= casts could be >> sparse only >> >> (ie not seen by the compiler) =E2=80=93 so the compiler would do the typ= e >> checking. >> >> >> >> David >> >> >> >> *From:* Shengjiu Wang >> *Sent:* 19 July 2022 12:07 >> *To:* David Laight >> *Cc:* Mark Brown ; 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 >> 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 =3D of_property_read_u32(np, "fsl,asrc-format", >> &asrc->asrc_format); >> > > > + ret =3D 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 typ= e. >> >> >> >> 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, MK= 1 >> 1PT, UK >> Registration No: 1397386 (Wales) >> >> >> >> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK= 1 >> 1PT, UK >> Registration No: 1397386 (Wales) >> >> P *Please consider the environment and don't print this e-mail unless >> you really need to* >> > --000000000000b58c3a05e4361134 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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 Lai= ght <David.= Laight@aculab.com> wrote:

grrr... top-posting because outluck is really = stupid :-(

=C2=A0

The definition seems to be:

typedef int __bitwise snd_pcm_format_t;

#define SNDRV_PCM_FORMAT_S8=C2=A0=C2=A0=C2=A0 ((__force snd_pcm_format_t) 0)

#define SNDRV_PCM_FORMAT_U8=C2=A0=C2=A0=C2=A0 ((__force snd_pcm_format_t) 1)

#define SNDRV_PCM_FORMAT_S16_LE=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((__force snd_pcm_format_t) 2)

...

(goes away and looks up __bitwIse)

=C2=A0

I think I=E2=80=99d add:<= /p>

#define snd_pcm_format(val) ((__force snd_pcm_= format_t)(val))


Wher= e is this definition? Which header file?
Thanks.


Here is the change based o= n your proposal.=C2=A0=C2=A0
Not sure if there is misundersta= nding.
Not sure if the definition can be put in pcm.h.=C2=A0

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
i= ndex 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)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 return= 1ULL << (__force int) pcm_format;
=C2=A0}

+#define snd_pcm= _format(val) ((__force snd_pcm_format_t)(val))
+
=C2=A0/**
=C2=A0 = * pcm_for_each_format - helper to iterate for each format type
=C2=A0 * = @f: the iterator variable in snd_pcm_format_t type
diff --git a/sound/so= c/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 544395efd605..dcfdfb6b= 3472 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_as= rc.c
@@ -1066,6 +1066,7 @@ static int fsl_asrc_probe(struct platform_dev= ice *pdev)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct resource *res;
=C2=A0 = =C2=A0 =C2=A0 =C2=A0 void __iomem *regs;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 int= irq, ret, i;
+ =C2=A0 =C2=A0 =C2=A0 u32 asrc_fmt =3D 0;
=C2=A0 =C2= =A0 =C2=A0 =C2=A0 u32 map_idx;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 char tmp[16];=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 u32 width;
@@ -1174,7 +1175,8 @@ static = int fsl_asrc_probe(struct platform_device *pdev)
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ret;
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 }

- =C2=A0 =C2=A0 =C2=A0 ret =3D of_property_read_u32(np, "= fsl,asrc-format", (u32 *)&asrc->asrc_format);
+ =C2=A0 =C2= =A0 =C2=A0 ret =3D of_property_read_u32(np, "fsl,asrc-format", &a= mp;asrc_fmt);
+ =C2=A0 =C2=A0 =C2=A0 asrc->asrc_format =3D snd_pcm_fo= rmat(asrc_fmt);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (ret) {
=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D of_property_read_u32(np, = "fsl,asrc-width", &width);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 if (ret) {
@@ -1197,7 +1199,7 @@ static int fsl= _asrc_probe(struct platform_device *pdev)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

- =C2= =A0 =C2=A0 =C2=A0 if (!(FSL_ASRC_FORMATS & (1ULL << (__force u32)= asrc->asrc_format))) {
+ =C2=A0 =C2=A0 =C2=A0 if (!(FSL_ASRC_FORMATS = & pcm_format_to_bits(asrc->asrc_format))) {
=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_warn(&pdev->dev, "unsupp= orted width, use default S24_LE\n");
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 asrc->asrc_format =3D SNDRV_PCM_FORMAT_S24_L= E;
=C2=A0
best regards
wang shengjiu

and use that to remove most of the casts.=C2=A0

But the ones where you have (u32 *)&xxx ar= e only valid because u32 and int are the same size.

That does sort of happen to be true, but someo= ne might look at all the values and

decide that u8 is big enough.

After which the code will still compile, but t= he data areas get corrupted.

So you really need to use a u32 =E2=80=98temp= =E2=80=99 variable.

=C2=A0

It would all be slightly less problematic if t= he =E2=80=98force=E2=80=99 casts could be sparse only<= /p>

(ie not seen by the compiler) =E2=80=93 so the= compiler would do the type checking.

=C2=A0

=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 David

=C2=A0

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<= /a>; nicoleotsu= ka@gmail.com; = lgirdwood@gmail.com; perex@perex.cz; t= iwai@suse.com; alsa-devel@alsa-project.org; linuxppc-dev@lists.ozlabs.org; linux-ke= rnel@vger.kernel.org
Subject: Re: [PATCH -next 2/5] ASoC: fsl_asrc: force cast the asrc_f= ormat type

=C2=A0

=C2=A0

=C2=A0

On Tue, Jul 19, 2022 at 6:34 PM David Laight <David.Laight@acul= ab.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 =3D of_property_read_u32(np, "fsl,asrc-format&quo= t;, &asrc->asrc_format);
> > > + ret =3D of_property_read_u32(np, "fsl,asrc-format&quo= t;, (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=C2=A0snd_pcm_for= mat_t cast to u32/int type.

=C2=A0

The code in=C2=A0include/sound/pcm.h also does such = __force cast.

#define _SNDRV_PCM_FMTBIT(fmt) =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0(1ULL << (__force int)SNDRV_PCM_FORMAT_##fmt)<= /u>

=C2=A0

The change I have made does not cause an issue.=C2= =A0=C2=A0

=C2=A0

Best regards

Wang shengjiu

=C2=A0

(Mark: You'll be gl= ad to hear that the office aircon is
broken again - two weeks lead time on the spare part.)

=C2=A0 =C2=A0 =C2=A0 =C2=A0 David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1= PT, UK
Registration No: 1397386 (Wales)



Registered Address Lakeside, Bramley Road, Mount Farm, Milton K= eynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

P Pleas= e consider the environment and don't print this e-mail unless you reall= y need to

--000000000000b58c3a05e4361134--