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

From: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>,
	"wg@grandegger.com" <wg@grandegger.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"pawel.moll@arm.com" <pawel.moll@arm.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"ijc+devicetree@hellion.org.uk" <ijc+devicetree@hellion.org.uk>,
	"galak@codeaurora.org" <galak@codeaurora.org>,
	"corbet@lwn.net" <corbet@lwn.net>
Cc: "linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"geert+renesas@glider.be" <geert+renesas@glider.be>,
	Chris Paterson <Chris.Paterson2@renesas.com>
Subject: RE: [PATCH] can: rcar_canfd: Add Renesas R-Car CAN FD driver
Date: Wed, 2 Mar 2016 10:08:16 +0000	[thread overview]
Message-ID: <SG2PR06MB1038DD32D284801452BA633EC3BC0@SG2PR06MB1038.apcprd06.prod.outlook.com> (raw)
In-Reply-To: <56D6B067.306@pengutronix.de>

Hi Marc,

> On 03/02/2016 09:41 AM, Ramesh Shanmugasundaram wrote:
> >> Is the channel loopback mode configurable per channel? If so, please
> >> remove the module parameter and make use of CAN_CTRLMODE_LOOPBACK to
> >> configure it.
> >
> > The loopback setting is not truly a per channel attribute. It requires
> > touching Rx rules which can be done only when the controller's global
> > state is reset (during probe).
> > CAN_CTRLMODE_LOOPBACK config option is possible only through .ndo_open
> > of that channel but the global controller state needs to be
> > operational by this time. As it is a global attribute & for the above
> > reason, I choose the module option.
> 
> I see, ok then.
> 
> >>> CAN FD mode supports both Classical CAN & CAN FD frame formats. The
> >>> controller supports ISO 11898-1:2015 CAN FD format only.
> >>>
> >>> This controller supports two channels and the driver can enable
> >>> either or both of the channels.
> >>>
> >>> Driver uses Rx FIFOs (one per channel) for reception & Common FIFOs
> >>> (one per channel) for transmission. Rx filter rules are configured
> >>> to the minimum (one per channel) and it accepts Standard, Extended,
> >>> Data & Remote Frame combinations.
> >>
> >> 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.

> > However, looking at it again, I should move the incrementing of head
> > after the "sts" handing to be apt. What do you think?
> 
> With one producer (one SW instance) and one consumer (the HW) you can
> write lockless code (if the HW allows it), but with two producers it's not
> possible.

For a channel represented as netdev, there is still one producer (one SW instance) isn't it? net acquires lock before calling xmit. The consumer is tx completion interrupt handler which updates per channel attributes only.

Sorry if I am annoying. I have tested this with two channels transmitting & receiving at the same time and it works fine. If I am missing lock, I would like to understand it thoroughly before making the change.

Thanks for the help,
Ramesh.