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

From: "Liu, Lingyu" <lingyu.liu@intel.com>
To: Przemek Kitszel <przemyslaw.kitszel@intel.com>,
	<intel-wired-lan@lists.osuosl.org>
Cc: kevin.tian@intel.com, yi.l.liu@intel.com,
	phani.r.burra@intel.com, Xu Ting <ting.xu@intel.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next V1 01/15] ice: Fix missing legacy 32byte RXDID in the supported bitmap
Date: Wed, 21 Jun 2023 17:16:21 +0800	[thread overview]
Message-ID: <30e05bf7-54ab-733b-f924-fa8ac49a3bf1@intel.com> (raw)
In-Reply-To: <85ae556e-4038-2618-c8be-7ca035daa024@intel.com>


On 6/20/2023 7:05 PM, Przemek Kitszel wrote:
> 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.
>
Hi Przemek, thanks for your valuable reviews. Update in V2.

>>
>> 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?
>
Brought back in V2. Thanks.

>> -     * 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")?
>
Thanks for the suggestion. Added in V2.

>> +     * 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