Re: [PATCH] ASoC: sti: uniperif: fix the undefined bitwise shift behavior problem

From: Su Hui <suhui@nfschina.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: arnaud.pouliquen@foss.st.com, lgirdwood@gmail.com,
	broonie@kernel.org, perex@perex.cz, tiwai@suse.com,
	nathan@kernel.org, ndesaulniers@google.com, morbo@google.com,
	justinstitt@google.com, alsa-devel@alsa-project.org,
	linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev, kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] ASoC: sti: uniperif: fix the undefined bitwise shift behavior problem
Date: Tue, 26 Mar 2024 13:30:04 +0800	[thread overview]
Message-ID: <e70c65b0-2e0d-613f-4f05-bda61377fc01@nfschina.com> (raw)
In-Reply-To: <5d850276-a872-45fb-9df2-2b72479787be@moroto.mountain>

Hi,
On 2024/3/25 22:25, Dan Carpenter wrote:
> On Mon, Mar 25, 2024 at 11:40:33AM +0800, Su Hui wrote:
>> --- a/sound/soc/sti/uniperif.h
>> +++ b/sound/soc/sti/uniperif.h
>> @@ -12,17 +12,28 @@
>>   
>>   #include <sound/dmaengine_pcm.h>
>>   
>> +#define SR_SHIFT(a, b)		({unsigned long __a = (a); \
>> +				unsigned int __b = (b); \
>> +				__b < BITS_PER_LONG ? \
>> +				__a >> __b : 0; })
> The code definitely looks buggy, but how do you know your solution is
> correct without testing it?

I only test some cases like SR_SHIFT(1, -1),SR_SHIFT(8,1), it seems have a right result.
Oh, maybe I understand it. When 'a' is a negative value like '(int)-1', SR_SHIFT(a, b) will
have some bugs.

>
> I don't like this solution at all.  This is basically a really
> complicated way of writing "if (b != -1)".  Instead of checking for -1,
> the better solution is to just stop passing -1.  If you review that
> file, every time it uses "-1" that's either dead code or a bug...

Agreed，some are dead codes which can be removed, but what should we do with the
following error codes like this one?

sound/soc/sti/uniperif.h
  415 #define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) \
  416         ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? 7 : -1)
  ...
  423 #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) \
  424         SET_UNIPERIF_REG(ip, \
  425                 UNIPERIF_CONFIG_OFFSET(ip), \
  426                 UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
How about this solution? If the condition is false, just skip it.

@@ -412,8 +412,7 @@
                 UNIPERIF_CONFIG_REPEAT_CHL_STS_MASK(ip), 1)
  
  /* BACK_STALL_REQ */
-#define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) \
-       ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? 7 : -1)
+#define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) 7
  #define UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip) 0x1
  #define GET_UNIPERIF_CONFIG_BACK_STALL_REQ(ip) \
         GET_UNIPERIF_REG(ip, \
@@ -421,10 +420,11 @@
                 UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \
                 UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip))
  #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) \
+       ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? -1 : \
         SET_UNIPERIF_REG(ip, \
                 UNIPERIF_CONFIG_OFFSET(ip), \
                 UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \
-               UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip), 0)
+               UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip), 0))
  #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_ENABLE(ip) \

Maybe should print some error log here.
I'm not sure about the safety of skipping SET_UNIPERIF_REG when the condition is false,

Would it be better to make the result of undefined shift equal to zero?

regards,
Su Hui
  
