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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Mar 2016 13:48:14 +0000
From:	Ramesh Shanmugasundaram <ramesh.shanmugasundaram@...renesas.com>
To:	Marc Kleine-Budde <mkl@...gutronix.de>,
	"wg@...ndegger.com" <wg@...ndegger.com>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"pawel.moll@....com" <pawel.moll@....com>,
	"mark.rutland@....com" <mark.rutland@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	"corbet@....net" <corbet@....net>
CC:	"linux-renesas-soc@...r.kernel.org" 
	<linux-renesas-soc@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"geert+renesas@...der.be" <geert+renesas@...der.be>,
	Chris Paterson <Chris.Paterson2@...esas.com>
Subject: RE: [PATCH] can: rcar_canfd: Add Renesas R-Car CAN FD driver

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