On Monday, 20 October 2025 at 16:56:08 Matias Ezequiel Vara Larsen [off-list ref] wrote:

On Tue, Oct 14, 2025 at 06:01:07PM +0200, Francesco Valla wrote:

quoted

On Tuesday, 14 October 2025 at 12:15:12 Matias Ezequiel Vara Larsen [off-list ref] wrote:

quoted

On Thu, Sep 11, 2025 at 10:59:40PM +0200, Francesco Valla wrote:

quoted

Hello Mikhail, Harald,

hoping there will be a v6 of this patch soon, a few comments:

On Monday, 8 January 2024 at 14:10:35 Mikhail Golubev-Ciuchea [off-list ref] wrote:

[...]

quoted

+
+/* Compare with m_can.c/m_can_echo_tx_event() */
+static int virtio_can_read_tx_queue(struct virtqueue *vq)
+{
+	struct virtio_can_priv *can_priv = vq->vdev->priv;
+	struct net_device *dev = can_priv->dev;
+	struct virtio_can_tx *can_tx_msg;
+	struct net_device_stats *stats;
+	unsigned long flags;
+	unsigned int len;
+	u8 result;
+
+	stats = &dev->stats;
+
+	/* Protect list and virtio queue operations */
+	spin_lock_irqsave(&can_priv->tx_lock, flags);
+
+	can_tx_msg = virtqueue_get_buf(vq, &len);
+	if (!can_tx_msg) {
+		spin_unlock_irqrestore(&can_priv->tx_lock, flags);
+		return 0; /* No more data */
+	}
+
+	if (unlikely(len < sizeof(struct virtio_can_tx_in))) {
+		netdev_err(dev, "TX ACK: Device sent no result code\n");
+		result = VIRTIO_CAN_RESULT_NOT_OK; /* Keep things going */
+	} else {
+		result = can_tx_msg->tx_in.result;
+	}
+
+	if (can_priv->can.state < CAN_STATE_BUS_OFF) {
+		/* Here also frames with result != VIRTIO_CAN_RESULT_OK are
+		 * echoed. Intentional to bring a waiting process in an upper
+		 * layer to an end.
+		 * TODO: Any better means to indicate a problem here?
+		 */
+		if (result != VIRTIO_CAN_RESULT_OK)
+			netdev_warn(dev, "TX ACK: Result = %u\n", result);

Maybe an error frame reporting CAN_ERR_CRTL_UNSPEC would be better?

I am not sure. In xilinx_can.c, CAN_ERR_CRTL_UNSPEC is indicated during
a problem in the rx path and this is the tx path. I think the comment
refers to improving the way the driver informs this error to the user
but I may be wrong.

Since we have no detail of what went wrong here, I suggested
CAN_ERR_CRTL_UNSPEC as it is "unspecified error", to be coupled with a
controller error with id CAN_ERR_CRTL; however, a different error might be
more appropriate.

For sure, at least in my experience, having a warn printed to kmsg is *not*
enough, as the application sending the message(s) would not be able to detect
the error.

quoted

quoted

For sure, counting the known errors as valid tx_packets and tx_bytes
is misleading.

I'll remove the counters below.

We don't really know what's wrong here - the packet might have been sent and
and then not ACK'ed, as well as any other error condition (as it happens in the
reference implementation from the original authors [1]). Echoing the packet
only "to bring a waiting process in an upper layer to an end" and incrementing
counters feels wrong, but maybe someone more expert than me can advise better
here.

I agree. IIUC, in case there has been a problem during transmission, I
should 1) indicate this by injecting a CAN_ERR_CRTL_UNSPEC package with
netif_rx() and 2) use can_free_echo_skb() and increment the tx_error
stats. Is this correct?

Matias

That's my understanding too! stats->tx_dropped should be the right value to
increment (see for example [1]).

[1] https://elixir.bootlin.com/linux/v6.17.3/source/drivers/net/can/ctucanfd/ctucanfd_base.c#L1035

Regards,
Francesco