Re: [PATCH net-next v3 1/6] net: ethernet: ti: am65-cpsw: Introduce multi queue Rx

[Roger Quadros]

On 23/07/2024 20:11, Joe Damato wrote:
> On Wed, Jul 03, 2024 at 04:51:32PM +0300, Roger Quadros wrote:
> 
> [...]
> 
>> @@ -699,6 +727,14 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  		goto fail_rx;
>>  	}
>>  
>> +	for (i = 0; i < common->rx_ch_num_flows ; i++) {
>> +		napi_enable(&common->rx_chns.flows[i].napi_rx);
>> +		if (common->rx_chns.flows[i].irq_disabled) {
>> +			common->rx_chns.flows[i].irq_disabled = false;
> 
> Just a minor nit (not a reason to hold this back): I've been
> encouraging folks to use the new netdev-genl APIs in their drivers
> to map NAPIs to queues and IRQs if possible because it allows for
> more expressive and interesting userland applications.
> 
> You may consider in the future using something vaguely like (this is
> untested psuedo-code I just typed out):
> 
>    netif_napi_set_irq(&common->rx_chns.flows[i].napi_rx,
>                       common->rx_chns.flows[i].irq);
> 
> and 
> 
>    netif_queue_set_napi(common->dev, i, NETDEV_QUEUE_TYPE_RX,
>                         &common->rx_chns.flows[i].napi_rx);
> 
> To link everything together (note that RTNL must be held while doing
> this -- I haven't checked your code path to see if that is true here).
> 
> For an example, see 64b62146ba9e ("net/mlx4: link NAPI instances to
> queues and IRQs). 
> 
> Doing this would allow userland to get data via netlink, which you
> can examine yourself by using cli.py like this:
> 
> python3 tools/net/ynl/cli.py \
>   --spec Documentation/netlink/specs/netdev.yaml \
>   --dump queue-get
> 
> python3 tools/net/ynl/cli.py \
>   --spec Documentation/netlink/specs/netdev.yaml \
>   --dump napi-get
> 

Thanks for the pionters. I will check and see if I can incorportate
this in the next spin.

>> +			enable_irq(common->rx_chns.flows[i].irq);
>> +		}
>> +	}
>> +
>>  	for (tx = 0; tx < common->tx_ch_num; tx++) {
>>  		ret = k3_udma_glue_enable_tx_chn(tx_chn[tx].tx_chn);
>>  		if (ret) {
>> @@ -710,12 +746,6 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  		napi_enable(&tx_chn[tx].napi_tx);
>>  	}
>>  
>> -	napi_enable(&common->napi_rx);
>> -	if (common->rx_irq_disabled) {
>> -		common->rx_irq_disabled = false;
>> -		enable_irq(rx_chn->irq);
>> -	}
>> -
>>  	dev_dbg(common->dev, "cpsw_nuss started\n");
>>  	return 0;
>>  
>> @@ -726,11 +756,24 @@ static int am65_cpsw_nuss_common_open(struct am65_cpsw_common *common)
>>  		tx--;
>>  	}
>>  
>> +	for (flow_idx = 0; i < common->rx_ch_num_flows; flow_idx++) {
>> +		flow = &rx_chn->flows[flow_idx];
>> +		if (!flow->irq_disabled) {
>> +			disable_irq(flow->irq);
>> +			flow->irq_disabled = true;
>> +		}
>> +		napi_disable(&flow->napi_rx);
>> +	}
>> +
>>  	k3_udma_glue_disable_rx_chn(rx_chn->rx_chn);
>>  
>>  fail_rx:
>> -	k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, 0, rx_chn,
>> -				  am65_cpsw_nuss_rx_cleanup, 0);
>> +	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, !!i);
>> +
>> +	am65_cpsw_destroy_xdp_rxqs(common);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -779,12 +822,12 @@ static int am65_cpsw_nuss_common_stop(struct am65_cpsw_common *common)
>>  			dev_err(common->dev, "rx teardown timeout\n");
>>  	}
>>  
>> -	napi_disable(&common->napi_rx);
>> -	hrtimer_cancel(&common->rx_hrtimer);
>> -
>> -	for (i = 0; i < AM65_CPSW_MAX_RX_FLOWS; i++)
>> -		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, rx_chn,
>> +	for (i = 0; i < common->rx_ch_num_flows; i++) {
>> +		napi_disable(&common->rx_chns.flows[i].napi_rx);
> 
> The inverse of the above is probably true somewhere around here;
> again a small piece of psuedo code for illustrative purposes:
> 
>    netif_queue_set_napi(common->dev, i, NETDEV_QUEUE_TYPE_RX, NULL);
> 
>> +		hrtimer_cancel(&common->rx_chns.flows[i].rx_hrtimer);
>> +		k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, i, &rx_chn->flows[i],
>>  					  am65_cpsw_nuss_rx_cleanup, !!i);
>> +	}
>>  
>>  	k3_udma_glue_disable_rx_chn(rx_chn->rx_chn);
>>  

-- 
cheers,
-roger