Patch for kvaser_usb

Patch for kvaser_usb

[Jonas Peterson]

Hello,

I've tried the kvaser_usb driver with my Kvaser USBcan Pro and no can
frames are properly received. It seems like they are all flagged as
log messages. I patched the driver and have used it for a while now.
Feel free to use it as you please.

Best regards,
Jonas Peterson

--- /drivers/net/can/usb/kvaser_usb.c 2013-04-26 16:20:19.748994249 +0200
+++ kvaser_usb.c 2012-12-14 15:59:10.017829818 +0100
@@ -858,6 +858,62 @@
  stats->rx_bytes += cf->can_dlc;
 }

+static void kvaser_usb_rx_can_log_msg(const struct kvaser_usb *dev,
+   const struct kvaser_msg *msg)
+{
+ struct kvaser_usb_net_priv *priv;
+ struct can_frame *cf;
+ struct sk_buff *skb;
+ struct net_device_stats *stats;
+ u8 channel = msg->u.log_message.channel;
+
+ if (channel >= dev->nchannels) {
+ dev_err(dev->udev->dev.parent,
+ "Invalid channel number (%d)\n", channel);
+ return;
+ }
+
+ priv = dev->nets[channel];
+ stats = &priv->netdev->stats;
+
+ if (msg->u.log_message.flags & (MSG_FLAG_ERROR_FRAME | MSG_FLAG_NERR |
+   MSG_FLAG_OVERRUN)) {
+ kvaser_usb_rx_can_err(priv, msg);
+ dev_err(dev->udev->dev.parent,
+ "Error frame (flags: 0x%02x)",
+     msg->u.log_message.flags);
+ return;
+ } else if (msg->u.log_message.flags & ~MSG_FLAG_REMOTE_FRAME) {
+ dev_err(dev->udev->dev.parent,
+ "Unhandled frame (flags: 0x%02x)",
+     msg->u.log_message.flags);
+ netdev_warn(priv->netdev,
+     "Unhandled frame (flags: 0x%02x)",
+     msg->u.log_message.flags);
+ return;
+ }
+
+ skb = alloc_can_skb(priv->netdev, &cf);
+ if (!skb) {
+ dev_err(dev->udev->dev.parent, "Alloc error");
+ stats->tx_dropped++;
+ return;
+ }
+
+ cf->can_id = msg->u.log_message.id;
+ cf->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.log_message.data, cf->can_dlc);
+
+ netif_rx(skb);
+
+ stats->rx_packets++;
+ stats->rx_bytes += cf->can_dlc;
+}
+
 static void kvaser_usb_start_chip_reply(const struct kvaser_usb *dev,
  const struct kvaser_msg *msg)
 {
@@ -921,8 +977,11 @@
  break;

  case CMD_LOG_MESSAGE:
- if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME)
+ if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME) {
  kvaser_usb_rx_error(dev, msg);
+ } else {
+ kvaser_usb_rx_can_log_msg(dev, msg);
+ }
  break;

  case CMD_TX_ACKNOWLEDGE:

Re: Patch for kvaser_usb

[Marc Kleine-Budde]

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

Hello Jonas,

adding Olivier Sobrie, the author of the driver to the loop.

On 04/26/2013 04:50 PM, Jonas Peterson wrote:
> I've tried the kvaser_usb driver with my Kvaser USBcan Pro and no can
> frames are properly received. It seems like they are all flagged as
> log messages. I patched the driver and have used it for a while now.
> Feel free to use it as you please.

Thanks for your feedback. Which firmware version of the Kvaser USBcan
Pro are you using?

Can I add a Signed-off-by[1] to your patch?

Marc

[1]
http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L298
> 
> Best regards,
> Jonas Peterson
> 
> --- /drivers/net/can/usb/kvaser_usb.c 2013-04-26 16:20:19.748994249 +0200
> +++ kvaser_usb.c 2012-12-14 15:59:10.017829818 +0100
> @@ -858,6 +858,62 @@
>   stats->rx_bytes += cf->can_dlc;
>  }
> 
> +static void kvaser_usb_rx_can_log_msg(const struct kvaser_usb *dev,
> +   const struct kvaser_msg *msg)
> +{
> + struct kvaser_usb_net_priv *priv;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + struct net_device_stats *stats;
> + u8 channel = msg->u.log_message.channel;
> +
> + if (channel >= dev->nchannels) {
> + dev_err(dev->udev->dev.parent,
> + "Invalid channel number (%d)\n", channel);
> + return;
> + }
> +
> + priv = dev->nets[channel];
> + stats = &priv->netdev->stats;
> +
> + if (msg->u.log_message.flags & (MSG_FLAG_ERROR_FRAME | MSG_FLAG_NERR |
> +   MSG_FLAG_OVERRUN)) {
> + kvaser_usb_rx_can_err(priv, msg);
> + dev_err(dev->udev->dev.parent,
> + "Error frame (flags: 0x%02x)",
> +     msg->u.log_message.flags);
> + return;
> + } else if (msg->u.log_message.flags & ~MSG_FLAG_REMOTE_FRAME) {
> + dev_err(dev->udev->dev.parent,
> + "Unhandled frame (flags: 0x%02x)",
> +     msg->u.log_message.flags);
> + netdev_warn(priv->netdev,
> +     "Unhandled frame (flags: 0x%02x)",
> +     msg->u.log_message.flags);
> + return;
> + }
> +
> + skb = alloc_can_skb(priv->netdev, &cf);
> + if (!skb) {
> + dev_err(dev->udev->dev.parent, "Alloc error");
> + stats->tx_dropped++;
> + return;
> + }
> +
> + cf->can_id = msg->u.log_message.id;
> + cf->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.log_message.data, cf->can_dlc);
> +
> + netif_rx(skb);
> +
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> +}
> +
>  static void kvaser_usb_start_chip_reply(const struct kvaser_usb *dev,
>   const struct kvaser_msg *msg)
>  {
> @@ -921,8 +977,11 @@
>   break;
> 
>   case CMD_LOG_MESSAGE:
> - if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME)
> + if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME) {
>   kvaser_usb_rx_error(dev, msg);
> + } else {
> + kvaser_usb_rx_can_log_msg(dev, msg);
> + }
>   break;
> 
>   case CMD_TX_ACKNOWLEDGE:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
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: 263 bytes --]

Re: Patch for kvaser_usb

[Jonas Peterson]

Sorry for the sparse information, will get back efter the week-end
with firmware information. Should be "latest by Dec 2012".

Yes, please add the signed-off-by.

Jonas

On Fri, Apr 26, 2013 at 5:00 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Hello Jonas,
>
> adding Olivier Sobrie, the author of the driver to the loop.
>
> On 04/26/2013 04:50 PM, Jonas Peterson wrote:
>> I've tried the kvaser_usb driver with my Kvaser USBcan Pro and no can
>> frames are properly received. It seems like they are all flagged as
>> log messages. I patched the driver and have used it for a while now.
>> Feel free to use it as you please.
>
> Thanks for your feedback. Which firmware version of the Kvaser USBcan
> Pro are you using?
>
> Can I add a Signed-off-by[1] to your patch?
>
> Marc
>
> [1]
> http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L298
>>
>> Best regards,
>> Jonas Peterson
>>
>> --- /drivers/net/can/usb/kvaser_usb.c 2013-04-26 16:20:19.748994249 +0200
>> +++ kvaser_usb.c 2012-12-14 15:59:10.017829818 +0100
>> @@ -858,6 +858,62 @@
>>   stats->rx_bytes += cf->can_dlc;
>>  }
>>
>> +static void kvaser_usb_rx_can_log_msg(const struct kvaser_usb *dev,
>> +   const struct kvaser_msg *msg)
>> +{
>> + struct kvaser_usb_net_priv *priv;
>> + struct can_frame *cf;
>> + struct sk_buff *skb;
>> + struct net_device_stats *stats;
>> + u8 channel = msg->u.log_message.channel;
>> +
>> + if (channel >= dev->nchannels) {
>> + dev_err(dev->udev->dev.parent,
>> + "Invalid channel number (%d)\n", channel);
>> + return;
>> + }
>> +
>> + priv = dev->nets[channel];
>> + stats = &priv->netdev->stats;
>> +
>> + if (msg->u.log_message.flags & (MSG_FLAG_ERROR_FRAME | MSG_FLAG_NERR |
>> +   MSG_FLAG_OVERRUN)) {
>> + kvaser_usb_rx_can_err(priv, msg);
>> + dev_err(dev->udev->dev.parent,
>> + "Error frame (flags: 0x%02x)",
>> +     msg->u.log_message.flags);
>> + return;
>> + } else if (msg->u.log_message.flags & ~MSG_FLAG_REMOTE_FRAME) {
>> + dev_err(dev->udev->dev.parent,
>> + "Unhandled frame (flags: 0x%02x)",
>> +     msg->u.log_message.flags);
>> + netdev_warn(priv->netdev,
>> +     "Unhandled frame (flags: 0x%02x)",
>> +     msg->u.log_message.flags);
>> + return;
>> + }
>> +
>> + skb = alloc_can_skb(priv->netdev, &cf);
>> + if (!skb) {
>> + dev_err(dev->udev->dev.parent, "Alloc error");
>> + stats->tx_dropped++;
>> + return;
>> + }
>> +
>> + cf->can_id = msg->u.log_message.id;
>> + cf->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.log_message.data, cf->can_dlc);
>> +
>> + netif_rx(skb);
>> +
>> + stats->rx_packets++;
>> + stats->rx_bytes += cf->can_dlc;
>> +}
>> +
>>  static void kvaser_usb_start_chip_reply(const struct kvaser_usb *dev,
>>   const struct kvaser_msg *msg)
>>  {
>> @@ -921,8 +977,11 @@
>>   break;
>>
>>   case CMD_LOG_MESSAGE:
>> - if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME)
>> + if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME) {
>>   kvaser_usb_rx_error(dev, msg);
>> + } else {
>> + kvaser_usb_rx_can_log_msg(dev, msg);
>> + }
>>   break;
>>
>>   case CMD_TX_ACKNOWLEDGE:
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
> --
> 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   |
>

Re: Patch for kvaser_usb

[Olivier Sobrie]

Hello Jonas,

On Fri, Apr 26, 2013 at 06:35:26PM +0200, Jonas Peterson wrote:
> Sorry for the sparse information, will get back efter the week-end
> with firmware information. Should be "latest by Dec 2012".
> 
> Yes, please add the signed-off-by.

Thank you for the patch, I'll check it next week with my devices (Leaf
Light and USBcan R).

Have a nice week-end,

Olivier

> 
> Jonas
> 
> On Fri, Apr 26, 2013 at 5:00 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > Hello Jonas,
> >
> > adding Olivier Sobrie, the author of the driver to the loop.
> >
> > On 04/26/2013 04:50 PM, Jonas Peterson wrote:
> >> I've tried the kvaser_usb driver with my Kvaser USBcan Pro and no can
> >> frames are properly received. It seems like they are all flagged as
> >> log messages. I patched the driver and have used it for a while now.
> >> Feel free to use it as you please.
> >
> > Thanks for your feedback. Which firmware version of the Kvaser USBcan
> > Pro are you using?
> >
> > Can I add a Signed-off-by[1] to your patch?
> >
> > Marc
> >
> > [1]
> > http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L298
> >>
> >> Best regards,
> >> Jonas Peterson
> >>
> >> --- /drivers/net/can/usb/kvaser_usb.c 2013-04-26 16:20:19.748994249 +0200
> >> +++ kvaser_usb.c 2012-12-14 15:59:10.017829818 +0100
> >> @@ -858,6 +858,62 @@
> >>   stats->rx_bytes += cf->can_dlc;
> >>  }
> >>
> >> +static void kvaser_usb_rx_can_log_msg(const struct kvaser_usb *dev,
> >> +   const struct kvaser_msg *msg)
> >> +{
> >> + struct kvaser_usb_net_priv *priv;
> >> + struct can_frame *cf;
> >> + struct sk_buff *skb;
> >> + struct net_device_stats *stats;
> >> + u8 channel = msg->u.log_message.channel;
> >> +
> >> + if (channel >= dev->nchannels) {
> >> + dev_err(dev->udev->dev.parent,
> >> + "Invalid channel number (%d)\n", channel);
> >> + return;
> >> + }
> >> +
> >> + priv = dev->nets[channel];
> >> + stats = &priv->netdev->stats;
> >> +
> >> + if (msg->u.log_message.flags & (MSG_FLAG_ERROR_FRAME | MSG_FLAG_NERR |
> >> +   MSG_FLAG_OVERRUN)) {
> >> + kvaser_usb_rx_can_err(priv, msg);
> >> + dev_err(dev->udev->dev.parent,
> >> + "Error frame (flags: 0x%02x)",
> >> +     msg->u.log_message.flags);
> >> + return;
> >> + } else if (msg->u.log_message.flags & ~MSG_FLAG_REMOTE_FRAME) {
> >> + dev_err(dev->udev->dev.parent,
> >> + "Unhandled frame (flags: 0x%02x)",
> >> +     msg->u.log_message.flags);
> >> + netdev_warn(priv->netdev,
> >> +     "Unhandled frame (flags: 0x%02x)",
> >> +     msg->u.log_message.flags);
> >> + return;
> >> + }
> >> +
> >> + skb = alloc_can_skb(priv->netdev, &cf);
> >> + if (!skb) {
> >> + dev_err(dev->udev->dev.parent, "Alloc error");
> >> + stats->tx_dropped++;
> >> + return;
> >> + }
> >> +
> >> + cf->can_id = msg->u.log_message.id;
> >> + cf->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.log_message.data, cf->can_dlc);
> >> +
> >> + netif_rx(skb);
> >> +
> >> + stats->rx_packets++;
> >> + stats->rx_bytes += cf->can_dlc;
> >> +}
> >> +
> >>  static void kvaser_usb_start_chip_reply(const struct kvaser_usb *dev,
> >>   const struct kvaser_msg *msg)
> >>  {
> >> @@ -921,8 +977,11 @@
> >>   break;
> >>
> >>   case CMD_LOG_MESSAGE:
> >> - if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME)
> >> + if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME) {
> >>   kvaser_usb_rx_error(dev, msg);
> >> + } else {
> >> + kvaser_usb_rx_can_log_msg(dev, msg);
> >> + }
> >>   break;
> >>
> >>   case CMD_TX_ACKNOWLEDGE:
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> >
> >
> > --
> > 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   |
> >

-- 
Olivier

Re: Patch for kvaser_usb

[Marc Kleine-Budde]

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

On 04/26/2013 10:51 PM, Olivier Sobrie wrote:
> On Fri, Apr 26, 2013 at 06:35:26PM +0200, Jonas Peterson wrote:
>> Sorry for the sparse information, will get back efter the week-end
>> with firmware information. Should be "latest by Dec 2012".
>>
>> Yes, please add the signed-off-by.
> 
> Thank you for the patch, I'll check it next week with my devices (Leaf
> Light and USBcan R).

Thanks.

> Have a nice week-end,

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: 263 bytes --]

flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Stephane Grosjean]

Hi Marc,

Playing with the flexcan driver, I've seen that the tx_bytes counter 
always equals 0 while tx_packets increases.
I simply removed the CAN_RAW_LOOPBACK option from my CAN socket, so I 
suppose that:

stats->tx_bytes += can_get_echo_skb(dev, 0);

doesn't do what it should.
Am I wrong?

Best regards,

Stéphane
--
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: flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Marc Kleine-Budde]

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

On 04/29/2013 12:40 PM, Stephane Grosjean wrote:
> Hi Marc,
> 
> Playing with the flexcan driver, I've seen that the tx_bytes counter
> always equals 0 while tx_packets increases.
> I simply removed the CAN_RAW_LOOPBACK option from my CAN socket, so I
> suppose that:
> 
> stats->tx_bytes += can_get_echo_skb(dev, 0);
> 
> doesn't do what it should.
> Am I wrong?

Let me see. Yes you're right. can_put_echo_skb() only queues the CAN
frame if CAN_RAW_LOOPBACK is set.

We either can change that can_put_echo_skb() always queues the CAN frame
or that can_get_echo_skb() returns the correct number of bytes even if
the CAN frame is not queued or put the mechanism in each driver.

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: 263 bytes --]

Re: flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Stephane Grosjean]

Le 29/04/2013 12:53, Marc Kleine-Budde a écrit :
> On 04/29/2013 12:40 PM, Stephane Grosjean wrote:
>> Hi Marc,
>>
>> Playing with the flexcan driver, I've seen that the tx_bytes counter
>> always equals 0 while tx_packets increases.
>> I simply removed the CAN_RAW_LOOPBACK option from my CAN socket, so I
>> suppose that:
>>
>> stats->tx_bytes += can_get_echo_skb(dev, 0);
>>
>> doesn't do what it should.
>> Am I wrong?
> Let me see. Yes you're right. can_put_echo_skb() only queues the CAN
> frame if CAN_RAW_LOOPBACK is set.
>
> We either can change that can_put_echo_skb() always queues the CAN frame
> or that can_get_echo_skb() returns the correct number of bytes even if
> the CAN frame is not queued or put the mechanism in each driver.
>
> Marc
>

Why not handling tx_bytes and tx_packets in "flexcan_start_xmit()" 
instead? Easier and faster change, wouldn't be it?
I also have seen that the flexcan ctrlr (iMx25) behaves oddly when rx 
overrun occurs: each time I get such an OVR INT., I also get some 
spurious TX INT. too, while I never sent anything on the CAN. So, for 
me, handling tx_bytes/tx_packets stats when sending the data would fix 
everything!

Stéphane
--
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: flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Marc Kleine-Budde]

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

On 04/29/2013 02:37 PM, Stephane Grosjean wrote:
>>> Playing with the flexcan driver, I've seen that the tx_bytes counter
>>> always equals 0 while tx_packets increases.
>>> I simply removed the CAN_RAW_LOOPBACK option from my CAN socket, so I
>>> suppose that:
>>>
>>> stats->tx_bytes += can_get_echo_skb(dev, 0);
>>>
>>> doesn't do what it should.
>>> Am I wrong?
>> Let me see. Yes you're right. can_put_echo_skb() only queues the CAN
>> frame if CAN_RAW_LOOPBACK is set.
>>
>> We either can change that can_put_echo_skb() always queues the CAN frame
>> or that can_get_echo_skb() returns the correct number of bytes even if
>> the CAN frame is not queued or put the mechanism in each driver.
>>
>> Marc
>>
> 
> Why not handling tx_bytes and tx_packets in "flexcan_start_xmit()"
> instead? Easier and faster change, wouldn't be it?

Yes, but it's not as correct. As technically the frame is send after the
tx_complete interrupt.

> I also have seen that the flexcan ctrlr (iMx25) behaves oddly when rx
> overrun occurs: each time I get such an OVR INT., I also get some
> spurious TX INT. too, while I never sent anything on the CAN. So, for
> me, handling tx_bytes/tx_packets stats when sending the data would fix
> everything!

If you get a spurious TX INT when during RX overruns, I don't think
changing the stats solves the TX INT problem. You only cure the effect
on the stats. What does else does a spurious TX INT do?

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: 263 bytes --]

Re: flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Wolfgang Grandegger]

On 04/29/2013 02:37 PM, Stephane Grosjean wrote:
> 
> Le 29/04/2013 12:53, Marc Kleine-Budde a écrit :
>> On 04/29/2013 12:40 PM, Stephane Grosjean wrote:
>>> Hi Marc,
>>>
>>> Playing with the flexcan driver, I've seen that the tx_bytes counter
>>> always equals 0 while tx_packets increases.
>>> I simply removed the CAN_RAW_LOOPBACK option from my CAN socket, so I
>>> suppose that:
>>>
>>> stats->tx_bytes += can_get_echo_skb(dev, 0);
>>>
>>> doesn't do what it should.
>>> Am I wrong?
>> Let me see. Yes you're right. can_put_echo_skb() only queues the CAN
>> frame if CAN_RAW_LOOPBACK is set.
>>
>> We either can change that can_put_echo_skb() always queues the CAN frame
>> or that can_get_echo_skb() returns the correct number of bytes even if
>> the CAN frame is not queued or put the mechanism in each driver.

Or we read it from "regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl".

>>
>> Marc
>>
> 
> Why not handling tx_bytes and tx_packets in "flexcan_start_xmit()"
> instead? Easier and faster change, wouldn't be it?

Ony messages going really out to the bus sould be counted, if possible.

> I also have seen that the flexcan ctrlr (iMx25) behaves oddly when rx
> overrun occurs: each time I get such an OVR INT., I also get some

OVR INT? What error do you mean?

> spurious TX INT. too, while I never sent anything on the CAN. So, for
> me, handling tx_bytes/tx_packets stats when sending the data would fix
> everything!

See above. What has the handling of tx_bytes/tx_packets stats to do with
spurious TX INT or the OVR INT?

Wolfgang.

Re: flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Stephane Grosjean]

Le 29/04/2013 14:55, Wolfgang Grandegger a écrit :
>> I also have seen that the flexcan ctrlr (iMx25) behaves oddly when rx
>> overrun occurs: each time I get such an OVR INT., I also get some
> OVR INT? What error do you mean?

         /* FIFO overflow */
         if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {


>> spurious TX INT. too, while I never sent anything on the CAN. So, for
>> me, handling tx_bytes/tx_packets stats when sending the data would fix
>> everything!
> See above. What has the handling of tx_bytes/tx_packets stats to do with
> spurious TX INT or the OVR INT?

         /* transmission complete interrupt */
         if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
                 stats->tx_bytes += can_get_echo_skb(dev, 0);
                 stats->tx_packets++;


I don't even know the reason for the moment but when any 
RX_FIFO_OVERFLOW interrupts occur, some (spurious) TX_BUF_ID follow too 
(to be more precise: FLEXCAN_TX_BUF_ID bit is sometimes set too in 
iflag1), which leads "tx_packets" to increase while no CAN frames have 
been sent at all! Didn't find any documentation nor errata about this, 
but it's a fact.

My proposal was only a quick 'n secure workaround for both issues: 
"tx_bytes always 0 when LOOPBACK not set" AND "wrong tx_packets (and 
tx_bytes) management on spurious TX INTerrupts".

Stéphane

--
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: flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Wolfgang Grandegger]

On 04/30/2013 08:47 AM, Stephane Grosjean wrote:
> Le 29/04/2013 14:55, Wolfgang Grandegger a écrit :
>>> I also have seen that the flexcan ctrlr (iMx25) behaves oddly when rx
>>> overrun occurs: each time I get such an OVR INT., I also get some
>> OVR INT? What error do you mean?
> 
>         /* FIFO overflow */
>         if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {

OK... and there is no good reason for a FIFO overflow, right? I mean the
reception rate is not really high.

>>> spurious TX INT. too, while I never sent anything on the CAN. So, for
>>> me, handling tx_bytes/tx_packets stats when sending the data would fix
>>> everything!
>> See above. What has the handling of tx_bytes/tx_packets stats to do with
>> spurious TX INT or the OVR INT?
> 
>         /* transmission complete interrupt */
>         if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
>                 stats->tx_bytes += can_get_echo_skb(dev, 0);
>                 stats->tx_packets++;
> 
> 
> I don't even know the reason for the moment but when any
> RX_FIFO_OVERFLOW interrupts occur, some (spurious) TX_BUF_ID follow too
> (to be more precise: FLEXCAN_TX_BUF_ID bit is sometimes set too in
> iflag1), which leads "tx_packets" to increase while no CAN frames have
> been sent at all! Didn't find any documentation nor errata about this,
> but it's a fact.

Hm, looks like a bug on this FLexcan variant, which is a very early one,
I think. Could you please printk "reg_iflag1",  "reg_iflag2" and
"reg_esr" at the beginning of "flexcan_irq" and show us the dmesg output
when such an error shows up?

> My proposal was only a quick 'n secure workaround for both issues:
> "tx_bytes always 0 when LOOPBACK not set" AND "wrong tx_packets (and
> tx_bytes) management on spurious TX INTerrupts".

Well, still two separate issues which needs to be addressed separately.

Wolfgang.

Re: flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Wolfgang Grandegger]

On 04/30/2013 12:19 PM, Wolfgang Grandegger wrote:
> On 04/30/2013 08:47 AM, Stephane Grosjean wrote:
>> Le 29/04/2013 14:55, Wolfgang Grandegger a écrit :
>>>> I also have seen that the flexcan ctrlr (iMx25) behaves oddly when rx
>>>> overrun occurs: each time I get such an OVR INT., I also get some
>>> OVR INT? What error do you mean?
>>
>>         /* FIFO overflow */
>>         if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
> 
> OK... and there is no good reason for a FIFO overflow, right? I mean the
> reception rate is not really high.
> 
>>>> spurious TX INT. too, while I never sent anything on the CAN. So, for
>>>> me, handling tx_bytes/tx_packets stats when sending the data would fix
>>>> everything!
>>> See above. What has the handling of tx_bytes/tx_packets stats to do with
>>> spurious TX INT or the OVR INT?
>>
>>         /* transmission complete interrupt */
>>         if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
>>                 stats->tx_bytes += can_get_echo_skb(dev, 0);
>>                 stats->tx_packets++;
>>
>>
>> I don't even know the reason for the moment but when any
>> RX_FIFO_OVERFLOW interrupts occur, some (spurious) TX_BUF_ID follow too
>> (to be more precise: FLEXCAN_TX_BUF_ID bit is sometimes set too in
>> iflag1), which leads "tx_packets" to increase while no CAN frames have
>> been sent at all! Didn't find any documentation nor errata about this,
>> but it's a fact.
> 
> Hm, looks like a bug on this FLexcan variant, which is a very early one,
> I think. Could you please printk "reg_iflag1",  "reg_iflag2" and
> "reg_esr" at the beginning of "flexcan_irq" and show us the dmesg output
> when such an error shows up?
> 
>> My proposal was only a quick 'n secure workaround for both issues:
>> "tx_bytes always 0 when LOOPBACK not set" AND "wrong tx_packets (and
>> tx_bytes) management on spurious TX INTerrupts".
> 
> Well, still two separate issues which needs to be addressed separately.

In 2012 a similar issue has been discussed on this list with the subjects:

 "CAN messages being lost on i.MX25 with flexcan"
 "can4linux compilation for i.mx25 under 2.6.39"

Eventually the interrupts on your system are also block for too long
resulting in FIFO overflows. Do you realize message losses?

Wolfgang.

Re: flexcan driver: tx_bytes counter never incremented when CAN_RAW_LOOPBACK removed?

[Kurt Van Dijck]

Marc,

just my opinion:

On Mon, Apr 29, 2013 at 12:53:20PM +0200, Marc Kleine-Budde wrote:
> On 04/29/2013 12:40 PM, Stephane Grosjean wrote:
> > Hi Marc,
> > 
> > Playing with the flexcan driver, I've seen that the tx_bytes counter
> > always equals 0 while tx_packets increases.
> > I simply removed the CAN_RAW_LOOPBACK option from my CAN socket, so I
> > suppose that:
> > 
> > stats->tx_bytes += can_get_echo_skb(dev, 0);
> > 
> > doesn't do what it should.
> > Am I wrong?
> 
> Let me see. Yes you're right. can_put_echo_skb() only queues the CAN
> frame if CAN_RAW_LOOPBACK is set.
> 
> We either can change that can_put_echo_skb() always queues the CAN frame
> or that can_get_echo_skb() returns the correct number of bytes even if
> the CAN frame is not queued or put the mechanism in each driver.

fixing can_get_echo_skb() appears far more straightforward & universally
applicable. The drawback, freeing an non-loopback'd SKB a few microseconds later
that strictly necessary, seems a not-so-big one.

Differentiating the can_get_echo_skb and introducing new structs
seem overkill to me for this task.

Kind regards,
Kurt

Re: Patch for kvaser_usb

[Marc Kleine-Budde]

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

On 04/26/2013 06:35 PM, Jonas Peterson wrote:
> Sorry for the sparse information, will get back efter the week-end
> with firmware information. Should be "latest by Dec 2012".

Does it have an exact version string?

> Yes, please add the signed-off-by.

Thanks.

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: 263 bytes --]

Re: Patch for kvaser_usb

[Jonas Peterson]

> Thanks for your feedback. Which firmware version of the Kvaser USBcan
> Pro are you using?

2.7.600

/Jonas


On Fri, Apr 26, 2013 at 5:00 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Hello Jonas,
>
> adding Olivier Sobrie, the author of the driver to the loop.
>
> On 04/26/2013 04:50 PM, Jonas Peterson wrote:
>> I've tried the kvaser_usb driver with my Kvaser USBcan Pro and no can
>> frames are properly received. It seems like they are all flagged as
>> log messages. I patched the driver and have used it for a while now.
>> Feel free to use it as you please.
>
> Thanks for your feedback. Which firmware version of the Kvaser USBcan
> Pro are you using?
>
> Can I add a Signed-off-by[1] to your patch?
>
> Marc
>
> [1]
> http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L298
>>
>> Best regards,
>> Jonas Peterson
>>
>> --- /drivers/net/can/usb/kvaser_usb.c 2013-04-26 16:20:19.748994249 +0200
>> +++ kvaser_usb.c 2012-12-14 15:59:10.017829818 +0100
>> @@ -858,6 +858,62 @@
>>   stats->rx_bytes += cf->can_dlc;
>>  }
>>
>> +static void kvaser_usb_rx_can_log_msg(const struct kvaser_usb *dev,
>> +   const struct kvaser_msg *msg)
>> +{
>> + struct kvaser_usb_net_priv *priv;
>> + struct can_frame *cf;
>> + struct sk_buff *skb;
>> + struct net_device_stats *stats;
>> + u8 channel = msg->u.log_message.channel;
>> +
>> + if (channel >= dev->nchannels) {
>> + dev_err(dev->udev->dev.parent,
>> + "Invalid channel number (%d)\n", channel);
>> + return;
>> + }
>> +
>> + priv = dev->nets[channel];
>> + stats = &priv->netdev->stats;
>> +
>> + if (msg->u.log_message.flags & (MSG_FLAG_ERROR_FRAME | MSG_FLAG_NERR |
>> +   MSG_FLAG_OVERRUN)) {
>> + kvaser_usb_rx_can_err(priv, msg);
>> + dev_err(dev->udev->dev.parent,
>> + "Error frame (flags: 0x%02x)",
>> +     msg->u.log_message.flags);
>> + return;
>> + } else if (msg->u.log_message.flags & ~MSG_FLAG_REMOTE_FRAME) {
>> + dev_err(dev->udev->dev.parent,
>> + "Unhandled frame (flags: 0x%02x)",
>> +     msg->u.log_message.flags);
>> + netdev_warn(priv->netdev,
>> +     "Unhandled frame (flags: 0x%02x)",
>> +     msg->u.log_message.flags);
>> + return;
>> + }
>> +
>> + skb = alloc_can_skb(priv->netdev, &cf);
>> + if (!skb) {
>> + dev_err(dev->udev->dev.parent, "Alloc error");
>> + stats->tx_dropped++;
>> + return;
>> + }
>> +
>> + cf->can_id = msg->u.log_message.id;
>> + cf->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.log_message.data, cf->can_dlc);
>> +
>> + netif_rx(skb);
>> +
>> + stats->rx_packets++;
>> + stats->rx_bytes += cf->can_dlc;
>> +}
>> +
>>  static void kvaser_usb_start_chip_reply(const struct kvaser_usb *dev,
>>   const struct kvaser_msg *msg)
>>  {
>> @@ -921,8 +977,11 @@
>>   break;
>>
>>   case CMD_LOG_MESSAGE:
>> - if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME)
>> + if (msg->u.log_message.flags & MSG_FLAG_ERROR_FRAME) {
>>   kvaser_usb_rx_error(dev, msg);
>> + } else {
>> + kvaser_usb_rx_can_log_msg(dev, msg);
>> + }
>>   break;
>>
>>   case CMD_TX_ACKNOWLEDGE:
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
> --
> 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   |
>

[PATCH] can: kvaser_usb: handle rx msg correctly

[Olivier Sobrie]

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.

Signed-off-by: Jonas Peterson <jonas.peterson@gmail.com>
Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
Hi Jonas and Marc,

I tested Jonas' patch with my devices and it looks to be good.
The Kvaser UsbCAN R had the same problem. When I wrote the driver I
didn't had this device and I never tested it... This patch was a good
reason for that :-)
However I modified the patch of Jonas in order to not duplicate
functions.

Thanks again to Jonas for the fix!

Olivier

 drivers/net/can/usb/kvaser_usb.c | 53 +++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 45cb9f3..0870529 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -834,22 +834,51 @@ 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]);
+	switch (msg->id) {
+	case CMD_LOG_MESSAGE:
+		cf->can_id = msg->u.log_message.id;
+		cf->can_dlc = get_can_dlc(msg->u.log_message.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);
+		break;
 
-	if (msg->id == CMD_RX_EXT_MESSAGE) {
+	case 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);
+		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);
+		break;
+
+	case CMD_RX_STD_MESSAGE:
+		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->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);
+		break;
+
+	default:
+		netdev_warn(priv->netdev,
+			    "Unhandled message (%d)\n", msg->id);
+		dev_kfree_skb(skb);
+		return;
+	}
 
 	netif_rx(skb);
 
@@ -911,6 +940,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 +949,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.1.5

Re: [PATCH] can: kvaser_usb: handle rx msg correctly

[Marc Kleine-Budde]

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

On 04/30/2013 11:40 PM, Olivier Sobrie wrote:
> 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.
> 
> Signed-off-by: Jonas Peterson <jonas.peterson@gmail.com>
> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
> Hi Jonas and Marc,
> 
> I tested Jonas' patch with my devices and it looks to be good.
> The Kvaser UsbCAN R had the same problem. When I wrote the driver I
> didn't had this device and I never tested it... This patch was a good
> reason for that :-)
> However I modified the patch of Jonas in order to not duplicate
> functions.
> 
> Thanks again to Jonas for the fix!

Thanks for testing and cleaning up, few comments inline.

Marc

> 
> Olivier
> 
>  drivers/net/can/usb/kvaser_usb.c | 53 +++++++++++++++++++++++++++++-----------
>  1 file changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index 45cb9f3..0870529 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -834,22 +834,51 @@ 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]);
> +	switch (msg->id) {
> +	case CMD_LOG_MESSAGE:
> +		cf->can_id = msg->u.log_message.id;

No need to mangle the can_id like the CMD_RX_EXT_MESSAGE or
CMD_RX_STD_MESSAGE case? What about the extended messages flag?

> +		cf->can_dlc = get_can_dlc(msg->u.log_message.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);

Is this endianness save? But the memcpy was already here. Keep it until
someone with a ppc or sparc complains.

> +		break;
>  
> -	if (msg->id == CMD_RX_EXT_MESSAGE) {
> +	case CMD_RX_EXT_MESSAGE:
>  		cf->can_id <<= 18;

who sets the can_id in the first place?

>  		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);
> +		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);
> +		break;
> +
> +	case CMD_RX_STD_MESSAGE:
> +		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->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);
> +		break;
> +
> +	default:
> +		netdev_warn(priv->netdev,
> +			    "Unhandled message (%d)\n", msg->id);
> +		dev_kfree_skb(skb);
> +		return;
> +	}
>  
>  	netif_rx(skb);
>  
> @@ -911,6 +940,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 +949,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;
> 


-- 
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: 263 bytes --]

Re: [PATCH] can: kvaser_usb: handle rx msg correctly

[Olivier Sobrie]

Hi Marc,

On Thu, May 02, 2013 at 12:35:53PM +0200, Marc Kleine-Budde wrote:
> On 04/30/2013 11:40 PM, Olivier Sobrie wrote:
> > --- a/drivers/net/can/usb/kvaser_usb.c
> > +++ b/drivers/net/can/usb/kvaser_usb.c
> > @@ -834,22 +834,51 @@ 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]);
> > +	switch (msg->id) {
> > +	case CMD_LOG_MESSAGE:
> > +		cf->can_id = msg->u.log_message.id;
> 
> No need to mangle the can_id like the CMD_RX_EXT_MESSAGE or
> CMD_RX_STD_MESSAGE case? What about the extended messages flag?

No not needed but I forgot a le32_to_cpu() for the can_id.
The hw set the CAN_EFF_FLAG when it is an extended frame.

> 
> > +		cf->can_dlc = get_can_dlc(msg->u.log_message.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);
> 
> Is this endianness save? But the memcpy was already here. Keep it until
> someone with a ppc or sparc complains.

msg->u.log_message.dlc is an u8 and I don't see why the memcpy would be
a problem.

> 
> > +		break;
> >  
> > -	if (msg->id == CMD_RX_EXT_MESSAGE) {
> > +	case CMD_RX_EXT_MESSAGE:
> >  		cf->can_id <<= 18;
> 
> who sets the can_id in the first place?

Damn I shouldn't have changed this by a switch case.
I fix this and send you an updated patch.

Thank you,

-- 
Olivier

[PATCH v2] can: kvaser_usb: handle rx msg correctly

[Olivier Sobrie]

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.

Signed-off-by: Jonas Peterson <jonas.peterson@gmail.com>
Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/can/usb/kvaser_usb.c | 49 ++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 45cb9f3..6c89360 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -834,22 +834,35 @@ 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_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;
-	}
+	if (msg->id == CMD_LOG_MESSAGE) {
+		cf->can_id = le32_to_cpu(msg->u.log_message.id);
+		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 +924,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 +933,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.1.5

Re: [PATCH v2] can: kvaser_usb: handle rx msg correctly

[Marc Kleine-Budde]

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

On 05/02/2013 08:57 PM, Olivier Sobrie wrote:
> 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.

Looks better...

> 
> Signed-off-by: Jonas Peterson <jonas.peterson@gmail.com>
> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
>  drivers/net/can/usb/kvaser_usb.c | 49 ++++++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index 45cb9f3..6c89360 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -834,22 +834,35 @@ 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_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;
> -	}
> +	if (msg->id == CMD_LOG_MESSAGE) {
> +		cf->can_id = le32_to_cpu(msg->u.log_message.id);

I'd like to see the mentioned mask 11 or 29 bits and add EFF_FLAG here.

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: 263 bytes --]

[PATCH v3] can: kvaser_usb: handle rx msg correctly

[Olivier Sobrie]

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.

Signed-off-by: Jonas Peterson <jonas.peterson@gmail.com>
Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
v3:
 - add KVASER_EXTENDED_FRAME define
 - call kvaser_usb_rx_error() in kvaser_usb_rx_can_msg when the frame is
   a CMD_LOG_MESSAGE and flag MSG_FLAG_ERROR_FRAME is set.

By reading the patch again I saw that I mixed up kvaser_usb_rx_can_msg and
kvaser_usb_rx_error when I cleaned the patch... If fixed that in this
version.
I should maybe merge these two functions to avoid confusion...

Olivier

 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.1.5

Re: [PATCH v3] can: kvaser_usb: handle rx msg correctly

[Marc Kleine-Budde]

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

On 05/07/2013 10:05 PM, Olivier Sobrie wrote:
> 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.
> 
> Signed-off-by: Jonas Peterson <jonas.peterson@gmail.com>
> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
> v3:
>  - add KVASER_EXTENDED_FRAME define
>  - call kvaser_usb_rx_error() in kvaser_usb_rx_can_msg when the frame is
>    a CMD_LOG_MESSAGE and flag MSG_FLAG_ERROR_FRAME is set.
> 
> By reading the patch again I saw that I mixed up kvaser_usb_rx_can_msg and
                                                                     ^^^

Do you want to merge kvaser_usb_rx_can_msg() or kvaser_usb_rx_can_err()?

> kvaser_usb_rx_error when I cleaned the patch... If fixed that in this
> version.
> I should maybe merge these two functions to avoid confusion...

If you want to merge kvaser_usb_rx_can_err() and kvaser_usb_rx_error()
it would make sense, to have the error handling functionality in one
function. I haven't looked at the code, though.

However, this would be a separate patch. I'm applying this patch as it
is to can/master with linux-stable on Cc.

Thanks,
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: 263 bytes --]

Re: [PATCH v3] can: kvaser_usb: handle rx msg correctly

[Olivier Sobrie]

Hi Marc,

On Wed, May 15, 2013 at 12:05:31PM +0200, Marc Kleine-Budde wrote:
> On 05/07/2013 10:05 PM, Olivier Sobrie wrote:
> > 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.
> > 
> > Signed-off-by: Jonas Peterson <jonas.peterson@gmail.com>
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> > v3:
> >  - add KVASER_EXTENDED_FRAME define
> >  - call kvaser_usb_rx_error() in kvaser_usb_rx_can_msg when the frame is
> >    a CMD_LOG_MESSAGE and flag MSG_FLAG_ERROR_FRAME is set.
> > 
> > By reading the patch again I saw that I mixed up kvaser_usb_rx_can_msg and
>                                                                      ^^^
> 
> Do you want to merge kvaser_usb_rx_can_msg() or kvaser_usb_rx_can_err()?

No, grrr I mixed up kvaser_usb_rx_can_msg and kvaser_usb_rx_can_err in my
comment. I meant kvaser_usb_rx_can_err() and kvaser_usb_rx_error().

> 
> > kvaser_usb_rx_error when I cleaned the patch... If fixed that in this
> > version.
> > I should maybe merge these two functions to avoid confusion...
> 
> If you want to merge kvaser_usb_rx_can_err() and kvaser_usb_rx_error()
> it would make sense, to have the error handling functionality in one
> function. I haven't looked at the code, though.

Yes that's what I'll maybe do, merging the two error functions in one.
I'll look at this more deeply when I've a more time.

> 
> However, this would be a separate patch. I'm applying this patch as it
> is to can/master with linux-stable on Cc.

Ok fine for me.

Thanks,

-- 
Olivier

Re: [PATCH] can: kvaser_usb: handle rx msg correctly

[Marc Kleine-Budde]

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

On 05/02/2013 08:06 PM, Olivier Sobrie wrote:
> Hi Marc,
> 
> On Thu, May 02, 2013 at 12:35:53PM +0200, Marc Kleine-Budde wrote:
>> On 04/30/2013 11:40 PM, Olivier Sobrie wrote:
>>> --- a/drivers/net/can/usb/kvaser_usb.c
>>> +++ b/drivers/net/can/usb/kvaser_usb.c
>>> @@ -834,22 +834,51 @@ 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]);
>>> +	switch (msg->id) {
>>> +	case CMD_LOG_MESSAGE:
>>> +		cf->can_id = msg->u.log_message.id;
>>
>> No need to mangle the can_id like the CMD_RX_EXT_MESSAGE or
>> CMD_RX_STD_MESSAGE case? What about the extended messages flag?
> 
> No not needed but I forgot a le32_to_cpu() for the can_id.
> The hw set the CAN_EFF_FLAG when it is an extended frame.

The hardware uses the same bit to mask an extended flag as the linux
kernel does it with CAN_EFF_FLAG? I feel more confident about this if
you add a flag that describes the hard EFF_FLAG and the do the "is EFF
flag set, mask 29 bit and set CAN_EFF_FLAG otherwise mask 11 bit" dance.

Can you please check the driver for other missing leXX_to_cpu and
cpu_to_leXX?

>>> +		cf->can_dlc = get_can_dlc(msg->u.log_message.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);
>>
>> Is this endianness save? But the memcpy was already here. Keep it until
>> someone with a ppc or sparc complains.
> 
> msg->u.log_message.dlc is an u8 and I don't see why the memcpy would be
> a problem.

It's not the dlc I'm talking about. I mean the data. There might be a
problem if you have a dlc of 5. Where is byte 5 located? But I think
it's safe as it is.

>>> +		break;
>>>  
>>> -	if (msg->id == CMD_RX_EXT_MESSAGE) {
>>> +	case CMD_RX_EXT_MESSAGE:
>>>  		cf->can_id <<= 18;
>>
>> who sets the can_id in the first place?
> 
> Damn I shouldn't have changed this by a switch case.
> I fix this and send you an updated patch.

Thanks,
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: 263 bytes --]

Re: [PATCH] can: kvaser_usb: handle rx msg correctly

[Olivier Sobrie]

On Fri, May 03, 2013 at 11:49:21AM +0200, Marc Kleine-Budde wrote:
> On 05/02/2013 08:06 PM, Olivier Sobrie wrote:
> > Hi Marc,
> > 
> > On Thu, May 02, 2013 at 12:35:53PM +0200, Marc Kleine-Budde wrote:
> >> On 04/30/2013 11:40 PM, Olivier Sobrie wrote:
> >>> --- a/drivers/net/can/usb/kvaser_usb.c
> >>> +++ b/drivers/net/can/usb/kvaser_usb.c
> >>> @@ -834,22 +834,51 @@ 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]);
> >>> +	switch (msg->id) {
> >>> +	case CMD_LOG_MESSAGE:
> >>> +		cf->can_id = msg->u.log_message.id;
> >>
> >> No need to mangle the can_id like the CMD_RX_EXT_MESSAGE or
> >> CMD_RX_STD_MESSAGE case? What about the extended messages flag?
> > 
> > No not needed but I forgot a le32_to_cpu() for the can_id.
> > The hw set the CAN_EFF_FLAG when it is an extended frame.
> 
> The hardware uses the same bit to mask an extended flag as the linux
> kernel does it with CAN_EFF_FLAG? I feel more confident about this if
> you add a flag that describes the hard EFF_FLAG and the do the "is EFF
> flag set, mask 29 bit and set CAN_EFF_FLAG otherwise mask 11 bit" dance.

Ok I send an updated patch soon.

> 
> Can you please check the driver for other missing leXX_to_cpu and
> cpu_to_leXX?

I quickly checked and didn't see other missing leXX_to_cpu or
cpu_to_leXX.

> 
> >>> +		cf->can_dlc = get_can_dlc(msg->u.log_message.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);
> >>
> >> Is this endianness save? But the memcpy was already here. Keep it until
> >> someone with a ppc or sparc complains.
> > 
> > msg->u.log_message.dlc is an u8 and I don't see why the memcpy would be
> > a problem.
> 
> It's not the dlc I'm talking about. I mean the data. There might be a
> problem if you have a dlc of 5. Where is byte 5 located? But I think
> it's safe as it is.
> 
> >>> +		break;
> >>>  
> >>> -	if (msg->id == CMD_RX_EXT_MESSAGE) {
> >>> +	case CMD_RX_EXT_MESSAGE:
> >>>  		cf->can_id <<= 18;
> >>
> >> who sets the can_id in the first place?
> > 
> > Damn I shouldn't have changed this by a switch case.
> > I fix this and send you an updated patch.
> 
> Thanks,
> 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   |
> 



-- 
Olivier