Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Fix packet still tx after gate close by reducing i226 MAC retry buffer

["Abdul Rahim, Faizal" <faizal.abdul.rahim@linux.intel.com>]

Hi Paul,

Thanks for reviewing.

On 1/7/2024 8:42 pm, Paul Menzel wrote:
> Dear Faizal,
> 
> 
> Thank you for your patch.
> 
> Am 01.07.24 um 12:00 schrieb Faizal Rahim:
>> AVNU testing uncovered that even when the taprio gate is closed,
>> some packets still transmit.
> 
> What is AVNU? *some* would fit on the line above.

AVNU stands for "Avnu Alliance." AVNU (Audio Video Bridging Network 
Alliance) is an industry consortium that promotes and certifies 
interoperability of devices implementing IEEE 802.1 standards for 
time-sensitive applications.

This AVNU test refers to AVNU certification test plan.
Should I add this information in the commit ?

>> A known i225/6 hardware errata states traffic might overflow the planned
> 
> Do you have an idea for that errata? Please document it. (I see you added 
> it at the end. Maybe use [1] notation for referencing it.)

Sure.

>> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
> 
> As you Cc’ed stable@vger.kernel.org, add a Fixes: tag?

Accidentally CC'ed stable@vger.kernel.org.
Since it's a hardware bug, not software, probably Fixes: tag not needed ?
Not sure which Fixes: commit to point to hmm.

I'll remove stable kernel email and omit Fixes: tag, is that okay?

>>   /* Returns the TSN specific registers to their default values after
>>    * the adapter is reset.
>>    */
>> @@ -91,6 +100,9 @@ static int igc_tsn_disable_offload(struct igc_adapter 
>> *adapter)
>>       wr32(IGC_TXPBS, I225_TXPBSIZE_DEFAULT);
>>       wr32(IGC_DTXMXPKTSZ, IGC_DTXMXPKTSZ_DEFAULT);
>> +    if (igc_is_device_id_i226(hw))
>> +        igc_tsn_restore_retx_default(adapter);
>> +
>>       tqavctrl = rd32(IGC_TQAVCTRL);
>>       tqavctrl &= ~(IGC_TQAVCTRL_TRANSMIT_MODE_TSN |
>>                 IGC_TQAVCTRL_ENHANCED_QAV | IGC_TQAVCTRL_FUTSCDDIS);
>> @@ -111,6 +123,25 @@ static int igc_tsn_disable_offload(struct 
>> igc_adapter *adapter)
>>       return 0;
>>   }
>> +/* To partially fix i226 HW errata, reduce MAC internal buffering from 
>> 192 Bytes
>> + * to 88 Bytes by setting RETX_CTL register using the recommendation from:
>> + * a) Ethernet Controller I225/I22 Specification Update Rev 2.1
>> + *    Item 9: TSN: Packet Transmission Might Cross the Qbv Window
>> + * b) I225/6 SW User Manual Rev 1.2.4: Section 8.11.5 Retry Buffer Control
>> + */
>> +static void igc_tsn_set_retx_qbvfullth(struct igc_adapter *adapter)
> 
> It’d put threshold in the name.

My earlier thought is that it is easier to look for the keyword "qbvfullth" 
in the i226 SW User Manual where you'll get a hit that brings you directly 
to that register. "qbvfullthreshold" would not.
There are some comments in the new code that links 'th' to 'threshold' for 
the reader.

But I'm okay to change it to "qbvfullthreshold".
Thoughts?

Regards,
Faizal