Received-SPF: pass (google.com: domain of linux-kernel+bounces-120169-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <a9bc4138-df79-112a-6470-90c86ba5bc95@nfschina.com>
Date: Wed, 27 Mar 2024 09:15:18 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH] ASoC: sti: uniperif: fix the undefined bitwise shift
 behavior problem
Content-Language: en-US
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
In-Reply-To: <e70c65b0-2e0d-613f-4f05-bda61377fc01@nfschina.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

This is a kindly resend email.
Sorry for the error style of last email :(
On 2024/3/26 13:30, Su Hui wrote:
> 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
>   
>