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: <SG2PR06MB10386D712BC8C345DF726523C3B10@SG2PR06MB1038.apcprd06.prod.outlook.com>
Date:	Mon, 7 Mar 2016 08:02:10 +0000
From:	Ramesh Shanmugasundaram <ramesh.shanmugasundaram@...renesas.com>
To:	Oliver Hartkopp <socketcan@...tkopp.net>,
	"mkl@...gutronix.de" <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 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. 

Thanks,
Ramesh