Re: [PATCH 3/8] igb: Use FIELD_GET() to extract Link Width

[Ilpo Järvinen]

On Tue, 12 Sep 2023, Jonathan Cameron wrote:

> On Mon, 11 Sep 2023 15:14:56 +0300
> Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
> 
> > Use FIELD_GET() to extract PCIe Negotiated Link Width field instead of
> > custom masking and shifting.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/intel/igb/e1000_mac.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igb/e1000_mac.c b/drivers/net/ethernet/intel/igb/e1000_mac.c
> > index caf91c6f52b4..5a23b9cfec6c 100644
> > --- a/drivers/net/ethernet/intel/igb/e1000_mac.c
> > +++ b/drivers/net/ethernet/intel/igb/e1000_mac.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /* Copyright(c) 2007 - 2018 Intel Corporation. */
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/if_ether.h>
> >  #include <linux/delay.h>
> >  #include <linux/pci.h>
> > @@ -50,9 +51,8 @@ s32 igb_get_bus_info_pcie(struct e1000_hw *hw)
> >  			break;
> >  		}
> >  
> > -		bus->width = (enum e1000_bus_width)((pcie_link_status &
> > -						     PCI_EXP_LNKSTA_NLW) >>
> > -						     PCI_EXP_LNKSTA_NLW_SHIFT);
> > +		bus->width = (enum e1000_bus_width)FIELD_GET(PCI_EXP_LNKSTA_NLW,
> > +							     pcie_link_status);
> 
> This cast is a bit ugly given it takes the values 0, 1, 2, 3 and
> we extra a field that the spec says contains 1, 2, 4, 8 etc
> Hence it only works because only 1 and 2 are used I think...  Not nice.

Not perfect but I guess the enum definition could use 
PCI_EXP_LNKSTA_NLW_X* to ensure at least the PCIe ones match.

> Also, whilst looking at this I note that e1000e has it's own defines
> for PCIE_LINK_WIDTH_MASK and PCIE_LINK_WIDTH_SHIFT 
> 
> Looks like those should be changed to use the standard defines.

Yes, thanks. I added a patch to address those duplicated defines and 
I also noticed it had a duplicated copy for PCI_EXP_LNKSTA which I also 
converted.

I'll send v2 which has most of your suggestions taken into account once 
the build bot has done its thing.

> For extra giggles there are two e1000_bus_width enum definitions in different
> headers.

No, there are actually 3 if one looks carefully, and many more if the 
ones named according to the driver are also counted all following this 
same "not nice" pattern. ;-)

That's 3 different drivers though which just happen to be similarly named 
so it's not entirely fair as it would be same as saying drivers x, y, and 
z have something with the same name. It's pretty obviously those come from 
copy paste though which usually means some common code might have been 
handy.

> Actual patch is good - just 'interesting' stuff noticed whilst looking 
> at it :) 

Yeah, I've plenty of 'interesting' stuff I've noticed while looking around 
on my todo list. I even thought I had that general PCI_EXP_* FIELD_GET() 
cleanup on it as I recall eyeing what it would take to find all of them 
but it seems I never added that there (now it is).

But then I was taking a look at these Link Width ones and there was just 
so much low-hanging fruit (some of which are like you put it, an 
excellent example of good cleanup) so I went to do that right away 
without considering all the other fields.

Thanks a lot for taking a look.


-- 
 i.