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

* RE: [PATCH 05/12] bnx2x: Read VPD with pci_vpd_alloc()
@ 2021-08-24 14:20 Prabhakar Kushwaha
  2021-08-24 14:30 ` Heiner Kallweit
  0 siblings, 1 reply; 5+ messages in thread
From: Prabhakar Kushwaha @ 2021-08-24 14:20 UTC (permalink / raw)
  To: Heiner Kallweit, Bjorn Helgaas, Ariel Elior,
	Sudarsana Reddy Kalluru, GR-everest-linux-l2, Jakub Kicinski,
	David Härdeman
  Cc: linux-pci, netdev, Shai Malin

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

^ permalink raw reply	[flat|nested] 5+ messages in thread