Received-SPF: pass (google.com: domain of linux-wireless+bounces-85-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <1bec6c93-aac4-eb95-f4f6-7b8fac65d967@nfschina.com>
Date: Mon, 27 Nov 2023 09:13:09 +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 v2 2/2] wifi: rtlwifi: rtl8821ae: phy: fix an undefined
 bitwise shift behavior
Content-Language: en-US
To: Ping-Ke Shih <pkshih@realtek.com>, "kvalo@kernel.org" <kvalo@kernel.org>,
 "nathan@kernel.org" <nathan@kernel.org>,
 "ndesaulniers@google.com" <ndesaulniers@google.com>,
 "dan.carpenter@linaro.org" <dan.carpenter@linaro.org>,
 "trix@redhat.com" <trix@redhat.com>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
 "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>,
 "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
 "llvm@lists.linux.dev" <llvm@lists.linux.dev>,
 "linville@tuxdriver.com" <linville@tuxdriver.com>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "lizetao1@huawei.com" <lizetao1@huawei.com>
From: Su Hui <suhui@nfschina.com>
In-Reply-To: <02f0a505b3a02a3c5e29ac1e327acd1fc946188c.camel@realtek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2023/11/24 19:19, Ping-Ke Shih wrote:
> On Fri, 2023-11-24 at 18:06 +0800, Su Hui wrote:
>> On 2023/11/24 16:51, Ping-Ke Shih wrote:
>>> Subject: [PATCH v2 2/2] wifi: rtlwifi: rtl8821ae: phy: fix an undefined bitwise shift behavior
>>>
>>> [...]
>>>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
>>>> b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
>>>> index 6df270e29e66..52ab1b0761c0 100644
>>>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
>>>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/phy.c
>>>> @@ -31,7 +31,12 @@ static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask)
>>>>    {
>>>>           u32 i = ffs(bitmask);
>>>>
>>>> -       return i ? i - 1 : 32;
>>>> +       if (!i) {
>>>> +               WARN_ON_ONCE(1);
>>>> +               return 0;
>>>> +       }
>>>> +
>>>> +       return i - 1;
>>>>    }
>>> Personally, I prefer to use __ffs(), because in normal case no need additional '-1',
>>> and abnormal cases should not happen.
>> Hi,  Ping-Ke
>>
>> Replace _rtl8821ae_phy_calculate_bit_shift() by __ffs(bitmask) is better,
>> but I'm not sure what callers should do when callers check bitmask is 0 before calling.
>> Maybe this check is useless?
>>
>> I can send a v3 patch if using  __ffs(bitmask) and no check for bitmask is fine.
>> Or could you send this patch if you have a better idea?
>> Thanks for your suggestion!
>>
> Can this work to you?
Looks good to me, briefer and better!
I will send v3 soon.
> static u32 _rtl8821ae_phy_calculate_bit_shift(u32 bitmask)
> {
> 	if (WARN_ON_ONCE(!bitmask))
> 		return 0;
>
> 	return __ffs(bitmask);
> }