[Intel-wired-lan] [PATCH iwl-next v6 3/4] ice: Fix VLAN and CRC strip co-existence

[Ahmed Zaki]

On 2023-09-05 22:09, Paul Menzel wrote:
> Dear Ahmed, dear Haiyue,
>
>
> Thank you for the patch. Maybe reword the commit message summary to:
>
> ice: Check CRC strip requirement for VLAN strip
>
> Am 05.09.23 um 20:50 schrieb Ahmed Zaki:
>> From: Haiyue Wang <haiyue.wang at intel.com>
>>
>> When VLAN strip is enabled, the CRC strip should not be allowed to be
>
> must not be disabled
>
> or:
>
> VLAN strip requires CRC strip according to datasheet ….
>
>> disabled. And when CRC strip is disabled, the VLAN strip should not be
>> allowed to be enabled.
>>
>> It needs to check CRC strip disable setting parameter firstly before
>> configuring the Rx/Tx queues, otherwise, in current error handling,
>> the already set Tx queue context doesn't rollback correctly, it will
>
> The verb roll back is spelled with space (simple past is rolled back).
>
>>
<..>
>>
>>   +/**
>> + * ice_vsi_is_rxq_crc_strip_dis - check if Rx queue CRC strip is 
>> disabled or not
>> + * @vsi: pointer to the VF VSI info
>> + */
>> +static bool ice_vsi_is_rxq_crc_strip_dis(struct ice_vsi *vsi)
>> +{
>> +    u16 i;
>
> Please use the non-fixed length type `unsigned int` [1].
>
>> +
>> +    ice_for_each_alloc_rxq(vsi, i)
>> +        if (vsi->rx_rings[i]->flags & ICE_RX_FLAGS_CRC_STRIP_DIS)
>> +            return true;
<..>
>> +            vf->vlan_strip_ena &= ~ICE_OUTER_VLAN_STRIP_ENA;
>>           }
>>       }
>>   @@ -3551,6 +3594,9 @@ static int 
>> ice_vc_dis_vlan_stripping_v2_msg(struct ice_vf *vf, u8 *msg)
>>           goto out;
>>       }
>>   +    if (ethertype_setting)
>> +        vf->vlan_strip_ena &= ~ICE_INNER_VLAN_STRIP_ENA;
>> +
>>   out:
>>       return ice_vc_send_msg_to_vf(vf, 
>> VIRTCHNL_OP_DISABLE_VLAN_STRIPPING_V2,
>>                        v_ret, NULL, 0);
>
> Reviewed-by: Paul Menzel <pmenzel at molgen.mpg.de>
>
>
> Kind regards,
>
> Paul
>
>
> [1]: 
> https://web.archive.org/web/*/https://notabs.org/coding/smallIntsBigPenalty.htm


Thanks for the review. This and your other comments will be included in v7.

Ahmed