netdev - RE: [PATCH v2] 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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SG2PR06MB1038B71C7921984611F72CF5C3B20@SG2PR06MB1038.apcprd06.prod.outlook.com>
Date:	Tue, 8 Mar 2016 08:57:50 +0000
From:	Ramesh Shanmugasundaram <ramesh.shanmugasundaram@...renesas.com>
To:	Oliver Hartkopp <socketcan@...tkopp.net>,
	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 v2] can: rcar_canfd: Add Renesas R-Car CAN FD driver

Hi Oliver,

Thanks for the comments.

> On 03/07/2016 09:32 AM, Ramesh Shanmugasundaram wrote:
> 
> >>>>> +	/* Ensure channel starts in FD mode */
> >>>>> +	if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD)) {
> >>>>> +		netdev_err(ndev, "enable can fd mode for channel %d\n",
> ch);
> >>>>> +		goto fail_mode;
> >>>>> +	}
> >>>>
> >>>> What's the reason behind this check?
> >>>>
> >>>> A CAN FD capable CAN controller can be either configured to run as
> >>>> CAN 2.0 (Classic CAN) or as CAN FD controller.
> >>>>
> >>>> So why are to throwing an error here and produce an initialization
> >>>> failure?
> >>>
> >>> When this controller is configured in FD mode and used only with CAN
> >>> 2.0 nodes, it still expects a DTSEG (data bitrate) configuration
> >>> same as NTSEG (nominal bitrate). This check, specifically in
> >>> ndo_open, ensures both are configured and will work fine with CAN
> >>> 2.0 nodes
> >>> (e.g.)
> >>>
> >>> "ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on"
> >>>
> >>> If I don't have this check, a configuration like this
> >>>
> >>> "ip link set can0 up type can bitrate 1000000"
> >>>
> >>> will bring up the controller without DTSEG configured.
> 
> What about spending some status flag or setting the data bitrate equal to
> the nominal bitrate unless a data bitrate is provided?

As you mentioned further down, when a user does this 

"ip link set can0 up type can bitrate 1000000"

the intention is to put the controller in CAN 2.0 mode. Even if we use a status flag or copy the data bitrate equal to the nominal bitrate, what would it achieve? It still cannot be a CAN 2.0 node - it is a CAN FD node configured with same nominal & data bitrate.

This is why I have this check in ndo_open, so that the user is aware it is a CAN FD node always and avoid misconfiguration like above with EOPNOTSUPP. 

> 
> >>
> >> That should bring up the controller in CAN 2.0 mode.
> >
> > Yes, that's the user's intention but the manual states DTSEG still need
> to be configured. In the above configuration, it will not be.
> > Besides, this will not be a "pure" CAN 2.0 node (i.e.) if a frame with
> length > 8 bytes is received the controller will "ACK" because in FD mode
> it can receive up to 64 bytes frame.
> 
> Oh. We probably mix something up here (CAN frame formats & bitrates).
> 
> A CAN2.0 frame and a CAN FD frame have very different representations on
> the wire! So if you see a FDF (former EDL) bit this is a CAN FD frame,
> which requires two bitrates (nominal/data bitrate) where the data bitrate
> has to be greater or equal the nominal bitrate.
> 
> The fact that the data bitrate is equal the nominal/arbitration bitrate
> has nothing to do with CAN2.0 then. Regarding your answer this is not even
> "a pure CAN2.0" node - it still looks like a CAN FD node with equal
> data/nominal bitrates.

I agree. May be I mixed up my wordings but my intention is same - the controller is still an FD node & not pure CAN 2.0 node. This is why I have the check.

> 
> The fact that a CAN FD frame has a size of 8 bytes doesn't make it a
> CAN2.0 frame :-)
> 
> >
> > The controller does support a "pure" classical CAN mode with a different
> set of register map itself.
> 
> Is this a can_rcar controller register mapping then?

Nope. This is a different IP compared to can_rcar. It has a different set of register map within the IP to act as a pure classical CAN 2.0 node. When I say "pure", it will pass CAN 2.0 conformance tests :-). It is worth adding this support? Do you think of a strong use case?
 
> 
> > Do you think a pure CAN 2.0 mode support would be beneficial? I can
> submit this in coming days on top of current submission.
> >
> > The current submission status is:
> >  - Controller operates in CAN FD mode only.
> >  - If needed to interoperate with CAN 2.0 nodes, data bitrate still need
> to be configured and it will work perfectly. However, it is not a "pure"
> CAN 2.0 node as mentioned above.
> 
> When you have a CAN FD /capable/ controller the idea is:
> 
> "ip link set can0 up type can bitrate 1000000"
> 
> The controller is in CAN2.0 mode:
> 
> 1. It can send and receive CAN2.0 frames @1MBit/s.
> 2. The MTU is set to 16 (sizeof(struct can_frame)) ; CAN_CTRLMODE_FD is
> unset.
> 3. The CAN controller is not CAN FD tolerant (will produce error frames)
> 
> "ip link set can0 up type can bitrate 1000000 dbitrate 1000000 fd on"
> 
> 1. It can send and receive CAN2.0 frames @1MBit/s.
> 2. It can send and receive CAN FD frames @1MBit/s (arbitration&data
> bitrate).
> 3. The MTU is set to 72 (sizeof(struct canfd_frame)) ; CAN_CTRLMODE_FD is
> set.
> 
> For CAN FD frames the data bitrate can be increased like:
> "ip link set can0 up type can bitrate 1000000 dbitrate 4000000 fd on"
> 
> So when CAN_CTRLMODE_FD is unset the controller should act like a "pure
> CAN2.0" node.

Yes & I am glad you clarified this expectation. I think we both agree on this. With my current submission status, the controller can act as a CAN FD node only. Do you agree that the check I had in ndo_open makes sense now?

Thanks,
Ramesh