pull-request: can 2013-06-03

pull-request: can 2013-06-03

[Marc Kleine-Budde]

Hello David,

here are there fixes for the v3.10 release cycle:

The first patch by Jonas Peterson and Olivier Sobrie fixes the reception of CAN
frames on Kvaser's "USBcan Pro" and "USBcan R" type hardware.

The last two patches by Olivier Sobrie (for esd_usb2) and me (for peak_usb)
change the memory handling for the USB messages from stack to kmalloc(), as
memory used for DMA should not be allocated on stack.

regards,
Marc

---

The following changes since commit 01cb71d2d47b78354358e4bb938bb06323e17498:

  net_sched: restore "overhead xxx" handling (2013-06-02 22:22:35 -0700)

are available in the git repository at:

  git://gitorious.org/linux-can/linux-can.git fixes-for-3.10

for you to fetch changes up to f14e22435a27ef183bbfa78f77ad86644c0b354c:

  net: can: peak_usb: Do not do dma on the stack (2013-06-03 14:05:32 +0200)

----------------------------------------------------------------
Jonas Peterson (1):
      net: can: kvaser_usb: fix reception on "USBcan Pro" and "USBcan R" type hardware.

Marc Kleine-Budde (1):
      net: can: peak_usb: Do not do dma on the stack

Olivier Sobrie (1):
      net: can: esd_usb2: Do not do dma on the stack

 drivers/net/can/usb/esd_usb2.c              | 127 +++++++++++++++++-----------
 drivers/net/can/usb/kvaser_usb.c            |  64 +++++++++-----
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c |  61 ++++++++-----
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h |   1 +
 4 files changed, 160 insertions(+), 93 deletions(-)

[PATCH 1/3] net: can: kvaser_usb: fix reception on "USBcan Pro" and "USBcan R" type hardware.

[Marc Kleine-Budde]

From: Jonas Peterson <jonas.peterson@gmail.com>

Unlike Kvaser Leaf light devices, some other Kvaser devices (like USBcan
Pro, USBcan R) receive CAN messages in CMD_LOG_MESSAGE frames. This
patch adds support for it.

Cc: linux-stable <stable@vger.kernel.org> # >= v3.8
Signed-off-by: Jonas Peterson <jonas.peterson@gmail.com>
Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/kvaser_usb.c | 64 +++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 45cb9f3..3b95465 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -136,6 +136,9 @@
 #define KVASER_CTRL_MODE_SELFRECEPTION	3
 #define KVASER_CTRL_MODE_OFF		4
 
+/* log message */
+#define KVASER_EXTENDED_FRAME		BIT(31)
+
 struct kvaser_msg_simple {
 	u8 tid;
 	u8 channel;
@@ -817,8 +820,13 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
 	priv = dev->nets[channel];
 	stats = &priv->netdev->stats;
 
-	if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME | MSG_FLAG_NERR |
-				  MSG_FLAG_OVERRUN)) {
+	if ((msg->u.rx_can.flag & MSG_FLAG_ERROR_FRAME) &&
+	    (msg->id == CMD_LOG_MESSAGE)) {
+		kvaser_usb_rx_error(dev, msg);
+		return;
+	} else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
+					 MSG_FLAG_NERR |
+					 MSG_FLAG_OVERRUN)) {
 		kvaser_usb_rx_can_err(priv, msg);
 		return;
 	} else if (msg->u.rx_can.flag & ~MSG_FLAG_REMOTE_FRAME) {
@@ -834,22 +842,40 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
 		return;
 	}
 
-	cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
-		     (msg->u.rx_can.msg[1] & 0x3f);
-	cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
+	if (msg->id == CMD_LOG_MESSAGE) {
+		cf->can_id = le32_to_cpu(msg->u.log_message.id);
+		if (cf->can_id & KVASER_EXTENDED_FRAME)
+			cf->can_id &= CAN_EFF_MASK | CAN_EFF_FLAG;
+		else
+			cf->can_id &= CAN_SFF_MASK;
 
-	if (msg->id == CMD_RX_EXT_MESSAGE) {
-		cf->can_id <<= 18;
-		cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
-			      ((msg->u.rx_can.msg[3] & 0xff) << 6) |
-			      (msg->u.rx_can.msg[4] & 0x3f);
-		cf->can_id |= CAN_EFF_FLAG;
-	}
+		cf->can_dlc = get_can_dlc(msg->u.log_message.dlc);
 
-	if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME)
-		cf->can_id |= CAN_RTR_FLAG;
-	else
-		memcpy(cf->data, &msg->u.rx_can.msg[6], cf->can_dlc);
+		if (msg->u.log_message.flags & MSG_FLAG_REMOTE_FRAME)
+			cf->can_id |= CAN_RTR_FLAG;
+		else
+			memcpy(cf->data, &msg->u.log_message.data,
+			       cf->can_dlc);
+	} else {
+		cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
+			     (msg->u.rx_can.msg[1] & 0x3f);
+
+		if (msg->id == CMD_RX_EXT_MESSAGE) {
+			cf->can_id <<= 18;
+			cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
+				      ((msg->u.rx_can.msg[3] & 0xff) << 6) |
+				      (msg->u.rx_can.msg[4] & 0x3f);
+			cf->can_id |= CAN_EFF_FLAG;
+		}
+
+		cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
+
+		if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME)
+			cf->can_id |= CAN_RTR_FLAG;
+		else
+			memcpy(cf->data, &msg->u.rx_can.msg[6],
+			       cf->can_dlc);
+	}
 
 	netif_rx(skb);
 
@@ -911,6 +937,7 @@ static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
 
 	case CMD_RX_STD_MESSAGE:
 	case CMD_RX_EXT_MESSAGE:
+	case CMD_LOG_MESSAGE:
 		kvaser_usb_rx_can_msg(dev, msg);
 		break;
 
@@ -919,11 +946,6 @@ static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
 		kvaser_usb_rx_error(dev, msg);
 		break;
 
-	case CMD_LOG_MESSAGE:
-		if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME)
-			kvaser_usb_rx_error(dev, msg);
-		break;
-
 	case CMD_TX_ACKNOWLEDGE:
 		kvaser_usb_tx_acknowledge(dev, msg);
 		break;
-- 
1.8.2.rc2

[PATCH 2/3] net: can: esd_usb2: Do not do dma on the stack

[Marc Kleine-Budde]

From: Olivier Sobrie <olivier@sobrie.be>

smatch reports the following warnings:
drivers/net/can/usb/esd_usb2.c:640 esd_usb2_start() error: doing dma on the stack (&msg)
drivers/net/can/usb/esd_usb2.c:846 esd_usb2_close() error: doing dma on the stack (&msg)
drivers/net/can/usb/esd_usb2.c:855 esd_usb2_close() error: doing dma on the stack (&msg)
drivers/net/can/usb/esd_usb2.c:923 esd_usb2_set_bittiming() error: doing dma on the stack (&msg)
drivers/net/can/usb/esd_usb2.c:1047 esd_usb2_probe() error: doing dma on the stack (&msg)
drivers/net/can/usb/esd_usb2.c:1053 esd_usb2_probe() error: doing dma on the stack (&msg)

See "Documentation/DMA-API-HOWTO.txt" section "What memory is DMA'able?"

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
Cc: Matthias Fuchs <matthias.fuchs@esd.eu>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/esd_usb2.c | 127 ++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 51 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb2.c b/drivers/net/can/usb/esd_usb2.c
index 9b74d1e..6aa7b32 100644
--- a/drivers/net/can/usb/esd_usb2.c
+++ b/drivers/net/can/usb/esd_usb2.c
@@ -612,9 +612,15 @@ static int esd_usb2_start(struct esd_usb2_net_priv *priv)
 {
 	struct esd_usb2 *dev = priv->usb2;
 	struct net_device *netdev = priv->netdev;
-	struct esd_usb2_msg msg;
+	struct esd_usb2_msg *msg;
 	int err, i;
 
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg) {
+		err = -ENOMEM;
+		goto out;
+	}
+
 	/*
 	 * Enable all IDs
 	 * The IDADD message takes up to 64 32 bit bitmasks (2048 bits).
@@ -628,33 +634,32 @@ static int esd_usb2_start(struct esd_usb2_net_priv *priv)
 	 * the number of the starting bitmask (0..64) to the filter.option
 	 * field followed by only some bitmasks.
 	 */
-	msg.msg.hdr.cmd = CMD_IDADD;
-	msg.msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
-	msg.msg.filter.net = priv->index;
-	msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
+	msg->msg.hdr.cmd = CMD_IDADD;
+	msg->msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+	msg->msg.filter.net = priv->index;
+	msg->msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
 	for (i = 0; i < ESD_MAX_ID_SEGMENT; i++)
-		msg.msg.filter.mask[i] = cpu_to_le32(0xffffffff);
+		msg->msg.filter.mask[i] = cpu_to_le32(0xffffffff);
 	/* enable 29bit extended IDs */
-	msg.msg.filter.mask[ESD_MAX_ID_SEGMENT] = cpu_to_le32(0x00000001);
+	msg->msg.filter.mask[ESD_MAX_ID_SEGMENT] = cpu_to_le32(0x00000001);
 
-	err = esd_usb2_send_msg(dev, &msg);
+	err = esd_usb2_send_msg(dev, msg);
 	if (err)
-		goto failed;
+		goto out;
 
 	err = esd_usb2_setup_rx_urbs(dev);
 	if (err)
-		goto failed;
+		goto out;
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
-	return 0;
-
-failed:
+out:
 	if (err == -ENODEV)
 		netif_device_detach(netdev);
+	if (err)
+		netdev_err(netdev, "couldn't start device: %d\n", err);
 
-	netdev_err(netdev, "couldn't start device: %d\n", err);
-
+	kfree(msg);
 	return err;
 }
 
@@ -833,26 +838,30 @@ nourbmem:
 static int esd_usb2_close(struct net_device *netdev)
 {
 	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
-	struct esd_usb2_msg msg;
+	struct esd_usb2_msg *msg;
 	int i;
 
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
 	/* Disable all IDs (see esd_usb2_start()) */
-	msg.msg.hdr.cmd = CMD_IDADD;
-	msg.msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
-	msg.msg.filter.net = priv->index;
-	msg.msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
+	msg->msg.hdr.cmd = CMD_IDADD;
+	msg->msg.hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+	msg->msg.filter.net = priv->index;
+	msg->msg.filter.option = ESD_ID_ENABLE; /* start with segment 0 */
 	for (i = 0; i <= ESD_MAX_ID_SEGMENT; i++)
-		msg.msg.filter.mask[i] = 0;
-	if (esd_usb2_send_msg(priv->usb2, &msg) < 0)
+		msg->msg.filter.mask[i] = 0;
+	if (esd_usb2_send_msg(priv->usb2, msg) < 0)
 		netdev_err(netdev, "sending idadd message failed\n");
 
 	/* set CAN controller to reset mode */
-	msg.msg.hdr.len = 2;
-	msg.msg.hdr.cmd = CMD_SETBAUD;
-	msg.msg.setbaud.net = priv->index;
-	msg.msg.setbaud.rsvd = 0;
-	msg.msg.setbaud.baud = cpu_to_le32(ESD_USB2_NO_BAUDRATE);
-	if (esd_usb2_send_msg(priv->usb2, &msg) < 0)
+	msg->msg.hdr.len = 2;
+	msg->msg.hdr.cmd = CMD_SETBAUD;
+	msg->msg.setbaud.net = priv->index;
+	msg->msg.setbaud.rsvd = 0;
+	msg->msg.setbaud.baud = cpu_to_le32(ESD_USB2_NO_BAUDRATE);
+	if (esd_usb2_send_msg(priv->usb2, msg) < 0)
 		netdev_err(netdev, "sending setbaud message failed\n");
 
 	priv->can.state = CAN_STATE_STOPPED;
@@ -861,6 +870,8 @@ static int esd_usb2_close(struct net_device *netdev)
 
 	close_candev(netdev);
 
+	kfree(msg);
+
 	return 0;
 }
 
@@ -886,7 +897,8 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
 {
 	struct esd_usb2_net_priv *priv = netdev_priv(netdev);
 	struct can_bittiming *bt = &priv->can.bittiming;
-	struct esd_usb2_msg msg;
+	struct esd_usb2_msg *msg;
+	int err;
 	u32 canbtr;
 	int sjw_shift;
 
@@ -912,15 +924,22 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
 	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
 		canbtr |= ESD_USB2_3_SAMPLES;
 
-	msg.msg.hdr.len = 2;
-	msg.msg.hdr.cmd = CMD_SETBAUD;
-	msg.msg.setbaud.net = priv->index;
-	msg.msg.setbaud.rsvd = 0;
-	msg.msg.setbaud.baud = cpu_to_le32(canbtr);
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->msg.hdr.len = 2;
+	msg->msg.hdr.cmd = CMD_SETBAUD;
+	msg->msg.setbaud.net = priv->index;
+	msg->msg.setbaud.rsvd = 0;
+	msg->msg.setbaud.baud = cpu_to_le32(canbtr);
 
 	netdev_info(netdev, "setting BTR=%#x\n", canbtr);
 
-	return esd_usb2_send_msg(priv->usb2, &msg);
+	err = esd_usb2_send_msg(priv->usb2, msg);
+
+	kfree(msg);
+	return err;
 }
 
 static int esd_usb2_get_berr_counter(const struct net_device *netdev,
@@ -1022,7 +1041,7 @@ static int esd_usb2_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
 	struct esd_usb2 *dev;
-	struct esd_usb2_msg msg;
+	struct esd_usb2_msg *msg;
 	int i, err;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
@@ -1037,27 +1056,33 @@ static int esd_usb2_probe(struct usb_interface *intf,
 
 	usb_set_intfdata(intf, dev);
 
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg) {
+		err = -ENOMEM;
+		goto free_msg;
+	}
+
 	/* query number of CAN interfaces (nets) */
-	msg.msg.hdr.cmd = CMD_VERSION;
-	msg.msg.hdr.len = 2;
-	msg.msg.version.rsvd = 0;
-	msg.msg.version.flags = 0;
-	msg.msg.version.drv_version = 0;
+	msg->msg.hdr.cmd = CMD_VERSION;
+	msg->msg.hdr.len = 2;
+	msg->msg.version.rsvd = 0;
+	msg->msg.version.flags = 0;
+	msg->msg.version.drv_version = 0;
 
-	err = esd_usb2_send_msg(dev, &msg);
+	err = esd_usb2_send_msg(dev, msg);
 	if (err < 0) {
 		dev_err(&intf->dev, "sending version message failed\n");
-		goto free_dev;
+		goto free_msg;
 	}
 
-	err = esd_usb2_wait_msg(dev, &msg);
+	err = esd_usb2_wait_msg(dev, msg);
 	if (err < 0) {
 		dev_err(&intf->dev, "no version message answer\n");
-		goto free_dev;
+		goto free_msg;
 	}
 
-	dev->net_count = (int)msg.msg.version_reply.nets;
-	dev->version = le32_to_cpu(msg.msg.version_reply.version);
+	dev->net_count = (int)msg->msg.version_reply.nets;
+	dev->version = le32_to_cpu(msg->msg.version_reply.version);
 
 	if (device_create_file(&intf->dev, &dev_attr_firmware))
 		dev_err(&intf->dev,
@@ -1075,10 +1100,10 @@ static int esd_usb2_probe(struct usb_interface *intf,
 	for (i = 0; i < dev->net_count; i++)
 		esd_usb2_probe_one_net(intf, i);
 
-	return 0;
-
-free_dev:
-	kfree(dev);
+free_msg:
+	kfree(msg);
+	if (err)
+		kfree(dev);
 done:
 	return err;
 }
-- 
1.8.2.rc2

[PATCH 3/3] net: can: peak_usb: Do not do dma on the stack

[Marc Kleine-Budde]

smatch reports the following warnings:
drivers/net/can/usb/peak_usb/pcan_usb_pro.c:514 pcan_usb_pro_drv_loaded() error: doing dma on the stack (buffer)
drivers/net/can/usb/peak_usb/pcan_usb_pro.c:878 pcan_usb_pro_init() error: doing dma on the stack (&fi)
drivers/net/can/usb/peak_usb/pcan_usb_pro.c:889 pcan_usb_pro_init() error: doing dma on the stack (&bi)

See "Documentation/DMA-API-HOWTO.txt" section "What memory is DMA'able?"

Cc: Stephane Grosjean <s.grosjean@peak-system.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 61 +++++++++++++++++++----------
 drivers/net/can/usb/peak_usb/pcan_usb_pro.h |  1 +
 2 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
index 30d79bf..8ee9d15 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
@@ -504,15 +504,24 @@ static int pcan_usb_pro_restart_async(struct peak_usb_device *dev,
 	return usb_submit_urb(urb, GFP_ATOMIC);
 }
 
-static void pcan_usb_pro_drv_loaded(struct peak_usb_device *dev, int loaded)
+static int pcan_usb_pro_drv_loaded(struct peak_usb_device *dev, int loaded)
 {
-	u8 buffer[16];
+	u8 *buffer;
+	int err;
+
+	buffer = kmalloc(PCAN_USBPRO_FCT_DRVLD_REQ_LEN, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
 
 	buffer[0] = 0;
 	buffer[1] = !!loaded;
 
-	pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_FCT,
-			      PCAN_USBPRO_FCT_DRVLD, buffer, sizeof(buffer));
+	err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_FCT,
+				    PCAN_USBPRO_FCT_DRVLD, buffer,
+				    PCAN_USBPRO_FCT_DRVLD_REQ_LEN);
+	kfree(buffer);
+
+	return err;
 }
 
 static inline
@@ -851,21 +860,24 @@ static int pcan_usb_pro_stop(struct peak_usb_device *dev)
  */
 static int pcan_usb_pro_init(struct peak_usb_device *dev)
 {
-	struct pcan_usb_pro_interface *usb_if;
 	struct pcan_usb_pro_device *pdev =
 			container_of(dev, struct pcan_usb_pro_device, dev);
+	struct pcan_usb_pro_interface *usb_if = NULL;
+	struct pcan_usb_pro_fwinfo *fi = NULL;
+	struct pcan_usb_pro_blinfo *bi = NULL;
+	int err;
 
 	/* do this for 1st channel only */
 	if (!dev->prev_siblings) {
-		struct pcan_usb_pro_fwinfo fi;
-		struct pcan_usb_pro_blinfo bi;
-		int err;
-
 		/* allocate netdevices common structure attached to first one */
 		usb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
 				 GFP_KERNEL);
-		if (!usb_if)
-			return -ENOMEM;
+		fi = kmalloc(sizeof(struct pcan_usb_pro_fwinfo), GFP_KERNEL);
+		bi = kmalloc(sizeof(struct pcan_usb_pro_blinfo), GFP_KERNEL);
+		if (!usb_if || !fi || !bi) {
+			err = -ENOMEM;
+			goto err_out;
+		}
 
 		/* number of ts msgs to ignore before taking one into account */
 		usb_if->cm_ignore_count = 5;
@@ -877,34 +889,34 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
 		 */
 		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
 					    PCAN_USBPRO_INFO_FW,
-					    &fi, sizeof(fi));
+					    fi, sizeof(*fi));
 		if (err) {
-			kfree(usb_if);
 			dev_err(dev->netdev->dev.parent,
 				"unable to read %s firmware info (err %d)\n",
 				pcan_usb_pro.name, err);
-			return err;
+			goto err_out;
 		}
 
 		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
 					    PCAN_USBPRO_INFO_BL,
-					    &bi, sizeof(bi));
+					    bi, sizeof(*bi));
 		if (err) {
-			kfree(usb_if);
 			dev_err(dev->netdev->dev.parent,
 				"unable to read %s bootloader info (err %d)\n",
 				pcan_usb_pro.name, err);
-			return err;
+			goto err_out;
 		}
 
+		/* tell the device the can driver is running */
+		err = pcan_usb_pro_drv_loaded(dev, 1);
+		if (err)
+			goto err_out;
+
 		dev_info(dev->netdev->dev.parent,
 		     "PEAK-System %s hwrev %u serial %08X.%08X (%u channels)\n",
 		     pcan_usb_pro.name,
-		     bi.hw_rev, bi.serial_num_hi, bi.serial_num_lo,
+		     bi->hw_rev, bi->serial_num_hi, bi->serial_num_lo,
 		     pcan_usb_pro.ctrl_count);
-
-		/* tell the device the can driver is running */
-		pcan_usb_pro_drv_loaded(dev, 1);
 	} else {
 		usb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
 	}
@@ -916,6 +928,13 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
 	pcan_usb_pro_set_led(dev, 0, 1);
 
 	return 0;
+
+ err_out:
+	kfree(bi);
+	kfree(fi);
+	kfree(usb_if);
+
+	return err;
 }
 
 static void pcan_usb_pro_exit(struct peak_usb_device *dev)
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
index a869918..32275af 100644
--- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
+++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
@@ -29,6 +29,7 @@
 
 /* Vendor Request value for XXX_FCT */
 #define PCAN_USBPRO_FCT_DRVLD		5 /* tell device driver is loaded */
+#define PCAN_USBPRO_FCT_DRVLD_REQ_LEN	16
 
 /* PCAN_USBPRO_INFO_BL vendor request record type */
 struct __packed pcan_usb_pro_blinfo {
-- 
1.8.2.rc2

Re: pull-request: can 2013-06-03

[David Miller]

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Mon,  3 Jun 2013 14:15:30 +0200

> here are there fixes for the v3.10 release cycle:
> 
> The first patch by Jonas Peterson and Olivier Sobrie fixes the reception of CAN
> frames on Kvaser's "USBcan Pro" and "USBcan R" type hardware.
> 
> The last two patches by Olivier Sobrie (for esd_usb2) and me (for peak_usb)
> change the memory handling for the USB messages from stack to kmalloc(), as
> memory used for DMA should not be allocated on stack.
 ...
>   git://gitorious.org/linux-can/linux-can.git fixes-for-3.10

Pulled, thanks Marc.

Re: [PATCH 3/3] net: can: peak_usb: Do not do dma on the stack

[Stephane Grosjean]

Hello Marc,

Going back into linux-can peak_usb, and I'm currently having a quick 
look to what has changed during these several past months.
I agree with the below patch that fixes the "DMAusage with USB core" 
issue, but - maybe I'm wrong - isn't there now amemory leak issue in 
"pcan_usb_pro_init()" function below?

I mean, you now allocate "fi" and "bi" memory objects but, AFAIK, they 
aren't freed anywhere in the function, don't they?

Regards,

Stéphane

Le 03/06/2013 14:15, Marc Kleine-Budde a écrit :
> smatch reports the following warnings:
> drivers/net/can/usb/peak_usb/pcan_usb_pro.c:514 pcan_usb_pro_drv_loaded() error: doing dma on the stack (buffer)
> drivers/net/can/usb/peak_usb/pcan_usb_pro.c:878 pcan_usb_pro_init() error: doing dma on the stack (&fi)
> drivers/net/can/usb/peak_usb/pcan_usb_pro.c:889 pcan_usb_pro_init() error: doing dma on the stack (&bi)
>
> See "Documentation/DMA-API-HOWTO.txt" section "What memory is DMA'able?"
>
> Cc: Stephane Grosjean <s.grosjean@peak-system.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>   drivers/net/can/usb/peak_usb/pcan_usb_pro.c | 61 +++++++++++++++++++----------
>   drivers/net/can/usb/peak_usb/pcan_usb_pro.h |  1 +
>   2 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> index 30d79bf..8ee9d15 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -504,15 +504,24 @@ static int pcan_usb_pro_restart_async(struct peak_usb_device *dev,
>   	return usb_submit_urb(urb, GFP_ATOMIC);
>   }
>   
> -static void pcan_usb_pro_drv_loaded(struct peak_usb_device *dev, int loaded)
> +static int pcan_usb_pro_drv_loaded(struct peak_usb_device *dev, int loaded)
>   {
> -	u8 buffer[16];
> +	u8 *buffer;
> +	int err;
> +
> +	buffer = kmalloc(PCAN_USBPRO_FCT_DRVLD_REQ_LEN, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
>   
>   	buffer[0] = 0;
>   	buffer[1] = !!loaded;
>   
> -	pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_FCT,
> -			      PCAN_USBPRO_FCT_DRVLD, buffer, sizeof(buffer));
> +	err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_FCT,
> +				    PCAN_USBPRO_FCT_DRVLD, buffer,
> +				    PCAN_USBPRO_FCT_DRVLD_REQ_LEN);
> +	kfree(buffer);
> +
> +	return err;
>   }
>   
>   static inline
> @@ -851,21 +860,24 @@ static int pcan_usb_pro_stop(struct peak_usb_device *dev)
>    */
>   static int pcan_usb_pro_init(struct peak_usb_device *dev)
>   {
> -	struct pcan_usb_pro_interface *usb_if;
>   	struct pcan_usb_pro_device *pdev =
>   			container_of(dev, struct pcan_usb_pro_device, dev);
> +	struct pcan_usb_pro_interface *usb_if = NULL;
> +	struct pcan_usb_pro_fwinfo *fi = NULL;
> +	struct pcan_usb_pro_blinfo *bi = NULL;
> +	int err;
>   
>   	/* do this for 1st channel only */
>   	if (!dev->prev_siblings) {
> -		struct pcan_usb_pro_fwinfo fi;
> -		struct pcan_usb_pro_blinfo bi;
> -		int err;
> -
>   		/* allocate netdevices common structure attached to first one */
>   		usb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
>   				 GFP_KERNEL);
> -		if (!usb_if)
> -			return -ENOMEM;
> +		fi = kmalloc(sizeof(struct pcan_usb_pro_fwinfo), GFP_KERNEL);
> +		bi = kmalloc(sizeof(struct pcan_usb_pro_blinfo), GFP_KERNEL);
> +		if (!usb_if || !fi || !bi) {
> +			err = -ENOMEM;
> +			goto err_out;
> +		}
>   
>   		/* number of ts msgs to ignore before taking one into account */
>   		usb_if->cm_ignore_count = 5;
> @@ -877,34 +889,34 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
>   		 */
>   		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
>   					    PCAN_USBPRO_INFO_FW,
> -					    &fi, sizeof(fi));
> +					    fi, sizeof(*fi));
>   		if (err) {
> -			kfree(usb_if);
>   			dev_err(dev->netdev->dev.parent,
>   				"unable to read %s firmware info (err %d)\n",
>   				pcan_usb_pro.name, err);
> -			return err;
> +			goto err_out;
>   		}
>   
>   		err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
>   					    PCAN_USBPRO_INFO_BL,
> -					    &bi, sizeof(bi));
> +					    bi, sizeof(*bi));
>   		if (err) {
> -			kfree(usb_if);
>   			dev_err(dev->netdev->dev.parent,
>   				"unable to read %s bootloader info (err %d)\n",
>   				pcan_usb_pro.name, err);
> -			return err;
> +			goto err_out;
>   		}
>   
> +		/* tell the device the can driver is running */
> +		err = pcan_usb_pro_drv_loaded(dev, 1);
> +		if (err)
> +			goto err_out;
> +
>   		dev_info(dev->netdev->dev.parent,
>   		     "PEAK-System %s hwrev %u serial %08X.%08X (%u channels)\n",
>   		     pcan_usb_pro.name,
> -		     bi.hw_rev, bi.serial_num_hi, bi.serial_num_lo,
> +		     bi->hw_rev, bi->serial_num_hi, bi->serial_num_lo,
>   		     pcan_usb_pro.ctrl_count);
> -
> -		/* tell the device the can driver is running */
> -		pcan_usb_pro_drv_loaded(dev, 1);
>   	} else {
>   		usb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
>   	}
> @@ -916,6 +928,13 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
>   	pcan_usb_pro_set_led(dev, 0, 1);
>   
>   	return 0;
> +
> + err_out:
> +	kfree(bi);
> +	kfree(fi);
> +	kfree(usb_if);
> +
> +	return err;
>   }
>   
>   static void pcan_usb_pro_exit(struct peak_usb_device *dev)
> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> index a869918..32275af 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.h
> @@ -29,6 +29,7 @@
>   
>   /* Vendor Request value for XXX_FCT */
>   #define PCAN_USBPRO_FCT_DRVLD		5 /* tell device driver is loaded */
> +#define PCAN_USBPRO_FCT_DRVLD_REQ_LEN	16
>   
>   /* PCAN_USBPRO_INFO_BL vendor request record type */
>   struct __packed pcan_usb_pro_blinfo {

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com

Re: [PATCH 3/3] net: can: peak_usb: Do not do dma on the stack

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4596 bytes --]

On 12/12/2013 03:44 PM, Stephane Grosjean wrote:
> Going back into linux-can peak_usb, and I'm currently having a quick
> look to what has changed during these several past months.
> I agree with the below patch that fixes the "DMAusage with USB core"
> issue, but - maybe I'm wrong - isn't there now amemory leak issue in
> "pcan_usb_pro_init()" function below?
> 
> I mean, you now allocate "fi" and "bi" memory objects but, AFAIK, they
> aren't freed anywhere in the function, don't they?

Thanks for pointing out.

[...]

>> b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> index 30d79bf..8ee9d15 100644
>> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
>> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
[...]
>> @@ -851,21 +860,24 @@ static int pcan_usb_pro_stop(struct
>> peak_usb_device *dev)
>>    */
>>   static int pcan_usb_pro_init(struct peak_usb_device *dev)
>>   {
>> -    struct pcan_usb_pro_interface *usb_if;
>>       struct pcan_usb_pro_device *pdev =
>>               container_of(dev, struct pcan_usb_pro_device, dev);
>> +    struct pcan_usb_pro_interface *usb_if = NULL;
>> +    struct pcan_usb_pro_fwinfo *fi = NULL;
>> +    struct pcan_usb_pro_blinfo *bi = NULL;
>> +    int err;
>>         /* do this for 1st channel only */
>>       if (!dev->prev_siblings) {
>> -        struct pcan_usb_pro_fwinfo fi;
>> -        struct pcan_usb_pro_blinfo bi;
>> -        int err;
>> -
>>           /* allocate netdevices common structure attached to first
>> one */
>>           usb_if = kzalloc(sizeof(struct pcan_usb_pro_interface),
>>                    GFP_KERNEL);
>> -        if (!usb_if)
>> -            return -ENOMEM;
>> +        fi = kmalloc(sizeof(struct pcan_usb_pro_fwinfo), GFP_KERNEL);
>> +        bi = kmalloc(sizeof(struct pcan_usb_pro_blinfo), GFP_KERNEL);
>> +        if (!usb_if || !fi || !bi) {
>> +            err = -ENOMEM;
>> +            goto err_out;
>> +        }
>>             /* number of ts msgs to ignore before taking one into
>> account */
>>           usb_if->cm_ignore_count = 5;
>> @@ -877,34 +889,34 @@ static int pcan_usb_pro_init(struct
>> peak_usb_device *dev)
>>            */
>>           err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
>>                           PCAN_USBPRO_INFO_FW,
>> -                        &fi, sizeof(fi));
>> +                        fi, sizeof(*fi));
>>           if (err) {
>> -            kfree(usb_if);
>>               dev_err(dev->netdev->dev.parent,
>>                   "unable to read %s firmware info (err %d)\n",
>>                   pcan_usb_pro.name, err);
>> -            return err;
>> +            goto err_out;
>>           }
>>             err = pcan_usb_pro_send_req(dev, PCAN_USBPRO_REQ_INFO,
>>                           PCAN_USBPRO_INFO_BL,
>> -                        &bi, sizeof(bi));
>> +                        bi, sizeof(*bi));
>>           if (err) {
>> -            kfree(usb_if);
>>               dev_err(dev->netdev->dev.parent,
>>                   "unable to read %s bootloader info (err %d)\n",
>>                   pcan_usb_pro.name, err);
>> -            return err;
>> +            goto err_out;
>>           }
>>   +        /* tell the device the can driver is running */
>> +        err = pcan_usb_pro_drv_loaded(dev, 1);
>> +        if (err)
>> +            goto err_out;
>> +
>>           dev_info(dev->netdev->dev.parent,
>>                "PEAK-System %s hwrev %u serial %08X.%08X (%u
>> channels)\n",
>>                pcan_usb_pro.name,
>> -             bi.hw_rev, bi.serial_num_hi, bi.serial_num_lo,
>> +             bi->hw_rev, bi->serial_num_hi, bi->serial_num_lo,
>>                pcan_usb_pro.ctrl_count);
>> -
>> -        /* tell the device the can driver is running */
>> -        pcan_usb_pro_drv_loaded(dev, 1);
>>       } else {
>>           usb_if = pcan_usb_pro_dev_if(dev->prev_siblings);
>>       }
>> @@ -916,6 +928,13 @@ static int pcan_usb_pro_init(struct
>> peak_usb_device *dev)
>>       pcan_usb_pro_set_led(dev, 0, 1);
>>         return 0;
>> +
>> + err_out:
>> +    kfree(bi);
>> +    kfree(fi);
>> +    kfree(usb_if);
>> +
>> +    return err;
>>   }
>>     static void pcan_usb_pro_exit(struct peak_usb_device *dev)

I'll send a fix.

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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]