Re: [PATCH v3 15/16] can: m_can: Implement BQL

[Simon Horman]

On Wed, Mar 15, 2023 at 12:05:45PM +0100, Markus Schneider-Pargmann wrote:
> Implement byte queue limiting in preparation for the use of xmit_more().
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>

Nits below aside, this looks good to me.

Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>

> ---
>  drivers/net/can/m_can/m_can.c | 49 +++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 3cb3d01e1a61..63d6e95717e3 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c

...

> @@ -999,29 +1001,34 @@ static int m_can_poll(struct napi_struct *napi, int quota)
>   * echo. timestamp is used for peripherals to ensure correct ordering
>   * by rx-offload, and is ignored for non-peripherals.
>   */
> -static void m_can_tx_update_stats(struct m_can_classdev *cdev,
> -				  unsigned int msg_mark,
> -				  u32 timestamp)
> +static unsigned int m_can_tx_update_stats(struct m_can_classdev *cdev,
> +					  unsigned int msg_mark, u32 timestamp)
>  {
>  	struct net_device *dev = cdev->net;
>  	struct net_device_stats *stats = &dev->stats;
> +	unsigned int frame_len;
>  
>  	if (cdev->is_peripheral)
>  		stats->tx_bytes +=
>  			can_rx_offload_get_echo_skb(&cdev->offload,
>  						    msg_mark,
>  						    timestamp,
> -						    NULL);
> +						    &frame_len);
>  	else
> -		stats->tx_bytes += can_get_echo_skb(dev, msg_mark, NULL);
> +		stats->tx_bytes += can_get_echo_skb(dev, msg_mark, &frame_len);
>  
>  	stats->tx_packets++;
> +
> +	return frame_len;
>  }
>  
> -static void m_can_finish_tx(struct m_can_classdev *cdev, int transmitted)
> +static void m_can_finish_tx(struct m_can_classdev *cdev, int transmitted,
> +			    int transmitted_frame_len)

nit: I think unsigned int would be a better type for transmitted_frame_len,
     as that is the type of the 3rd argument to netdev_completed_queue()

>  {
>  	unsigned long irqflags;
>  
> +	netdev_completed_queue(cdev->net, transmitted, transmitted_frame_len);
> +
>  	spin_lock_irqsave(&cdev->tx_handling_spinlock, irqflags);
>  	if (cdev->tx_fifo_in_flight >= cdev->tx_fifo_size && transmitted > 0)
>  		netif_wake_queue(cdev->net);
> @@ -1060,6 +1067,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
>  	int err = 0;
>  	unsigned int msg_mark;
>  	int processed = 0;
> +	int processed_frame_len = 0;

Likewise, here.

>  	struct m_can_classdev *cdev = netdev_priv(dev);
>  
> @@ -1088,7 +1096,9 @@ static int m_can_echo_tx_event(struct net_device *dev)
>  		fgi = (++fgi >= cdev->mcfg[MRAM_TXE].num ? 0 : fgi);
>  
>  		/* update stats */
> -		m_can_tx_update_stats(cdev, msg_mark, timestamp);
> +		processed_frame_len += m_can_tx_update_stats(cdev, msg_mark,
> +							     timestamp);
> +
>  		++processed;
>  	}
>  
> @@ -1096,7 +1106,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
>  		m_can_write(cdev, M_CAN_TXEFA, FIELD_PREP(TXEFA_EFAI_MASK,
>  							  ack_fgi));
>  
> -	m_can_finish_tx(cdev, processed);
> +	m_can_finish_tx(cdev, processed, processed_frame_len);
>  
>  	return err;
>  }

...