[PATCH 00/12] PCI/VPD: Convert more users to the new VPD API functions

[PATCH 00/12] PCI/VPD: Convert more users to the new VPD API functions

[Heiner Kallweit]

This series converts more users to the new VPD API functions.

bnx2 patches have been tested with a BCM5709 card.
The other patches are compile-tested, except cxlflash.

Heiner Kallweit (12):
  sfc: falcon: Read VPD with pci_vpd_alloc()
  sfc: falcon: Search VPD with pci_vpd_find_ro_info_keyword()
  bnx2: Search VPD with pci_vpd_find_ro_info_keyword()
  bnx2: Replace open-coded version with swab32s()
  bnx2x: Read VPD with pci_vpd_alloc()
  bnx2x: Search VPD with pci_vpd_find_ro_info_keyword()
  bnxt: Read VPD with pci_vpd_alloc()
  bnxt: Search VPD with pci_vpd_find_ro_info_keyword()
  cxgb4: Validate VPD checksum with pci_vpd_check_csum()
  cxgb4: Remove unused vpd_param member ec
  cxgb4: Search VPD with pci_vpd_find_ro_info_keyword()
  scsi: cxlflash: Search VPD with pci_vpd_find_ro_info_keyword()

 drivers/net/ethernet/broadcom/bnx2.c          | 46 +++-------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h   |  1 -
 .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 91 ++++---------------
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 49 +++-------
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h    |  2 -
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c    | 76 ++++++----------
 drivers/net/ethernet/sfc/falcon/efx.c         | 79 ++++------------
 drivers/scsi/cxlflash/main.c                  | 34 ++-----
 8 files changed, 98 insertions(+), 280 deletions(-)

-- 
2.33.0

[PATCH 01/12] sfc: falcon: Read VPD with pci_vpd_alloc()

[Heiner Kallweit]

This is the same as 5119e20facfa "sfc: Read VPD with pci_vpd_alloc()",
just for the falcon chip version.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/sfc/falcon/efx.c | 30 +++++++++++++--------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
index c177ea0f3..5ab1e863d 100644
--- a/drivers/net/ethernet/sfc/falcon/efx.c
+++ b/drivers/net/ethernet/sfc/falcon/efx.c
@@ -2780,22 +2780,18 @@ static void ef4_pci_remove(struct pci_dev *pci_dev)
 };
 
 /* NIC VPD information
- * Called during probe to display the part number of the
- * installed NIC.  VPD is potentially very large but this should
- * always appear within the first 512 bytes.
+ * Called during probe to display the part number of the installed NIC.
  */
-#define SFC_VPD_LEN 512
 static void ef4_probe_vpd_strings(struct ef4_nic *efx)
 {
 	struct pci_dev *dev = efx->pci_dev;
-	char vpd_data[SFC_VPD_LEN];
-	ssize_t vpd_size;
 	int ro_start, ro_size, i, j;
+	unsigned int vpd_size;
+	u8 *vpd_data;
 
-	/* Get the vpd data from the device */
-	vpd_size = pci_read_vpd(dev, 0, sizeof(vpd_data), vpd_data);
-	if (vpd_size <= 0) {
-		netif_err(efx, drv, efx->net_dev, "Unable to read VPD\n");
+	vpd_data = pci_vpd_alloc(dev, &vpd_size);
+	if (IS_ERR(vpd_data)) {
+		pci_warn(dev, "Unable to read VPD\n");
 		return;
 	}
 
@@ -2803,7 +2799,7 @@ static void ef4_probe_vpd_strings(struct ef4_nic *efx)
 	ro_start = pci_vpd_find_tag(vpd_data, vpd_size, PCI_VPD_LRDT_RO_DATA);
 	if (ro_start < 0) {
 		netif_err(efx, drv, efx->net_dev, "VPD Read-only not found\n");
-		return;
+		goto out;
 	}
 
 	ro_size = pci_vpd_lrdt_size(&vpd_data[ro_start]);
@@ -2816,14 +2812,14 @@ static void ef4_probe_vpd_strings(struct ef4_nic *efx)
 	i = pci_vpd_find_info_keyword(vpd_data, i, j, "PN");
 	if (i < 0) {
 		netif_err(efx, drv, efx->net_dev, "Part number not found\n");
-		return;
+		goto out;
 	}
 
 	j = pci_vpd_info_field_size(&vpd_data[i]);
 	i += PCI_VPD_INFO_FLD_HDR_SIZE;
 	if (i + j > vpd_size) {
 		netif_err(efx, drv, efx->net_dev, "Incomplete part number\n");
-		return;
+		goto out;
 	}
 
 	netif_info(efx, drv, efx->net_dev,
@@ -2834,21 +2830,23 @@ static void ef4_probe_vpd_strings(struct ef4_nic *efx)
 	i = pci_vpd_find_info_keyword(vpd_data, i, j, "SN");
 	if (i < 0) {
 		netif_err(efx, drv, efx->net_dev, "Serial number not found\n");
-		return;
+		goto out;
 	}
 
 	j = pci_vpd_info_field_size(&vpd_data[i]);
 	i += PCI_VPD_INFO_FLD_HDR_SIZE;
 	if (i + j > vpd_size) {
 		netif_err(efx, drv, efx->net_dev, "Incomplete serial number\n");
-		return;
+		goto out;
 	}
 
 	efx->vpd_sn = kmalloc(j + 1, GFP_KERNEL);
 	if (!efx->vpd_sn)
-		return;
+		goto out;
 
 	snprintf(efx->vpd_sn, j + 1, "%s", &vpd_data[i]);
+out:
+	kfree(vpd_data);
 }
 
 
-- 
2.33.0

Re: [PATCH 01/12] sfc: falcon: Read VPD with pci_vpd_alloc()

[kernel test robot]

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

Hi Heiner,

I love your patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on pci/next mkp-scsi/for-next linus/master v5.14-rc6 next-20210820]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heiner-Kallweit/PCI-VPD-Convert-more-users-to-the-new-VPD-API-functions/20210822-220229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f04e3b53e818526cc8b869af3804e375c0a48abf
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heiner-Kallweit/PCI-VPD-Convert-more-users-to-the-new-VPD-API-functions/20210822-220229
        git checkout f04e3b53e818526cc8b869af3804e375c0a48abf
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/sfc/falcon/efx.c: In function 'ef4_probe_vpd_strings':
   drivers/net/ethernet/sfc/falcon/efx.c:2792:20: error: implicit declaration of function 'pci_vpd_alloc'; did you mean 'pci_pool_alloc'? [-Werror=implicit-function-declaration]
    2792 |         vpd_data = pci_vpd_alloc(dev, &vpd_size);
         |                    ^~~~~~~~~~~~~
         |                    pci_pool_alloc
>> drivers/net/ethernet/sfc/falcon/efx.c:2792:18: warning: assignment to 'u8 *' {aka 'unsigned char *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
    2792 |         vpd_data = pci_vpd_alloc(dev, &vpd_size);
         |                  ^
   cc1: some warnings being treated as errors


vim +2792 drivers/net/ethernet/sfc/falcon/efx.c

  2781	
  2782	/* NIC VPD information
  2783	 * Called during probe to display the part number of the installed NIC.
  2784	 */
  2785	static void ef4_probe_vpd_strings(struct ef4_nic *efx)
  2786	{
  2787		struct pci_dev *dev = efx->pci_dev;
  2788		int ro_start, ro_size, i, j;
  2789		unsigned int vpd_size;
  2790		u8 *vpd_data;
  2791	
> 2792		vpd_data = pci_vpd_alloc(dev, &vpd_size);
  2793		if (IS_ERR(vpd_data)) {
  2794			pci_warn(dev, "Unable to read VPD\n");
  2795			return;
  2796		}
  2797	
  2798		/* Get the Read only section */
  2799		ro_start = pci_vpd_find_tag(vpd_data, vpd_size, PCI_VPD_LRDT_RO_DATA);
  2800		if (ro_start < 0) {
  2801			netif_err(efx, drv, efx->net_dev, "VPD Read-only not found\n");
  2802			goto out;
  2803		}
  2804	
  2805		ro_size = pci_vpd_lrdt_size(&vpd_data[ro_start]);
  2806		j = ro_size;
  2807		i = ro_start + PCI_VPD_LRDT_TAG_SIZE;
  2808		if (i + j > vpd_size)
  2809			j = vpd_size - i;
  2810	
  2811		/* Get the Part number */
  2812		i = pci_vpd_find_info_keyword(vpd_data, i, j, "PN");
  2813		if (i < 0) {
  2814			netif_err(efx, drv, efx->net_dev, "Part number not found\n");
  2815			goto out;
  2816		}
  2817	
  2818		j = pci_vpd_info_field_size(&vpd_data[i]);
  2819		i += PCI_VPD_INFO_FLD_HDR_SIZE;
  2820		if (i + j > vpd_size) {
  2821			netif_err(efx, drv, efx->net_dev, "Incomplete part number\n");
  2822			goto out;
  2823		}
  2824	
  2825		netif_info(efx, drv, efx->net_dev,
  2826			   "Part Number : %.*s\n", j, &vpd_data[i]);
  2827	
  2828		i = ro_start + PCI_VPD_LRDT_TAG_SIZE;
  2829		j = ro_size;
  2830		i = pci_vpd_find_info_keyword(vpd_data, i, j, "SN");
  2831		if (i < 0) {
  2832			netif_err(efx, drv, efx->net_dev, "Serial number not found\n");
  2833			goto out;
  2834		}
  2835	
  2836		j = pci_vpd_info_field_size(&vpd_data[i]);
  2837		i += PCI_VPD_INFO_FLD_HDR_SIZE;
  2838		if (i + j > vpd_size) {
  2839			netif_err(efx, drv, efx->net_dev, "Incomplete serial number\n");
  2840			goto out;
  2841		}
  2842	
  2843		efx->vpd_sn = kmalloc(j + 1, GFP_KERNEL);
  2844		if (!efx->vpd_sn)
  2845			goto out;
  2846	
  2847		snprintf(efx->vpd_sn, j + 1, "%s", &vpd_data[i]);
  2848	out:
  2849		kfree(vpd_data);
  2850	}
  2851	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68217 bytes --]

[PATCH 02/12] sfc: falcon: Search VPD with pci_vpd_find_ro_info_keyword()

[Heiner Kallweit]

This is the same as 37838aa437c7 "sfc: Search VPD with
pci_vpd_find_ro_info_keyword()", just for the falcon chip version.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/sfc/falcon/efx.c | 65 ++++++---------------------
 1 file changed, 14 insertions(+), 51 deletions(-)

diff --git a/drivers/net/ethernet/sfc/falcon/efx.c b/drivers/net/ethernet/sfc/falcon/efx.c
index 5ab1e863d..423bdf812 100644
--- a/drivers/net/ethernet/sfc/falcon/efx.c
+++ b/drivers/net/ethernet/sfc/falcon/efx.c
@@ -2785,9 +2785,9 @@ static void ef4_pci_remove(struct pci_dev *pci_dev)
 static void ef4_probe_vpd_strings(struct ef4_nic *efx)
 {
 	struct pci_dev *dev = efx->pci_dev;
-	int ro_start, ro_size, i, j;
-	unsigned int vpd_size;
+	unsigned int vpd_size, kw_len;
 	u8 *vpd_data;
+	int start;
 
 	vpd_data = pci_vpd_alloc(dev, &vpd_size);
 	if (IS_ERR(vpd_data)) {
@@ -2795,57 +2795,20 @@ static void ef4_probe_vpd_strings(struct ef4_nic *efx)
 		return;
 	}
 
-	/* Get the Read only section */
-	ro_start = pci_vpd_find_tag(vpd_data, vpd_size, PCI_VPD_LRDT_RO_DATA);
-	if (ro_start < 0) {
-		netif_err(efx, drv, efx->net_dev, "VPD Read-only not found\n");
-		goto out;
-	}
-
-	ro_size = pci_vpd_lrdt_size(&vpd_data[ro_start]);
-	j = ro_size;
-	i = ro_start + PCI_VPD_LRDT_TAG_SIZE;
-	if (i + j > vpd_size)
-		j = vpd_size - i;
-
-	/* Get the Part number */
-	i = pci_vpd_find_info_keyword(vpd_data, i, j, "PN");
-	if (i < 0) {
-		netif_err(efx, drv, efx->net_dev, "Part number not found\n");
-		goto out;
-	}
-
-	j = pci_vpd_info_field_size(&vpd_data[i]);
-	i += PCI_VPD_INFO_FLD_HDR_SIZE;
-	if (i + j > vpd_size) {
-		netif_err(efx, drv, efx->net_dev, "Incomplete part number\n");
-		goto out;
-	}
-
-	netif_info(efx, drv, efx->net_dev,
-		   "Part Number : %.*s\n", j, &vpd_data[i]);
-
-	i = ro_start + PCI_VPD_LRDT_TAG_SIZE;
-	j = ro_size;
-	i = pci_vpd_find_info_keyword(vpd_data, i, j, "SN");
-	if (i < 0) {
-		netif_err(efx, drv, efx->net_dev, "Serial number not found\n");
-		goto out;
-	}
-
-	j = pci_vpd_info_field_size(&vpd_data[i]);
-	i += PCI_VPD_INFO_FLD_HDR_SIZE;
-	if (i + j > vpd_size) {
-		netif_err(efx, drv, efx->net_dev, "Incomplete serial number\n");
-		goto out;
-	}
+	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
+					     PCI_VPD_RO_KEYWORD_PARTNO, &kw_len);
+	if (start < 0)
+		pci_warn(dev, "Part number not found or incomplete\n");
+	else
+		pci_info(dev, "Part Number : %.*s\n", kw_len, vpd_data + start);
 
-	efx->vpd_sn = kmalloc(j + 1, GFP_KERNEL);
-	if (!efx->vpd_sn)
-		goto out;
+	start = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
+					     PCI_VPD_RO_KEYWORD_SERIALNO, &kw_len);
+	if (start < 0)
+		pci_warn(dev, "Serial number not found or incomplete\n");
+	else
+		efx->vpd_sn = kmemdup_nul(vpd_data + start, kw_len, GFP_KERNEL);
 
-	snprintf(efx->vpd_sn, j + 1, "%s", &vpd_data[i]);
-out:
 	kfree(vpd_data);
 }
 
-- 
2.33.0

[PATCH 03/12] bnx2: Search VPD with pci_vpd_find_ro_info_keyword()

[Heiner Kallweit]

Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/broadcom/bnx2.c | 33 +++++++---------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index 89ee1c0e9..de1a60a95 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -8033,9 +8033,9 @@ bnx2_get_pci_speed(struct bnx2 *bp)
 static void
 bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 {
+	unsigned int len;
 	int rc, i, j;
 	u8 *data;
-	unsigned int block_end, rosize, len;
 
 #define BNX2_VPD_NVRAM_OFFSET	0x300
 #define BNX2_VPD_LEN		128
@@ -8057,38 +8057,21 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 		data[i + 3] = data[i + BNX2_VPD_LEN];
 	}
 
-	i = pci_vpd_find_tag(data, BNX2_VPD_LEN, PCI_VPD_LRDT_RO_DATA);
-	if (i < 0)
-		goto vpd_done;
-
-	rosize = pci_vpd_lrdt_size(&data[i]);
-	i += PCI_VPD_LRDT_TAG_SIZE;
-	block_end = i + rosize;
-
-	if (block_end > BNX2_VPD_LEN)
-		goto vpd_done;
-
-	j = pci_vpd_find_info_keyword(data, i, rosize,
-				      PCI_VPD_RO_KEYWORD_MFR_ID);
+	j = pci_vpd_find_ro_info_keyword(data, BNX2_VPD_LEN,
+					 PCI_VPD_RO_KEYWORD_MFR_ID, &len);
 	if (j < 0)
 		goto vpd_done;
 
-	len = pci_vpd_info_field_size(&data[j]);
-
-	j += PCI_VPD_INFO_FLD_HDR_SIZE;
-	if (j + len > block_end || len != 4 ||
-	    memcmp(&data[j], "1028", 4))
+	if (len != 4 || memcmp(&data[j], "1028", 4))
 		goto vpd_done;
 
-	j = pci_vpd_find_info_keyword(data, i, rosize,
-				      PCI_VPD_RO_KEYWORD_VENDOR0);
+	j = pci_vpd_find_ro_info_keyword(data, BNX2_VPD_LEN,
+					 PCI_VPD_RO_KEYWORD_VENDOR0,
+					 &len);
 	if (j < 0)
 		goto vpd_done;
 
-	len = pci_vpd_info_field_size(&data[j]);
-
-	j += PCI_VPD_INFO_FLD_HDR_SIZE;
-	if (j + len > block_end || len > BNX2_MAX_VER_SLEN)
+	if (len > BNX2_MAX_VER_SLEN)
 		goto vpd_done;
 
 	memcpy(bp->fw_version, &data[j], len);
-- 
2.33.0

[PATCH 04/12] bnx2: Replace open-coded version with swab32s()

[Heiner Kallweit]

Use swab32s() instead of open-coding it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/broadcom/bnx2.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2.c b/drivers/net/ethernet/broadcom/bnx2.c
index de1a60a95..599fc1436 100644
--- a/drivers/net/ethernet/broadcom/bnx2.c
+++ b/drivers/net/ethernet/broadcom/bnx2.c
@@ -8041,21 +8041,16 @@ bnx2_read_vpd_fw_ver(struct bnx2 *bp)
 #define BNX2_VPD_LEN		128
 #define BNX2_MAX_VER_SLEN	30
 
-	data = kmalloc(256, GFP_KERNEL);
+	data = kmalloc(BNX2_VPD_LEN, GFP_KERNEL);
 	if (!data)
 		return;
 
-	rc = bnx2_nvram_read(bp, BNX2_VPD_NVRAM_OFFSET, data + BNX2_VPD_LEN,
-			     BNX2_VPD_LEN);
+	rc = bnx2_nvram_read(bp, BNX2_VPD_NVRAM_OFFSET, data, BNX2_VPD_LEN);
 	if (rc)
 		goto vpd_done;
 
-	for (i = 0; i < BNX2_VPD_LEN; i += 4) {
-		data[i] = data[i + BNX2_VPD_LEN + 3];
-		data[i + 1] = data[i + BNX2_VPD_LEN + 2];
-		data[i + 2] = data[i + BNX2_VPD_LEN + 1];
-		data[i + 3] = data[i + BNX2_VPD_LEN];
-	}
+	for (i = 0; i < BNX2_VPD_LEN; i += 4)
+		swab32s((u32 *)&data[i]);
 
 	j = pci_vpd_find_ro_info_keyword(data, BNX2_VPD_LEN,
 					 PCI_VPD_RO_KEYWORD_MFR_ID, &len);
-- 
2.33.0

[PATCH 05/12] bnx2x: Read VPD with pci_vpd_alloc()

[Heiner Kallweit]

Use pci_vpd_alloc() to dynamically allocate a properly sized buffer and
read the full VPD data into it.

This simplifies the code, and we no longer have to make assumptions about
VPD size.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h   |  1 -
 .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 44 +++++--------------
 2 files changed, 10 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index d04994840..e789430f4 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -2407,7 +2407,6 @@ void bnx2x_igu_clear_sb_gen(struct bnx2x *bp, u8 func, u8 idu_sb_id,
 #define ETH_MAX_RX_CLIENTS_E2		ETH_MAX_RX_CLIENTS_E1H
 #endif
 
-#define BNX2X_VPD_LEN			128
 #define VENDOR_ID_LEN			4
 
 #define VF_ACQUIRE_THRESH		3
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 6d9813491..0466adf8d 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12189,50 +12189,29 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)
 
 static void bnx2x_read_fwinfo(struct bnx2x *bp)
 {
-	int cnt, i, block_end, rodi;
-	char vpd_start[BNX2X_VPD_LEN+1];
+	int i, block_end, rodi;
 	char str_id_reg[VENDOR_ID_LEN+1];
 	char str_id_cap[VENDOR_ID_LEN+1];
-	char *vpd_data;
-	char *vpd_extended_data = NULL;
-	u8 len;
+	unsigned int vpd_len;
+	u8 *vpd_data, len;
 
-	cnt = pci_read_vpd(bp->pdev, 0, BNX2X_VPD_LEN, vpd_start);
 	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
 
-	if (cnt < BNX2X_VPD_LEN)
-		goto out_not_found;
+	vpd_data = pci_vpd_alloc(bp->pdev, &vpd_len);
+	if (IS_ERR(vpd_data))
+		return;
 
 	/* VPD RO tag should be first tag after identifier string, hence
 	 * we should be able to find it in first BNX2X_VPD_LEN chars
 	 */
-	i = pci_vpd_find_tag(vpd_start, BNX2X_VPD_LEN, PCI_VPD_LRDT_RO_DATA);
+	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
 	if (i < 0)
 		goto out_not_found;
 
 	block_end = i + PCI_VPD_LRDT_TAG_SIZE +
-		    pci_vpd_lrdt_size(&vpd_start[i]);
-
+		    pci_vpd_lrdt_size(&vpd_data[i]);
 	i += PCI_VPD_LRDT_TAG_SIZE;
 
-	if (block_end > BNX2X_VPD_LEN) {
-		vpd_extended_data = kmalloc(block_end, GFP_KERNEL);
-		if (vpd_extended_data  == NULL)
-			goto out_not_found;
-
-		/* read rest of vpd image into vpd_extended_data */
-		memcpy(vpd_extended_data, vpd_start, BNX2X_VPD_LEN);
-		cnt = pci_read_vpd(bp->pdev, BNX2X_VPD_LEN,
-				   block_end - BNX2X_VPD_LEN,
-				   vpd_extended_data + BNX2X_VPD_LEN);
-		if (cnt < (block_end - BNX2X_VPD_LEN))
-			goto out_not_found;
-		vpd_data = vpd_extended_data;
-	} else
-		vpd_data = vpd_start;
-
-	/* now vpd_data holds full vpd content in both cases */
-
 	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
 				   PCI_VPD_RO_KEYWORD_MFR_ID);
 	if (rodi < 0)
@@ -12258,17 +12237,14 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)
 
 			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
 
-			if (len < 32 && (len + rodi) <= BNX2X_VPD_LEN) {
+			if (len < 32 && (len + rodi) <= vpd_len) {
 				memcpy(bp->fw_ver, &vpd_data[rodi], len);
 				bp->fw_ver[len] = ' ';
 			}
 		}
-		kfree(vpd_extended_data);
-		return;
 	}
 out_not_found:
-	kfree(vpd_extended_data);
-	return;
+	kfree(vpd_data);
 }
 
 static void bnx2x_set_modes_bitmap(struct bnx2x *bp)
-- 
2.33.0

Re: [PATCH 05/12] bnx2x: Read VPD with pci_vpd_alloc()

[kernel test robot]

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

Hi Heiner,

I love your patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on pci/next mkp-scsi/for-next linus/master v5.14-rc6 next-20210820]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heiner-Kallweit/PCI-VPD-Convert-more-users-to-the-new-VPD-API-functions/20210822-220229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/11d3b0532e225fec84b84c082ff913ab35cecd29
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heiner-Kallweit/PCI-VPD-Convert-more-users-to-the-new-VPD-API-functions/20210822-220229
        git checkout 11d3b0532e225fec84b84c082ff913ab35cecd29
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c: In function 'bnx2x_read_fwinfo':
   drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12200:20: error: implicit declaration of function 'pci_vpd_alloc'; did you mean 'pci_pool_alloc'? [-Werror=implicit-function-declaration]
   12200 |         vpd_data = pci_vpd_alloc(bp->pdev, &vpd_len);
         |                    ^~~~~~~~~~~~~
         |                    pci_pool_alloc
>> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12200:18: warning: assignment to 'u8 *' {aka 'unsigned char *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
   12200 |         vpd_data = pci_vpd_alloc(bp->pdev, &vpd_len);
         |                  ^
   cc1: some warnings being treated as errors


vim +12200 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c

 12189	
 12190	static void bnx2x_read_fwinfo(struct bnx2x *bp)
 12191	{
 12192		int i, block_end, rodi;
 12193		char str_id_reg[VENDOR_ID_LEN+1];
 12194		char str_id_cap[VENDOR_ID_LEN+1];
 12195		unsigned int vpd_len;
 12196		u8 *vpd_data, len;
 12197	
 12198		memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
 12199	
 12200		vpd_data = pci_vpd_alloc(bp->pdev, &vpd_len);
 12201		if (IS_ERR(vpd_data))
 12202			return;
 12203	
 12204		/* VPD RO tag should be first tag after identifier string, hence
 12205		 * we should be able to find it in first BNX2X_VPD_LEN chars
 12206		 */
 12207		i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
 12208		if (i < 0)
 12209			goto out_not_found;
 12210	
 12211		block_end = i + PCI_VPD_LRDT_TAG_SIZE +
 12212			    pci_vpd_lrdt_size(&vpd_data[i]);
 12213		i += PCI_VPD_LRDT_TAG_SIZE;
 12214	
 12215		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
 12216					   PCI_VPD_RO_KEYWORD_MFR_ID);
 12217		if (rodi < 0)
 12218			goto out_not_found;
 12219	
 12220		len = pci_vpd_info_field_size(&vpd_data[rodi]);
 12221	
 12222		if (len != VENDOR_ID_LEN)
 12223			goto out_not_found;
 12224	
 12225		rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
 12226	
 12227		/* vendor specific info */
 12228		snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);
 12229		snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
 12230		if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||
 12231		    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {
 12232	
 12233			rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
 12234							PCI_VPD_RO_KEYWORD_VENDOR0);
 12235			if (rodi >= 0) {
 12236				len = pci_vpd_info_field_size(&vpd_data[rodi]);
 12237	
 12238				rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
 12239	
 12240				if (len < 32 && (len + rodi) <= vpd_len) {
 12241					memcpy(bp->fw_ver, &vpd_data[rodi], len);
 12242					bp->fw_ver[len] = ' ';
 12243				}
 12244			}
 12245		}
 12246	out_not_found:
 12247		kfree(vpd_data);
 12248	}
 12249	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68217 bytes --]

[PATCH 06/12] bnx2x: Search VPD with pci_vpd_find_ro_info_keyword()

[Heiner Kallweit]

Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
simplify the code.

str_id_reg and str_id_cap hold the same string and are used in the same
comparison. This doesn't make sense, use one string str_id instead.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 57 +++++--------------
 1 file changed, 15 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 0466adf8d..2c7bfc416 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12189,11 +12189,10 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)
 
 static void bnx2x_read_fwinfo(struct bnx2x *bp)
 {
-	int i, block_end, rodi;
-	char str_id_reg[VENDOR_ID_LEN+1];
-	char str_id_cap[VENDOR_ID_LEN+1];
-	unsigned int vpd_len;
-	u8 *vpd_data, len;
+	char str_id[VENDOR_ID_LEN + 1];
+	unsigned int vpd_len, kw_len;
+	u8 *vpd_data;
+	int rodi;
 
 	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
 
@@ -12201,46 +12200,20 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)
 	if (IS_ERR(vpd_data))
 		return;
 
-	/* VPD RO tag should be first tag after identifier string, hence
-	 * we should be able to find it in first BNX2X_VPD_LEN chars
-	 */
-	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
-	if (i < 0)
-		goto out_not_found;
-
-	block_end = i + PCI_VPD_LRDT_TAG_SIZE +
-		    pci_vpd_lrdt_size(&vpd_data[i]);
-	i += PCI_VPD_LRDT_TAG_SIZE;
-
-	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
-				   PCI_VPD_RO_KEYWORD_MFR_ID);
-	if (rodi < 0)
-		goto out_not_found;
-
-	len = pci_vpd_info_field_size(&vpd_data[rodi]);
-
-	if (len != VENDOR_ID_LEN)
+	rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
+					    PCI_VPD_RO_KEYWORD_MFR_ID, &kw_len);
+	if (rodi < 0 || kw_len != VENDOR_ID_LEN)
 		goto out_not_found;
 
-	rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
-
 	/* vendor specific info */
-	snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);
-	snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
-	if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||
-	    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {
-
-		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
-						PCI_VPD_RO_KEYWORD_VENDOR0);
-		if (rodi >= 0) {
-			len = pci_vpd_info_field_size(&vpd_data[rodi]);
-
-			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
-
-			if (len < 32 && (len + rodi) <= vpd_len) {
-				memcpy(bp->fw_ver, &vpd_data[rodi], len);
-				bp->fw_ver[len] = ' ';
-			}
+	snprintf(str_id, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
+	if (!strncmp(str_id, &vpd_data[rodi], VENDOR_ID_LEN)) {
+		rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
+						    PCI_VPD_RO_KEYWORD_VENDOR0,
+						    &kw_len);
+		if (rodi >= 0 && kw_len < sizeof(bp->fw_ver)) {
+			memcpy(bp->fw_ver, &vpd_data[rodi], kw_len);
+			bp->fw_ver[kw_len] = ' ';
 		}
 	}
 out_not_found:
-- 
2.33.0

Re: [PATCH 06/12] bnx2x: Search VPD with pci_vpd_find_ro_info_keyword()

[Bjorn Helgaas]

On Sun, Aug 22, 2021 at 03:54:23PM +0200, Heiner Kallweit wrote:
> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
> simplify the code.
> 
> str_id_reg and str_id_cap hold the same string and are used in the same
> comparison. This doesn't make sense, use one string str_id instead.

str_id_reg is printed with "%04x" (lower-case hex letters) and
str_id_cap with "%04X" (upper-case hex letters), so the previous code
would match either 0xabcd or 0xABCD.  After this patch, we'd match
only the latter.

PCI_VENDOR_ID_DELL is 0x1028, so it shouldn't make any difference,
which makes me wonder why somebody bothered with both.

But it does seem like a potential landmine to change the case
sensitivity.  Maybe strncasecmp() instead?

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 57 +++++--------------
>  1 file changed, 15 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index 0466adf8d..2c7bfc416 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -12189,11 +12189,10 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)
>  
>  static void bnx2x_read_fwinfo(struct bnx2x *bp)
>  {
> -	int i, block_end, rodi;
> -	char str_id_reg[VENDOR_ID_LEN+1];
> -	char str_id_cap[VENDOR_ID_LEN+1];
> -	unsigned int vpd_len;
> -	u8 *vpd_data, len;
> +	char str_id[VENDOR_ID_LEN + 1];
> +	unsigned int vpd_len, kw_len;
> +	u8 *vpd_data;
> +	int rodi;
>  
>  	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
>  
> @@ -12201,46 +12200,20 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)
>  	if (IS_ERR(vpd_data))
>  		return;
>  
> -	/* VPD RO tag should be first tag after identifier string, hence
> -	 * we should be able to find it in first BNX2X_VPD_LEN chars
> -	 */
> -	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
> -	if (i < 0)
> -		goto out_not_found;
> -
> -	block_end = i + PCI_VPD_LRDT_TAG_SIZE +
> -		    pci_vpd_lrdt_size(&vpd_data[i]);
> -	i += PCI_VPD_LRDT_TAG_SIZE;
> -
> -	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
> -				   PCI_VPD_RO_KEYWORD_MFR_ID);
> -	if (rodi < 0)
> -		goto out_not_found;
> -
> -	len = pci_vpd_info_field_size(&vpd_data[rodi]);
> -
> -	if (len != VENDOR_ID_LEN)
> +	rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
> +					    PCI_VPD_RO_KEYWORD_MFR_ID, &kw_len);
> +	if (rodi < 0 || kw_len != VENDOR_ID_LEN)
>  		goto out_not_found;
>  
> -	rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
> -
>  	/* vendor specific info */
> -	snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);
> -	snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
> -	if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||
> -	    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {
> -
> -		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
> -						PCI_VPD_RO_KEYWORD_VENDOR0);
> -		if (rodi >= 0) {
> -			len = pci_vpd_info_field_size(&vpd_data[rodi]);
> -
> -			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
> -
> -			if (len < 32 && (len + rodi) <= vpd_len) {
> -				memcpy(bp->fw_ver, &vpd_data[rodi], len);
> -				bp->fw_ver[len] = ' ';
> -			}
> +	snprintf(str_id, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
> +	if (!strncmp(str_id, &vpd_data[rodi], VENDOR_ID_LEN)) {
> +		rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
> +						    PCI_VPD_RO_KEYWORD_VENDOR0,
> +						    &kw_len);
> +		if (rodi >= 0 && kw_len < sizeof(bp->fw_ver)) {
> +			memcpy(bp->fw_ver, &vpd_data[rodi], kw_len);
> +			bp->fw_ver[kw_len] = ' ';
>  		}
>  	}
>  out_not_found:
> -- 
> 2.33.0
> 
> 

Re: [PATCH 06/12] bnx2x: Search VPD with pci_vpd_find_ro_info_keyword()

[Heiner Kallweit]

On 24.08.2021 19:02, Bjorn Helgaas wrote:
> On Sun, Aug 22, 2021 at 03:54:23PM +0200, Heiner Kallweit wrote:
>> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
>> simplify the code.
>>
>> str_id_reg and str_id_cap hold the same string and are used in the same
>> comparison. This doesn't make sense, use one string str_id instead.
> 
> str_id_reg is printed with "%04x" (lower-case hex letters) and
> str_id_cap with "%04X" (upper-case hex letters), so the previous code
> would match either 0xabcd or 0xABCD.  After this patch, we'd match
> only the latter.
> 
Right, I missed this difference. strncasecmp() would be an easy solution.
Alternatively we could avoid this stringification and string comparison
by using kstrtouint_from_user():

kstrtouint_from_user(&vpd_data[rodi], kw_len, 16, &val)
if (val == PCI_VENDOR_ID_DELL)

But if there's no strong preference then I'd say we go the easy way.
Would you like me to re-send or are you going to adjust the patch?

> PCI_VENDOR_ID_DELL is 0x1028, so it shouldn't make any difference,
> which makes me wonder why somebody bothered with both.
> 
> But it does seem like a potential landmine to change the case
> sensitivity.  Maybe strncasecmp() instead?
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 57 +++++--------------
>>  1 file changed, 15 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> index 0466adf8d..2c7bfc416 100644
>> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> @@ -12189,11 +12189,10 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)
>>  
>>  static void bnx2x_read_fwinfo(struct bnx2x *bp)
>>  {
>> -	int i, block_end, rodi;
>> -	char str_id_reg[VENDOR_ID_LEN+1];
>> -	char str_id_cap[VENDOR_ID_LEN+1];
>> -	unsigned int vpd_len;
>> -	u8 *vpd_data, len;
>> +	char str_id[VENDOR_ID_LEN + 1];
>> +	unsigned int vpd_len, kw_len;
>> +	u8 *vpd_data;
>> +	int rodi;
>>  
>>  	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
>>  
>> @@ -12201,46 +12200,20 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)
>>  	if (IS_ERR(vpd_data))
>>  		return;
>>  
>> -	/* VPD RO tag should be first tag after identifier string, hence
>> -	 * we should be able to find it in first BNX2X_VPD_LEN chars
>> -	 */
>> -	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
>> -	if (i < 0)
>> -		goto out_not_found;
>> -
>> -	block_end = i + PCI_VPD_LRDT_TAG_SIZE +
>> -		    pci_vpd_lrdt_size(&vpd_data[i]);
>> -	i += PCI_VPD_LRDT_TAG_SIZE;
>> -
>> -	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
>> -				   PCI_VPD_RO_KEYWORD_MFR_ID);
>> -	if (rodi < 0)
>> -		goto out_not_found;
>> -
>> -	len = pci_vpd_info_field_size(&vpd_data[rodi]);
>> -
>> -	if (len != VENDOR_ID_LEN)
>> +	rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
>> +					    PCI_VPD_RO_KEYWORD_MFR_ID, &kw_len);
>> +	if (rodi < 0 || kw_len != VENDOR_ID_LEN)
>>  		goto out_not_found;
>>  
>> -	rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
>> -
>>  	/* vendor specific info */
>> -	snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);
>> -	snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
>> -	if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||
>> -	    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {
>> -
>> -		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
>> -						PCI_VPD_RO_KEYWORD_VENDOR0);
>> -		if (rodi >= 0) {
>> -			len = pci_vpd_info_field_size(&vpd_data[rodi]);
>> -
>> -			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
>> -
>> -			if (len < 32 && (len + rodi) <= vpd_len) {
>> -				memcpy(bp->fw_ver, &vpd_data[rodi], len);
>> -				bp->fw_ver[len] = ' ';
>> -			}
>> +	snprintf(str_id, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
>> +	if (!strncmp(str_id, &vpd_data[rodi], VENDOR_ID_LEN)) {
>> +		rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
>> +						    PCI_VPD_RO_KEYWORD_VENDOR0,
>> +						    &kw_len);
>> +		if (rodi >= 0 && kw_len < sizeof(bp->fw_ver)) {
>> +			memcpy(bp->fw_ver, &vpd_data[rodi], kw_len);
>> +			bp->fw_ver[kw_len] = ' ';
>>  		}
>>  	}
>>  out_not_found:
>> -- 
>> 2.33.0
>>
>>

Re: [PATCH 06/12] bnx2x: Search VPD with pci_vpd_find_ro_info_keyword()

[Bjorn Helgaas]

On Tue, Aug 24, 2021 at 08:01:34PM +0200, Heiner Kallweit wrote:
> On 24.08.2021 19:02, Bjorn Helgaas wrote:
> > On Sun, Aug 22, 2021 at 03:54:23PM +0200, Heiner Kallweit wrote:
> >> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
> >> simplify the code.
> >>
> >> str_id_reg and str_id_cap hold the same string and are used in the same
> >> comparison. This doesn't make sense, use one string str_id instead.
> > 
> > str_id_reg is printed with "%04x" (lower-case hex letters) and
> > str_id_cap with "%04X" (upper-case hex letters), so the previous code
> > would match either 0xabcd or 0xABCD.  After this patch, we'd match
> > only the latter.
> > 
> Right, I missed this difference. strncasecmp() would be an easy solution.
> Alternatively we could avoid this stringification and string comparison
> by using kstrtouint_from_user():
> 
> kstrtouint_from_user(&vpd_data[rodi], kw_len, 16, &val)
> if (val == PCI_VENDOR_ID_DELL)
> 
> But if there's no strong preference then I'd say we go the easy way.
> Would you like me to re-send or are you going to adjust the patch?

I adjusted it, thanks!

> > PCI_VENDOR_ID_DELL is 0x1028, so it shouldn't make any difference,
> > which makes me wonder why somebody bothered with both.
> > 
> > But it does seem like a potential landmine to change the case
> > sensitivity.  Maybe strncasecmp() instead?
> > 
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 57 +++++--------------
> >>  1 file changed, 15 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> >> index 0466adf8d..2c7bfc416 100644
> >> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> >> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> >> @@ -12189,11 +12189,10 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)
> >>  
> >>  static void bnx2x_read_fwinfo(struct bnx2x *bp)
> >>  {
> >> -	int i, block_end, rodi;
> >> -	char str_id_reg[VENDOR_ID_LEN+1];
> >> -	char str_id_cap[VENDOR_ID_LEN+1];
> >> -	unsigned int vpd_len;
> >> -	u8 *vpd_data, len;
> >> +	char str_id[VENDOR_ID_LEN + 1];
> >> +	unsigned int vpd_len, kw_len;
> >> +	u8 *vpd_data;
> >> +	int rodi;
> >>  
> >>  	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
> >>  
> >> @@ -12201,46 +12200,20 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)
> >>  	if (IS_ERR(vpd_data))
> >>  		return;
> >>  
> >> -	/* VPD RO tag should be first tag after identifier string, hence
> >> -	 * we should be able to find it in first BNX2X_VPD_LEN chars
> >> -	 */
> >> -	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
> >> -	if (i < 0)
> >> -		goto out_not_found;
> >> -
> >> -	block_end = i + PCI_VPD_LRDT_TAG_SIZE +
> >> -		    pci_vpd_lrdt_size(&vpd_data[i]);
> >> -	i += PCI_VPD_LRDT_TAG_SIZE;
> >> -
> >> -	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
> >> -				   PCI_VPD_RO_KEYWORD_MFR_ID);
> >> -	if (rodi < 0)
> >> -		goto out_not_found;
> >> -
> >> -	len = pci_vpd_info_field_size(&vpd_data[rodi]);
> >> -
> >> -	if (len != VENDOR_ID_LEN)
> >> +	rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
> >> +					    PCI_VPD_RO_KEYWORD_MFR_ID, &kw_len);
> >> +	if (rodi < 0 || kw_len != VENDOR_ID_LEN)
> >>  		goto out_not_found;
> >>  
> >> -	rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
> >> -
> >>  	/* vendor specific info */
> >> -	snprintf(str_id_reg, VENDOR_ID_LEN + 1, "%04x", PCI_VENDOR_ID_DELL);
> >> -	snprintf(str_id_cap, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
> >> -	if (!strncmp(str_id_reg, &vpd_data[rodi], VENDOR_ID_LEN) ||
> >> -	    !strncmp(str_id_cap, &vpd_data[rodi], VENDOR_ID_LEN)) {
> >> -
> >> -		rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
> >> -						PCI_VPD_RO_KEYWORD_VENDOR0);
> >> -		if (rodi >= 0) {
> >> -			len = pci_vpd_info_field_size(&vpd_data[rodi]);
> >> -
> >> -			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
> >> -
> >> -			if (len < 32 && (len + rodi) <= vpd_len) {
> >> -				memcpy(bp->fw_ver, &vpd_data[rodi], len);
> >> -				bp->fw_ver[len] = ' ';
> >> -			}
> >> +	snprintf(str_id, VENDOR_ID_LEN + 1, "%04X", PCI_VENDOR_ID_DELL);
> >> +	if (!strncmp(str_id, &vpd_data[rodi], VENDOR_ID_LEN)) {
> >> +		rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
> >> +						    PCI_VPD_RO_KEYWORD_VENDOR0,
> >> +						    &kw_len);
> >> +		if (rodi >= 0 && kw_len < sizeof(bp->fw_ver)) {
> >> +			memcpy(bp->fw_ver, &vpd_data[rodi], kw_len);
> >> +			bp->fw_ver[kw_len] = ' ';
> >>  		}
> >>  	}
> >>  out_not_found:
> >> -- 
> >> 2.33.0
> >>
> >>
> 

[PATCH 07/12] bnxt: Read VPD with pci_vpd_alloc()

[Heiner Kallweit]

Use pci_vpd_alloc() to dynamically allocate a properly sized buffer and
read the full VPD data into it.

This simplifies the code, and we no longer have to make assumptions about
VPD size.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 893bdaf03..00a9b7126 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -13171,22 +13171,17 @@ static int bnxt_init_mac_addr(struct bnxt *bp)
 	return rc;
 }
 
-#define BNXT_VPD_LEN	512
 static void bnxt_vpd_read_info(struct bnxt *bp)
 {
 	struct pci_dev *pdev = bp->pdev;
 	int i, len, pos, ro_size, size;
-	ssize_t vpd_size;
+	unsigned int vpd_size;
 	u8 *vpd_data;
 
-	vpd_data = kmalloc(BNXT_VPD_LEN, GFP_KERNEL);
-	if (!vpd_data)
+	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
+	if (IS_ERR(vpd_data)) {
+		pci_warn(pdev, "Unable to read VPD\n");
 		return;
-
-	vpd_size = pci_read_vpd(pdev, 0, BNXT_VPD_LEN, vpd_data);
-	if (vpd_size <= 0) {
-		netdev_err(bp->dev, "Unable to read VPD\n");
-		goto exit;
 	}
 
 	i = pci_vpd_find_tag(vpd_data, vpd_size, PCI_VPD_LRDT_RO_DATA);
-- 
2.33.0

Re: [PATCH 07/12] bnxt: Read VPD with pci_vpd_alloc()

[kernel test robot]

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

Hi Heiner,

I love your patch! Perhaps something to improve:

[auto build test WARNING on scsi/for-next]
[also build test WARNING on pci/next mkp-scsi/for-next linus/master v5.14-rc6 next-20210820]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Heiner-Kallweit/PCI-VPD-Convert-more-users-to-the-new-VPD-API-functions/20210822-220229
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5658a697843355ac5fbf26ae3b7c57dd0d794238
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Heiner-Kallweit/PCI-VPD-Convert-more-users-to-the-new-VPD-API-functions/20210822-220229
        git checkout 5658a697843355ac5fbf26ae3b7c57dd0d794238
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_request_irq':
   drivers/net/ethernet/broadcom/bnxt/bnxt.c:9000:16: warning: variable 'j' set but not used [-Wunused-but-set-variable]
    9000 |         int i, j, rc = 0;
         |                ^
   drivers/net/ethernet/broadcom/bnxt/bnxt.c: In function 'bnxt_vpd_read_info':
   drivers/net/ethernet/broadcom/bnxt/bnxt.c:12987:20: error: implicit declaration of function 'pci_vpd_alloc'; did you mean 'pci_pool_alloc'? [-Werror=implicit-function-declaration]
   12987 |         vpd_data = pci_vpd_alloc(pdev, &vpd_size);
         |                    ^~~~~~~~~~~~~
         |                    pci_pool_alloc
>> drivers/net/ethernet/broadcom/bnxt/bnxt.c:12987:18: warning: assignment to 'u8 *' {aka 'unsigned char *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
   12987 |         vpd_data = pci_vpd_alloc(pdev, &vpd_size);
         |                  ^
   cc1: some warnings being treated as errors


vim +12987 drivers/net/ethernet/broadcom/bnxt/bnxt.c

 12979	
 12980	static void bnxt_vpd_read_info(struct bnxt *bp)
 12981	{
 12982		struct pci_dev *pdev = bp->pdev;
 12983		int i, len, pos, ro_size, size;
 12984		unsigned int vpd_size;
 12985		u8 *vpd_data;
 12986	
 12987		vpd_data = pci_vpd_alloc(pdev, &vpd_size);
 12988		if (IS_ERR(vpd_data)) {
 12989			pci_warn(pdev, "Unable to read VPD\n");
 12990			return;
 12991		}
 12992	
 12993		i = pci_vpd_find_tag(vpd_data, vpd_size, PCI_VPD_LRDT_RO_DATA);
 12994		if (i < 0) {
 12995			netdev_err(bp->dev, "VPD READ-Only not found\n");
 12996			goto exit;
 12997		}
 12998	
 12999		ro_size = pci_vpd_lrdt_size(&vpd_data[i]);
 13000		i += PCI_VPD_LRDT_TAG_SIZE;
 13001		if (i + ro_size > vpd_size)
 13002			goto exit;
 13003	
 13004		pos = pci_vpd_find_info_keyword(vpd_data, i, ro_size,
 13005						PCI_VPD_RO_KEYWORD_PARTNO);
 13006		if (pos < 0)
 13007			goto read_sn;
 13008	
 13009		len = pci_vpd_info_field_size(&vpd_data[pos]);
 13010		pos += PCI_VPD_INFO_FLD_HDR_SIZE;
 13011		if (len + pos > vpd_size)
 13012			goto read_sn;
 13013	
 13014		size = min(len, BNXT_VPD_FLD_LEN - 1);
 13015		memcpy(bp->board_partno, &vpd_data[pos], size);
 13016	
 13017	read_sn:
 13018		pos = pci_vpd_find_info_keyword(vpd_data, i, ro_size,
 13019						PCI_VPD_RO_KEYWORD_SERIALNO);
 13020		if (pos < 0)
 13021			goto exit;
 13022	
 13023		len = pci_vpd_info_field_size(&vpd_data[pos]);
 13024		pos += PCI_VPD_INFO_FLD_HDR_SIZE;
 13025		if (len + pos > vpd_size)
 13026			goto exit;
 13027	
 13028		size = min(len, BNXT_VPD_FLD_LEN - 1);
 13029		memcpy(bp->board_serialno, &vpd_data[pos], size);
 13030	exit:
 13031		kfree(vpd_data);
 13032	}
 13033	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68217 bytes --]

[PATCH 08/12] bnxt: Search VPD with pci_vpd_find_ro_info_keyword()

[Heiner Kallweit]

Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 38 ++++++-----------------
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 00a9b7126..5df00a520 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -13174,8 +13174,8 @@ static int bnxt_init_mac_addr(struct bnxt *bp)
 static void bnxt_vpd_read_info(struct bnxt *bp)
 {
 	struct pci_dev *pdev = bp->pdev;
-	int i, len, pos, ro_size, size;
-	unsigned int vpd_size;
+	unsigned int vpd_size, kw_len;
+	int pos, size;
 	u8 *vpd_data;
 
 	vpd_data = pci_vpd_alloc(pdev, &vpd_size);
@@ -13184,42 +13184,22 @@ static void bnxt_vpd_read_info(struct bnxt *bp)
 		return;
 	}
 
-	i = pci_vpd_find_tag(vpd_data, vpd_size, PCI_VPD_LRDT_RO_DATA);
-	if (i < 0) {
-		netdev_err(bp->dev, "VPD READ-Only not found\n");
-		goto exit;
-	}
-
-	ro_size = pci_vpd_lrdt_size(&vpd_data[i]);
-	i += PCI_VPD_LRDT_TAG_SIZE;
-	if (i + ro_size > vpd_size)
-		goto exit;
-
-	pos = pci_vpd_find_info_keyword(vpd_data, i, ro_size,
-					PCI_VPD_RO_KEYWORD_PARTNO);
+	pos = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
+					   PCI_VPD_RO_KEYWORD_PARTNO, &kw_len);
 	if (pos < 0)
 		goto read_sn;
 
-	len = pci_vpd_info_field_size(&vpd_data[pos]);
-	pos += PCI_VPD_INFO_FLD_HDR_SIZE;
-	if (len + pos > vpd_size)
-		goto read_sn;
-
-	size = min(len, BNXT_VPD_FLD_LEN - 1);
+	size = min_t(int, kw_len, BNXT_VPD_FLD_LEN - 1);
 	memcpy(bp->board_partno, &vpd_data[pos], size);
 
 read_sn:
-	pos = pci_vpd_find_info_keyword(vpd_data, i, ro_size,
-					PCI_VPD_RO_KEYWORD_SERIALNO);
+	pos = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
+					   PCI_VPD_RO_KEYWORD_SERIALNO,
+					   &kw_len);
 	if (pos < 0)
 		goto exit;
 
-	len = pci_vpd_info_field_size(&vpd_data[pos]);
-	pos += PCI_VPD_INFO_FLD_HDR_SIZE;
-	if (len + pos > vpd_size)
-		goto exit;
-
-	size = min(len, BNXT_VPD_FLD_LEN - 1);
+	size = min_t(int, kw_len, BNXT_VPD_FLD_LEN - 1);
 	memcpy(bp->board_serialno, &vpd_data[pos], size);
 exit:
 	kfree(vpd_data);
-- 
2.33.0

[PATCH 09/12] cxgb4: Validate VPD checksum with pci_vpd_check_csum()

[Heiner Kallweit]

Validate the VPD checksum with pci_vpd_check_csum() to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index 6606fb8b3..1ae3ee994 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -2745,7 +2745,7 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
 {
 	int i, ret = 0, addr;
 	int ec, sn, pn, na;
-	u8 *vpd, csum, base_val = 0;
+	u8 *vpd, base_val = 0;
 	unsigned int vpdr_len, kw_offset, id_len;
 
 	vpd = vmalloc(VPD_LEN);
@@ -2800,13 +2800,9 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
 	var += PCI_VPD_INFO_FLD_HDR_SIZE; \
 } while (0)
 
-	FIND_VPD_KW(i, "RV");
-	for (csum = 0; i >= 0; i--)
-		csum += vpd[i];
-
-	if (csum) {
-		dev_err(adapter->pdev_dev,
-			"corrupted VPD EEPROM, actual csum %u\n", csum);
+	ret = pci_vpd_check_csum(vpd, VPD_LEN);
+	if (ret) {
+		dev_err(adapter->pdev_dev, "VPD checksum incorrect or missing\n");
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.33.0

[PATCH 10/12] cxgb4: Remove unused vpd_param member ec

[Heiner Kallweit]

Member ec isn't used, so remove it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h | 2 --
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 5 +----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index 9058f09f9..ecea3cdd3 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -84,7 +84,6 @@ extern struct mutex uld_mutex;
 enum {
 	MAX_NPORTS	= 4,     /* max # of ports */
 	SERNUM_LEN	= 24,    /* Serial # length */
-	EC_LEN		= 16,    /* E/C length */
 	ID_LEN		= 16,    /* ID length */
 	PN_LEN		= 16,    /* Part Number length */
 	MACADDR_LEN	= 12,    /* MAC Address length */
@@ -391,7 +390,6 @@ struct tp_params {
 
 struct vpd_params {
 	unsigned int cclk;
-	u8 ec[EC_LEN + 1];
 	u8 sn[SERNUM_LEN + 1];
 	u8 id[ID_LEN + 1];
 	u8 pn[PN_LEN + 1];
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index 1ae3ee994..2aeb2f80f 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -2744,7 +2744,7 @@ int t4_seeprom_wp(struct adapter *adapter, bool enable)
 int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
 {
 	int i, ret = 0, addr;
-	int ec, sn, pn, na;
+	int sn, pn, na;
 	u8 *vpd, base_val = 0;
 	unsigned int vpdr_len, kw_offset, id_len;
 
@@ -2807,7 +2807,6 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
 		goto out;
 	}
 
-	FIND_VPD_KW(ec, "EC");
 	FIND_VPD_KW(sn, "SN");
 	FIND_VPD_KW(pn, "PN");
 	FIND_VPD_KW(na, "NA");
@@ -2815,8 +2814,6 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
 
 	memcpy(p->id, vpd + PCI_VPD_LRDT_TAG_SIZE, id_len);
 	strim(p->id);
-	memcpy(p->ec, vpd + ec, EC_LEN);
-	strim(p->ec);
 	i = pci_vpd_info_field_size(vpd + sn - PCI_VPD_INFO_FLD_HDR_SIZE);
 	memcpy(p->sn, vpd + sn, min(i, SERNUM_LEN));
 	strim(p->sn);
-- 
2.33.0

[PATCH 11/12] cxgb4: Search VPD with pci_vpd_find_ro_info_keyword()

[Heiner Kallweit]

Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 67 +++++++++-------------
 1 file changed, 27 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
index 2aeb2f80f..5e8ac42ac 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
@@ -2743,10 +2743,9 @@ int t4_seeprom_wp(struct adapter *adapter, bool enable)
  */
 int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
 {
-	int i, ret = 0, addr;
-	int sn, pn, na;
+	unsigned int id_len, pn_len, sn_len, na_len;
+	int sn, pn, na, addr, ret = 0;
 	u8 *vpd, base_val = 0;
-	unsigned int vpdr_len, kw_offset, id_len;
 
 	vpd = vmalloc(VPD_LEN);
 	if (!vpd)
@@ -2772,60 +2771,48 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
 	}
 
 	id_len = pci_vpd_lrdt_size(vpd);
-	if (id_len > ID_LEN)
-		id_len = ID_LEN;
 
-	i = pci_vpd_find_tag(vpd, VPD_LEN, PCI_VPD_LRDT_RO_DATA);
-	if (i < 0) {
-		dev_err(adapter->pdev_dev, "missing VPD-R section\n");
+	ret = pci_vpd_check_csum(vpd, VPD_LEN);
+	if (ret) {
+		dev_err(adapter->pdev_dev, "VPD checksum incorrect or missing\n");
 		ret = -EINVAL;
 		goto out;
 	}
 
-	vpdr_len = pci_vpd_lrdt_size(&vpd[i]);
-	kw_offset = i + PCI_VPD_LRDT_TAG_SIZE;
-	if (vpdr_len + kw_offset > VPD_LEN) {
-		dev_err(adapter->pdev_dev, "bad VPD-R length %u\n", vpdr_len);
-		ret = -EINVAL;
+	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN,
+					   PCI_VPD_RO_KEYWORD_SERIALNO, &sn_len);
+	if (ret < 0)
 		goto out;
-	}
+	sn = ret;
 
-#define FIND_VPD_KW(var, name) do { \
-	var = pci_vpd_find_info_keyword(vpd, kw_offset, vpdr_len, name); \
-	if (var < 0) { \
-		dev_err(adapter->pdev_dev, "missing VPD keyword " name "\n"); \
-		ret = -EINVAL; \
-		goto out; \
-	} \
-	var += PCI_VPD_INFO_FLD_HDR_SIZE; \
-} while (0)
-
-	ret = pci_vpd_check_csum(vpd, VPD_LEN);
-	if (ret) {
-		dev_err(adapter->pdev_dev, "VPD checksum incorrect or missing\n");
-		ret = -EINVAL;
+	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN,
+					   PCI_VPD_RO_KEYWORD_PARTNO, &pn_len);
+	if (ret < 0)
 		goto out;
-	}
+	pn = ret;
 
-	FIND_VPD_KW(sn, "SN");
-	FIND_VPD_KW(pn, "PN");
-	FIND_VPD_KW(na, "NA");
-#undef FIND_VPD_KW
+	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN, "NA", &na_len);
+	if (ret < 0)
+		goto out;
+	na = ret;
 
-	memcpy(p->id, vpd + PCI_VPD_LRDT_TAG_SIZE, id_len);
+	memcpy(p->id, vpd + PCI_VPD_LRDT_TAG_SIZE, min_t(int, id_len, ID_LEN));
 	strim(p->id);
-	i = pci_vpd_info_field_size(vpd + sn - PCI_VPD_INFO_FLD_HDR_SIZE);
-	memcpy(p->sn, vpd + sn, min(i, SERNUM_LEN));
+	memcpy(p->sn, vpd + sn, min_t(int, sn_len, SERNUM_LEN));
 	strim(p->sn);
-	i = pci_vpd_info_field_size(vpd + pn - PCI_VPD_INFO_FLD_HDR_SIZE);
-	memcpy(p->pn, vpd + pn, min(i, PN_LEN));
+	memcpy(p->pn, vpd + pn, min_t(int, pn_len, PN_LEN));
 	strim(p->pn);
-	memcpy(p->na, vpd + na, min(i, MACADDR_LEN));
+	memcpy(p->na, vpd + na, min_t(int, na_len, MACADDR_LEN));
 	strim((char *)p->na);
 
 out:
 	vfree(vpd);
-	return ret < 0 ? ret : 0;
+	if (ret < 0) {
+		dev_err(adapter->pdev_dev, "error reading VPD\n");
+		return ret;
+	}
+
+	return 0;
 }
 
 /**
-- 
2.33.0

Re: [PATCH 11/12] cxgb4: Search VPD with pci_vpd_find_ro_info_keyword()

[Bjorn Helgaas]

On Sun, Aug 22, 2021 at 03:59:21PM +0200, Heiner Kallweit wrote:
> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
> simplify the code.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 67 +++++++++-------------
>  1 file changed, 27 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
> index 2aeb2f80f..5e8ac42ac 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
> @@ -2743,10 +2743,9 @@ int t4_seeprom_wp(struct adapter *adapter, bool enable)
>   */
>  int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
>  {
> -	int i, ret = 0, addr;
> -	int sn, pn, na;
> +	unsigned int id_len, pn_len, sn_len, na_len;
> +	int sn, pn, na, addr, ret = 0;
>  	u8 *vpd, base_val = 0;
> -	unsigned int vpdr_len, kw_offset, id_len;
>  
>  	vpd = vmalloc(VPD_LEN);
>  	if (!vpd)
> @@ -2772,60 +2771,48 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
>  	}
>  
>  	id_len = pci_vpd_lrdt_size(vpd);
> -	if (id_len > ID_LEN)
> -		id_len = ID_LEN;
>  
> -	i = pci_vpd_find_tag(vpd, VPD_LEN, PCI_VPD_LRDT_RO_DATA);
> -	if (i < 0) {
> -		dev_err(adapter->pdev_dev, "missing VPD-R section\n");
> +	ret = pci_vpd_check_csum(vpd, VPD_LEN);
> +	if (ret) {
> +		dev_err(adapter->pdev_dev, "VPD checksum incorrect or missing\n");
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  
> -	vpdr_len = pci_vpd_lrdt_size(&vpd[i]);
> -	kw_offset = i + PCI_VPD_LRDT_TAG_SIZE;
> -	if (vpdr_len + kw_offset > VPD_LEN) {
> -		dev_err(adapter->pdev_dev, "bad VPD-R length %u\n", vpdr_len);
> -		ret = -EINVAL;
> +	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN,
> +					   PCI_VPD_RO_KEYWORD_SERIALNO, &sn_len);
> +	if (ret < 0)
>  		goto out;
> -	}
> +	sn = ret;
>  
> -#define FIND_VPD_KW(var, name) do { \
> -	var = pci_vpd_find_info_keyword(vpd, kw_offset, vpdr_len, name); \
> -	if (var < 0) { \
> -		dev_err(adapter->pdev_dev, "missing VPD keyword " name "\n"); \

Just for the record, I guess this patch gives up these error messages
that mention the specific keyword that's missing?  Not really an issue
for *me*, since the people generating the VPD content should be able to
easily validate this and figure out any errors.  Just pointing it out
in case the cxgb4 folks are attached to the messages.

> -		ret = -EINVAL; \
> -		goto out; \
> -	} \
> -	var += PCI_VPD_INFO_FLD_HDR_SIZE; \
> -} while (0)
> -
> -	ret = pci_vpd_check_csum(vpd, VPD_LEN);
> -	if (ret) {
> -		dev_err(adapter->pdev_dev, "VPD checksum incorrect or missing\n");
> -		ret = -EINVAL;
> +	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN,
> +					   PCI_VPD_RO_KEYWORD_PARTNO, &pn_len);
> +	if (ret < 0)
>  		goto out;
> -	}
> +	pn = ret;
>  
> -	FIND_VPD_KW(sn, "SN");
> -	FIND_VPD_KW(pn, "PN");
> -	FIND_VPD_KW(na, "NA");
> -#undef FIND_VPD_KW
> +	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN, "NA", &na_len);
> +	if (ret < 0)
> +		goto out;
> +	na = ret;
>  
> -	memcpy(p->id, vpd + PCI_VPD_LRDT_TAG_SIZE, id_len);
> +	memcpy(p->id, vpd + PCI_VPD_LRDT_TAG_SIZE, min_t(int, id_len, ID_LEN));
>  	strim(p->id);
> -	i = pci_vpd_info_field_size(vpd + sn - PCI_VPD_INFO_FLD_HDR_SIZE);
> -	memcpy(p->sn, vpd + sn, min(i, SERNUM_LEN));
> +	memcpy(p->sn, vpd + sn, min_t(int, sn_len, SERNUM_LEN));
>  	strim(p->sn);
> -	i = pci_vpd_info_field_size(vpd + pn - PCI_VPD_INFO_FLD_HDR_SIZE);
> -	memcpy(p->pn, vpd + pn, min(i, PN_LEN));
> +	memcpy(p->pn, vpd + pn, min_t(int, pn_len, PN_LEN));
>  	strim(p->pn);
> -	memcpy(p->na, vpd + na, min(i, MACADDR_LEN));
> +	memcpy(p->na, vpd + na, min_t(int, na_len, MACADDR_LEN));
>  	strim((char *)p->na);
>  
>  out:
>  	vfree(vpd);
> -	return ret < 0 ? ret : 0;
> +	if (ret < 0) {
> +		dev_err(adapter->pdev_dev, "error reading VPD\n");
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  /**
> -- 
> 2.33.0
> 
> 

Re: [PATCH 11/12] cxgb4: Search VPD with pci_vpd_find_ro_info_keyword()

[Heiner Kallweit]

On 24.08.2021 19:11, Bjorn Helgaas wrote:
> On Sun, Aug 22, 2021 at 03:59:21PM +0200, Heiner Kallweit wrote:
>> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
>> simplify the code.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/ethernet/chelsio/cxgb4/t4_hw.c | 67 +++++++++-------------
>>  1 file changed, 27 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
>> index 2aeb2f80f..5e8ac42ac 100644
>> --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
>> +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
>> @@ -2743,10 +2743,9 @@ int t4_seeprom_wp(struct adapter *adapter, bool enable)
>>   */
>>  int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
>>  {
>> -	int i, ret = 0, addr;
>> -	int sn, pn, na;
>> +	unsigned int id_len, pn_len, sn_len, na_len;
>> +	int sn, pn, na, addr, ret = 0;
>>  	u8 *vpd, base_val = 0;
>> -	unsigned int vpdr_len, kw_offset, id_len;
>>  
>>  	vpd = vmalloc(VPD_LEN);
>>  	if (!vpd)
>> @@ -2772,60 +2771,48 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
>>  	}
>>  
>>  	id_len = pci_vpd_lrdt_size(vpd);
>> -	if (id_len > ID_LEN)
>> -		id_len = ID_LEN;
>>  
>> -	i = pci_vpd_find_tag(vpd, VPD_LEN, PCI_VPD_LRDT_RO_DATA);
>> -	if (i < 0) {
>> -		dev_err(adapter->pdev_dev, "missing VPD-R section\n");
>> +	ret = pci_vpd_check_csum(vpd, VPD_LEN);
>> +	if (ret) {
>> +		dev_err(adapter->pdev_dev, "VPD checksum incorrect or missing\n");
>>  		ret = -EINVAL;
>>  		goto out;
>>  	}
>>  
>> -	vpdr_len = pci_vpd_lrdt_size(&vpd[i]);
>> -	kw_offset = i + PCI_VPD_LRDT_TAG_SIZE;
>> -	if (vpdr_len + kw_offset > VPD_LEN) {
>> -		dev_err(adapter->pdev_dev, "bad VPD-R length %u\n", vpdr_len);
>> -		ret = -EINVAL;
>> +	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN,
>> +					   PCI_VPD_RO_KEYWORD_SERIALNO, &sn_len);
>> +	if (ret < 0)
>>  		goto out;
>> -	}
>> +	sn = ret;
>>  
>> -#define FIND_VPD_KW(var, name) do { \
>> -	var = pci_vpd_find_info_keyword(vpd, kw_offset, vpdr_len, name); \
>> -	if (var < 0) { \
>> -		dev_err(adapter->pdev_dev, "missing VPD keyword " name "\n"); \
> 
> Just for the record, I guess this patch gives up these error messages
> that mention the specific keyword that's missing?  Not really an issue
> for *me*, since the people generating the VPD content should be able to
> easily validate this and figure out any errors.  Just pointing it out
> in case the cxgb4 folks are attached to the messages.
> 
Right. My thought was: With userspace tools like "lspci -vv" or
"od <path>/vpd" it's easy to check the actual content of VPD and find
out which keyword is missing. Therefore the missing keyword doesn't
necessarily has to be referenced in an error message.

>> -		ret = -EINVAL; \
>> -		goto out; \
>> -	} \
>> -	var += PCI_VPD_INFO_FLD_HDR_SIZE; \
>> -} while (0)
>> -
>> -	ret = pci_vpd_check_csum(vpd, VPD_LEN);
>> -	if (ret) {
>> -		dev_err(adapter->pdev_dev, "VPD checksum incorrect or missing\n");
>> -		ret = -EINVAL;
>> +	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN,
>> +					   PCI_VPD_RO_KEYWORD_PARTNO, &pn_len);
>> +	if (ret < 0)
>>  		goto out;
>> -	}
>> +	pn = ret;
>>  
>> -	FIND_VPD_KW(sn, "SN");
>> -	FIND_VPD_KW(pn, "PN");
>> -	FIND_VPD_KW(na, "NA");
>> -#undef FIND_VPD_KW
>> +	ret = pci_vpd_find_ro_info_keyword(vpd, VPD_LEN, "NA", &na_len);
>> +	if (ret < 0)
>> +		goto out;
>> +	na = ret;
>>  
>> -	memcpy(p->id, vpd + PCI_VPD_LRDT_TAG_SIZE, id_len);
>> +	memcpy(p->id, vpd + PCI_VPD_LRDT_TAG_SIZE, min_t(int, id_len, ID_LEN));
>>  	strim(p->id);
>> -	i = pci_vpd_info_field_size(vpd + sn - PCI_VPD_INFO_FLD_HDR_SIZE);
>> -	memcpy(p->sn, vpd + sn, min(i, SERNUM_LEN));
>> +	memcpy(p->sn, vpd + sn, min_t(int, sn_len, SERNUM_LEN));
>>  	strim(p->sn);
>> -	i = pci_vpd_info_field_size(vpd + pn - PCI_VPD_INFO_FLD_HDR_SIZE);
>> -	memcpy(p->pn, vpd + pn, min(i, PN_LEN));
>> +	memcpy(p->pn, vpd + pn, min_t(int, pn_len, PN_LEN));
>>  	strim(p->pn);
>> -	memcpy(p->na, vpd + na, min(i, MACADDR_LEN));
>> +	memcpy(p->na, vpd + na, min_t(int, na_len, MACADDR_LEN));
>>  	strim((char *)p->na);
>>  
>>  out:
>>  	vfree(vpd);
>> -	return ret < 0 ? ret : 0;
>> +	if (ret < 0) {
>> +		dev_err(adapter->pdev_dev, "error reading VPD\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>  }
>>  
>>  /**
>> -- 
>> 2.33.0
>>
>>

[PATCH 12/12] scsi: cxlflash: Search VPD with pci_vpd_find_ro_info_keyword()

[Heiner Kallweit]

Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/scsi/cxlflash/main.c | 34 ++++++----------------------------
 1 file changed, 6 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 2f1894588..b2730e859 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -1629,8 +1629,8 @@ static int read_vpd(struct cxlflash_cfg *cfg, u64 wwpn[])
 {
 	struct device *dev = &cfg->dev->dev;
 	struct pci_dev *pdev = cfg->dev;
-	int rc = 0;
-	int ro_start, ro_size, i, j, k;
+	int i, k, rc = 0;
+	unsigned int kw_size;
 	ssize_t vpd_size;
 	char vpd_data[CXLFLASH_VPD_LEN];
 	char tmp_buf[WWPN_BUF_LEN] = { 0 };
@@ -1648,24 +1648,6 @@ static int read_vpd(struct cxlflash_cfg *cfg, u64 wwpn[])
 		goto out;
 	}
 
-	/* Get the read only section offset */
-	ro_start = pci_vpd_find_tag(vpd_data, vpd_size, PCI_VPD_LRDT_RO_DATA);
-	if (unlikely(ro_start < 0)) {
-		dev_err(dev, "%s: VPD Read-only data not found\n", __func__);
-		rc = -ENODEV;
-		goto out;
-	}
-
-	/* Get the read only section size, cap when extends beyond read VPD */
-	ro_size = pci_vpd_lrdt_size(&vpd_data[ro_start]);
-	j = ro_size;
-	i = ro_start + PCI_VPD_LRDT_TAG_SIZE;
-	if (unlikely((i + j) > vpd_size)) {
-		dev_dbg(dev, "%s: Might need to read more VPD (%d > %ld)\n",
-			__func__, (i + j), vpd_size);
-		ro_size = vpd_size - i;
-	}
-
 	/*
 	 * Find the offset of the WWPN tag within the read only
 	 * VPD data and validate the found field (partials are
@@ -1681,11 +1663,9 @@ static int read_vpd(struct cxlflash_cfg *cfg, u64 wwpn[])
 	 * ports programmed and operate in an undefined state.
 	 */
 	for (k = 0; k < cfg->num_fc_ports; k++) {
-		j = ro_size;
-		i = ro_start + PCI_VPD_LRDT_TAG_SIZE;
-
-		i = pci_vpd_find_info_keyword(vpd_data, i, j, wwpn_vpd_tags[k]);
-		if (i < 0) {
+		i = pci_vpd_find_ro_info_keyword(vpd_data, vpd_size,
+						 wwpn_vpd_tags[k], &kw_size);
+		if (i == -ENOENT) {
 			if (wwpn_vpd_required)
 				dev_err(dev, "%s: Port %d WWPN not found\n",
 					__func__, k);
@@ -1693,9 +1673,7 @@ static int read_vpd(struct cxlflash_cfg *cfg, u64 wwpn[])
 			continue;
 		}
 
-		j = pci_vpd_info_field_size(&vpd_data[i]);
-		i += PCI_VPD_INFO_FLD_HDR_SIZE;
-		if (unlikely((i + j > vpd_size) || (j != WWPN_LEN))) {
+		if (i < 0 || kw_size != WWPN_LEN) {
 			dev_err(dev, "%s: Port %d WWPN incomplete or bad VPD\n",
 				__func__, k);
 			rc = -ENODEV;
-- 
2.33.0

Re: [PATCH 00/12] PCI/VPD: Convert more users to the new VPD API functions

[Bjorn Helgaas]

On Sun, Aug 22, 2021 at 03:46:20PM +0200, Heiner Kallweit wrote:
> This series converts more users to the new VPD API functions.
> 
> bnx2 patches have been tested with a BCM5709 card.
> The other patches are compile-tested, except cxlflash.
> 
> Heiner Kallweit (12):
>   sfc: falcon: Read VPD with pci_vpd_alloc()
>   sfc: falcon: Search VPD with pci_vpd_find_ro_info_keyword()
>   bnx2: Search VPD with pci_vpd_find_ro_info_keyword()
>   bnx2: Replace open-coded version with swab32s()
>   bnx2x: Read VPD with pci_vpd_alloc()
>   bnx2x: Search VPD with pci_vpd_find_ro_info_keyword()
>   bnxt: Read VPD with pci_vpd_alloc()
>   bnxt: Search VPD with pci_vpd_find_ro_info_keyword()
>   cxgb4: Validate VPD checksum with pci_vpd_check_csum()
>   cxgb4: Remove unused vpd_param member ec
>   cxgb4: Search VPD with pci_vpd_find_ro_info_keyword()
>   scsi: cxlflash: Search VPD with pci_vpd_find_ro_info_keyword()
> 
>  drivers/net/ethernet/broadcom/bnx2.c          | 46 +++-------
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x.h   |  1 -
>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 91 ++++---------------
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 49 +++-------
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4.h    |  2 -
>  drivers/net/ethernet/chelsio/cxgb4/t4_hw.c    | 76 ++++++----------
>  drivers/net/ethernet/sfc/falcon/efx.c         | 79 ++++------------
>  drivers/scsi/cxlflash/main.c                  | 34 ++-----
>  8 files changed, 98 insertions(+), 280 deletions(-)

I added these to pci/vpd for v5.15, thanks!

RE: [PATCH 05/12] bnx2x: Read VPD with pci_vpd_alloc()

[Prabhakar Kushwaha]

Hi Heiner,

> -----Original Message-----
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Sent: Sunday, August 22, 2021 7:23 PM
> To: Bjorn Helgaas <bhelgaas@google.com>; Ariel Elior <aelior@marvell.com>;
> Sudarsana Reddy Kalluru <skalluru@marvell.com>; GR-everest-linux-l2 <GR-
> everest-linux-l2@marvell.com>; Jakub Kicinski <kuba@kernel.org>; David
> Härdeman <david@hardeman.nu>
> Cc: linux-pci@vger.kernel.org; netdev@vger.kernel.org
> Subject: [PATCH 05/12] bnx2x: Read VPD with pci_vpd_alloc()
> 
> External Email
> 
> ----------------------------------------------------------------------
> Use pci_vpd_alloc() to dynamically allocate a properly sized buffer and
> read the full VPD data into it.
> 
> This simplifies the code, and we no longer have to make assumptions about
> VPD size.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x.h   |  1 -
>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 44 +++++--------------
>  2 files changed, 10 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> index d04994840..e789430f4 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> @@ -2407,7 +2407,6 @@ void bnx2x_igu_clear_sb_gen(struct bnx2x *bp, u8
> func, u8 idu_sb_id,
>  #define ETH_MAX_RX_CLIENTS_E2		ETH_MAX_RX_CLIENTS_E1H
>  #endif
> 
> -#define BNX2X_VPD_LEN			128
>  #define VENDOR_ID_LEN			4
> 
>  #define VF_ACQUIRE_THRESH		3
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index 6d9813491..0466adf8d 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -12189,50 +12189,29 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)
> 
>  static void bnx2x_read_fwinfo(struct bnx2x *bp)
>  {
> -	int cnt, i, block_end, rodi;
> -	char vpd_start[BNX2X_VPD_LEN+1];
> +	int i, block_end, rodi;
>  	char str_id_reg[VENDOR_ID_LEN+1];
>  	char str_id_cap[VENDOR_ID_LEN+1];
> -	char *vpd_data;
> -	char *vpd_extended_data = NULL;
> -	u8 len;
> +	unsigned int vpd_len;
> +	u8 *vpd_data, len;
> 
> -	cnt = pci_read_vpd(bp->pdev, 0, BNX2X_VPD_LEN, vpd_start);
>  	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
> 
> -	if (cnt < BNX2X_VPD_LEN)
> -		goto out_not_found;
> +	vpd_data = pci_vpd_alloc(bp->pdev, &vpd_len);

Definition of pci_vpd_alloc() is below as per repo "git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git" and   branch wip/heiner-vpd-api
void *pci_vpd_alloc(struct pci_dev *dev, unsigned int *size)
{
        unsigned int len = dev->vpd.len;
        void *buf;
--
--
        if (size)
                *size = len;
}
Here is len is already part of pci_dev.  

So why cannot same be set in caller function i.e. vpd_len = pb->pdev->vpd.len


> +	if (IS_ERR(vpd_data))
> +		return;
> 
>  	/* VPD RO tag should be first tag after identifier string, hence
>  	 * we should be able to find it in first BNX2X_VPD_LEN chars
>  	 */
> -	i = pci_vpd_find_tag(vpd_start, BNX2X_VPD_LEN,
> PCI_VPD_LRDT_RO_DATA);
> +	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
>  	if (i < 0)
>  		goto out_not_found;
> 
>  	block_end = i + PCI_VPD_LRDT_TAG_SIZE +
> -		    pci_vpd_lrdt_size(&vpd_start[i]);
> -
> +		    pci_vpd_lrdt_size(&vpd_data[i]);
>  	i += PCI_VPD_LRDT_TAG_SIZE;
> 
> -	if (block_end > BNX2X_VPD_LEN) {
> -		vpd_extended_data = kmalloc(block_end, GFP_KERNEL);
> -		if (vpd_extended_data  == NULL)
> -			goto out_not_found;
> -
> -		/* read rest of vpd image into vpd_extended_data */
> -		memcpy(vpd_extended_data, vpd_start, BNX2X_VPD_LEN);
> -		cnt = pci_read_vpd(bp->pdev, BNX2X_VPD_LEN,
> -				   block_end - BNX2X_VPD_LEN,
> -				   vpd_extended_data + BNX2X_VPD_LEN);
> -		if (cnt < (block_end - BNX2X_VPD_LEN))
> -			goto out_not_found;
> -		vpd_data = vpd_extended_data;
> -	} else
> -		vpd_data = vpd_start;
> -
> -	/* now vpd_data holds full vpd content in both cases */
> -
>  	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
>  				   PCI_VPD_RO_KEYWORD_MFR_ID);
>  	if (rodi < 0)
> @@ -12258,17 +12237,14 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)
> 
>  			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
> 
> -			if (len < 32 && (len + rodi) <= BNX2X_VPD_LEN) {
> +			if (len < 32 && (len + rodi) <= vpd_len) {
>  				memcpy(bp->fw_ver, &vpd_data[rodi], len);
>  				bp->fw_ver[len] = ' ';
>  			}
>  		}
> -		kfree(vpd_extended_data);
> -		return;
>  	}
>  out_not_found:
> -	kfree(vpd_extended_data);
> -	return;
> +	kfree(vpd_data);

As vpd_data allocation done in PCI layer. 
It will be logical to also free vpd_data in PCI layer.

--pk

Re: [PATCH 05/12] bnx2x: Read VPD with pci_vpd_alloc()

[Heiner Kallweit]

On 24.08.2021 16:20, Prabhakar Kushwaha wrote:
> Hi Heiner,
> 
>> -----Original Message-----
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> Sent: Sunday, August 22, 2021 7:23 PM
>> To: Bjorn Helgaas <bhelgaas@google.com>; Ariel Elior <aelior@marvell.com>;
>> Sudarsana Reddy Kalluru <skalluru@marvell.com>; GR-everest-linux-l2 <GR-
>> everest-linux-l2@marvell.com>; Jakub Kicinski <kuba@kernel.org>; David
>> Härdeman <david@hardeman.nu>
>> Cc: linux-pci@vger.kernel.org; netdev@vger.kernel.org
>> Subject: [PATCH 05/12] bnx2x: Read VPD with pci_vpd_alloc()
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> Use pci_vpd_alloc() to dynamically allocate a properly sized buffer and
>> read the full VPD data into it.
>>
>> This simplifies the code, and we no longer have to make assumptions about
>> VPD size.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/bnx2x/bnx2x.h   |  1 -
>>  .../net/ethernet/broadcom/bnx2x/bnx2x_main.c  | 44 +++++--------------
>>  2 files changed, 10 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>> index d04994840..e789430f4 100644
>> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
>> @@ -2407,7 +2407,6 @@ void bnx2x_igu_clear_sb_gen(struct bnx2x *bp, u8
>> func, u8 idu_sb_id,
>>  #define ETH_MAX_RX_CLIENTS_E2		ETH_MAX_RX_CLIENTS_E1H
>>  #endif
>>
>> -#define BNX2X_VPD_LEN			128
>>  #define VENDOR_ID_LEN			4
>>
>>  #define VF_ACQUIRE_THRESH		3
>> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> index 6d9813491..0466adf8d 100644
>> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
>> @@ -12189,50 +12189,29 @@ static int bnx2x_get_hwinfo(struct bnx2x *bp)
>>
>>  static void bnx2x_read_fwinfo(struct bnx2x *bp)
>>  {
>> -	int cnt, i, block_end, rodi;
>> -	char vpd_start[BNX2X_VPD_LEN+1];
>> +	int i, block_end, rodi;
>>  	char str_id_reg[VENDOR_ID_LEN+1];
>>  	char str_id_cap[VENDOR_ID_LEN+1];
>> -	char *vpd_data;
>> -	char *vpd_extended_data = NULL;
>> -	u8 len;
>> +	unsigned int vpd_len;
>> +	u8 *vpd_data, len;
>>
>> -	cnt = pci_read_vpd(bp->pdev, 0, BNX2X_VPD_LEN, vpd_start);
>>  	memset(bp->fw_ver, 0, sizeof(bp->fw_ver));
>>
>> -	if (cnt < BNX2X_VPD_LEN)
>> -		goto out_not_found;
>> +	vpd_data = pci_vpd_alloc(bp->pdev, &vpd_len);
> 
> Definition of pci_vpd_alloc() is below as per repo "git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git" and   branch wip/heiner-vpd-api
> void *pci_vpd_alloc(struct pci_dev *dev, unsigned int *size)
> {
>         unsigned int len = dev->vpd.len;
>         void *buf;
> --
> --
>         if (size)
>                 *size = len;
> }
> Here is len is already part of pci_dev.  
> 
> So why cannot same be set in caller function i.e. vpd_len = pb->pdev->vpd.len
> 
Internals of struct pci_vpd shouldn't be referenced outside PCI core.
Also you can't use vpd.len w/o checking vpd.cap.
pci_vpd_alloc() encapsulates these internals.

> 
>> +	if (IS_ERR(vpd_data))
>> +		return;
>>
>>  	/* VPD RO tag should be first tag after identifier string, hence
>>  	 * we should be able to find it in first BNX2X_VPD_LEN chars
>>  	 */
>> -	i = pci_vpd_find_tag(vpd_start, BNX2X_VPD_LEN,
>> PCI_VPD_LRDT_RO_DATA);
>> +	i = pci_vpd_find_tag(vpd_data, vpd_len, PCI_VPD_LRDT_RO_DATA);
>>  	if (i < 0)
>>  		goto out_not_found;
>>
>>  	block_end = i + PCI_VPD_LRDT_TAG_SIZE +
>> -		    pci_vpd_lrdt_size(&vpd_start[i]);
>> -
>> +		    pci_vpd_lrdt_size(&vpd_data[i]);
>>  	i += PCI_VPD_LRDT_TAG_SIZE;
>>
>> -	if (block_end > BNX2X_VPD_LEN) {
>> -		vpd_extended_data = kmalloc(block_end, GFP_KERNEL);
>> -		if (vpd_extended_data  == NULL)
>> -			goto out_not_found;
>> -
>> -		/* read rest of vpd image into vpd_extended_data */
>> -		memcpy(vpd_extended_data, vpd_start, BNX2X_VPD_LEN);
>> -		cnt = pci_read_vpd(bp->pdev, BNX2X_VPD_LEN,
>> -				   block_end - BNX2X_VPD_LEN,
>> -				   vpd_extended_data + BNX2X_VPD_LEN);
>> -		if (cnt < (block_end - BNX2X_VPD_LEN))
>> -			goto out_not_found;
>> -		vpd_data = vpd_extended_data;
>> -	} else
>> -		vpd_data = vpd_start;
>> -
>> -	/* now vpd_data holds full vpd content in both cases */
>> -
>>  	rodi = pci_vpd_find_info_keyword(vpd_data, i, block_end,
>>  				   PCI_VPD_RO_KEYWORD_MFR_ID);
>>  	if (rodi < 0)
>> @@ -12258,17 +12237,14 @@ static void bnx2x_read_fwinfo(struct bnx2x *bp)
>>
>>  			rodi += PCI_VPD_INFO_FLD_HDR_SIZE;
>>
>> -			if (len < 32 && (len + rodi) <= BNX2X_VPD_LEN) {
>> +			if (len < 32 && (len + rodi) <= vpd_len) {
>>  				memcpy(bp->fw_ver, &vpd_data[rodi], len);
>>  				bp->fw_ver[len] = ' ';
>>  			}
>>  		}
>> -		kfree(vpd_extended_data);
>> -		return;
>>  	}
>>  out_not_found:
>> -	kfree(vpd_extended_data);
>> -	return;
>> +	kfree(vpd_data);
> 
> As vpd_data allocation done in PCI layer. 
> It will be logical to also free vpd_data in PCI layer.
> 
The idea is right, however a call like pci_vpd_free() would be a one-liner
calling kfree(), and I doubt it's really worth it.

> --pk
> 
Heiner