Messages in this thread | | | From | Yury Norov <> | Date | Tue, 25 Jan 2022 15:10:00 -0800 | Subject | Re: [PATCH 10/54] net: ethernet: replace bitmap_weight with bitmap_empty for qlogic |
| |
On Tue, Jan 25, 2022 at 2:15 PM David Laight <David.Laight@aculab.com> wrote: > > From: Yury Norov > > Sent: 25 January 2022 21:10 > > On Mon, Jan 24, 2022 at 4:29 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Sun, Jan 23, 2022 at 10:38:41AM -0800, Yury Norov wrote: > > > > qlogic/qed code calls bitmap_weight() to check if any bit of a given > > > > bitmap is set. It's better to use bitmap_empty() in that case because > > > > bitmap_empty() stops traversing the bitmap as soon as it finds first > > > > set bit, while bitmap_weight() counts all bits unconditionally. > > > > > > > - if (bitmap_weight((unsigned long *)&pmap[item], 64 * 8)) > > > > + if (!bitmap_empty((unsigned long *)&pmap[item], 64 * 8)) > > > > > > > - (bitmap_weight((unsigned long *)&pmap[item], > > > > + (!bitmap_empty((unsigned long *)&pmap[item], > > > > > > Side note, these castings reminds me previous discussion and I'm wondering > > > if you have this kind of potentially problematic places in your TODO as > > > subject to fix. > > > > In the discussion you mentioned above, the u32* was cast to u64*, > > which is wrong. The code > > here is safe because in the worst case, it casts u64* to u32*. This > > would be OK wrt > > -Werror=array-bounds. > > > > The function itself looks like doing this unsigned long <-> u64 > > conversions just for printing > > purpose. I'm not a qlogic expert, so let's wait what people say? > > It'll be wrong on BE systems.
The bitmap_weigh() result will be correct. As you can see, the address is 64-bit aligned anyways. The array boundary violation will never happen as well.
DP_NOTICE() may be wrong, or may not. It depends on how important the absolute position of the bit in the printed bitmap is. Nevertheless, printk("%pb") is better and should be used.
This whole concern may be simply irrelevant if QED is not supported on 32-bit BE machines. From what I can see, at least Infiniband requires 64BIT.
Thanks, Yury
> You just can't cast the argument it has to be long[]. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
| |