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: <56DD36F5.2000308@pengutronix.de>
Date:	Mon, 7 Mar 2016 09:08:21 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Ramesh Shanmugasundaram <ramesh.shanmugasundaram@...renesas.com>,
	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

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.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)