Re: [Intel-wired-lan] [PATCH iwl-next V1 01/15] ice: Fix missing legacy 32byte RXDID in the supported bitmap

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 6/20/23 11:59, Lingyu Liu wrote:
> From: Xu Ting <ting.xu@intel.com>
> 
> 32byte legacy descriptor format is preassigned.
> Commit e753df8fbca5 ("ice: Add support Flex RXD") created a
> supported RXDIDs bitmap according to DDP package. But it missed
> the legacy 32byte RXDID since it is not listed in the package.
> This patch adds this RXDID to the bitmap.

Please reword into imperative mood, perhaps staring with "Mark 32byte 
legacy descriptor format as supported int the supported RXDIDs flags", 
or similar.

> 
> Signed-off-by: Xu Ting <ting.xu@intel.com>
> Signed-off-by: Lingyu Liu <lingyu.liu@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_virtchnl.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> index 92490fe655ea..ae1e09f0875b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
> @@ -2615,12 +2615,14 @@ static int ice_vc_query_rxdid(struct ice_vf *vf)
>   		goto err;
>   	}
>   
> -	/* Read flexiflag registers to determine whether the
> -	 * corresponding RXDID is configured and supported or not.

This part of the comment was valuable too (esp given all the bit 
twiddling in the loop). Perhaps you could bring it back?

> -	 * Since Legacy 16byte descriptor format is not supported,
> -	 * start from Legacy 32byte descriptor.
> +	/* RXDIDs supported by DDP package can be read from the register
> +	 * to get the supported RXDID bitmap. But the legacy 32byte RXDID
> +	 * is not listed in DDP package, add it in the bitmap manually.

I had to grep defines of ICE_RXDID_FLEX_NIC and ICE_RXDID_LEGACY_1 to 
ensure that the code is correct (it is), comment above has certainly 
helped me. Perhaps you could add "(and skip check for it in the loop)" 
at the end of the above sentence (after "manually")?

> +	 * Legacy 16byte descriptor is not supported.
>   	 */
> -	for (i = ICE_RXDID_LEGACY_1; i < ICE_FLEX_DESC_RXDID_MAX_NUM; i++) {
> +	rxdid->supported_rxdids |= BIT(ICE_RXDID_LEGACY_1);
> +
> +	for (i = ICE_RXDID_FLEX_NIC; i < ICE_FLEX_DESC_RXDID_MAX_NUM; i++) {
>   		regval = rd32(hw, GLFLXP_RXDID_FLAGS(i, 0));
>   		if ((regval >> GLFLXP_RXDID_FLAGS_FLEXIFLAG_4N_S)
>   			& GLFLXP_RXDID_FLAGS_FLEXIFLAG_4N_M)

Codewise it's ok!
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan