Re: [bug report] rtw89: add Realtek 802.11ax driver

[Dan Carpenter <error27@gmail.com>]

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