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

[Roger Quadros <rogerq@kernel.org>]

On 24/07/2024 00:10, Brett Creeley wrote:
> 
> 
> On 7/3/2024 6:51 AM, Roger Quadros wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> am65-cpsw can support up to 8 queues at Rx.
>> Use a macro AM65_CPSW_MAX_RX_QUEUES to indicate that.
>> As there is only one DMA channel for RX traffic, the
>> 8 queues come as 8 flows in that channel.
>>
>> By default, we will start with 1 flow as defined by the
>> macro AM65_CPSW_DEFAULT_RX_CHN_FLOWS.
>>
>> User can change the number of flows by ethtool like so
>> 'ethtool -L ethx rx <N>'
>>
>> All traffic will still come on flow 0. To get traffic on
>> different flows the Classifiers will need to be set up.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> ---
>> Changelog:
>> v3:
>> - style fixes: reverse xmas tree and checkpatch.pl --max-line-length=80
>> - typo fix: Classifer -> Classifier
>> - added Reviewed-by Simon Horman
>> ---
>>   drivers/net/ethernet/ti/am65-cpsw-ethtool.c |  62 +++--
>>   drivers/net/ethernet/ti/am65-cpsw-nuss.c    | 367 ++++++++++++++++------------
>>   drivers/net/ethernet/ti/am65-cpsw-nuss.h    |  36 +--
>>   3 files changed, 284 insertions(+), 181 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
>> index a1d0935d1ebe..01e3967852e0 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
>> @@ -429,7 +429,7 @@ static void am65_cpsw_get_channels(struct net_device *ndev,
>>
>>          ch->max_rx = AM65_CPSW_MAX_RX_QUEUES;
>>          ch->max_tx = AM65_CPSW_MAX_TX_QUEUES;
>> -       ch->rx_count = AM65_CPSW_MAX_RX_QUEUES;
>> +       ch->rx_count = common->rx_ch_num_flows;
>>          ch->tx_count = common->tx_ch_num;
>>   }
>>
>> @@ -448,8 +448,10 @@ static int am65_cpsw_set_channels(struct net_device *ndev,
>>                  return -EBUSY;
>>
>>          am65_cpsw_nuss_remove_tx_chns(common);
>> +       am65_cpsw_nuss_remove_rx_chns(common);
>>
>> -       return am65_cpsw_nuss_update_tx_chns(common, chs->tx_count);
>> +       return am65_cpsw_nuss_update_tx_rx_chns(common, chs->tx_count,
>> +                                               chs->rx_count);
>>   }
>>
>>   static void
>> @@ -920,11 +922,13 @@ static int am65_cpsw_get_coalesce(struct net_device *ndev, struct ethtool_coales
>>                                    struct netlink_ext_ack *extack)
>>   {
>>          struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
>> +       struct am65_cpsw_rx_flow *rx_flow;
>>          struct am65_cpsw_tx_chn *tx_chn;
>>
>>          tx_chn = &common->tx_chns[0];
>> +       rx_flow = &common->rx_chns.flows[0];
>>
>> -       coal->rx_coalesce_usecs = common->rx_pace_timeout / 1000;
>> +       coal->rx_coalesce_usecs = rx_flow->rx_pace_timeout / 1000;
>>          coal->tx_coalesce_usecs = tx_chn->tx_pace_timeout / 1000;
>>
>>          return 0;
>> @@ -934,14 +938,26 @@ static int am65_cpsw_get_per_queue_coalesce(struct net_device *ndev, u32 queue,
>>                                              struct ethtool_coalesce *coal)
>>   {
>>          struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
>> +       struct am65_cpsw_rx_flow *rx_flow;
>>          struct am65_cpsw_tx_chn *tx_chn;
>>
>> -       if (queue >= AM65_CPSW_MAX_TX_QUEUES)
>> +       if (queue >= AM65_CPSW_MAX_TX_QUEUES &&
>> +           queue >= AM65_CPSW_MAX_RX_QUEUES)
>>                  return -EINVAL;
>>
>> -       tx_chn = &common->tx_chns[queue];
>> +       if (queue < AM65_CPSW_MAX_TX_QUEUES) {
>> +               tx_chn = &common->tx_chns[queue];
>> +               coal->tx_coalesce_usecs = tx_chn->tx_pace_timeout / 1000;
>> +       } else {
>> +               coal->tx_coalesce_usecs = ~0;
>> +       }
>>
>> -       coal->tx_coalesce_usecs = tx_chn->tx_pace_timeout / 1000;
>> +       if (queue < AM65_CPSW_MAX_RX_QUEUES) {
>> +               rx_flow = &common->rx_chns.flows[queue];
>> +               coal->rx_coalesce_usecs = rx_flow->rx_pace_timeout / 1000;
>> +       } else {
>> +               coal->rx_coalesce_usecs = ~0;
>> +       }
> 
> Minor nit, but after removing the dead code below the check for queue being greater than max values, I think am65_cpsw_get_coalesce() and am65_get_per_queue_coalesce() are identical except the "u32 queue" argument.
> 
> I think you could do something like the following:
> 
> static int am65_cpsw_get_per_queue_coalesce(struct net_device *ndev,
>                   struct ethtool_coalesce *coal,
>                   struct netlink_ext_ack *extack)
> {
>     return __am65_cpsw_get_coalesce(ndev, coal, 0);
> }
> 
> static int am65_cpsw_get_coalesce(struct net_device *ndev, u32 queue,
>                   struct ethtool_coalesce *coal,
>                   struct netlink_ext_ack *extack,
>                   u32 )
> {
>     return __am65_cpsw_get_coalesce(ndev, coal, queue);
> }
> 

Sure, this is much nicer.

>>
>>          return 0;
>>   }
>> @@ -951,9 +967,11 @@ static int am65_cpsw_set_coalesce(struct net_device *ndev, struct ethtool_coales
>>                                    struct netlink_ext_ack *extack)
>>   {
>>          struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
>> +       struct am65_cpsw_rx_flow *rx_flow;
>>          struct am65_cpsw_tx_chn *tx_chn;
>>
>>          tx_chn = &common->tx_chns[0];
>> +       rx_flow = &common->rx_chns.flows[0];
>>
>>          if (coal->rx_coalesce_usecs && coal->rx_coalesce_usecs < 20)
>>                  return -EINVAL;
>> @@ -961,7 +979,7 @@ static int am65_cpsw_set_coalesce(struct net_device *ndev, struct ethtool_coales
>>          if (coal->tx_coalesce_usecs && coal->tx_coalesce_usecs < 20)
>>                  return -EINVAL;
> 
> Why does this return -EINVAL here, but am65_cpsw_set_per_queue_coalesce() prints a dev_info() and then set the value to 20?
> 
> Would it better to have consistent behavior? Maybe I'm missing some context or reasoning here?

Consistent behaviour is better.

> 
>>
>> -       common->rx_pace_timeout = coal->rx_coalesce_usecs * 1000;
>> +       rx_flow->rx_pace_timeout = coal->rx_coalesce_usecs * 1000;
>>          tx_chn->tx_pace_timeout = coal->tx_coalesce_usecs * 1000;
>>
>>          return 0;
>> @@ -971,20 +989,36 @@ static int am65_cpsw_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
>>                                              struct ethtool_coalesce *coal)
>>   {
>>          struct am65_cpsw_common *common = am65_ndev_to_common(ndev);
>> +       struct am65_cpsw_rx_flow *rx_flow;
>>          struct am65_cpsw_tx_chn *tx_chn;
>>
>> -       if (queue >= AM65_CPSW_MAX_TX_QUEUES)
>> +       if (queue >= AM65_CPSW_MAX_TX_QUEUES &&
>> +           queue >= AM65_CPSW_MAX_RX_QUEUES)
>>                  return -EINVAL;
>>
>> -       tx_chn = &common->tx_chns[queue];
>> +       if (queue < AM65_CPSW_MAX_TX_QUEUES) {
>> +               tx_chn = &common->tx_chns[queue];
>> +
>> +               if (coal->tx_coalesce_usecs && coal->tx_coalesce_usecs < 20) {
>> +                       dev_info(common->dev, "defaulting to min value of 20us for tx-usecs for tx-%u\n",
>> +                                queue);
>> +                       coal->tx_coalesce_usecs = 20;
>> +               }
>>
>> -       if (coal->tx_coalesce_usecs && coal->tx_coalesce_usecs < 20) {
>> -               dev_info(common->dev, "defaulting to min value of 20us for tx-usecs for tx-%u\n",
>> -                        queue);
>> -               coal->tx_coalesce_usecs = 20;
>> +               tx_chn->tx_pace_timeout = coal->tx_coalesce_usecs * 1000;
>>          }
>>
>> -       tx_chn->tx_pace_timeout = coal->tx_coalesce_usecs * 1000;
>> +       if (queue < AM65_CPSW_MAX_RX_QUEUES) {
>> +               rx_flow = &common->rx_chns.flows[queue];
>> +
>> +               if (coal->rx_coalesce_usecs && coal->rx_coalesce_usecs < 20) {
>> +                       dev_info(common->dev, "defaulting to min value of 20us for rx-usecs for rx-%u\n",
>> +                                queue);
> 
> Would it make more sense to just return -EINVAL here similar to the standard "set_coalesce"?

Yes, I'll do that in next spin.

> 
>> +                       coal->rx_coalesce_usecs = 20;
>> +               }
>> +
>> +               rx_flow->rx_pace_timeout = coal->rx_coalesce_usecs * 1000;
>> +       }
>>
>>          return 0;
>>   }
> 
> I think my comment to the "get" and "get_per_queue" versions of these functions also applies here, but only if the behavior of returning -EINVAL or setting a value for the user is the same between the "set" and "set_per_queue".

Thanks for the review!

> 
> Thanks,
> 
> Brett
> 
> <snip>
> 

-- 
cheers,
-roger