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

[Heiner Kallweit <hkallweit1@gmail.com>]

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