[PATCH 0/3] can: kvaser_pciefd: Fix ISR race conditions

[PATCH 0/3] can: kvaser_pciefd: Fix ISR race conditions

[Axel Forsman]

This patch series fixes a couple of race conditions in the
kvaser_pciefd driver surfaced by enabling MSI interrupts and the new
Kvaser PCIe 8xCAN.

Axel Forsman (3):
  can: kvaser_pciefd: Force IRQ edge in case of nested IRQ
  can: kvaser_pciefd: Fix echo_skb race conditions
  can: kvaser_pciefd: Continue parsing DMA buf after dropped RX

 drivers/net/can/kvaser_pciefd.c | 161 ++++++++++++++++++--------------
 1 file changed, 92 insertions(+), 69 deletions(-)

-- 
2.47.2

[PATCH 1/3] can: kvaser_pciefd: Force IRQ edge in case of nested IRQ

[Axel Forsman]

Avoid the driver missing IRQs by temporarily masking IRQs in the ISR
to enforce an edge even if a different IRQ is signalled before handled
IRQs are cleared.

Fixes: 48f827d4f48f ("can: kvaser_pciefd: Move reset of DMA RX buffers to the end of the ISR")
Cc: stable@vger.kernel.org
Signed-off-by: Axel Forsman <axfo@kvaser.com>
Tested-by: Jimmy Assarsson <extja@kvaser.com>
Reviewed-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/kvaser_pciefd.c | 83 ++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 44 deletions(-)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index fa04a7ced02b..0d1b895509c3 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -1646,24 +1646,28 @@ static int kvaser_pciefd_read_buffer(struct kvaser_pciefd *pcie, int dma_buf)
 	return res;
 }
 
-static u32 kvaser_pciefd_receive_irq(struct kvaser_pciefd *pcie)
+static void kvaser_pciefd_receive_irq(struct kvaser_pciefd *pcie)
 {
+	__le32 __iomem *srb_cmd_reg = KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_CMD_REG;
 	u32 irq = ioread32(KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_IRQ_REG);
 
-	if (irq & KVASER_PCIEFD_SRB_IRQ_DPD0)
-		kvaser_pciefd_read_buffer(pcie, 0);
+	iowrite32(irq, KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_IRQ_REG);
 
-	if (irq & KVASER_PCIEFD_SRB_IRQ_DPD1)
+	if (irq & KVASER_PCIEFD_SRB_IRQ_DPD0) {
+		kvaser_pciefd_read_buffer(pcie, 0);
+		iowrite32(KVASER_PCIEFD_SRB_CMD_RDB0, srb_cmd_reg); /* Rearm buffer */
+	}
+
+	if (irq & KVASER_PCIEFD_SRB_IRQ_DPD1) {
 		kvaser_pciefd_read_buffer(pcie, 1);
+		iowrite32(KVASER_PCIEFD_SRB_CMD_RDB1, srb_cmd_reg); /* Rearm buffer */
+	}
 
 	if (unlikely(irq & KVASER_PCIEFD_SRB_IRQ_DOF0 ||
 		     irq & KVASER_PCIEFD_SRB_IRQ_DOF1 ||
 		     irq & KVASER_PCIEFD_SRB_IRQ_DUF0 ||
 		     irq & KVASER_PCIEFD_SRB_IRQ_DUF1))
 		dev_err(&pcie->pci->dev, "DMA IRQ error 0x%08X\n", irq);
-
-	iowrite32(irq, KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_IRQ_REG);
-	return irq;
 }
 
 static void kvaser_pciefd_transmit_irq(struct kvaser_pciefd_can *can)
@@ -1691,29 +1695,22 @@ static irqreturn_t kvaser_pciefd_irq_handler(int irq, void *dev)
 	struct kvaser_pciefd *pcie = (struct kvaser_pciefd *)dev;
 	const struct kvaser_pciefd_irq_mask *irq_mask = pcie->driver_data->irq_mask;
 	u32 pci_irq = ioread32(KVASER_PCIEFD_PCI_IRQ_ADDR(pcie));
-	u32 srb_irq = 0;
-	u32 srb_release = 0;
 	int i;
 
 	if (!(pci_irq & irq_mask->all))
 		return IRQ_NONE;
 
+	iowrite32(0, KVASER_PCIEFD_PCI_IEN_ADDR(pcie));
+
 	if (pci_irq & irq_mask->kcan_rx0)
-		srb_irq = kvaser_pciefd_receive_irq(pcie);
+		kvaser_pciefd_receive_irq(pcie);
 
 	for (i = 0; i < pcie->nr_channels; i++) {
 		if (pci_irq & irq_mask->kcan_tx[i])
 			kvaser_pciefd_transmit_irq(pcie->can[i]);
 	}
 
-	if (srb_irq & KVASER_PCIEFD_SRB_IRQ_DPD0)
-		srb_release |= KVASER_PCIEFD_SRB_CMD_RDB0;
-
-	if (srb_irq & KVASER_PCIEFD_SRB_IRQ_DPD1)
-		srb_release |= KVASER_PCIEFD_SRB_CMD_RDB1;
-
-	if (srb_release)
-		iowrite32(srb_release, KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_CMD_REG);
+	iowrite32(irq_mask->all, KVASER_PCIEFD_PCI_IEN_ADDR(pcie));
 
 	return IRQ_HANDLED;
 }
@@ -1733,13 +1730,22 @@ static void kvaser_pciefd_teardown_can_ctrls(struct kvaser_pciefd *pcie)
 	}
 }
 
+static void kvaser_pciefd_disable_irq_srcs(struct kvaser_pciefd *pcie)
+{
+	unsigned int i;
+
+	/* Masking PCI_IRQ is insufficient as running ISR will unmask it */
+	iowrite32(0, KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_IEN_REG);
+	for (i = 0; i < pcie->nr_channels; ++i)
+		iowrite32(0, pcie->can[i]->reg_base + KVASER_PCIEFD_KCAN_IEN_REG);
+}
+
 static int kvaser_pciefd_probe(struct pci_dev *pdev,
 			       const struct pci_device_id *id)
 {
 	int ret;
 	struct kvaser_pciefd *pcie;
 	const struct kvaser_pciefd_irq_mask *irq_mask;
-	void __iomem *irq_en_base;
 
 	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
@@ -1805,8 +1811,7 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
 		  KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_IEN_REG);
 
 	/* Enable PCI interrupts */
-	irq_en_base = KVASER_PCIEFD_PCI_IEN_ADDR(pcie);
-	iowrite32(irq_mask->all, irq_en_base);
+	iowrite32(irq_mask->all, KVASER_PCIEFD_PCI_IEN_ADDR(pcie));
 	/* Ready the DMA buffers */
 	iowrite32(KVASER_PCIEFD_SRB_CMD_RDB0,
 		  KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_CMD_REG);
@@ -1820,8 +1825,7 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
 	return 0;
 
 err_free_irq:
-	/* Disable PCI interrupts */
-	iowrite32(0, irq_en_base);
+	kvaser_pciefd_disable_irq_srcs(pcie);
 	free_irq(pcie->pci->irq, pcie);
 
 err_pci_free_irq_vectors:
@@ -1844,35 +1848,26 @@ static int kvaser_pciefd_probe(struct pci_dev *pdev,
 	return ret;
 }
 
-static void kvaser_pciefd_remove_all_ctrls(struct kvaser_pciefd *pcie)
-{
-	int i;
-
-	for (i = 0; i < pcie->nr_channels; i++) {
-		struct kvaser_pciefd_can *can = pcie->can[i];
-
-		if (can) {
-			iowrite32(0, can->reg_base + KVASER_PCIEFD_KCAN_IEN_REG);
-			unregister_candev(can->can.dev);
-			del_timer(&can->bec_poll_timer);
-			kvaser_pciefd_pwm_stop(can);
-			free_candev(can->can.dev);
-		}
-	}
-}
-
 static void kvaser_pciefd_remove(struct pci_dev *pdev)
 {
 	struct kvaser_pciefd *pcie = pci_get_drvdata(pdev);
+	unsigned int i;
 
-	kvaser_pciefd_remove_all_ctrls(pcie);
+	for (i = 0; i < pcie->nr_channels; ++i) {
+		struct kvaser_pciefd_can *can = pcie->can[i];
 
-	/* Disable interrupts */
-	iowrite32(0, KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_CTRL_REG);
-	iowrite32(0, KVASER_PCIEFD_PCI_IEN_ADDR(pcie));
+		unregister_candev(can->can.dev);
+		del_timer(&can->bec_poll_timer);
+		kvaser_pciefd_pwm_stop(can);
+	}
 
+	kvaser_pciefd_disable_irq_srcs(pcie);
 	free_irq(pcie->pci->irq, pcie);
 	pci_free_irq_vectors(pcie->pci);
+
+	for (i = 0; i < pcie->nr_channels; ++i)
+		free_candev(pcie->can[i]->can.dev);
+
 	pci_iounmap(pdev, pcie->reg_base);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
-- 
2.47.2

Re: [PATCH 1/3] can: kvaser_pciefd: Force IRQ edge in case of nested IRQ

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

On 31.03.2025 09:25:26, Axel Forsman wrote:
> Avoid the driver missing IRQs by temporarily masking IRQs in the ISR
> to enforce an edge even if a different IRQ is signalled before handled
> IRQs are cleared.
> 
> Fixes: 48f827d4f48f ("can: kvaser_pciefd: Move reset of DMA RX buffers to the end of the ISR")
> Cc: stable@vger.kernel.org
> Signed-off-by: Axel Forsman <axfo@kvaser.com>
> Tested-by: Jimmy Assarsson <extja@kvaser.com>
> Reviewed-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 83 ++++++++++++++++-----------------
>  1 file changed, 39 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index fa04a7ced02b..0d1b895509c3 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -1646,24 +1646,28 @@ static int kvaser_pciefd_read_buffer(struct kvaser_pciefd *pcie, int dma_buf)
>  	return res;
>  }
>  
> -static u32 kvaser_pciefd_receive_irq(struct kvaser_pciefd *pcie)
> +static void kvaser_pciefd_receive_irq(struct kvaser_pciefd *pcie)
>  {
> +	__le32 __iomem *srb_cmd_reg = KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_CMD_REG;

Why is this an __le32? The struct kvaser_pciefd::reg_base is __iomem
void *.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] can: kvaser_pciefd: Force IRQ edge in case of nested IRQ

[Axel Forsman]

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 31.03.2025 09:25:26, Axel Forsman wrote:
>> -static u32 kvaser_pciefd_receive_irq(struct kvaser_pciefd *pcie)
>> +static void kvaser_pciefd_receive_irq(struct kvaser_pciefd *pcie)
>>  {
>> +	__le32 __iomem *srb_cmd_reg = KVASER_PCIEFD_SRB_ADDR(pcie) + KVASER_PCIEFD_SRB_CMD_REG;
>
> Why is this an __le32? The struct kvaser_pciefd::reg_base is __iomem
> void *.

Just as a hint that the register is 32-bit.
But you are right, I will change to "void __iomem *" for consistency
in the next iteration.


/Axel

[PATCH 2/3] can: kvaser_pciefd: Fix echo_skb race conditions

[Axel Forsman]

The functions kvaser_pciefd_start_xmit() and
kvaser_pciefd_handle_ack_packet() raced to stop/wake TX queues and
get/put echo skbs, as kvaser_pciefd_can->echo_lock was only ever taken
when transmitting. E.g., this caused the following error:

    can_put_echo_skb: BUG! echo_skb 5 is occupied!

Instead, use the synchronization helpers in netdev_queues.h. As those
piggyback on BQL barriers, start updating in-flight packets and bytes
counts as well.

Cc: stable@vger.kernel.org
Signed-off-by: Axel Forsman <axfo@kvaser.com>
Tested-by: Jimmy Assarsson <extja@kvaser.com>
Reviewed-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/kvaser_pciefd.c | 70 ++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 22 deletions(-)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 0d1b895509c3..6251a1ddfa7e 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -16,6 +16,7 @@
 #include <linux/netdevice.h>
 #include <linux/pci.h>
 #include <linux/timer.h>
+#include <net/netdev_queues.h>
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Kvaser AB <support@kvaser.com>");
@@ -412,8 +413,9 @@ struct kvaser_pciefd_can {
 	u8 cmd_seq;
 	int err_rep_cnt;
 	int echo_idx;
+	unsigned int completed_tx_pkts;
+	unsigned int completed_tx_bytes;
 	spinlock_t lock; /* Locks sensitive registers (e.g. MODE) */
-	spinlock_t echo_lock; /* Locks the message echo buffer */
 	struct timer_list bec_poll_timer;
 	struct completion start_comp, flush_comp;
 };
@@ -745,11 +747,24 @@ static int kvaser_pciefd_stop(struct net_device *netdev)
 		del_timer(&can->bec_poll_timer);
 	}
 	can->can.state = CAN_STATE_STOPPED;
+	netdev_reset_queue(netdev);
 	close_candev(netdev);
 
 	return ret;
 }
 
+static unsigned int kvaser_pciefd_tx_avail(struct kvaser_pciefd_can *can)
+{
+	u8 count = FIELD_GET(KVASER_PCIEFD_KCAN_TX_NR_PACKETS_CURRENT_MASK,
+		ioread32(can->reg_base + KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG));
+
+	if (count < can->can.echo_skb_max) /* Free TX FIFO slot? */
+		/* Avoid reusing unacked seqno */
+		return !can->can.echo_skb[can->echo_idx];
+	else
+		return 0;
+}
+
 static int kvaser_pciefd_prepare_tx_packet(struct kvaser_pciefd_tx_packet *p,
 					   struct kvaser_pciefd_can *can,
 					   struct sk_buff *skb)
@@ -797,23 +812,31 @@ static netdev_tx_t kvaser_pciefd_start_xmit(struct sk_buff *skb,
 					    struct net_device *netdev)
 {
 	struct kvaser_pciefd_can *can = netdev_priv(netdev);
-	unsigned long irq_flags;
 	struct kvaser_pciefd_tx_packet packet;
+	unsigned int frame_len = 0;
 	int nr_words;
-	u8 count;
 
 	if (can_dev_dropped_skb(netdev, skb))
 		return NETDEV_TX_OK;
 
+	/*
+	 * Without room for a new message, stop the queue until at least
+	 * one successful transmit.
+	 */
+	if (!netif_subqueue_maybe_stop(netdev, 0, kvaser_pciefd_tx_avail(can), 1, 1))
+		return NETDEV_TX_BUSY;
+
 	nr_words = kvaser_pciefd_prepare_tx_packet(&packet, can, skb);
 
-	spin_lock_irqsave(&can->echo_lock, irq_flags);
 	/* Prepare and save echo skb in internal slot */
-	can_put_echo_skb(skb, netdev, can->echo_idx, 0);
+	frame_len = can_skb_get_frame_len(skb);
+	can_put_echo_skb(skb, netdev, can->echo_idx, frame_len);
 
 	/* Move echo index to the next slot */
 	can->echo_idx = (can->echo_idx + 1) % can->can.echo_skb_max;
 
+	netdev_sent_queue(netdev, frame_len);
+
 	/* Write header to fifo */
 	iowrite32(packet.header[0],
 		  can->reg_base + KVASER_PCIEFD_KCAN_FIFO_REG);
@@ -836,15 +859,6 @@ static netdev_tx_t kvaser_pciefd_start_xmit(struct sk_buff *skb,
 			     KVASER_PCIEFD_KCAN_FIFO_LAST_REG);
 	}
 
-	count = FIELD_GET(KVASER_PCIEFD_KCAN_TX_NR_PACKETS_CURRENT_MASK,
-			  ioread32(can->reg_base + KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG));
-	/* No room for a new message, stop the queue until at least one
-	 * successful transmit
-	 */
-	if (count >= can->can.echo_skb_max || can->can.echo_skb[can->echo_idx])
-		netif_stop_queue(netdev);
-	spin_unlock_irqrestore(&can->echo_lock, irq_flags);
-
 	return NETDEV_TX_OK;
 }
 
@@ -970,6 +984,8 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)
 		can->kv_pcie = pcie;
 		can->cmd_seq = 0;
 		can->err_rep_cnt = 0;
+		can->completed_tx_pkts = 0;
+		can->completed_tx_bytes = 0;
 		can->bec.txerr = 0;
 		can->bec.rxerr = 0;
 
@@ -987,7 +1003,6 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)
 		can->can.clock.freq = pcie->freq;
 		can->can.echo_skb_max = min(KVASER_PCIEFD_CAN_TX_MAX_COUNT, tx_nr_packets_max - 1);
 		can->echo_idx = 0;
-		spin_lock_init(&can->echo_lock);
 		spin_lock_init(&can->lock);
 
 		can->can.bittiming_const = &kvaser_pciefd_bittiming_const;
@@ -1510,19 +1525,16 @@ static int kvaser_pciefd_handle_ack_packet(struct kvaser_pciefd *pcie,
 		netdev_dbg(can->can.dev, "Packet was flushed\n");
 	} else {
 		int echo_idx = FIELD_GET(KVASER_PCIEFD_PACKET_SEQ_MASK, p->header[0]);
-		int len;
-		u8 count;
+		unsigned int len, frame_len = 0;
 		struct sk_buff *skb;
 
 		skb = can->can.echo_skb[echo_idx];
 		if (skb)
 			kvaser_pciefd_set_skb_timestamp(pcie, skb, p->timestamp);
-		len = can_get_echo_skb(can->can.dev, echo_idx, NULL);
-		count = FIELD_GET(KVASER_PCIEFD_KCAN_TX_NR_PACKETS_CURRENT_MASK,
-				  ioread32(can->reg_base + KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG));
+		len = can_get_echo_skb(can->can.dev, echo_idx, &frame_len);
 
-		if (count < can->can.echo_skb_max && netif_queue_stopped(can->can.dev))
-			netif_wake_queue(can->can.dev);
+		can->completed_tx_pkts++;
+		can->completed_tx_bytes += frame_len;
 
 		if (!one_shot_fail) {
 			can->can.dev->stats.tx_bytes += len;
@@ -1638,11 +1650,25 @@ static int kvaser_pciefd_read_buffer(struct kvaser_pciefd *pcie, int dma_buf)
 {
 	int pos = 0;
 	int res = 0;
+	unsigned int i;
 
 	do {
 		res = kvaser_pciefd_read_packet(pcie, &pos, dma_buf);
 	} while (!res && pos > 0 && pos < KVASER_PCIEFD_DMA_SIZE);
 
+	for (i = 0; i < pcie->nr_channels; ++i) {
+		struct kvaser_pciefd_can *can = pcie->can[i];
+
+		if (!can->completed_tx_pkts)
+			continue;
+		netif_subqueue_completed_wake(can->can.dev, 0,
+					      can->completed_tx_pkts,
+					      can->completed_tx_bytes,
+					      kvaser_pciefd_tx_avail(can), 1);
+		can->completed_tx_pkts = 0;
+		can->completed_tx_bytes = 0;
+	}
+
 	return res;
 }
 
-- 
2.47.2

Re: [PATCH 2/3] can: kvaser_pciefd: Fix echo_skb race conditions

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 7920 bytes --]

On 31.03.2025 09:25:27, Axel Forsman wrote:
> The functions kvaser_pciefd_start_xmit() and
> kvaser_pciefd_handle_ack_packet() raced to stop/wake TX queues and
> get/put echo skbs, as kvaser_pciefd_can->echo_lock was only ever taken
> when transmitting. E.g., this caused the following error:
> 
>     can_put_echo_skb: BUG! echo_skb 5 is occupied!
> 
> Instead, use the synchronization helpers in netdev_queues.h. As those
> piggyback on BQL barriers, start updating in-flight packets and bytes
> counts as well.

This looks like it does in the right direction. Using the
netif_subqueue_completed helpers is a great idea.

What usually works even better is to have 2 counters and a mask:
- unsigned int tx_head, tx_tail
- TXFIFO_DEPTH

The tx_head is incremented in the xmit function, tail is incremented in
the tx_done function.

There's no need to check how many buffers are free in the HW.

Have a look at the rockchip-canfd driver for an example.

Some comments inline.

> Cc: stable@vger.kernel.org
> Signed-off-by: Axel Forsman <axfo@kvaser.com>
> Tested-by: Jimmy Assarsson <extja@kvaser.com>
> Reviewed-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 70 ++++++++++++++++++++++-----------
>  1 file changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index 0d1b895509c3..6251a1ddfa7e 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -16,6 +16,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/pci.h>
>  #include <linux/timer.h>
> +#include <net/netdev_queues.h>
>  
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_AUTHOR("Kvaser AB <support@kvaser.com>");
> @@ -412,8 +413,9 @@ struct kvaser_pciefd_can {
>  	u8 cmd_seq;
>  	int err_rep_cnt;
>  	int echo_idx;
> +	unsigned int completed_tx_pkts;
> +	unsigned int completed_tx_bytes;
>  	spinlock_t lock; /* Locks sensitive registers (e.g. MODE) */
> -	spinlock_t echo_lock; /* Locks the message echo buffer */
>  	struct timer_list bec_poll_timer;
>  	struct completion start_comp, flush_comp;
>  };
> @@ -745,11 +747,24 @@ static int kvaser_pciefd_stop(struct net_device *netdev)
>  		del_timer(&can->bec_poll_timer);
>  	}
>  	can->can.state = CAN_STATE_STOPPED;
> +	netdev_reset_queue(netdev);
>  	close_candev(netdev);
>  
>  	return ret;
>  }
>  
> +static unsigned int kvaser_pciefd_tx_avail(struct kvaser_pciefd_can *can)
> +{
> +	u8 count = FIELD_GET(KVASER_PCIEFD_KCAN_TX_NR_PACKETS_CURRENT_MASK,
> +		ioread32(can->reg_base + KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG));
> +
> +	if (count < can->can.echo_skb_max) /* Free TX FIFO slot? */
> +		/* Avoid reusing unacked seqno */
> +		return !can->can.echo_skb[can->echo_idx];
> +	else
> +		return 0;
> +}
> +
>  static int kvaser_pciefd_prepare_tx_packet(struct kvaser_pciefd_tx_packet *p,
>  					   struct kvaser_pciefd_can *can,
>  					   struct sk_buff *skb)
> @@ -797,23 +812,31 @@ static netdev_tx_t kvaser_pciefd_start_xmit(struct sk_buff *skb,
>  					    struct net_device *netdev)
>  {
>  	struct kvaser_pciefd_can *can = netdev_priv(netdev);
> -	unsigned long irq_flags;
>  	struct kvaser_pciefd_tx_packet packet;
> +	unsigned int frame_len = 0;
>  	int nr_words;
> -	u8 count;
>  
>  	if (can_dev_dropped_skb(netdev, skb))
>  		return NETDEV_TX_OK;
>  
> +	/*
> +	 * Without room for a new message, stop the queue until at least
> +	 * one successful transmit.
> +	 */
> +	if (!netif_subqueue_maybe_stop(netdev, 0, kvaser_pciefd_tx_avail(can), 1, 1))
> +		return NETDEV_TX_BUSY;

Returning NETDEV_TX_BUSY is quite expensive, stop the queue at the end
of this function, if the buffers are full.

> +
>  	nr_words = kvaser_pciefd_prepare_tx_packet(&packet, can, skb);
>  
> -	spin_lock_irqsave(&can->echo_lock, irq_flags);
>  	/* Prepare and save echo skb in internal slot */
> -	can_put_echo_skb(skb, netdev, can->echo_idx, 0);
> +	frame_len = can_skb_get_frame_len(skb);
> +	can_put_echo_skb(skb, netdev, can->echo_idx, frame_len);
>  
>  	/* Move echo index to the next slot */
>  	can->echo_idx = (can->echo_idx + 1) % can->can.echo_skb_max;
>  
> +	netdev_sent_queue(netdev, frame_len);
> +
>  	/* Write header to fifo */
>  	iowrite32(packet.header[0],
>  		  can->reg_base + KVASER_PCIEFD_KCAN_FIFO_REG);
> @@ -836,15 +859,6 @@ static netdev_tx_t kvaser_pciefd_start_xmit(struct sk_buff *skb,
>  			     KVASER_PCIEFD_KCAN_FIFO_LAST_REG);
>  	}
>  
> -	count = FIELD_GET(KVASER_PCIEFD_KCAN_TX_NR_PACKETS_CURRENT_MASK,
> -			  ioread32(can->reg_base + KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG));
> -	/* No room for a new message, stop the queue until at least one
> -	 * successful transmit
> -	 */
> -	if (count >= can->can.echo_skb_max || can->can.echo_skb[can->echo_idx])
> -		netif_stop_queue(netdev);
> -	spin_unlock_irqrestore(&can->echo_lock, irq_flags);
> -
>  	return NETDEV_TX_OK;
>  }
>  
> @@ -970,6 +984,8 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)
>  		can->kv_pcie = pcie;
>  		can->cmd_seq = 0;
>  		can->err_rep_cnt = 0;
> +		can->completed_tx_pkts = 0;
> +		can->completed_tx_bytes = 0;
>  		can->bec.txerr = 0;
>  		can->bec.rxerr = 0;
>  
> @@ -987,7 +1003,6 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)
>  		can->can.clock.freq = pcie->freq;
>  		can->can.echo_skb_max = min(KVASER_PCIEFD_CAN_TX_MAX_COUNT, tx_nr_packets_max - 1);
>  		can->echo_idx = 0;
> -		spin_lock_init(&can->echo_lock);
>  		spin_lock_init(&can->lock);
>  
>  		can->can.bittiming_const = &kvaser_pciefd_bittiming_const;
> @@ -1510,19 +1525,16 @@ static int kvaser_pciefd_handle_ack_packet(struct kvaser_pciefd *pcie,
>  		netdev_dbg(can->can.dev, "Packet was flushed\n");
>  	} else {
>  		int echo_idx = FIELD_GET(KVASER_PCIEFD_PACKET_SEQ_MASK, p->header[0]);
> -		int len;
> -		u8 count;
> +		unsigned int len, frame_len = 0;
>  		struct sk_buff *skb;
>  
>  		skb = can->can.echo_skb[echo_idx];
>  		if (skb)
>  			kvaser_pciefd_set_skb_timestamp(pcie, skb, p->timestamp);
> -		len = can_get_echo_skb(can->can.dev, echo_idx, NULL);
> -		count = FIELD_GET(KVASER_PCIEFD_KCAN_TX_NR_PACKETS_CURRENT_MASK,
> -				  ioread32(can->reg_base + KVASER_PCIEFD_KCAN_TX_NR_PACKETS_REG));
> +		len = can_get_echo_skb(can->can.dev, echo_idx, &frame_len);
>  
> -		if (count < can->can.echo_skb_max && netif_queue_stopped(can->can.dev))
> -			netif_wake_queue(can->can.dev);
> +		can->completed_tx_pkts++;
> +		can->completed_tx_bytes += frame_len;
>  
>  		if (!one_shot_fail) {
>  			can->can.dev->stats.tx_bytes += len;
> @@ -1638,11 +1650,25 @@ static int kvaser_pciefd_read_buffer(struct kvaser_pciefd *pcie, int dma_buf)
>  {
>  	int pos = 0;
>  	int res = 0;
> +	unsigned int i;
>  
>  	do {
>  		res = kvaser_pciefd_read_packet(pcie, &pos, dma_buf);
>  	} while (!res && pos > 0 && pos < KVASER_PCIEFD_DMA_SIZE);
>  
> +	for (i = 0; i < pcie->nr_channels; ++i) {
> +		struct kvaser_pciefd_can *can = pcie->can[i];
> +
> +		if (!can->completed_tx_pkts)
> +			continue;
> +		netif_subqueue_completed_wake(can->can.dev, 0,
> +					      can->completed_tx_pkts,
> +					      can->completed_tx_bytes,
> +					      kvaser_pciefd_tx_avail(can), 1);

You can do this as soon as as one packet is finished, if you want to
avoid too frequent wakeups, use threshold of more than 1.

Marc

> +		can->completed_tx_pkts = 0;
> +		can->completed_tx_bytes = 0;
> +	}
> +
>  	return res;
>  }
>  
> -- 
> 2.47.2
> 
> 
> 

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] can: kvaser_pciefd: Fix echo_skb race conditions

[Axel Forsman]

Marc Kleine-Budde <mkl@pengutronix.de> writes:

> On 31.03.2025 09:25:27, Axel Forsman wrote:
>> The functions kvaser_pciefd_start_xmit() and
>> kvaser_pciefd_handle_ack_packet() raced to stop/wake TX queues and
>> get/put echo skbs, as kvaser_pciefd_can->echo_lock was only ever taken
>> when transmitting. E.g., this caused the following error:
>> 
>>     can_put_echo_skb: BUG! echo_skb 5 is occupied!
>> 
>> Instead, use the synchronization helpers in netdev_queues.h. As those
>> piggyback on BQL barriers, start updating in-flight packets and bytes
>> counts as well.
>
> This looks like it does in the right direction. Using the
> netif_subqueue_completed helpers is a great idea.
>
> What usually works even better is to have 2 counters and a mask:
> - unsigned int tx_head, tx_tail
> - TXFIFO_DEPTH
>
> The tx_head is incremented in the xmit function, tail is incremented in
> the tx_done function.
>
> There's no need to check how many buffers are free in the HW.
>
> Have a look at the rockchip-canfd driver for an example.

An attempt was made to keep this a bugfix-only commit,
but I do agree it is nicer to maintain an in-flight counter
instead of querying the device.

(A mask is inapplicable,
as the size of the device TX FIFO is not necessarily a power-of-2,
though the packets have sequence numbers so it does not matter.)

I guess a write barrier would be needed before tx_tail is incremented,
for the xmit function not to see it before __can_get_echo_skb() has
cleared the echo skb slot? (Or else, can_put_echo_skb() errors if the
slot is already non-NULL.) It is not obvious to me how rockchip-canfd
handles this?

>> +	/*
>> +	 * Without room for a new message, stop the queue until at least
>> +	 * one successful transmit.
>> +	 */
>> +	if (!netif_subqueue_maybe_stop(netdev, 0, kvaser_pciefd_tx_avail(can), 1, 1))
>> +		return NETDEV_TX_BUSY;
>
> Returning NETDEV_TX_BUSY is quite expensive, stop the queue at the end
> of this function, if the buffers are full.

Will do.

>> @@ -1638,11 +1650,25 @@ static int kvaser_pciefd_read_buffer(struct kvaser_pciefd *pcie, int dma_buf)
>>  {
>>  	int pos = 0;
>>  	int res = 0;
>> +	unsigned int i;
>>  
>>  	do {
>>  		res = kvaser_pciefd_read_packet(pcie, &pos, dma_buf);
>>  	} while (!res && pos > 0 && pos < KVASER_PCIEFD_DMA_SIZE);
>>  
>> +	for (i = 0; i < pcie->nr_channels; ++i) {
>> +		struct kvaser_pciefd_can *can = pcie->can[i];
>> +
>> +		if (!can->completed_tx_pkts)
>> +			continue;
>> +		netif_subqueue_completed_wake(can->can.dev, 0,
>> +					      can->completed_tx_pkts,
>> +					      can->completed_tx_bytes,
>> +					      kvaser_pciefd_tx_avail(can), 1);
>
> You can do this as soon as as one packet is finished, if you want to
> avoid too frequent wakeups, use threshold of more than 1.

I did that initially, but changed it to follow the advice of the
netdev_tx_completed_queue() docstring. Since the RX buffer for a single
IRQ may have multiple packets, would not BQL see incorrect intervals in
that case?


Thanks for the review
Axel Forsman

[PATCH 3/3] can: kvaser_pciefd: Continue parsing DMA buf after dropped RX

[Axel Forsman]

Going bus-off on a channel doing RX could result in dropped packets.

As netif_running() gets cleared before the channel abort procedure,
the handling of any last RDATA packets would see netif_rx() return
non-zero to signal a dropped packet. kvaser_pciefd_read_buffer() dealt
with this "error" by breaking out of processing the remaining DMA RX
buffer.

Only return an error from kvaser_pciefd_read_buffer() due to packet
corruption, otherwise handle it internally.

Cc: stable@vger.kernel.org
Signed-off-by: Axel Forsman <axfo@kvaser.com>
Tested-by: Jimmy Assarsson <extja@kvaser.com>
Reviewed-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/kvaser_pciefd.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 6251a1ddfa7e..1f4004204fa8 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -1216,7 +1216,7 @@ static int kvaser_pciefd_handle_data_packet(struct kvaser_pciefd *pcie,
 		skb = alloc_canfd_skb(priv->dev, &cf);
 		if (!skb) {
 			priv->dev->stats.rx_dropped++;
-			return -ENOMEM;
+			return 0;
 		}
 
 		cf->len = can_fd_dlc2len(dlc);
@@ -1228,7 +1228,7 @@ static int kvaser_pciefd_handle_data_packet(struct kvaser_pciefd *pcie,
 		skb = alloc_can_skb(priv->dev, (struct can_frame **)&cf);
 		if (!skb) {
 			priv->dev->stats.rx_dropped++;
-			return -ENOMEM;
+			return 0;
 		}
 		can_frame_set_cc_len((struct can_frame *)cf, dlc, priv->ctrlmode);
 	}
@@ -1246,7 +1246,9 @@ static int kvaser_pciefd_handle_data_packet(struct kvaser_pciefd *pcie,
 	priv->dev->stats.rx_packets++;
 	kvaser_pciefd_set_skb_timestamp(pcie, skb, p->timestamp);
 
-	return netif_rx(skb);
+	netif_rx(skb);
+
+	return 0;
 }
 
 static void kvaser_pciefd_change_state(struct kvaser_pciefd_can *can,
-- 
2.47.2