[v3,13/16] can: m_can: Introduce a tx_fifo_in_flight counter
Commit Message
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>
---
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(-)
Comments
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>
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.
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
>
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
> >
On Tue, Jun 20, 2023 at 02:53:54PM +0200, Markus Schneider-Pargmann wrote:
> 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?
Yes, I think so.
@@ -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)
@@ -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;