Re: [PATCH net v3 1/2] net: ethernet: ti: am65-cpsw: Fix multi queue Rx on J7

[Roger Quadros <rogerq@kernel.org>]

Hi Simon,

On 05/11/2024 15:21, Simon Horman wrote:
> On Fri, Nov 01, 2024 at 12:18:50PM +0200, Roger Quadros wrote:
>> On J7 platforms, setting up multiple RX flows was failing
>> as the RX free descriptor ring 0 is shared among all flows
>> and we did not allocate enough elements in the RX free descriptor
>> ring 0 to accommodate for all RX flows.
>>
>> This issue is not present on AM62 as separate pair of
>> rings are used for free and completion rings for each flow.
>>
>> Fix this by allocating enough elements for RX free descriptor
>> ring 0.
>>
>> However, we can no longer rely on desc_idx (descriptor based
>> offsets) to identify the pages in the respective flows as
>> free descriptor ring includes elements for all flows.
>> To solve this, introduce a new swdata data structure to store
>> flow_id and page. This can be used to identify which flow (page_pool)
>> and page the descriptor belonged to when popped out of the
>> RX rings.
>>
>> Fixes: da70d184a8c3 ("net: ethernet: ti: am65-cpsw: Introduce multi queue Rx")
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
>> @@ -764,8 +759,8 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  
>>  fail_rx:
>>  	for (i = 0; i < common->rx_ch_num_flows; i++)
>> -		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, &rx_chn->flows[i],
>> -					  am65_cpsw_nuss_rx_cleanup, 0);
>> +		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, rx_chn,
>> +					  am65_cpsw_nuss_rx_cleanup, !!i);
> 
> Hi Roger,
> 
> I wonder if, as a follow-up, the skip_fdq (last) parameter of
> k3_udma_glue_reset_rx_chn() can be dropped. It seems that all callers
> follow the pattern above of passing i as the flow_num (2nd) argument,
> and !!i as the skip_fdq argument. If so, k3_udma_glue_reset_rx_chn could
> simply derive skip_fdq as !!flow_num.

Added Peter to the discussion.

My understanding is that this is not always the case as some users can still
decide to use separate free descriptor rings per flow. i.e. K3_RINGACC_RING_SHARED
flag not set in rxfdq_cfg.

But we could infer that bit from the flow configuration that is passed into
k3_udma_glue_rx_flow_init() and save this information into k3_udma_glue_rx_channel.

This could then be used internally and we drop skip_fdq argument.

Peter, do you agree?

-- 
cheers,
-roger