| Message ID | 20230911121501.21910-4-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | None | expand |
On Mon, 11 Sep 2023 15:14:56 +0300 Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> 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@linux.intel.com> > --- > 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. 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. For extra giggles there are two e1000_bus_width enum definitions in different headers. Actual patch is good - just 'interesting' stuff noticed whilst looking at it :) Jonathan > } > > reg = rd32(E1000_STATUS);
On Tue, 12 Sep 2023, Jonathan Cameron wrote: > On Mon, 11 Sep 2023 15:14:56 +0300 > Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> 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@linux.intel.com> > > --- > > 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.
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); } reg = rd32(E1000_STATUS);
Use FIELD_GET() to extract PCIe Negotiated Link Width field instead of custom masking and shifting. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/net/ethernet/intel/igb/e1000_mac.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)