Message-ID: <b8845a87-e27a-4834-b510-f9fde51a364c@intel.com>
Date: Wed, 21 Aug 2024 15:52:51 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
	<horms@...nel.org>, <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
	<alexandr.lobakin@...el.com>
Subject: Re: [PATCH iwl-next v10 04/14] iavf: add support for negotiating
 flexible RXDID format

From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Wed, 21 Aug 2024 14:15:29 +0200

> From: Jacob Keller <jacob.e.keller@...el.com>
> 
> Enable support for VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC, to enable the VF
> driver the ability to determine what Rx descriptor formats are
> available. This requires sending an additional message during
> initialization and reset, the VIRTCHNL_OP_GET_SUPPORTED_RXDIDS. This
> operation requests the supported Rx descriptor IDs available from the
> PF.
> 
> This is treated the same way that VLAN V2 capabilities are handled. Add
> a new set of extended capability flags, used to process send and receipt
> of the VIRTCHNL_OP_GET_SUPPORTED_RXDIDS message.
> 
> This ensures we finish negotiating for the supported descriptor formats
> prior to beginning configuration of receive queues.
> 
> This change stores the supported format bitmap into the iavf_adapter
> structure. Additionally, if VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC is enabled
> by the PF, we need to make sure that the Rx queue configuration
> specifies the format.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@...el.com>

[...]

> +/**
> + * iavf_select_rx_desc_format - Select Rx descriptor format
> + * @adapter: adapter private structure
> + *
> + * Select what Rx descriptor format based on availability and enabled
> + * features.
> + *
> + * Return: the desired RXDID to select for a given Rx queue, as defined by
> + *         enum virtchnl_rxdid_format.
> + */
> +static u8 iavf_select_rx_desc_format(const struct iavf_adapter *adapter)
> +{
> +	u64 rxdids = adapter->supp_rxdids;
> +
> +	/* If we did not negotiate VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC, we must
> +	 * stick with the default value of the legacy 32 byte format.
> +	 */
> +	if (!IAVF_RXDID_ALLOWED(adapter))
> +		return VIRTCHNL_RXDID_1_32B_BASE;
> +
> +	/* Warn if the PF does not list support for the default legacy
> +	 * descriptor format. This shouldn't happen, as this is the format
> +	 * used if VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC is not supported. It is
> +	 * likely caused by a bug in the PF implementation failing to indicate
> +	 * support for the format.
> +	 */
> +	if (!(rxdids & VIRTCHNL_RXDID_1_32B_BASE_M))
> +		dev_warn(&adapter->pdev->dev, "PF does not list support for default Rx descriptor format\n");

pci_warn() or netdev_warn() if netdev is available here, sorry if I
didn't mention this earlier =\

> +
> +	return VIRTCHNL_RXDID_1_32B_BASE;
> +}
> +
>  /**
>   * iavf_configure_rx - Configure Receive Unit after Reset
>   * @adapter: board private structure

[...]

> @@ -262,6 +276,37 @@ int iavf_get_vf_vlan_v2_caps(struct iavf_adapter *adapter)
>  	return err;
>  }
>  
> +int iavf_get_vf_supported_rxdids(struct iavf_adapter *adapter)
> +{
> +	struct iavf_hw *hw = &adapter->hw;
> +	struct iavf_arq_event_info event;
> +	u32 len = sizeof(u64);
> +	enum virtchnl_ops op;
> +	enum iavf_status err;
> +	u8 rxdids;
> +
> +	event.msg_buf = &rxdids;
> +	event.buf_len = len;

This looks suspicious. @rxdids is u8, while @len is sizeof(u64), i.e 8
bytes, not 1. Is this intended? Or maybe @rxdids should be u64 here as
well, just like adapter->supported_rxdids?

> +
> +	while (1) {

@op can be declared right here.
@err can be also declared right here if you address the comment below.

> +		/* When the AQ is empty, iavf_clean_arq_element will return
> +		 * nonzero and this loop will terminate.
> +		 */
> +		err = iavf_clean_arq_element(hw, &event, NULL);
> +		if (err != IAVF_SUCCESS)
> +			return err;
> +		op = le32_to_cpu(event.desc.cookie_high);
> +		if (op == VIRTCHNL_OP_GET_SUPPORTED_RXDIDS)

When one of the elements you want to compare is a compile-time constant,
you will get more optimized code if you do

		__le32 op;

		op = event.desc.cookie_high;
		if (op == cpu_to_le32(VIRTCHNL_OP_GET_SUPPORTED_RXDIDS))

because then you won't need to byteswap a variable and constants get
byteswapped at compilation time.

But given that iavf runs on LE 99% of time and it's not hotpath, it's up
to you whether to do it like that here or just leave as it is.

> +			break;
> +	}
> +
> +	err = le32_to_cpu(event.desc.cookie_low);
> +	if (!err)

	if (!event.desc.cookie_low)

Because 0 == le32_to_cpu(0), it's always 0.
So you don't need @err here and it can be declared inside the loop above.

> +		adapter->supp_rxdids = rxdids;
> +
> +	return 0;
> +}
> +
>  /**
>   * iavf_configure_queues
>   * @adapter: adapter structure
> @@ -308,6 +353,8 @@ void iavf_configure_queues(struct iavf_adapter *adapter)
>  		vqpi->rxq.dma_ring_addr = adapter->rx_rings[i].dma;
>  		vqpi->rxq.max_pkt_size = max_frame;
>  		vqpi->rxq.databuffer_size = adapter->rx_rings[i].rx_buf_len;
> +		if (IAVF_RXDID_ALLOWED(adapter))
> +			vqpi->rxq.rxdid = adapter->rxdid;
>  		if (CRC_OFFLOAD_ALLOWED(adapter))
>  			vqpi->rxq.crc_disable = !!(adapter->netdev->features &
>  						   NETIF_F_RXFCS);
> @@ -2372,6 +2419,10 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
>  			aq_required;
>  		}
>  		break;
> +	case VIRTCHNL_OP_GET_SUPPORTED_RXDIDS:
> +		memcpy(&adapter->supp_rxdids, msg,
> +		       min_t(u16, msglen, sizeof(adapter->supp_rxdids)));

Why is this needed if you assign ->supp_rxdids in
iavf_get_vf_supported_rxdids()? Or is this something different?

I'd also say this memcpy() is not safe. ->supp_rxdids is u64. If somehow
@msglen is less than 8 bytes, you'd probably get a corrupted u64 value.
I think you should compare @msglen to sizeof(u64) and bail out if it's
different. If it's the same, you should just do

		adapter->supp_rxdids = *(u64 *)msg;

> +		break;
>  	case VIRTCHNL_OP_ENABLE_QUEUES:
>  		/* enable transmits */
>  		iavf_irq_enable(adapter, true);
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> index b60df6e9b3e7..3c2d6a504aa0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> @@ -2709,12 +2709,12 @@ static int ice_vc_set_rss_hena(struct ice_vf *vf, u8 *msg)
>  static int ice_vc_query_rxdid(struct ice_vf *vf)
>  {
>  	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
> -	struct virtchnl_supported_rxdids *rxdid = NULL;
>  	struct ice_hw *hw = &vf->pf->hw;
>  	struct ice_pf *pf = vf->pf;
> -	int len = 0;
> -	int ret, i;
> +	u32 len = sizeof(u64);
>  	u32 regval;
> +	u64 rxdid;
> +	int ret, i;

RCT broke here =\

>  
>  	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
>  		v_ret = VIRTCHNL_STATUS_ERR_PARAM;

Thanks,
Olek

Powered by blists - more mailing lists