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]
Date:	Mon, 7 Mar 2016 08:32:19 +0000
From:	Ramesh Shanmugasundaram <ramesh.shanmugasundaram@...renesas.com>
To:	Marc Kleine-Budde <mkl@...gutronix.de>,
	Oliver Hartkopp <socketcan@...tkopp.net>,
	"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 Marc,

> On 03/07/2016 09:02 AM, Ramesh Shanmugasundaram wrote:
> > Hi Oliver,
> >
> > Thanks for the review comments.
> >
> >> On 03/03/2016 04:38 PM, Ramesh Shanmugasundaram wrote:
> >>
> >>> Changes since v1:
> >>> 	* Removed testmodes & debugfs code (suggested by Oliver H)
> >>> 	* Fixed tx path race issue by introducing lock (suggested by Marc K)
> >>> 	* Removed __maybe_unused attribute of rcar_canfd_of_table
> >>>
> >>
> >>
> >>>  obj-$(CONFIG_CAN_M_CAN)		+= m_can/
> >>>  obj-$(CONFIG_CAN_RCAR)		+= rcar_can.o
> >>> +obj-$(CONFIG_CANFD_RCAR)	+= rcar_canfd.o
> >>
> >> Just for the sake of an ordered directory structure:
> >>
> >> Would it make sense to create a rcar directory where rcar_can.c and
> >> rcar_canfd.c are placed?
> >>
> > Yes. I'll place them in rcar dir.
> >
> >> Additionally (besides of one accident with the obsolete PCH_CAN) all
> >> CAN drivers start with CONFIG_CAN_...
> >>
> >> Following that scheme the config option should be named
> >>
> >> 	CONFIG_CAN_RCAR_CANFD
> >>
> >> as we had in CONFIG_CAN_IFI_CANFD.
> >
> > Agreed.
> >
> >>
> >>>  obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
> >>>  obj-$(CONFIG_CAN_SUN4I)		+= sun4i_can.o
> >>>  obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
> >>> diff --git a/drivers/net/can/rcar_canfd.c
> >>> b/drivers/net/can/rcar_canfd.c
> >>
> >> (..)
> >>
> >>> +/* Channel priv data */
> >>> +struct rcar_canfd_channel {
> >>> +	struct can_priv can;			/* Must be the first member */
> >>> +	struct net_device *ndev;
> >>> +	struct rcar_canfd_global *gpriv;	/* Controller reference */
> >>> +	void __iomem *base;			/* Register base address */
> >>> +#ifdef CONFIG_DEBUG_FS
> >>> +	struct dentry *dev_root;
> >>> +#endif /* CONFIG_DEBUG_FS */
> >>
> >> Some missed stuff from debugfs removal?
> >
> > :-(. Sorry. Will clean up.
> >
> >>
> >>> +	struct napi_struct napi;
> >>> +	u8  tx_len[RCANFD_FIFO_DEPTH];		/* For net stats */
> >>> +	u32 tx_head;				/* Incremented on xmit */
> >>> +	u32 tx_tail;				/* Incremented on xmit done */
> >>> +	u32 channel;				/* Channel number */
> >>> +	spinlock_t tx_lock;			/* To protect tx path */
> >>> +};
> >>
> >> (..)
> >>
> >>> +static int rcar_canfd_start(struct net_device *ndev) {
> >>> +	struct rcar_canfd_channel *priv = netdev_priv(ndev);
> >>> +	int err = -EOPNOTSUPP;
> >>> +	u32 sts, ch = priv->channel;
> >>> +	u32 ridx = ch + RCANFD_RFFIFO_IDX;
> >>> +
> >>> +	/* 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.
> 
> 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.

The controller does support a "pure" classical CAN mode with a different set of register map itself. 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.

Thanks,
Ramesh