Probably it is not necessary to protect the first half of the tx-irq:
```c
static void c_can_do_tx(struct net_device *dev)
{
struct c_can_priv *priv = netdev_priv(dev);
struct c_can_tx_ring *tx_ring = &priv->tx;
struct net_device_stats *stats = &dev->stats;
u32 idx, obj, pkts = 0, bytes = 0, pend;
u8 tail;
if (priv->msg_obj_tx_last > 32)
pend = priv->read_reg32(priv, C_CAN_INTPND3_REG);
else
pend = priv->read_reg(priv, C_CAN_INTPND2_REG);
while ((idx = ffs(pend))) {
idx--;
pend &= ~BIT(idx);
obj = idx + priv->msg_obj_tx_first;
/* We use IF_NAPI interface instead of IF_TX because we
* are called from c_can_poll(), which runs inside
* NAPI. We are not transmitting.
*/
c_can_inval_tx_object(dev, IF_NAPI, obj);
bytes += can_get_echo_skb(dev, idx, NULL);
pkts++;
}
if (!pkts)
return;
////////////// spinlock starting here
tx_ring->tail += pkts;
if (c_can_get_tx_free(priv, tx_ring)) {
/* Make sure that anybody stopping the queue after
* this sees the new tx_ring->tail.
*/
smp_mb();
////////////// Do we get a problem here because this might call the send routine?! I have no idea. Maybe this should be moved to the end of the function and be removed out of the spin lock section
netif_wake_queue(priv->dev); // is this the equivalent to stop queue?
}
stats->tx_bytes += bytes;
stats->tx_packets += pkts;
tail = c_can_get_tx_tail(tx_ring);
if (priv->type == BOSCH_D_CAN && tail == 0) {
u8 head = c_can_get_tx_head(tx_ring);
/* Start transmission for all cached messages */
for (idx = tail; idx < head; idx++) {
obj = idx + priv->msg_obj_tx_first;
c_can_object_put(dev, IF_NAPI, obj, IF_COMM_TXRQST);
}
}
}
```
In the sending routine, I think most stuff needs to be protected. Some parts could be rearranged to reduce the amount of code in the critical area.
```c
static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
struct net_device *dev)
{
struct can_frame *frame = (struct can_frame *)skb->data;
struct c_can_priv *priv = netdev_priv(dev);
struct c_can_tx_ring *tx_ring = &priv->tx;
u32 idx, obj, cmd = IF_COMM_TX;
if (can_dev_dropped_skb(dev, skb))
return NETDEV_TX_OK;
if (c_can_tx_busy(priv, tx_ring))
return NETDEV_TX_BUSY;
/////// spinlock starting here
idx = c_can_get_tx_head(tx_ring); /// this probably doesnâ??t need the spinlock yet, because it just reads and the tx-irq does not modify
tx_ring->head++; // this needs to be protected because tx-irq will use it to decide how many tx request bits it needs to set
if (c_can_get_tx_free(priv, tx_ring) == 0) // if this wasnâ??t protected, the queue could possibly be stopped although a message object just became free, which would not be too bad
netif_stop_queue(dev);
if (idx < c_can_get_tx_tail(tx_ring)) // this needs to be protected
cmd &= ~IF_COMM_TXRQST; /* Cache the message */
/* Store the message in the interface so we can call
* can_put_echo_skb(). We must do this before we enable
* transmit as we might race against do_tx().
*/
c_can_setup_tx_object(dev, IF_TX, frame, idx); // this needs to be protected because it calls c_can_inval_tx_object
can_put_echo_skb(skb, dev, idx, 0); // this probably doesnâ??t need protection
obj = idx + priv->msg_obj_tx_first;
c_can_object_put(dev, IF_TX, obj, cmd); // this needs protection again
return NETDEV_TX_OK;
}
```
I have done extensive tests with my patch, and it works well in my system. For me, it would be sufficient :-D.
In the meantime, I found that another driver had the same problem. They chose a similar solution
https://lore.kernel.org/lkml/20180327162802.559928021@xxxxxxxxxxxxxxxxxxx/
>
> > Thank you in advance for your time and assistance. I look forward to hearing
> your suggestions or advice.
>
> Thank you for the detailed report. Let me ask you: do you want to write the
> final patch? If yes, we can guide you into the process.
It would be an honour to do this :-)
>
> Yours sincerely,
> Vincent Mailhol
Best Regards,
Stefan Schmidt
