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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
>
> > ---
> > 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
> >
