Re: [PATCH RFC net-next v2] net: airoha: Add TCP LRO support

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Tue, 26 May 2026 23:08:14 +0200

>> From: Lorenzo Bianconi <lorenzo@kernel.org>
>> Date: Tue, 26 May 2026 08:58:05 +0200

[...]

>>> @@ -587,6 +630,85 @@ static int airoha_qdma_get_gdm_port(struct airoha_eth *eth,
>>>  	return port >= ARRAY_SIZE(eth->ports) ? -EINVAL : port;
>>>  }
>>>  
>>> +static int airoha_qdma_lro_rx_process(struct airoha_queue *q,
>>> +				      struct airoha_qdma_desc *desc)
>>> +{
>>> +	u32 desc_ctrl = le32_to_cpu(READ_ONCE(desc->ctrl));
>>> +	u32 msg1 = le32_to_cpu(READ_ONCE(desc->msg1));
>>> +	u32 msg2 = le32_to_cpu(READ_ONCE(desc->msg2));
>>> +	u32 msg3 = le32_to_cpu(READ_ONCE(desc->msg3));
>>
>> Why are these READ_ONCE()s needed? Does desc come from the HW (sorry I
>> didn't follow the whole code flow) or...?
> 
> Correct, ctrl, msg1, msg2 and msg3 are subfields of the DMA descriptor read by
> airoha_qdma_rx_process() from the NIC. I guess here we have a similar issue as
> the one fixed in [0]
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=4ae0604a0673e11e2075b178387151fcad5111b5

Hmm, I never believed that we need READ_ONCE()s when reading HW
descriptors, I always thought dma_alloc_coherent() is enough.
But maybe I've been mistaking...

[...]
>>> +
>>> +		if (*topt == cpu_to_be32((TCPOPT_NOP << 24) |
>>
>> Shouldn't this be `((u32)TCPOPT_NOP) << 24` to avoid sign issues?
> 
> I guess this is same approach used in [1]. Am I missing something?
> 
> [1] https://github.com/torvalds/linux/blob/master/net/ipv4/tcp_ipv4.c#L823

Yeah I also notice this in one more patch when scrolling netdev
yesterday. I guess it's okay here since TCPOPT_NOP is small and
constant and doesn't reach bit 31.

> 
> Regards,
> Lorenzo

Thanks,
Olek