Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Dec 2022 07:21:05 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [bug report] rtw89: add Realtek 802.11ax driver
Message-ID: <Y5f9sUhoY/vqy0cQ@kadam>
References: <Y5dJymTl15fBnQtV@kili>
 <e87d10cd962d4d118b86a280f986f6ef@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e87d10cd962d4d118b86a280f986f6ef@realtek.com>
Precedence: bulk

On Tue, Dec 13, 2022 at 12:46:22AM +0000, Ping-Ke Shih wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter <error27@gmail.com>
> > Sent: Monday, December 12, 2022 11:34 PM
> > To: Ping-Ke Shih <pkshih@realtek.com>
> > Cc: linux-wireless@vger.kernel.org
> > Subject: [bug report] rtw89: add Realtek 802.11ax driver
> > 
> > Hello Ping-Ke Shih,
> > 
> > The patch e3ec7017f6a2: "rtw89: add Realtek 802.11ax driver" from Oct
> > 11, 2021, leads to the following potential issue (just from reading
> > the code):
> > 
> > drivers/net/wireless/realtek/rtw89/core.h
> >     3878 static inline u32
> >     3879 rtw89_read32_mask(struct rtw89_dev *rtwdev, u32 addr, u32 mask)
> >     3880 {
> >     3881         u32 shift = __ffs(mask);
> >     3882         u32 orig;
> >     3883         u32 ret;
> >     3884
> >     3885         orig = rtw89_read32(rtwdev, addr);
> > --> 3886         ret = (orig & mask) >> shift;
> > 
> > I think this line should be:
> > 
> > 	ret = (orig & mask) >> (shift - 1);
> > 
> > A typical mask here is 0xff so __ffs() is 1 because the first bit is
> > set.  This code will do: ret = (orig & 0xff) >> 1;  I think it should be
> > ret = (orig & 0xff) >> 0;
> > 
> > If the mask was 0xff00 I would expect >> 8 instead of >> 9 etc.
> 
> ffs(0xff)=1 and __ffs(0xff)=0, so I think original is correct.

Oh, wow.  You're right.  I hadn't realized it worked like that.

Sorry!

regards,
dan carpenter