[PATCH 0/8] PCI/treewide: PCIe Link Width field access cleanup

[PATCH 0/8] PCI/treewide: PCIe Link Width field access cleanup

[Ilpo Järvinen]

Instead of custom code to extract the PCIe Negotiated and Maximum Link
Width fields, make the code more obvious using FIELD_GET/PREP().

Ilpo Järvinen (8):
  IB/hfi1: Use FIELD_GET() to extract Link Width
  media: cobalt: Use FIELD_GET() to extract Link Width
  igb: Use FIELD_GET() to extract Link Width
  PCI: tegra194: Use FIELD_GET()/FIELD_PREP() with Link Width fields
  PCI: mvebu: Use FIELD_PREP() with Link Width
  PCI: Use FIELD_GET() to extract Link Width
  scsi: esas2r: Use FIELD_GET() to extract Link Width
  scsi: qla2xxx: Use FIELD_GET() to extract Link Width

 drivers/infiniband/hw/hfi1/pcie.c          |  5 +++--
 drivers/media/pci/cobalt/cobalt-driver.c   | 11 ++++++-----
 drivers/net/ethernet/intel/igb/e1000_mac.c |  6 +++---
 drivers/pci/controller/dwc/pcie-tegra194.c |  9 ++++-----
 drivers/pci/controller/pci-mvebu.c         |  2 +-
 drivers/pci/pci-sysfs.c                    |  5 ++---
 drivers/pci/pci.c                          |  6 +++---
 drivers/scsi/esas2r/esas2r_ioctl.c         |  8 ++++----
 drivers/scsi/qla2xxx/qla_os.c              |  3 ++-
 9 files changed, 28 insertions(+), 27 deletions(-)

-- 
2.30.2

[PATCH 1/8] IB/hfi1: Use FIELD_GET() to extract Link Width

[Ilpo Järvinen]

Use FIELD_GET() to extract PCIe Negotiated Link Width field instead of
custom masking and shifting.

While at it, also fix function's comment.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/infiniband/hw/hfi1/pcie.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
index 08732e1ac966..d497e4c623c1 100644
--- a/drivers/infiniband/hw/hfi1/pcie.c
+++ b/drivers/infiniband/hw/hfi1/pcie.c
@@ -3,6 +3,7 @@
  * Copyright(c) 2015 - 2019 Intel Corporation.
  */
 
+#include <linux/bitfield.h>
 #include <linux/pci.h>
 #include <linux/io.h>
 #include <linux/delay.h>
@@ -210,10 +211,10 @@ static u32 extract_speed(u16 linkstat)
 	return speed;
 }
 
-/* return the PCIe link speed from the given link status */
+/* return the PCIe Link Width from the given link status */
 static u32 extract_width(u16 linkstat)
 {
-	return (linkstat & PCI_EXP_LNKSTA_NLW) >> PCI_EXP_LNKSTA_NLW_SHIFT;
+	return FIELD_GET(PCI_EXP_LNKSTA_NLW, linkstat);
 }
 
 /* read the link status and set dd->{lbus_width,lbus_speed,lbus_info} */
-- 
2.30.2

Re: [PATCH 1/8] IB/hfi1: Use FIELD_GET() to extract Link Width

[Jonathan Cameron]

On Mon, 11 Sep 2023 15:14:54 +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.
> 
> While at it, also fix function's comment.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/infiniband/hw/hfi1/pcie.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hfi1/pcie.c
> index 08732e1ac966..d497e4c623c1 100644
> --- a/drivers/infiniband/hw/hfi1/pcie.c
> +++ b/drivers/infiniband/hw/hfi1/pcie.c
> @@ -3,6 +3,7 @@
>   * Copyright(c) 2015 - 2019 Intel Corporation.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/pci.h>
>  #include <linux/io.h>
>  #include <linux/delay.h>
> @@ -210,10 +211,10 @@ static u32 extract_speed(u16 linkstat)
>  	return speed;
>  }
>  
> -/* return the PCIe link speed from the given link status */
> +/* return the PCIe Link Width from the given link status */
>  static u32 extract_width(u16 linkstat)
>  {
> -	return (linkstat & PCI_EXP_LNKSTA_NLW) >> PCI_EXP_LNKSTA_NLW_SHIFT;
> +	return FIELD_GET(PCI_EXP_LNKSTA_NLW, linkstat);

The helper seems like overkill now.  Maybe just push this code inline
and drop the wrapper.  I don't think the comment is necessary after
that as we are putting it in a bus_width field and the register is
obviously link status from the naming.

	dd->lbus_width = FIELD_GET(PCI_EXP_LINKSTA_NLW, linkstat);

>  }
>  
>  /* read the link status and set dd->{lbus_width,lbus_speed,lbus_info} */

[PATCH 2/8] media: cobalt: Use FIELD_GET() to extract Link Width

[Ilpo Järvinen]

Use FIELD_GET() to extract PCIe Negotiated and Maximum Link Width fields
instead of custom masking and shifting.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/media/pci/cobalt/cobalt-driver.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/pci/cobalt/cobalt-driver.c b/drivers/media/pci/cobalt/cobalt-driver.c
index 74edcc76d12f..6e1a0614e6d0 100644
--- a/drivers/media/pci/cobalt/cobalt-driver.c
+++ b/drivers/media/pci/cobalt/cobalt-driver.c
@@ -8,6 +8,7 @@
  *  All rights reserved.
  */
 
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <media/i2c/adv7604.h>
 #include <media/i2c/adv7842.h>
@@ -210,17 +211,17 @@ void cobalt_pcie_status_show(struct cobalt *cobalt)
 	pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &stat);
 	cobalt_info("PCIe link capability 0x%08x: %s per lane and %u lanes\n",
 			capa, get_link_speed(capa),
-			(capa & PCI_EXP_LNKCAP_MLW) >> 4);
+			FIELD_GET(PCI_EXP_LNKCAP_MLW, capa));
 	cobalt_info("PCIe link control 0x%04x\n", ctrl);
 	cobalt_info("PCIe link status 0x%04x: %s per lane and %u lanes\n",
 		    stat, get_link_speed(stat),
-		    (stat & PCI_EXP_LNKSTA_NLW) >> 4);
+		    FIELD_GET(PCI_EXP_LNKSTA_NLW, stat));
 
 	/* Bus */
 	pcie_capability_read_dword(pci_bus_dev, PCI_EXP_LNKCAP, &capa);
 	cobalt_info("PCIe bus link capability 0x%08x: %s per lane and %u lanes\n",
 			capa, get_link_speed(capa),
-			(capa & PCI_EXP_LNKCAP_MLW) >> 4);
+			FIELD_GET(PCI_EXP_LNKCAP_MLW, capa));
 
 	/* Slot */
 	pcie_capability_read_dword(pci_dev, PCI_EXP_SLTCAP, &capa);
@@ -239,7 +240,7 @@ static unsigned pcie_link_get_lanes(struct cobalt *cobalt)
 	if (!pci_is_pcie(pci_dev))
 		return 0;
 	pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &link);
-	return (link & PCI_EXP_LNKSTA_NLW) >> 4;
+	return FIELD_GET(PCI_EXP_LNKSTA_NLW, link);
 }
 
 static unsigned pcie_bus_link_get_lanes(struct cobalt *cobalt)
@@ -250,7 +251,7 @@ static unsigned pcie_bus_link_get_lanes(struct cobalt *cobalt)
 	if (!pci_is_pcie(pci_dev))
 		return 0;
 	pcie_capability_read_dword(pci_dev, PCI_EXP_LNKCAP, &link);
-	return (link & PCI_EXP_LNKCAP_MLW) >> 4;
+	return FIELD_GET(PCI_EXP_LNKCAP_MLW, link);
 }
 
 static void msi_config_show(struct cobalt *cobalt, struct pci_dev *pci_dev)
-- 
2.30.2

[Intel-wired-lan] [PATCH 3/8] igb: Use FIELD_GET() to extract Link Width

[Ilpo Järvinen]

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);
 	}
 
 	reg = rd32(E1000_STATUS);
-- 
2.30.2

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Ilpo Järvinen]

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);
 	}
 
 	reg = rd32(E1000_STATUS);
-- 
2.30.2

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

[Jonathan Cameron]

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);

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Jonathan Cameron]

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);

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

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.


-- 
 i.

[-- Attachment #2: Type: text/plain, Size: 162 bytes --]

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

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.


-- 
 i.

[PATCH 4/8] PCI: tegra194: Use FIELD_GET()/FIELD_PREP() with Link Width fields

[Ilpo Järvinen]

Use FIELD_GET() to extract PCIe Negotiated Link Width field instead of
custom masking and shifting.

Similarly, change custom code that misleadingly used
PCI_EXP_LNKSTA_NLW_SHIFT to prepare value for PCI_EXP_LNKCAP write
to use FIELD_PREP() with correct field define (PCI_EXP_LNKCAP_MLW).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index 4bba31502ce1..248cd9347e8f 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -9,6 +9,7 @@
  * Author: Vidya Sagar <vidyas@nvidia.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>
@@ -346,8 +347,7 @@ static void apply_bad_link_workaround(struct dw_pcie_rp *pp)
 	 */
 	val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
 	if (val & PCI_EXP_LNKSTA_LBMS) {
-		current_link_width = (val & PCI_EXP_LNKSTA_NLW) >>
-				     PCI_EXP_LNKSTA_NLW_SHIFT;
+		current_link_width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
 		if (pcie->init_link_width > current_link_width) {
 			dev_warn(pci->dev, "PCIe link is bad, width reduced\n");
 			val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
@@ -760,8 +760,7 @@ static void tegra_pcie_enable_system_interrupts(struct dw_pcie_rp *pp)
 
 	val_w = dw_pcie_readw_dbi(&pcie->pci, pcie->pcie_cap_base +
 				  PCI_EXP_LNKSTA);
-	pcie->init_link_width = (val_w & PCI_EXP_LNKSTA_NLW) >>
-				PCI_EXP_LNKSTA_NLW_SHIFT;
+	pcie->init_link_width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val_w);
 
 	val_w = dw_pcie_readw_dbi(&pcie->pci, pcie->pcie_cap_base +
 				  PCI_EXP_LNKCTL);
@@ -920,7 +919,7 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
 	/* Configure Max lane width from DT */
 	val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP);
 	val &= ~PCI_EXP_LNKCAP_MLW;
-	val |= (pcie->num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT);
+	val |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, pcie->num_lanes);
 	dw_pcie_writel_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP, val);
 
 	/* Clear Slot Clock Configuration bit if SRNS configuration */
-- 
2.30.2

Re: [PATCH 4/8] PCI: tegra194: Use FIELD_GET()/FIELD_PREP() with Link Width fields

[Jonathan Cameron]

On Mon, 11 Sep 2023 15:14:57 +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.
> 
> Similarly, change custom code that misleadingly used
> PCI_EXP_LNKSTA_NLW_SHIFT to prepare value for PCI_EXP_LNKCAP write
> to use FIELD_PREP() with correct field define (PCI_EXP_LNKCAP_MLW).

Excellent example for why this changes is a good cleanup beyond
reducing line lengths.  Harder to use the wrong define if you
are using one rather that two :)

Jonathan

> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/controller/dwc/pcie-tegra194.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 4bba31502ce1..248cd9347e8f 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -9,6 +9,7 @@
>   * Author: Vidya Sagar <vidyas@nvidia.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
> @@ -346,8 +347,7 @@ static void apply_bad_link_workaround(struct dw_pcie_rp *pp)
>  	 */
>  	val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKSTA);
>  	if (val & PCI_EXP_LNKSTA_LBMS) {
> -		current_link_width = (val & PCI_EXP_LNKSTA_NLW) >>
> -				     PCI_EXP_LNKSTA_NLW_SHIFT;
> +		current_link_width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val);
>  		if (pcie->init_link_width > current_link_width) {
>  			dev_warn(pci->dev, "PCIe link is bad, width reduced\n");
>  			val = dw_pcie_readw_dbi(pci, pcie->pcie_cap_base +
> @@ -760,8 +760,7 @@ static void tegra_pcie_enable_system_interrupts(struct dw_pcie_rp *pp)
>  
>  	val_w = dw_pcie_readw_dbi(&pcie->pci, pcie->pcie_cap_base +
>  				  PCI_EXP_LNKSTA);
> -	pcie->init_link_width = (val_w & PCI_EXP_LNKSTA_NLW) >>
> -				PCI_EXP_LNKSTA_NLW_SHIFT;
> +	pcie->init_link_width = FIELD_GET(PCI_EXP_LNKSTA_NLW, val_w);
>  
>  	val_w = dw_pcie_readw_dbi(&pcie->pci, pcie->pcie_cap_base +
>  				  PCI_EXP_LNKCTL);
> @@ -920,7 +919,7 @@ static int tegra_pcie_dw_host_init(struct dw_pcie_rp *pp)
>  	/* Configure Max lane width from DT */
>  	val = dw_pcie_readl_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP);
>  	val &= ~PCI_EXP_LNKCAP_MLW;
> -	val |= (pcie->num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT);
> +	val |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, pcie->num_lanes);
>  	dw_pcie_writel_dbi(pci, pcie->pcie_cap_base + PCI_EXP_LNKCAP, val);
>  
>  	/* Clear Slot Clock Configuration bit if SRNS configuration */

[PATCH 5/8] PCI: mvebu: Use FIELD_PREP() with Link Width

[Ilpo Järvinen]

mvebu_pcie_setup_hw() setups the Maximum Link Width field in the Link
Capabilities registers using an open-coded variant of FIELD_PREP() with
a literal in shift. Improve readability by using
FIELD_PREP(PCI_EXP_LNKCAP_MLW, ...).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-mvebu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 60810a1fbfb7..29fe09c99e7d 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -264,7 +264,7 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	 */
 	lnkcap = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
 	lnkcap &= ~PCI_EXP_LNKCAP_MLW;
-	lnkcap |= (port->is_x4 ? 4 : 1) << 4;
+	lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, port->is_x4 ? 4 : 1);
 	mvebu_writel(port, lnkcap, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
 
 	/* Disable Root Bridge I/O space, memory space and bus mastering. */
-- 
2.30.2

[PATCH 5/8] PCI: mvebu: Use FIELD_PREP() with Link Width

[Ilpo Järvinen]

mvebu_pcie_setup_hw() setups the Maximum Link Width field in the Link
Capabilities registers using an open-coded variant of FIELD_PREP() with
a literal in shift. Improve readability by using
FIELD_PREP(PCI_EXP_LNKCAP_MLW, ...).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-mvebu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 60810a1fbfb7..29fe09c99e7d 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -264,7 +264,7 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
 	 */
 	lnkcap = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
 	lnkcap &= ~PCI_EXP_LNKCAP_MLW;
-	lnkcap |= (port->is_x4 ? 4 : 1) << 4;
+	lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, port->is_x4 ? 4 : 1);
 	mvebu_writel(port, lnkcap, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP);
 
 	/* Disable Root Bridge I/O space, memory space and bus mastering. */
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] PCI: Use FIELD_GET() to extract Link Width

[Ilpo Järvinen]

Use FIELD_GET() to extract PCIe Negotiated and Maximum Link Width fields
instead of custom masking and shifting.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci-sysfs.c | 5 ++---
 drivers/pci/pci.c       | 6 +++---
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index d9eede2dbc0e..5a6241044c3c 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -12,7 +12,7 @@
  * Modeled after usb's driverfs.c
  */
 
-
+#include <linux/bitfield.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/pci.h>
@@ -230,8 +230,7 @@ static ssize_t current_link_width_show(struct device *dev,
 	if (err)
 		return -EINVAL;
 
-	return sysfs_emit(buf, "%u\n",
-		(linkstat & PCI_EXP_LNKSTA_NLW) >> PCI_EXP_LNKSTA_NLW_SHIFT);
+	return sysfs_emit(buf, "%u\n", FIELD_GET(PCI_EXP_LNKSTA_NLW, linkstat));
 }
 static DEVICE_ATTR_RO(current_link_width);
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59c01d68c6d5..a8adc34dc86f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/bitfield.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/dmi.h>
@@ -6257,8 +6258,7 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
 		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
 
 		next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
-		next_width = (lnksta & PCI_EXP_LNKSTA_NLW) >>
-			PCI_EXP_LNKSTA_NLW_SHIFT;
+		next_width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
 
 		next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
 
@@ -6330,7 +6330,7 @@ enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev)
 
 	pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap);
 	if (lnkcap)
-		return (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
+		return FIELD_GET(PCI_EXP_LNKCAP_MLW, lnkcap);
 
 	return PCIE_LNK_WIDTH_UNKNOWN;
 }
-- 
2.30.2

[PATCH 7/8] scsi: esas2r: Use FIELD_GET() to extract Link Width

[Ilpo Järvinen]

Use FIELD_GET() to extract PCIe Negotiated and Maximum Link Width fields
instead of custom masking and shifting.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/scsi/esas2r/esas2r_ioctl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c b/drivers/scsi/esas2r/esas2r_ioctl.c
index 055d2e87a2c8..3252780fd099 100644
--- a/drivers/scsi/esas2r/esas2r_ioctl.c
+++ b/drivers/scsi/esas2r/esas2r_ioctl.c
@@ -41,6 +41,8 @@
  * USA.
  */
 
+#include <linux/bitfield.h>
+
 #include "esas2r.h"
 
 /*
@@ -797,11 +799,9 @@ static int hba_ioctl_callback(struct esas2r_adapter *a,
 			gai->pci.link_speed_max =
 				(u8)(caps & PCI_EXP_LNKCAP_SLS);
 			gai->pci.link_width_curr =
-				(u8)((stat & PCI_EXP_LNKSTA_NLW)
-				     >> PCI_EXP_LNKSTA_NLW_SHIFT);
+				(u8)FIELD_GET(PCI_EXP_LNKSTA_NLW, stat);
 			gai->pci.link_width_max =
-				(u8)((caps & PCI_EXP_LNKCAP_MLW)
-				     >> 4);
+				(u8)FIELD_GET(PCI_EXP_LNKCAP_MLW, caps);
 		}
 
 		gai->pci.msi_vector_cnt = 1;
-- 
2.30.2

Re: [PATCH 7/8] scsi: esas2r: Use FIELD_GET() to extract Link Width

[Jonathan Cameron]

On Mon, 11 Sep 2023 15:15:00 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Use FIELD_GET() to extract PCIe Negotiated and Maximum Link Width fields
> instead of custom masking and shifting.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/scsi/esas2r/esas2r_ioctl.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c b/drivers/scsi/esas2r/esas2r_ioctl.c
> index 055d2e87a2c8..3252780fd099 100644
> --- a/drivers/scsi/esas2r/esas2r_ioctl.c
> +++ b/drivers/scsi/esas2r/esas2r_ioctl.c
> @@ -41,6 +41,8 @@
>   * USA.
>   */
>  
> +#include <linux/bitfield.h>
> +
>  #include "esas2r.h"
>  
>  /*
> @@ -797,11 +799,9 @@ static int hba_ioctl_callback(struct esas2r_adapter *a,
>  			gai->pci.link_speed_max =
>  				(u8)(caps & PCI_EXP_LNKCAP_SLS);
Better to convert the other field gets as well.

I'm curious as to why the u8 casts are here. The masking should have
kept the compiler happy that it is fine to assign these without
the casts and no chance of overflow.

>  			gai->pci.link_width_curr =
> -				(u8)((stat & PCI_EXP_LNKSTA_NLW)
> -				     >> PCI_EXP_LNKSTA_NLW_SHIFT);  
> +				(u8)FIELD_GET(PCI_EXP_LNKSTA_NLW, stat);
>  			gai->pci.link_width_max =
> -				(u8)((caps & PCI_EXP_LNKCAP_MLW)
> -				     >> 4);  
> +				(u8)FIELD_GET(PCI_EXP_LNKCAP_MLW, caps);
>  		}
>  
>  		gai->pci.msi_vector_cnt = 1;

[PATCH 8/8] scsi: qla2xxx: Use FIELD_GET() to extract Link Width

[Ilpo Järvinen]

Use FIELD_GET() to extract PCIe Maximum Link Width field instead of
custom masking and shifting.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/scsi/qla2xxx/qla_os.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 877e4f446709..0c97a5e4249c 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -5,6 +5,7 @@
  */
 #include "qla_def.h"
 
+#include <linux/bitfield.h>
 #include <linux/moduleparam.h>
 #include <linux/vmalloc.h>
 #include <linux/delay.h>
@@ -632,7 +633,7 @@ qla24xx_pci_info_str(struct scsi_qla_host *vha, char *str, size_t str_len)
 
 		pcie_capability_read_dword(ha->pdev, PCI_EXP_LNKCAP, &lstat);
 		lspeed = lstat & PCI_EXP_LNKCAP_SLS;
-		lwidth = (lstat & PCI_EXP_LNKCAP_MLW) >> 4;
+		lwidth = FIELD_GET(PCI_EXP_LNKCAP_MLW, lstat);
 
 		switch (lspeed) {
 		case 1:
-- 
2.30.2

Re: [PATCH 8/8] scsi: qla2xxx: Use FIELD_GET() to extract Link Width

[Jonathan Cameron]

On Mon, 11 Sep 2023 15:15:01 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Use FIELD_GET() to extract PCIe Maximum Link Width field instead of
> custom masking and shifting.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/scsi/qla2xxx/qla_os.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 877e4f446709..0c97a5e4249c 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -5,6 +5,7 @@
>   */
>  #include "qla_def.h"
>  
> +#include <linux/bitfield.h>
>  #include <linux/moduleparam.h>
>  #include <linux/vmalloc.h>
>  #include <linux/delay.h>
> @@ -632,7 +633,7 @@ qla24xx_pci_info_str(struct scsi_qla_host *vha, char *str, size_t str_len)
>  
>  		pcie_capability_read_dword(ha->pdev, PCI_EXP_LNKCAP, &lstat);
>  		lspeed = lstat & PCI_EXP_LNKCAP_SLS;
> -		lwidth = (lstat & PCI_EXP_LNKCAP_MLW) >> 4;
> +		lwidth = FIELD_GET(PCI_EXP_LNKCAP_MLW, lstat);

As previous.  Whilst I'm happy to see this change I'd prefer to see it
used in all similar cases so do the lspeed one just above as well.

As a reviewer I don't want to care about the alignment of a particular
field and hence whether it needs shifting or just masking.
I want to review the header once to see it matches the spec, then never
look at it again!

Jonathan

>  
>  		switch (lspeed) {
>  		case 1:

Re: [PATCH 0/8] PCI/treewide: PCIe Link Width field access cleanup

[Jonathan Cameron]

On Mon, 11 Sep 2023 15:14:53 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Instead of custom code to extract the PCIe Negotiated and Maximum Link
> Width fields, make the code more obvious using FIELD_GET/PREP().
> 
> Ilpo Järvinen (8):
>   IB/hfi1: Use FIELD_GET() to extract Link Width
>   media: cobalt: Use FIELD_GET() to extract Link Width
>   igb: Use FIELD_GET() to extract Link Width
>   PCI: tegra194: Use FIELD_GET()/FIELD_PREP() with Link Width fields
>   PCI: mvebu: Use FIELD_PREP() with Link Width
>   PCI: Use FIELD_GET() to extract Link Width
>   scsi: esas2r: Use FIELD_GET() to extract Link Width
>   scsi: qla2xxx: Use FIELD_GET() to extract Link Width

I'd like to see this done more generally rather than just
for this one field, but I guess it's a step in the right direction.
I'm particularly in favour of the cases where 4 was used
rather than the define.

> 
>  drivers/infiniband/hw/hfi1/pcie.c          |  5 +++--

As an example, immediately above the link width code is link
speed code.  Using FIELD_GET there as well would avoid people
having to 'know' that field is at 0 shift.

So for what you have 
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
but I'd like to see this taken further.

A few minor comments in some of the individual patches.

>  drivers/media/pci/cobalt/cobalt-driver.c   | 11 ++++++-----
>  drivers/net/ethernet/intel/igb/e1000_mac.c |  6 +++---
>  drivers/pci/controller/dwc/pcie-tegra194.c |  9 ++++-----
>  drivers/pci/controller/pci-mvebu.c         |  2 +-
>  drivers/pci/pci-sysfs.c                    |  5 ++---
>  drivers/pci/pci.c                          |  6 +++---
>  drivers/scsi/esas2r/esas2r_ioctl.c         |  8 ++++----
>  drivers/scsi/qla2xxx/qla_os.c              |  3 ++-
>  9 files changed, 28 insertions(+), 27 deletions(-)
>