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
