On 05.08.2021 22:16:06, Dario Binacchi wrote:

quoted

quoted

--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h

@@ -200,6 +200,7 @@ struct c_can_priv {
 	atomic_t sie_pending;
 	unsigned long tx_dir;
 	int last_status;
+	spinlock_t tx_lock;

What does the spin lock protect?

[...]

quoted

quoted

@@ -483,7 +469,11 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 	if (c_can_get_tx_free(tx_ring) == 0)
 		netif_stop_queue(dev);
 
-	obj = idx + priv->msg_obj_tx_first;
+	spin_lock_bh(&priv->tx_lock);

What does the spin_lock protect? The ndo_start_xmit function is properly
serialized by the networking core.

The spin_lock protects the access to the IF_TX interface.

How? You only use the spin_lock in c_can_start_xmit(), but not anywhere
else.

Enabling the transmission of cached messages occur inside interrupt

The call chain is c_can_poll() -> c_can_do_tx(), and c_can_poll() is
called from NAPI, which is not the IRQ handler.

and the use of the IF_RX interface, which would avoid the use of the
spinlock, has not been validated by the tests.

What do you mean be has not been validated?

The driver already uses IF_RX to avoid concurrent access in
c_can_do_tx() for c_can_inval_tx_object() [1], why not use IF_RX for
c_can_object_put(), too?

[1] https://lore.kernel.org/r/20210302215435.18286-4-dariobin@libero.it (local)

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |