Re: [PATCH net-next 4/5] i40e: Refactor and rename i40e_read_pba_string()

[Jacob Keller]

On 10/13/2023 10:07 AM, Ivan Vecera wrote:
> Function i40e_read_pba_string() is currently unused but will be used
> by subsequent patch to provide board ID via devlink device info.
> 
> The function reads PBA block from NVM so it cannot be called during
> adapter reset and as we would like to provide PBA ID via devlink
> info it is better to read the PBA ID during i40e_probe() and cache
> it in i40e_hw structure to avoid a waiting for potential adapter
> reset in devlink info callback.
> 
> So...
> - Remove pba_num and pba_num_size arguments from the function,
>   allocate resource managed buffer to store PBA ID string and
>   save resulting pointer to i40e_hw->pba_id field
> - Make the function void as the PBA ID can be missing and in this
>   case (or in case of NVM reading failure) the i40e_hw->pba_id
>   will be NULL
> - Rename the function to i40e_get_pba_string() to align with other
>   functions like i40e_get_oem_version() i40e_get_port_mac_addr()...
> - Call this function on init during i40e_probe()
> 

And the PBA value shouldn't be changing even with a new NVM image
flashed to the device, so its not something that is likely to have
updated at run time, thus saving during probe is sufficient.

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>

> Signed-off-by: Ivan Vecera <ivecera@xxxxxxxxxx>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_common.c | 58 +++++++++++--------
>  drivers/net/ethernet/intel/i40e/i40e_main.c   |  1 +
>  .../net/ethernet/intel/i40e/i40e_prototype.h  |  3 +-
>  drivers/net/ethernet/intel/i40e/i40e_type.h   |  3 +
>  4 files changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 6d1042ca0317..04db9cdc7d94 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -821,62 +821,72 @@ void i40e_pre_tx_queue_cfg(struct i40e_hw *hw, u32 queue, bool enable)
>  }
>  
>  /**
> - *  i40e_read_pba_string - Reads part number string from EEPROM
> + *  i40e_get_pba_string - Reads part number string from EEPROM
>   *  @hw: pointer to hardware structure
> - *  @pba_num: stores the part number string from the EEPROM
> - *  @pba_num_size: part number string buffer length
>   *
> - *  Reads the part number string from the EEPROM.
> + *  Reads the part number string from the EEPROM and stores it
> + *  into newly allocated buffer and saves resulting pointer
> + *  to i40e_hw->pba_id field.
>   **/
> -int i40e_read_pba_string(struct i40e_hw *hw, u8 *pba_num,
> -			 u32 pba_num_size)
> +void i40e_get_pba_string(struct i40e_hw *hw)
>  {
> +#define I40E_NVM_PBA_FLAGS_BLK_PRESENT	0xFAFA
>  	u16 pba_word = 0;
>  	u16 pba_size = 0;
>  	u16 pba_ptr = 0;
> -	int status = 0;
> -	u16 i = 0;
> +	int status;
> +	char *ptr;
> +	u16 i;
>  
>  	status = i40e_read_nvm_word(hw, I40E_SR_PBA_FLAGS, &pba_word);
> -	if (status || (pba_word != 0xFAFA)) {
> -		hw_dbg(hw, "Failed to read PBA flags or flag is invalid.\n");
> -		return status;
> +	if (status) {
> +		hw_dbg(hw, "Failed to read PBA flags.\n");
> +		return;
> +	}
> +	if (pba_word != I40E_NVM_PBA_FLAGS_BLK_PRESENT) {
> +		hw_dbg(hw, "PBA block is not present.\n");
> +		return;
>  	}
>  
>  	status = i40e_read_nvm_word(hw, I40E_SR_PBA_BLOCK_PTR, &pba_ptr);
>  	if (status) {
>  		hw_dbg(hw, "Failed to read PBA Block pointer.\n");
> -		return status;
> +		return;
>  	}
>  
>  	status = i40e_read_nvm_word(hw, pba_ptr, &pba_size);
>  	if (status) {
>  		hw_dbg(hw, "Failed to read PBA Block size.\n");
> -		return status;
> +		return;
>  	}
>  
>  	/* Subtract one to get PBA word count (PBA Size word is included in
> -	 * total size)
> +	 * total size) and advance pointer to first PBA word.
>  	 */
>  	pba_size--;
> -	if (pba_num_size < (((u32)pba_size * 2) + 1)) {
> -		hw_dbg(hw, "Buffer too small for PBA data.\n");
> -		return -EINVAL;
> +	pba_ptr++;
> +	if (!pba_size) {
> +		hw_dbg(hw, "PBA ID is empty.\n");
> +		return;
>  	}
>  
> +	ptr = devm_kzalloc(i40e_hw_to_dev(hw), pba_size * 2 + 1, GFP_KERNEL);
> +	if (!ptr)
> +		return;
> +	hw->pba_id = ptr;
> +
>  	for (i = 0; i < pba_size; i++) {
> -		status = i40e_read_nvm_word(hw, (pba_ptr + 1) + i, &pba_word);
> +		status = i40e_read_nvm_word(hw, pba_ptr + i, &pba_word);
>  		if (status) {
>  			hw_dbg(hw, "Failed to read PBA Block word %d.\n", i);
> -			return status;
> +			devm_kfree(i40e_hw_to_dev(hw), hw->pba_id);
> +			hw->pba_id = NULL;
> +			return;
>  		}
>  
> -		pba_num[(i * 2)] = (pba_word >> 8) & 0xFF;
> -		pba_num[(i * 2) + 1] = pba_word & 0xFF;
> +		*ptr++ = (pba_word >> 8) & 0xFF;
> +		*ptr++ = pba_word & 0xFF;
>  	}
> -	pba_num[(pba_size * 2)] = '\0';
> -
> -	return status;
>  }
>  
>  /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index ba8fb0556216..3157d14d9b12 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -15846,6 +15846,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		goto err_pf_reset;
>  	}
>  	i40e_get_oem_version(hw);
> +	i40e_get_pba_string(hw);
>  
>  	/* provide nvm, fw, api versions, vendor:device id, subsys vendor:device id */
>  	i40e_nvm_version_str(hw, nvm_ver, sizeof(nvm_ver));
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> index 46b9a05ceb91..001162042050 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> @@ -341,8 +341,7 @@ i40e_aq_configure_partition_bw(struct i40e_hw *hw,
>  			       struct i40e_aqc_configure_partition_bw_data *bw_data,
>  			       struct i40e_asq_cmd_details *cmd_details);
>  int i40e_get_port_mac_addr(struct i40e_hw *hw, u8 *mac_addr);
> -int i40e_read_pba_string(struct i40e_hw *hw, u8 *pba_num,
> -			 u32 pba_num_size);
> +void i40e_get_pba_string(struct i40e_hw *hw);
>  void i40e_pre_tx_queue_cfg(struct i40e_hw *hw, u32 queue, bool enable);
>  /* prototype for functions used for NVM access */
>  int i40e_init_nvm(struct i40e_hw *hw);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
> index dc7cd16ad8fb..aff6dc6afbe2 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_type.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
> @@ -493,6 +493,9 @@ struct i40e_hw {
>  	struct i40e_nvm_info nvm;
>  	struct i40e_fc_info fc;
>  
> +	/* PBA ID */
> +	const char *pba_id;
> +
>  	/* pci info */
>  	u16 device_id;
>  	u16 vendor_id;