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

[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

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

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: