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  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, 8 Aug 2016 16:05:46 +0200
From:	Andreas Werner <andreas.werner@....de>
To:	Wolfgang Grandegger <wg@...ndegger.com>
CC:	Andreas Werner <andreas.werner@....de>, <mkl@...gutronix.de>,
	<linux-can@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <davem@...emloft.net>,
	<jthumshirn@...e.de>, <andy@...nerandy.de>,
	<michael.miehling@....de>
Subject: Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller
 driver

On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote:
> Hello,
> 
> Am 08.08.2016 um 13:39 schrieb Andreas Werner:
> >On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote:
> >>Hello Andreas,
> >>
> >>a first quick review....
> >>
> >>Am 26.07.2016 um 11:16 schrieb Andreas Werner:
> >>>This CAN Controller is found on MEN Chameleon FPGAs.
> >>>
> >>>The driver/device supports the CAN2.0 specification.
> >>>There are 255 RX and 255 Tx buffer within the IP. The
> >>>pointer for the buffer are handled by HW to make the
> >>>access from within the driver as simple as possible.
> >>>
> >>>The driver also supports parameters to configure the
> >>>buffer level interrupt for RX/TX as well as a RX timeout
> >>>interrupt.
> >>>
> >>>With this configuration options, the driver/device
> >>>provides flexibility for different types of usecases.
> >>>
> >>>Signed-off-by: Andreas Werner <andreas.werner@....de>
> >>>---
> >>>drivers/net/can/Kconfig        |  10 +
> >>>drivers/net/can/Makefile       |   1 +
> >>>drivers/net/can/men_z192_can.c | 989 +++++++++++++++++++++++++++++++++++++++++
> >>>3 files changed, 1000 insertions(+)
> >>>create mode 100644 drivers/net/can/men_z192_can.c
> 
> ---snip---
> 
> >>>+/* Buffer level control values */
> >>>+#define MEN_Z192_MIN_BUF_LVL	0
> >>>+#define MEN_Z192_MAX_BUF_LVL	254
> >>>+#define MEN_Z192_RX_BUF_LVL_DEF	5
> >>>+#define MEN_Z192_TX_BUF_LVL_DEF	5
> >>>+#define MEN_Z192_RX_TOUT_MIN	0
> >>>+#define MEN_Z192_RX_TOUT_MAX	65535
> >>>+#define MEN_Z192_RX_TOUT_DEF	1000
> >>>+
> >>>+static int txlvl = MEN_Z192_TX_BUF_LVL_DEF;
> >>>+module_param(txlvl, int, S_IRUGO);
> >>>+MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default="
> >>>+		 __MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")");
> >>>+
> >>>+static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF;
> >>>+module_param(rxlvl, int, S_IRUGO);
> >>>+MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default="
> >>>+		 __MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")");
> >>>+
> >>
> >>What impact does the level have on the latency? Could you please add some
> >>comments.
> >
> >It has a impact on the latency.
> >rxlvl = 0 -> if one frame got received, a IRQ will be generated
> >rxlvl = 254 -> if 255 frames got received, a IRQ will be generated
> 
> Well, what's your usecase for rxlvl > 0? For me it's not obvious what it can
> be good for. The application usually wants the message as soon as possible.
> Anyway, the default should be *0*. For RX and TX.
> 

The HW provides such feature and the driver should be able to control it.
It was developed to control the IRQ load (like NAPI) by defining a level of the buffer
when the IRQ got asserted.

I aggree with you to set the default to "0" which is the main usecase.

> >>>+static int rx_timeout = MEN_Z192_RX_TOUT_DEF;
> >>>+module_param(rx_timeout, int, S_IRUGO);
> >>>+MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default="
> >>>+		 __MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")");
> >>
> >>Ditto. What is "rx_timeout" good for.
> >>
> >
> >The rx timeout is used im combination with the rxlvl to assert the
> >if the buffer level is not reached within this timeout.
> 
> What event will the application receive in case of a timeout.
> 

Its just to control the time when the RX IRQ will be asserted if the buffer
level is not reached.
Imagine if the rx_timeout is not existing and the rxlvl is set to 50 and 
only 30 packets are received. The RX IRQ will be never asserted.

By defining the rx_timeout, we can control the time when the RX IRQ is asserted
if the buffer level is not reached.

The application does not receive any special signal, its just the RX IRQ.

> >Both, the timeout and the level are used to give the user as much
> >control over the latency and the IRQ handling as possible.
> >With this two options, the driver can be configured for different
> >use cases.
> >
> >I will add this as the comment to make it more clear.
> 
> Even a bit more would be appreciated.
> 

Sure...

> 
> ---snip---
> 
> >>>+static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr)
> >>>+{
> >>>+	struct net_device_stats *stats = &ndev->stats;
> >>>+	struct men_z192 *priv = netdev_priv(ndev);
> >>>+	struct men_z192_cf_buf __iomem *cf_buf;
> >>>+	struct can_frame *cf;
> >>>+	struct sk_buff *skb;
> >>>+	u32 cf_offset;
> >>>+	u32 length;
> >>>+	u32 data;
> >>>+	u32 id;
> >>>+
> >>>+	skb = alloc_can_skb(ndev, &cf);
> >>>+	if (unlikely(!skb)) {
> >>>+		stats->rx_dropped++;
> >>>+		return 0;
> >>>+	}
> >>>+
> >>>+	cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr;
> >>>+
> >>>+	cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset;
> >>>+	length = readl(&cf_buf->length) & MEN_Z192_CFBUF_LEN;
> >>>+	id = readl(&cf_buf->can_id);
> >>>+
> >>>+	if (id & MEN_Z192_CFBUF_IDE) {
> >>>+		/* Extended frame */
> >>>+		cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3;
> >>>+		cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >>
> >>>+				MEN_Z192_CFBUF_ID2_SHIFT;
> >>>+
> >>>+		cf->can_id |= CAN_EFF_FLAG;
> >>>+
> >>>+		if (id & MEN_Z192_CFBUF_E_RTR)
> >>>+			cf->can_id |= CAN_RTR_FLAG;
> >>>+	} else {
> >>>+		/* Standard frame */
> >>>+		cf->can_id = (id & MEN_Z192_CFBUF_ID1) >>
> >>>+				MEN_Z192_CFBUF_ID1_SHIFT;
> >>>+
> >>>+		if (id & MEN_Z192_CFBUF_S_RTR)
> >>>+			cf->can_id |= CAN_RTR_FLAG;
> >>>+	}
> >>>+
> >>>+	cf->can_dlc = get_can_dlc(length);
> >>>+
> >>>+	/* remote transmission request frame
> >>>+	 * contains no data field even if the
> >>>+	 * data length is set to a value > 0
> >>>+	 */
> >>>+	if (!(cf->can_id & CAN_RTR_FLAG)) {
> >>>+		if (cf->can_dlc > 0) {
> >>>+			data = readl(&cf_buf->data[0]);
> >>>+			*(__be32 *)cf->data = cpu_to_be32(data);
> >>
> >>Do you really need the extra copy?
> >>
> >>>+		}
> >>>+		if (cf->can_dlc > 4) {
> >>>+			data = readl(&cf_buf->data[1]);
> >>>+			*(__be32 *)(cf->data + 4) = cpu_to_be32(data);
> >>
> >>Ditto.
> >
> >No its not really needed. I thought its more clean and more readable than
> >putting this in one line withouth the copy.
> 
> It should be fast in the first place.
> 

Ok, will change that.

[...]

> 
> >>>+static int men_z192_set_mode(struct net_device *ndev, enum can_mode mode)
> >>>+{
> >>>+	int ret;
> >>>+
> >>>+	switch (mode) {
> >>>+	case CAN_MODE_START:
> >>>+		ret = men_z192_start(ndev);
> >>>+		if (ret)
> >>>+			return ret;
> >>
> >>"if (ret)" means always an error. Therefore s/ret/err/ is clearer. Here and
> >>in many other places.
> >>
> >
> >Yes and no. I think its a general question about the naming of those variables.
> >I will check all the variables in the driver if it really makes sense
> >to rename it.
> >
> >For my opinion, "ret" is more generic. But you are right, "err" would be more
> >readable in some places.
> 
> 	if (err)
> 
> makes immediately clear that it's an error case. ret is more general, e.g.
> for the return value of read/write:
> 
>         if (ret < 0)
> 		error-case
>         else if (ret == 0)
> 		end-of-file
> 	else
> 		btyes-read
> 		
> Just my personal preference to make the code more readable.

Ok, I will think about it.

> 
> >>>+
> >>>+		netif_wake_queue(ndev);
> >>>+		break;
> >>>+	default:
> >>>+		return -EOPNOTSUPP;
> >>>+	}
> >>>+
> >>>+	return 0;
> >>>+static int men_z192_probe(struct mcb_device *mdev,
> >>>+			  const struct mcb_device_id *id)
> >>>+{
> >>>+	struct device *dev = &mdev->dev;
> >>>+	struct men_z192 *priv;
> >>>+	struct net_device *ndev;
> >>>+	void __iomem *dev_base;
> >>>+	struct resource *mem;
> >>>+	u32 timebase;
> >>>+	int ret = 0;
> >>>+	int irq;
> >>>+
> >>>+	mem = mcb_request_mem(mdev, dev_name(dev));
> >>>+	if (IS_ERR(mem)) {
> >>>+		dev_err(dev, "failed to request device memory");
> >>>+		return PTR_ERR(mem);
> >>>+	}
> >>>+
> >>>+	dev_base = ioremap(mem->start, resource_size(mem));
> >>>+	if (!dev_base) {
> >>>+		dev_err(dev, "failed to ioremap device memory");
> >>>+		ret = -ENXIO;
> >>>+		goto out_release;
> >>>+	}
> >>>+
> >>>+	irq = mcb_get_irq(mdev);
> >>>+	if (irq <= 0) {
> >>>+		ret = -ENODEV;
> >>>+		goto out_unmap;
> >>>+	}
> >>>+
> >>>+	ndev = alloc_candev(sizeof(struct men_z192), 1);
> >>
> >>You specify here one echo_skb but it's not used anywhere. Local loopback
> >>seems not to be implemented.
> >>
> >
> >Agree with you, will set it to "0".
> 
> No, the local loopback is mandetory!
> 

Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c
it is not mandatory. In the Documentation/networking/can.txt
there is also a "should" and a fallback mechnism if the driver
does not support the local loopback.
I'm currently ok with this fallback mechanism.

Anyway I am not sure that the driver can handle the echo skb correctly.
If i understand it correctly, the can_get_echo_skb() is normally called
on a "TX done IRQ" to get the skb and loop it back. 
I do not have such a "TX done IRQ" and have not implemented implemented
and added the local looback.

May be I can put and get the echo skb within the xmit function?
Does this make sense?

Regards
Andy


> Wolfgang.

Powered by blists - more mailing lists