RE: [PATCH] can: rcar_canfd: Add Renesas R-Car CAN FD driver

[Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>]

Hi Marc,

Sorry for the late response.

> On 03/02/2016 11:08 AM, Ramesh Shanmugasundaram wrote:
> >>>> I see no locking for the tx-path.
> >>>
> >>> I am not sure why locking is needed in tx-path?
> >>
> >> If the tx-path is shared between both channels you need locking as
> >> the networking subsystem may send one frame to each controller at the
> >> same time.
> >
> > Yes. Tx completion interrupt is shared by both channels but the Tx
> > FIFO, echo skbs, head, tail are maintained on a per channel basis.
> > Yes, net subsystem can send one frame to each channel at the same time
> > and the tx completion interrupt handler will traverse each channel &
> > update per channel context accordingly.
> 
> Ok. Which instruction starts the transmit? Please add a comment to the
> code.

Please see

---<snip>---

+	/* Start Tx: Write 0xff to CFPC to increment the CPU-side
+	 * pointer for the Common FIFO
+	 */
+	rcar_canfd_write(priv, RCANFD_CFPCTR(ch, RCANFD_CFFIFO_IDX), 0xff);

---<snip>---

> You stop the tx_queue after this, so your code is racy, as the
> interrupt might happen in between.

It is a very good point. Thank you. 
I managed to reproduce this issue once with 50Kbps rate and ignoring -ENOBUFS in user space. It also exposed another subtle issue in the echo skb management where the tx_done loop pushes the skb before the actual ACK for that index happens. Fixed both issues.
As you mentioned, I have introduced a lock to protect this critical section. I'll send the updates in the next v2 patch.

Thanks,
Ramesh