[PATCH] net: dwc_eth_qos: Pad descriptors to cacheline size

[Marek Vasut]

On 4/29/20 9:51 PM, Stephen Warren wrote:
> On 4/29/20 1:14 PM, Marek Vasut wrote:
>> The DWMAC4 IP has the possibility to skip up to 7 64bit words after
>> the descriptor, use this to pad the descriptors to cacheline size.
> 
>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> 
>>  /* Descriptors */
>> +/*
> 
> Nit: Blank line between those two comments to match the existing style.
> 
>> + * #if EQOS_DESCRIPTOR_SIZE < ARCH_DMA_MINALIGN fails if we use sizeof()
>> + * or offsetof() to calculate descriptor size, since neither is allowed
>> + * in C preprocessor macros, so just hard-code this to 16=4*sizeof(u32).
>> + */
>> +#define EQOS_DESCRIPTOR_SIZE	16
>> +#define EQOS_DESCRIPTOR_PAD	((ARCH_DMA_MINALIGN - EQOS_DESCRIPTOR_SIZE) / 4)
>> +struct eqos_desc {
>> +	u32 des0;
>> +	u32 des1;
>> +	u32 des2;
>> +	u32 des3;
>> +	u32 __pad[EQOS_DESCRIPTOR_PAD];
>> +};
> 
> I think the padding needs to be optional based on the same condition in
> the following existing warning logic, and the warning logic may need to
> be removed/updated to account for the fact that padding is now being
> used to avoid the issue:
> 
> (quoting this from existing code)
>> /*
>>  * Warn if the cache-line size is larger than the descriptor size. In such
>>  * cases the driver will likely fail because the CPU needs to flush the cache
>>  * when requeuing RX buffers, therefore descriptors written by the hardware
>>  * may be discarded. Architectures with full IO coherence, such as x86, do not
>>  * experience this issue, and hence are excluded from this condition.
>>  *
>>  * This can be fixed by defining CONFIG_SYS_NONCACHED_MEMORY which will cause
>>  * the driver to allocate descriptors from a pool of non-cached memory.
>>  */
>> #if EQOS_DESCRIPTOR_SIZE < ARCH_DMA_MINALIGN
>> #if !defined(CONFIG_SYS_NONCACHED_MEMORY) && \
>>         !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) && !defined(CONFIG_X86)
>> #warning Cache line size is larger than descriptor size
>> #endif
>> #endif
> 
> (back to quoting from the patch)
>> -#define EQOS_DESCRIPTOR_WORDS	4
>> -#define EQOS_DESCRIPTOR_SIZE	(EQOS_DESCRIPTOR_WORDS * 4)
> ...
>> -struct eqos_desc {
>> -	u32 des0;
>> -	u32 des1;
>> -	u32 des2;
>> -	u32 des3;
>> -};
> 
> This patch would be a lot smaller if those definitions were kept in
> their existing location instead of moving them...

Could you at least test it on the tegra ?