Patchwork kvaser_usb: fix "dma on the stack" errors

login
register
mail settings
Submitter Olivier Sobrie
Date Nov. 23, 2012, 1:30 p.m.
Message ID <1353677428-15805-1-git-send-email-olivier@sobrie.be>
Download mbox | patch
Permalink /patch/201319/
State Superseded
Delegated to: David Miller
Headers show

Comments

Olivier Sobrie - Nov. 23, 2012, 1:30 p.m.
The dma buffer given to usb_bulk_msg() must be allocated and not on
the stack.
See Documentation/DMA-API-HOWTO.txt section "What memory is DMA'able?"

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
Here is the incremental patch.
Thank you Greg !

Olivier

 drivers/net/can/usb/kvaser_usb.c |  110 ++++++++++++++++++++++++--------------
 1 file changed, 69 insertions(+), 41 deletions(-)
Olivier Sobrie - Nov. 23, 2012, 1:40 p.m.
On Fri, Nov 23, 2012 at 02:30:28PM +0100, Olivier Sobrie wrote:
> The dma buffer given to usb_bulk_msg() must be allocated and not on
> the stack.
> See Documentation/DMA-API-HOWTO.txt section "What memory is DMA'able?"
> 
> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
> Here is the incremental patch.
> Thank you Greg !
> 
> Olivier
> 
>  drivers/net/can/usb/kvaser_usb.c |  110 ++++++++++++++++++++++++--------------
>  1 file changed, 69 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index 8807bf8..7ac6e82 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -421,14 +421,21 @@ end:
>  static int kvaser_usb_send_simple_msg(const struct kvaser_usb *dev,
>  				      u8 msg_id, int channel)
>  {
> -	struct kvaser_msg msg = {
> -		.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_simple),
> -		.id = msg_id,
> -		.u.simple.channel = channel,
> -		.u.simple.tid = 0xff,
> -	};
> -
> -	return kvaser_usb_send_msg(dev, &msg);
> +	struct kvaser_msg *msg;
> +	int rc;
> +
> +	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +

Doh! I removed by mistake the line "msg->id = msg_id"... grr

> +	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_simple);
> +	msg->u.simple.channel = channel;
> +	msg->u.simple.tid = 0xff;
> +
> +	rc = kvaser_usb_send_msg(dev, msg);
> +
> +	kfree(msg);
> +	return rc;
>  }
>  
>  static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
> @@ -1057,20 +1064,27 @@ static int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev)
>  
>  static int kvaser_usb_set_opt_mode(const struct kvaser_usb_net_priv *priv)
>  {
> -	struct kvaser_msg msg = {
> -		.id = CMD_SET_CTRL_MODE,
> -		.len = MSG_HEADER_LEN +
> -		       sizeof(struct kvaser_msg_ctrl_mode),
> -		.u.ctrl_mode.tid = 0xff,
> -		.u.ctrl_mode.channel = priv->channel,
> -	};
> +	struct kvaser_msg *msg;
> +	int rc;
> +
> +	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->id = CMD_SET_CTRL_MODE;
> +	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_ctrl_mode);
> +	msg->u.ctrl_mode.tid = 0xff;
> +	msg->u.ctrl_mode.channel = priv->channel;
>  
>  	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> -		msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_SILENT;
> +		msg->u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_SILENT;
>  	else
> -		msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_NORMAL;
> +		msg->u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_NORMAL;
> +
> +	rc = kvaser_usb_send_msg(priv->dev, msg);
>  
> -	return kvaser_usb_send_msg(priv->dev, &msg);
> +	kfree(msg);
> +	return rc;
>  }
>  
>  static int kvaser_usb_start_chip(struct kvaser_usb_net_priv *priv)
> @@ -1163,15 +1177,22 @@ static int kvaser_usb_stop_chip(struct kvaser_usb_net_priv *priv)
>  
>  static int kvaser_usb_flush_queue(struct kvaser_usb_net_priv *priv)
>  {
> -	struct kvaser_msg msg = {
> -		.id = CMD_FLUSH_QUEUE,
> -		.len = MSG_HEADER_LEN +
> -		       sizeof(struct kvaser_msg_flush_queue),
> -		.u.flush_queue.channel = priv->channel,
> -		.u.flush_queue.flags = 0x00,
> -	};
> -
> -	return kvaser_usb_send_msg(priv->dev, &msg);
> +	struct kvaser_msg *msg;
> +	int rc;
> +
> +	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->id = CMD_FLUSH_QUEUE;
> +	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_flush_queue);
> +	msg->u.flush_queue.channel = priv->channel;
> +	msg->u.flush_queue.flags = 0x00;
> +
> +	rc = kvaser_usb_send_msg(priv->dev, msg);
> +
> +	kfree(msg);
> +	return rc;
>  }
>  
>  static int kvaser_usb_close(struct net_device *netdev)
> @@ -1364,24 +1385,31 @@ static int kvaser_usb_set_bittiming(struct net_device *netdev)
>  	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
>  	struct can_bittiming *bt = &priv->can.bittiming;
>  	struct kvaser_usb *dev = priv->dev;
> -	struct kvaser_msg msg = {
> -		.id = CMD_SET_BUS_PARAMS,
> -		.len = MSG_HEADER_LEN +
> -		       sizeof(struct kvaser_msg_busparams),
> -		.u.busparams.channel = priv->channel,
> -		.u.busparams.tid = 0xff,
> -		.u.busparams.bitrate = cpu_to_le32(bt->bitrate),
> -		.u.busparams.sjw = bt->sjw,
> -		.u.busparams.tseg1 = bt->prop_seg + bt->phase_seg1,
> -		.u.busparams.tseg2 = bt->phase_seg2,
> -	};
> +	struct kvaser_msg *msg;
> +	int rc;
> +
> +	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->id = CMD_SET_BUS_PARAMS;
> +	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_busparams);
> +	msg->u.busparams.channel = priv->channel;
> +	msg->u.busparams.tid = 0xff;
> +	msg->u.busparams.bitrate = cpu_to_le32(bt->bitrate);
> +	msg->u.busparams.sjw = bt->sjw;
> +	msg->u.busparams.tseg1 = bt->prop_seg + bt->phase_seg1;
> +	msg->u.busparams.tseg2 = bt->phase_seg2;
>  
>  	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> -		msg.u.busparams.no_samp = 3;
> +		msg->u.busparams.no_samp = 3;
>  	else
> -		msg.u.busparams.no_samp = 1;
> +		msg->u.busparams.no_samp = 1;
> +
> +	rc = kvaser_usb_send_msg(dev, msg);
>  
> -	return kvaser_usb_send_msg(dev, &msg);
> +	kfree(msg);
> +	return rc;
>  }
>  
>  static int kvaser_usb_set_mode(struct net_device *netdev,
> -- 
> 1.7.9.5
>
Marc Kleine-Budde - Nov. 23, 2012, 1:48 p.m.
On 11/23/2012 02:40 PM, Olivier Sobrie wrote:
> On Fri, Nov 23, 2012 at 02:30:28PM +0100, Olivier Sobrie wrote:
>> The dma buffer given to usb_bulk_msg() must be allocated and not on
>> the stack.
>> See Documentation/DMA-API-HOWTO.txt section "What memory is DMA'able?"
>>
>> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
>> ---
>> Here is the incremental patch.
>> Thank you Greg !
>>
>> Olivier
>>
>>  drivers/net/can/usb/kvaser_usb.c |  110 ++++++++++++++++++++++++--------------
>>  1 file changed, 69 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
>> index 8807bf8..7ac6e82 100644
>> --- a/drivers/net/can/usb/kvaser_usb.c
>> +++ b/drivers/net/can/usb/kvaser_usb.c
>> @@ -421,14 +421,21 @@ end:
>>  static int kvaser_usb_send_simple_msg(const struct kvaser_usb *dev,
>>  				      u8 msg_id, int channel)
>>  {
>> -	struct kvaser_msg msg = {
>> -		.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_simple),
>> -		.id = msg_id,
>> -		.u.simple.channel = channel,
>> -		.u.simple.tid = 0xff,
>> -	};
>> -
>> -	return kvaser_usb_send_msg(dev, &msg);
>> +	struct kvaser_msg *msg;
>> +	int rc;
>> +
>> +	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
>> +	if (!msg)
>> +		return -ENOMEM;
>> +
> 
> Doh! I removed by mistake the line "msg->id = msg_id"... grr

Please send a v2 version of this patch with this problem fixed.

MarcMarc

Patch

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 8807bf8..7ac6e82 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -421,14 +421,21 @@  end:
 static int kvaser_usb_send_simple_msg(const struct kvaser_usb *dev,
 				      u8 msg_id, int channel)
 {
-	struct kvaser_msg msg = {
-		.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_simple),
-		.id = msg_id,
-		.u.simple.channel = channel,
-		.u.simple.tid = 0xff,
-	};
-
-	return kvaser_usb_send_msg(dev, &msg);
+	struct kvaser_msg *msg;
+	int rc;
+
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_simple);
+	msg->u.simple.channel = channel;
+	msg->u.simple.tid = 0xff;
+
+	rc = kvaser_usb_send_msg(dev, msg);
+
+	kfree(msg);
+	return rc;
 }
 
 static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
@@ -1057,20 +1064,27 @@  static int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev)
 
 static int kvaser_usb_set_opt_mode(const struct kvaser_usb_net_priv *priv)
 {
-	struct kvaser_msg msg = {
-		.id = CMD_SET_CTRL_MODE,
-		.len = MSG_HEADER_LEN +
-		       sizeof(struct kvaser_msg_ctrl_mode),
-		.u.ctrl_mode.tid = 0xff,
-		.u.ctrl_mode.channel = priv->channel,
-	};
+	struct kvaser_msg *msg;
+	int rc;
+
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->id = CMD_SET_CTRL_MODE;
+	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_ctrl_mode);
+	msg->u.ctrl_mode.tid = 0xff;
+	msg->u.ctrl_mode.channel = priv->channel;
 
 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
-		msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_SILENT;
+		msg->u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_SILENT;
 	else
-		msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_NORMAL;
+		msg->u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_NORMAL;
+
+	rc = kvaser_usb_send_msg(priv->dev, msg);
 
-	return kvaser_usb_send_msg(priv->dev, &msg);
+	kfree(msg);
+	return rc;
 }
 
 static int kvaser_usb_start_chip(struct kvaser_usb_net_priv *priv)
@@ -1163,15 +1177,22 @@  static int kvaser_usb_stop_chip(struct kvaser_usb_net_priv *priv)
 
 static int kvaser_usb_flush_queue(struct kvaser_usb_net_priv *priv)
 {
-	struct kvaser_msg msg = {
-		.id = CMD_FLUSH_QUEUE,
-		.len = MSG_HEADER_LEN +
-		       sizeof(struct kvaser_msg_flush_queue),
-		.u.flush_queue.channel = priv->channel,
-		.u.flush_queue.flags = 0x00,
-	};
-
-	return kvaser_usb_send_msg(priv->dev, &msg);
+	struct kvaser_msg *msg;
+	int rc;
+
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->id = CMD_FLUSH_QUEUE;
+	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_flush_queue);
+	msg->u.flush_queue.channel = priv->channel;
+	msg->u.flush_queue.flags = 0x00;
+
+	rc = kvaser_usb_send_msg(priv->dev, msg);
+
+	kfree(msg);
+	return rc;
 }
 
 static int kvaser_usb_close(struct net_device *netdev)
@@ -1364,24 +1385,31 @@  static int kvaser_usb_set_bittiming(struct net_device *netdev)
 	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
 	struct can_bittiming *bt = &priv->can.bittiming;
 	struct kvaser_usb *dev = priv->dev;
-	struct kvaser_msg msg = {
-		.id = CMD_SET_BUS_PARAMS,
-		.len = MSG_HEADER_LEN +
-		       sizeof(struct kvaser_msg_busparams),
-		.u.busparams.channel = priv->channel,
-		.u.busparams.tid = 0xff,
-		.u.busparams.bitrate = cpu_to_le32(bt->bitrate),
-		.u.busparams.sjw = bt->sjw,
-		.u.busparams.tseg1 = bt->prop_seg + bt->phase_seg1,
-		.u.busparams.tseg2 = bt->phase_seg2,
-	};
+	struct kvaser_msg *msg;
+	int rc;
+
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->id = CMD_SET_BUS_PARAMS;
+	msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_busparams);
+	msg->u.busparams.channel = priv->channel;
+	msg->u.busparams.tid = 0xff;
+	msg->u.busparams.bitrate = cpu_to_le32(bt->bitrate);
+	msg->u.busparams.sjw = bt->sjw;
+	msg->u.busparams.tseg1 = bt->prop_seg + bt->phase_seg1;
+	msg->u.busparams.tseg2 = bt->phase_seg2;
 
 	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
-		msg.u.busparams.no_samp = 3;
+		msg->u.busparams.no_samp = 3;
 	else
-		msg.u.busparams.no_samp = 1;
+		msg->u.busparams.no_samp = 1;
+
+	rc = kvaser_usb_send_msg(dev, msg);
 
-	return kvaser_usb_send_msg(dev, &msg);
+	kfree(msg);
+	return rc;
 }
 
 static int kvaser_usb_set_mode(struct net_device *netdev,