Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Jun 2023 14:53:54 +0200
From:   Markus Schneider-Pargmann <msp@baylibre.com>
To:     Simon Horman <simon.horman@corigine.com>
Cc:     Marc Kleine-Budde <mkl@pengutronix.de>,
        Chandrasekar Ramakrishnan <rcsekar@samsung.com>,
        Wolfgang Grandegger <wg@grandegger.com>,
        Vincent MAILHOL <mailhol.vincent@wanadoo.fr>,
        linux-can@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 13/16] can: m_can: Introduce a tx_fifo_in_flight
 counter
Message-ID: <20230620125354.ipok6i43lvow66t4@blmsp>
References: <20230315110546.2518305-1-msp@baylibre.com>
 <20230315110546.2518305-14-msp@baylibre.com>
 <ZBSPJHkcLG7gkZ7I@corigine.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZBSPJHkcLG7gkZ7I@corigine.com>
Precedence: bulk

Hi Simon,

On Fri, Mar 17, 2023 at 05:02:44PM +0100, Simon Horman wrote:
> On Wed, Mar 15, 2023 at 12:05:43PM +0100, Markus Schneider-Pargmann wrote:
> > Keep track of the number of transmits in flight.
> > 
> > This patch prepares the driver to control the network interface queue
> > based on this counter. By itself this counter be
> > implemented with an atomic, but as we need to do other things in the
> > critical sections later I am using a spinlock instead.
> > 
> > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>

Thank you for all your reviews, very helpful.

> 
> Nit, assuming the values are always positive, I think
> that unsigned might be a more appropriate type than int
> for the tx_fifo_in_flight field, and associated function
> parameters and local variables.

I agree that tx_fifo_in_flight is and should always be a positive value.
However as the code is operating with ++ and -- exclusively I would
personally prefer int here as that shows off-by-one errors much easier
in case there are any at some point.

Is that fine for you?

Best,
Markus

> 
> That notwithstanding,
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 
> > ---
> >  drivers/net/can/m_can/m_can.c | 41 ++++++++++++++++++++++++++++++++++-
> >  drivers/net/can/m_can/m_can.h |  4 ++++
> >  2 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 27d36bcc094c..4ad8f08f8284 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -442,6 +442,7 @@ static u32 m_can_get_timestamp(struct m_can_classdev *cdev)
> >  static void m_can_clean(struct net_device *net)
> >  {
> >  	struct m_can_classdev *cdev = netdev_priv(net);
> > +	unsigned long irqflags;
> >  
> >  	for (int i = 0; i != cdev->tx_fifo_size; ++i) {
> >  		if (!cdev->tx_ops[i].skb)
> > @@ -453,6 +454,10 @@ static void m_can_clean(struct net_device *net)
> >  
> >  	for (int i = 0; i != cdev->can.echo_skb_max; ++i)
> >  		can_free_echo_skb(cdev->net, i, NULL);
> > +
> > +	spin_lock_irqsave(&cdev->tx_handling_spinlock, irqflags);
> > +	cdev->tx_fifo_in_flight = 0;
> > +	spin_unlock_irqrestore(&cdev->tx_handling_spinlock, irqflags);
> >  }
> >  
> >  /* For peripherals, pass skb to rx-offload, which will push skb from
> > @@ -1023,6 +1028,24 @@ static void m_can_tx_update_stats(struct m_can_classdev *cdev,
> >  	stats->tx_packets++;
> >  }
> >  
> > +static void m_can_finish_tx(struct m_can_classdev *cdev, int transmitted)
> > +{
> > +	unsigned long irqflags;
> > +
> > +	spin_lock_irqsave(&cdev->tx_handling_spinlock, irqflags);
> > +	cdev->tx_fifo_in_flight -= transmitted;
> > +	spin_unlock_irqrestore(&cdev->tx_handling_spinlock, irqflags);
> > +}
> > +
> > +static void m_can_start_tx(struct m_can_classdev *cdev)
> > +{
> > +	unsigned long irqflags;
> > +
> > +	spin_lock_irqsave(&cdev->tx_handling_spinlock, irqflags);
> > +	++cdev->tx_fifo_in_flight;
> > +	spin_unlock_irqrestore(&cdev->tx_handling_spinlock, irqflags);
> > +}
> > +
> >  static int m_can_echo_tx_event(struct net_device *dev)
> >  {
> >  	u32 txe_count = 0;
> > @@ -1032,6 +1055,7 @@ static int m_can_echo_tx_event(struct net_device *dev)
> >  	int i = 0;
> >  	int err = 0;
> >  	unsigned int msg_mark;
> > +	int processed = 0;
> >  
> >  	struct m_can_classdev *cdev = netdev_priv(dev);
> >  
> > @@ -1061,12 +1085,15 @@ static int m_can_echo_tx_event(struct net_device *dev)
> >  
> >  		/* update stats */
> >  		m_can_tx_update_stats(cdev, msg_mark, timestamp);
> > +		++processed;
> >  	}
> >  
> >  	if (ack_fgi != -1)
> >  		m_can_write(cdev, M_CAN_TXEFA, FIELD_PREP(TXEFA_EFAI_MASK,
> >  							  ack_fgi));
> >  
> > +	m_can_finish_tx(cdev, processed);
> > +
> >  	return err;
> >  }
> >  
> > @@ -1161,6 +1188,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
> >  				timestamp = m_can_get_timestamp(cdev);
> >  			m_can_tx_update_stats(cdev, 0, timestamp);
> >  			netif_wake_queue(dev);
> > +			m_can_finish_tx(cdev, 1);
> >  		}
> >  	} else  {
> >  		if (ir & (IR_TEFN | IR_TEFW)) {
> > @@ -1846,11 +1874,22 @@ static netdev_tx_t m_can_start_peripheral_xmit(struct m_can_classdev *cdev,
> >  	}
> >  
> >  	netif_stop_queue(cdev->net);
> > +
> > +	m_can_start_tx(cdev);
> > +
> >  	m_can_tx_queue_skb(cdev, skb);
> >  
> >  	return NETDEV_TX_OK;
> >  }
> >  
> > +static netdev_tx_t m_can_start_fast_xmit(struct m_can_classdev *cdev,
> > +					 struct sk_buff *skb)
> > +{
> > +	m_can_start_tx(cdev);
> > +
> > +	return m_can_tx_handler(cdev, skb);
> > +}
> > +
> >  static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> >  				    struct net_device *dev)
> >  {
> > @@ -1862,7 +1901,7 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> >  	if (cdev->is_peripheral)
> >  		return m_can_start_peripheral_xmit(cdev, skb);
> >  	else
> > -		return m_can_tx_handler(cdev, skb);
> > +		return m_can_start_fast_xmit(cdev, skb);
> >  }
> >  
> >  static int m_can_open(struct net_device *dev)
> > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > index 2e1a52980a18..e230cf320a6c 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -109,6 +109,10 @@ struct m_can_classdev {
> >  	// Store this internally to avoid fetch delays on peripheral chips
> >  	int tx_fifo_putidx;
> >  
> > +	/* Protects shared state between start_xmit and m_can_isr */
> > +	spinlock_t tx_handling_spinlock;
> > +	int tx_fifo_in_flight;
> > +
> >  	struct m_can_tx_op *tx_ops;
> >  	int tx_fifo_size;
> >  	int next_tx_op;
> > -- 
> > 2.39.2
> >