From: Ahmed S. Darwish <hidden> Date: 2014-12-23 15:47:07
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
(Generated over 3.19.0-rc1)
From: Ahmed S. Darwish <hidden> Date: 2014-12-23 15:53:28
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 630 +++++++++++++++++++++++++++++++--------
1 file changed, 505 insertions(+), 125 deletions(-)
(Generated over 3.19.0-rc1 + generic bugfix at
can-kvaser_usb-Don-t-free-packets-when-tight-on-URBs.patch)
@@ -615,37 +801,83 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)priv->tx_contexts[i].echo_index=MAX_TX_URBS;}-staticvoidkvaser_usb_rx_error(conststructkvaser_usb*dev,-conststructkvaser_msg*msg)+staticvoidkvaser_report_error_event(conststructkvaser_usb*dev,+structkvaser_error_summary*es);++/*+*Reporterrortouserspaceiffthecontroller'serrorscounterhas+*increased,orwe'retheonlychannelseeingthebuserrorstate.+*+*AsreportedbyUSBCANsheets,"the CAN controller has difficulties+*totellwhetheranerrorframearrivedonchannel1oronchannel2."+*Thus,errorcountersarecomparedwiththeirearliervaluesto+*determinewhichchannelwasresponsiblefortheerrorevent.+*/+staticvoidusbcan_report_error_if_applicable(conststructkvaser_usb*dev,+structkvaser_error_summary*es){-structcan_frame*cf;-structsk_buff*skb;-structnet_device_stats*stats;structkvaser_usb_net_priv*priv;-unsignedintnew_state;-u8channel,status,txerr,rxerr,error_factor;+intold_tx_err_count,old_rx_err_count,channel,report_error;++channel=es->channel;+if(channel>=dev->nchannels){+dev_err(dev->udev->dev.parent,+"Invalid channel number (%d)\n",channel);+return;+}++priv=dev->nets[channel];+old_tx_err_count=priv->bec.txerr;+old_rx_err_count=priv->bec.rxerr;++report_error=0;+if(es->txerr>old_tx_err_count){+es->usbcan.error_state|=USBCAN_ERROR_STATE_TX_ERROR;+report_error=1;+}+if(es->rxerr>old_rx_err_count){+es->usbcan.error_state|=USBCAN_ERROR_STATE_RX_ERROR;+report_error=1;+}+if((es->status&M16C_STATE_BUS_ERROR)&&+!(es->usbcan.other_ch_status&M16C_STATE_BUS_ERROR)){+es->usbcan.error_state|=USBCAN_ERROR_STATE_BUSERROR;+report_error=1;+}++if(report_error)+kvaser_report_error_event(dev,es);+}++/*+*ExtracterrorsummaryfromaLeaf-baseddeviceerrormessage+*/+staticvoidleaf_extract_error_from_msg(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_error_summaryes={0,};switch(msg->id){caseCMD_CAN_ERROR_EVENT:-channel=msg->u.error_event.channel;-status=msg->u.error_event.status;-txerr=msg->u.error_event.tx_errors_count;-rxerr=msg->u.error_event.rx_errors_count;-error_factor=msg->u.error_event.error_factor;+es.channel=msg->u.leaf.error_event.channel;+es.status=msg->u.leaf.error_event.status;+es.txerr=msg->u.leaf.error_event.tx_errors_count;+es.rxerr=msg->u.leaf.error_event.rx_errors_count;+es.leaf.error_factor=msg->u.leaf.error_event.error_factor;break;-caseCMD_LOG_MESSAGE:-channel=msg->u.log_message.channel;-status=msg->u.log_message.data[0];-txerr=msg->u.log_message.data[2];-rxerr=msg->u.log_message.data[3];-error_factor=msg->u.log_message.data[1];+caseLEAF_CMD_LOG_MESSAGE:+es.channel=msg->u.leaf.log_message.channel;+es.status=msg->u.leaf.log_message.data[0];+es.txerr=msg->u.leaf.log_message.data[2];+es.rxerr=msg->u.leaf.log_message.data[3];+es.leaf.error_factor=msg->u.leaf.log_message.data[1];break;caseCMD_CHIP_STATE_EVENT:-channel=msg->u.chip_state_event.channel;-status=msg->u.chip_state_event.status;-txerr=msg->u.chip_state_event.tx_errors_count;-rxerr=msg->u.chip_state_event.rx_errors_count;-error_factor=0;+es.channel=msg->u.leaf.chip_state_event.channel;+es.status=msg->u.leaf.chip_state_event.status;+es.txerr=msg->u.leaf.chip_state_event.tx_errors_count;+es.rxerr=msg->u.leaf.chip_state_event.rx_errors_count;+es.leaf.error_factor=0;break;default:dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",
@@ -653,16 +885,92 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,return;}-if(channel>=dev->nchannels){+kvaser_report_error_event(dev,&es);+}++/*+*ExtractsummaryfromaUSBCANII-baseddeviceerrormessage.+*/+staticvoidusbcan_extract_error_from_msg(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_error_summaryes={0,};++switch(msg->id){++/* Sometimes errors are sent as unsolicited chip state events */+caseCMD_CHIP_STATE_EVENT:+es.channel=msg->u.usbcan.chip_state_event.channel;+es.status=msg->u.usbcan.chip_state_event.status;+es.txerr=msg->u.usbcan.chip_state_event.tx_errors_count;+es.rxerr=msg->u.usbcan.chip_state_event.rx_errors_count;+usbcan_report_error_if_applicable(dev,&es);+break;++caseCMD_CAN_ERROR_EVENT:+es.channel=0;+es.status=msg->u.usbcan.error_event.status_ch0;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch0;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch0;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch1;+usbcan_report_error_if_applicable(dev,&es);++/* For error events, the USBCAN firmware does not support+*morethan2channels:ch0,andch1.*/+if(dev->nchannels>1){+es.channel=1;+es.status=msg->u.usbcan.error_event.status_ch1;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch1;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch1;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch0;+usbcan_report_error_if_applicable(dev,&es);+}+break;++default:+dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",+msg->id);+}+}++staticvoidkvaser_usb_rx_error(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+switch(dev->family){+caseKVASER_LEAF:+leaf_extract_error_from_msg(dev,msg);+break;+caseKVASER_USBCAN:+usbcan_extract_error_from_msg(dev,msg);+break;+default:dev_err(dev->udev->dev.parent,-"Invalid channel number (%d)\n",channel);+"Invalid device family (%d)\n",dev->family);return;}+}-priv=dev->nets[channel];+staticvoidkvaser_report_error_event(conststructkvaser_usb*dev,+structkvaser_error_summary*es)+{+structcan_frame*cf;+structsk_buff*skb;+structnet_device_stats*stats;+structkvaser_usb_net_priv*priv;+unsignedintnew_state;++if(es->channel>=dev->nchannels){+dev_err(dev->udev->dev.parent,+"Invalid channel number (%d)\n",es->channel);+return;+}++priv=dev->nets[es->channel];stats=&priv->netdev->stats;-if(status&M16C_STATE_BUS_RESET){+if(es->status&M16C_STATE_BUS_RESET){kvaser_usb_unlink_tx_urbs(priv);return;}
@@ -1596,6 +1964,18 @@ static int kvaser_usb_probe(struct usb_interface *intf,if(!dev)return-ENOMEM;+if(LEAF_PRODUCT_ID(id->idProduct)){+dev->family=KVASER_LEAF;+dev->max_channels=LEAF_MAX_NET_DEVICES;+}elseif(USBCAN_PRODUCT_ID(id->idProduct)){+dev->family=KVASER_USBCAN;+dev->max_channels=USBCAN_MAX_NET_DEVICES;+}else{+dev_err(&intf->dev,"Product ID (%d) does not belong to any "+"known Kvaser USB family",id->idProduct);+return-ENODEV;+}+err=kvaser_usb_get_endpoints(intf,&dev->bulk_in,&dev->bulk_out);if(err){dev_err(&intf->dev,"Cannot get usb endpoint(s)");
@@ -1608,7 +1988,7 @@ static int kvaser_usb_probe(struct usb_interface *intf,usb_set_intfdata(intf,dev);-for(i=0;i<MAX_NET_DEVICES;i++)+for(i=0;i<dev->max_channels;i++)kvaser_usb_send_simple_msg(dev,CMD_RESET_CHIP,i);err=kvaser_usb_get_software_info(dev);
Hello Ahmed,
On Tue, Dec 23, 2014 at 05:46:54PM +0200, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Signed-off-by: Ahmed S. Darwish <redacted>
Instead of using an extra variable, you can also set skb to NULL here.
Or maybe better, you can move the dev_kfree_skb() in the two previous
tests (in the check of variables urb and buf).
Thank you,
Olivier
quoted hunk
ret = NETDEV_TX_BUSY;
goto releasebuf;
}
@@ -1364,8 +1366,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, if (unlikely(err)) { can_free_echo_skb(netdev, context->echo_index);- skb = NULL; /* set to NULL to avoid double free in- * dev_kfree_skb(skb) */+ kfree_skb_on_error = false; atomic_dec(&priv->active_tx_urbs); usb_unanchor_urb(urb);
Hello Ahmed,
On Tue, Dec 23, 2014 at 05:53:11PM +0200, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
Good, thank you :-) I'll try to test the patch during the next
week-end. Small remarks below.
quoted hunk
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 630 +++++++++++++++++++++++++++++++--------
1 file changed, 505 insertions(+), 125 deletions(-)
(Generated over 3.19.0-rc1 + generic bugfix at
can-kvaser_usb-Don-t-free-packets-when-tight-on-URBs.patch)
@@ -1181,7 +1534,7 @@ static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev) dev->rxbuf[i], dev->rxbuf_dma[i]);- for (i = 0; i < MAX_NET_DEVICES; i++) {+ for (i = 0; i < dev->max_channels; i++) {
here too... or replace it by nchannels.
quoted hunk
struct kvaser_usb_net_priv *priv = dev->nets[i];
if (priv)
@@ -1286,6 +1639,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, struct kvaser_msg *msg; int i, err; int ret = NETDEV_TX_OK;+ uint8_t *msg_tx_can_flags; bool kfree_skb_on_error = true; if (can_dropped_invalid_skb(netdev, skb))
@@ -1326,7 +1694,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc); if (cf->can_id & CAN_RTR_FLAG)- msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;+ *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME; for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) { if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
@@ -1596,6 +1964,18 @@ static int kvaser_usb_probe(struct usb_interface *intf, if (!dev) return -ENOMEM;+ if (LEAF_PRODUCT_ID(id->idProduct)) {+ dev->family = KVASER_LEAF;+ dev->max_channels = LEAF_MAX_NET_DEVICES;+ } else if (USBCAN_PRODUCT_ID(id->idProduct)) {+ dev->family = KVASER_USBCAN;+ dev->max_channels = USBCAN_MAX_NET_DEVICES;+ } else {+ dev_err(&intf->dev, "Product ID (%d) does not belong to any "+ "known Kvaser USB family", id->idProduct);+ return -ENODEV;+ }
Is it really required to keep max_channels in the kvaser_usb structure?
If I looked correctly, you use this variable as a replacement for
MAX_NET_DEVICES in the code and MAX_NET_DEVICES is only used in probe
and disconnect functions. I think it can even be replaced by nchannels
in the disconnect path. So I also think that it don't need to be in the
kvaser_usb structure.
quoted hunk
+
err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out);
if (err) {
dev_err(&intf->dev, "Cannot get usb endpoint(s)");
@@ -1608,7 +1988,7 @@ static int kvaser_usb_probe(struct usb_interface *intf, usb_set_intfdata(intf, dev);- for (i = 0; i < MAX_NET_DEVICES; i++)+ for (i = 0; i < dev->max_channels; i++) kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, i);
Someone reported me that recent leaf firmwares go in trouble when
you send a command for a channel that does not exist. Instead of
max_channels, you can use nchannels here and move the reset command
in the kvaser_usb_init_one() function.
I've a patch for this but It is not tested yet. I'll send it next week-end after
I did some tests.
From: Ahmed S. Darwish <hidden> Date: 2014-12-24 15:04:33
Hi Olivier,
On Wed, Dec 24, 2014 at 01:36:27PM +0100, Olivier Sobrie wrote:
Hello Ahmed,
On Tue, Dec 23, 2014 at 05:53:11PM +0200, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
Good, thank you :-) I'll try to test the patch during the next
week-end. Small remarks below.
Great! thanks and Merry Christmas :-)
quoted
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 630 +++++++++++++++++++++++++++++++--------
1 file changed, 505 insertions(+), 125 deletions(-)
(Generated over 3.19.0-rc1 + generic bugfix at
can-kvaser_usb-Don-t-free-packets-when-tight-on-URBs.patch)
Quite true, but it unfortunately fails when the symbol is
used in array size declaration as in below:
struct kvaser_usb {
...
struct kvaser_usb_net_priv *nets[MAX_NET_DEVICES];
...
}
include/linux/kernel.h:713:19: error: braced-group within
expression allowed only inside a function
#define max(x, y) ({ \
^
quoted
+
+/*
+ * Kvaser USB CAN dongles are divided into two major families:
+ * - Leaf: Based on Renesas M32C, running firmware labeled as 'filo'
+ * - UsbcanII: Based on Renesas M16C, running firmware labeled as 'helios'
+ */
+enum kvaser_usb_family {
+ KVASER_LEAF,
+ KVASER_USBCAN,
+};
+
#define MAX_TX_URBS 16
#define MAX_RX_URBS 4
#define START_TIMEOUT 1000 /* msecs */
@@ -463,7 +636,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev) if (err) return err;- dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);+ switch (dev->family) {+ case KVASER_LEAF:+ dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);+ break;+ case KVASER_USBCAN:+ dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);+ break;+ default:+ dev_err(dev->udev->dev.parent,+ "Invalid device family (%d)\n", dev->family);+ return -EINVAL;+ } return 0; }
@@ -482,7 +666,7 @@ static int kvaser_usb_get_card_info(struct kvaser_usb *dev) return err; dev->nchannels = msg.u.cardinfo.nchannels;- if (dev->nchannels > MAX_NET_DEVICES)+ if (dev->nchannels > dev->max_channels) return -EINVAL;
IMHO, you can keep MAX_NET_DEVICES here.
The UsbcanII firmware hardcodes a maximum of 2 channels in
its protocol. This is unfortunately due to its inability to
tell whether an error event is from CAN channel 0 or ch 1,
and also due to its error_event format:
struct usbcan_msg_error_event {
u8 tid;
u8 padding;
u8 tx_errors_count_ch0;
u8 rx_errors_count_ch0;
u8 tx_errors_count_ch1;
u8 rx_errors_count_ch1;
u8 status_ch0;
u8 status_ch1;
__le16 time;
} __packed;
But since we have MAX_NET_DEVICES = 3, and given the above,
if the UsbcanII firmware reported to us having more than 2
channels, then it's:
a) most probably a memory corruption bug either in the firmware
or in the driver
b) an updated device/firmware we cannot support yet, since
we cannot arbitrate the origin of error events quite correctly
(especially in the case of CAN_ERR_BUSERROR, where the error
counters stays the same and we have to resort to other hacks.
Kindly check usbcan_report_error_if_applicable().)
So allowing more than 2 channels given the current set of
affairs will really induce correctness problems :-(
@@ -1181,7 +1534,7 @@ static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev) dev->rxbuf[i], dev->rxbuf_dma[i]);- for (i = 0; i < MAX_NET_DEVICES; i++) {+ for (i = 0; i < dev->max_channels; i++) {
here too... or replace it by nchannels.
Yes, indeed. nchannels is the correct choice here, especially since
kvaser_usb_init_one() is called "dev->nchannels" times too.
quoted
struct kvaser_usb_net_priv *priv = dev->nets[i];
if (priv)
@@ -1286,6 +1639,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, struct kvaser_msg *msg; int i, err; int ret = NETDEV_TX_OK;+ uint8_t *msg_tx_can_flags; bool kfree_skb_on_error = true; if (can_dropped_invalid_skb(netdev, skb))
@@ -1326,7 +1694,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc); if (cf->can_id & CAN_RTR_FLAG)- msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;+ *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME; for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) { if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
@@ -1596,6 +1964,18 @@ static int kvaser_usb_probe(struct usb_interface *intf, if (!dev) return -ENOMEM;+ if (LEAF_PRODUCT_ID(id->idProduct)) {+ dev->family = KVASER_LEAF;+ dev->max_channels = LEAF_MAX_NET_DEVICES;+ } else if (USBCAN_PRODUCT_ID(id->idProduct)) {+ dev->family = KVASER_USBCAN;+ dev->max_channels = USBCAN_MAX_NET_DEVICES;+ } else {+ dev_err(&intf->dev, "Product ID (%d) does not belong to any "+ "known Kvaser USB family", id->idProduct);+ return -ENODEV;+ }+
Is it really required to keep max_channels in the kvaser_usb structure?
If I looked correctly, you use this variable as a replacement for
MAX_NET_DEVICES in the code and MAX_NET_DEVICES is only used in probe
and disconnect functions. I think it can even be replaced by nchannels
in the disconnect path. So I also think that it don't need to be in the
kvaser_usb structure.
hmmm.. given the current state of error arbitration explained
above, where I cannot accept a dev->nchannels > 2, I guess we
have two options:
a) Remove max_channels, and hardcode the channels count
correctness logic as follows:
dev->nchannels = msg.u.cardinfo.nchannels;
if ((dev->family == USBCAN && dev->nchannels > USBCAN_MAX_NET_DEVICES)
|| (dev->family == LEAF && dev->nchannels > LEAF_MAX_NET_DEVICES))
return -EINVAL
b) Leave max_channels in 'struct kvaser_usb' as is.
I personally prefer the solution at 'b)' but I can do it as
in 'a)' if you prefer :-)
quoted
err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out);
if (err) {
dev_err(&intf->dev, "Cannot get usb endpoint(s)");
@@ -1608,7 +1988,7 @@ static int kvaser_usb_probe(struct usb_interface *intf, usb_set_intfdata(intf, dev);- for (i = 0; i < MAX_NET_DEVICES; i++)+ for (i = 0; i < dev->max_channels; i++) kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, i);
Someone reported me that recent leaf firmwares go in trouble when
you send a command for a channel that does not exist. Instead of
max_channels, you can use nchannels here and move the reset command
in the kvaser_usb_init_one() function.
I've a patch for this but It is not tested yet. I'll send it next week-end after
I did some tests.
Great. I guess I can submit a 3-patch series now
(kfree_skb fix + the above fix + driver).
quoted
err = kvaser_usb_get_software_info(dev);
Thank you,
Thanks a lot for your review.
P.S. the Gmail mailer you've used messed badly with the patch
code identation; I had to manually restore it back to make the
discussion meaningful for others :-)
Regards,
--
Darwish
From: Ahmed S. Darwish <hidden> Date: 2014-12-24 15:52:58
Hi Olivier,
On Wed, Dec 24, 2014 at 01:31:20PM +0100, Olivier Sobrie wrote:
Hello Ahmed,
On Tue, Dec 23, 2014 at 05:46:54PM +0200, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Signed-off-by: Ahmed S. Darwish <redacted>
Instead of using an extra variable, you can also set skb to NULL here.
Or maybe better, you can move the dev_kfree_skb() in the two previous
tests (in the check of variables urb and buf).
Nice, I'll move dev_kfree_skb() to the two earlier tests then.
Thanks,
P.S. mailer and patch identation; had to manually fix them
before replying (but thanks for the quick review, ofc ;-))
Thank you,
Olivier
quoted
ret = NETDEV_TX_BUSY;
goto releasebuf;
}
@@ -1364,8 +1366,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, if (unlikely(err)) { can_free_echo_skb(netdev, context->echo_index);- skb = NULL; /* set to NULL to avoid double free in- * dev_kfree_skb(skb) */+ kfree_skb_on_error = false; atomic_dec(&priv->active_tx_urbs); usb_unanchor_urb(urb);
From: Ahmed S. Darwish <hidden> Date: 2014-12-25 01:57:05
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Note:
Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
is a driver bug in and out of itself: it means that our start/stop
queue flow control is broken.
This patch only fixes the (buggy) error handling code; the root
cause shall be fixed in a later commit.
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
(Marc, Greg, I believe this should also be added to -stable?)
From: Ahmed S. Darwish <hidden> Date: 2014-12-25 02:00:16
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in very high frequency (*), closing the CAN channel while
all the transmissions are on (#), opening the device again (@),
then sending a small number of packets would make the driver
enter an almost infinite loop of:
[....]
[15959.853988] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853990] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853991] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853993] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853994] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853995] kvaser_usb 4-3:1.0 can0: cannot find free context
[....]
_dragging the whole system down_ in the process due to the
excessive logging output.
Initially, this has caused random panics in the kernel due to a
buggy error recovery path. That got fixed in an earlier commit.(%)
This patch aims at solving the root cause. -->
16 tx URBs and contexts are allocated per CAN channel per USB
device. Such URBs are protected by:
a) A simple atomic counter, up to a value of MAX_TX_URBS (16)
b) A flag in each URB context, stating if it's free
c) The fact that ndo_start_xmit calls are themselves protected
by the networking layers higher above
After grabbing one of the tx URBs, if the driver noticed that all
of them are now taken, it stops the netif transmission queue.
Such queue is worken up again only if an acknowedgment was received
from the firmware on one of our earlier-sent frames.
Meanwhile, upon channel close (#), the driver sends a CMD_STOP_CHIP
to the firmware, effectively closing all further communication. In
the high traffic case, the atomic counter remains at MAX_TX_URBS,
and all the URB contexts remain marked as active. While opening
the channel again (@), it cannot send any further frames since no
more free tx URB contexts are available.
Reset all tx URB contexts upon CAN channel close.
(*) 50 parallel instances of `cangen0 -g 0 -ix`
(#) `ifconfig can0 down`
(@) `ifconfig can0 up`
(%) "can: kvaser_usb: Don't free packets when tight on URBs"
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 3 +++
1 file changed, 3 insertions(+)
(Marc, Greg, this also should be added to -stable?)
From: Ahmed S. Darwish <hidden> Date: 2014-12-25 02:03:15
From: Ahmed S. Darwish <redacted>
"Someone reported me that recent leaf firmwares go in trouble when
you send a command for a channel that does not exist. Instead ...
you can move the reset command to kvaser_usb_init_one() function."
Suggested-by: Olivier Sobrie <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
From: Ahmed S. Darwish <hidden> Date: 2014-12-25 02:05:01
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 627 +++++++++++++++++++++++++++++++--------
2 files changed, 510 insertions(+), 125 deletions(-)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
@@ -615,37 +800,83 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)priv->tx_contexts[i].echo_index=MAX_TX_URBS;}-staticvoidkvaser_usb_rx_error(conststructkvaser_usb*dev,-conststructkvaser_msg*msg)+staticvoidkvaser_report_error_event(conststructkvaser_usb*dev,+structkvaser_error_summary*es);++/*+*Reporterrortouserspaceiffthecontroller'serrorscounterhas+*increased,orwe'retheonlychannelseeingthebuserrorstate.+*+*AsreportedbyUSBCANsheets,"the CAN controller has difficulties+*totellwhetheranerrorframearrivedonchannel1oronchannel2."+*Thus,errorcountersarecomparedwiththeirearliervaluesto+*determinewhichchannelwasresponsiblefortheerrorevent.+*/+staticvoidusbcan_report_error_if_applicable(conststructkvaser_usb*dev,+structkvaser_error_summary*es){-structcan_frame*cf;-structsk_buff*skb;-structnet_device_stats*stats;structkvaser_usb_net_priv*priv;-unsignedintnew_state;-u8channel,status,txerr,rxerr,error_factor;+intold_tx_err_count,old_rx_err_count,channel,report_error;++channel=es->channel;+if(channel>=dev->nchannels){+dev_err(dev->udev->dev.parent,+"Invalid channel number (%d)\n",channel);+return;+}++priv=dev->nets[channel];+old_tx_err_count=priv->bec.txerr;+old_rx_err_count=priv->bec.rxerr;++report_error=0;+if(es->txerr>old_tx_err_count){+es->usbcan.error_state|=USBCAN_ERROR_STATE_TX_ERROR;+report_error=1;+}+if(es->rxerr>old_rx_err_count){+es->usbcan.error_state|=USBCAN_ERROR_STATE_RX_ERROR;+report_error=1;+}+if((es->status&M16C_STATE_BUS_ERROR)&&+!(es->usbcan.other_ch_status&M16C_STATE_BUS_ERROR)){+es->usbcan.error_state|=USBCAN_ERROR_STATE_BUSERROR;+report_error=1;+}++if(report_error)+kvaser_report_error_event(dev,es);+}++/*+*ExtracterrorsummaryfromaLeaf-baseddeviceerrormessage+*/+staticvoidleaf_extract_error_from_msg(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_error_summaryes={0,};switch(msg->id){caseCMD_CAN_ERROR_EVENT:-channel=msg->u.error_event.channel;-status=msg->u.error_event.status;-txerr=msg->u.error_event.tx_errors_count;-rxerr=msg->u.error_event.rx_errors_count;-error_factor=msg->u.error_event.error_factor;+es.channel=msg->u.leaf.error_event.channel;+es.status=msg->u.leaf.error_event.status;+es.txerr=msg->u.leaf.error_event.tx_errors_count;+es.rxerr=msg->u.leaf.error_event.rx_errors_count;+es.leaf.error_factor=msg->u.leaf.error_event.error_factor;break;-caseCMD_LOG_MESSAGE:-channel=msg->u.log_message.channel;-status=msg->u.log_message.data[0];-txerr=msg->u.log_message.data[2];-rxerr=msg->u.log_message.data[3];-error_factor=msg->u.log_message.data[1];+caseLEAF_CMD_LOG_MESSAGE:+es.channel=msg->u.leaf.log_message.channel;+es.status=msg->u.leaf.log_message.data[0];+es.txerr=msg->u.leaf.log_message.data[2];+es.rxerr=msg->u.leaf.log_message.data[3];+es.leaf.error_factor=msg->u.leaf.log_message.data[1];break;caseCMD_CHIP_STATE_EVENT:-channel=msg->u.chip_state_event.channel;-status=msg->u.chip_state_event.status;-txerr=msg->u.chip_state_event.tx_errors_count;-rxerr=msg->u.chip_state_event.rx_errors_count;-error_factor=0;+es.channel=msg->u.leaf.chip_state_event.channel;+es.status=msg->u.leaf.chip_state_event.status;+es.txerr=msg->u.leaf.chip_state_event.tx_errors_count;+es.rxerr=msg->u.leaf.chip_state_event.rx_errors_count;+es.leaf.error_factor=0;break;default:dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",
@@ -653,16 +884,92 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,return;}-if(channel>=dev->nchannels){+kvaser_report_error_event(dev,&es);+}++/*+*ExtractsummaryfromaUSBCANII-baseddeviceerrormessage.+*/+staticvoidusbcan_extract_error_from_msg(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_error_summaryes={0,};++switch(msg->id){++/* Sometimes errors are sent as unsolicited chip state events */+caseCMD_CHIP_STATE_EVENT:+es.channel=msg->u.usbcan.chip_state_event.channel;+es.status=msg->u.usbcan.chip_state_event.status;+es.txerr=msg->u.usbcan.chip_state_event.tx_errors_count;+es.rxerr=msg->u.usbcan.chip_state_event.rx_errors_count;+usbcan_report_error_if_applicable(dev,&es);+break;++caseCMD_CAN_ERROR_EVENT:+es.channel=0;+es.status=msg->u.usbcan.error_event.status_ch0;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch0;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch0;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch1;+usbcan_report_error_if_applicable(dev,&es);++/* For error events, the USBCAN firmware does not support+*morethan2channels:ch0,andch1.*/+if(dev->nchannels>1){+es.channel=1;+es.status=msg->u.usbcan.error_event.status_ch1;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch1;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch1;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch0;+usbcan_report_error_if_applicable(dev,&es);+}+break;++default:+dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",+msg->id);+}+}++staticvoidkvaser_usb_rx_error(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+switch(dev->family){+caseKVASER_LEAF:+leaf_extract_error_from_msg(dev,msg);+break;+caseKVASER_USBCAN:+usbcan_extract_error_from_msg(dev,msg);+break;+default:dev_err(dev->udev->dev.parent,-"Invalid channel number (%d)\n",channel);+"Invalid device family (%d)\n",dev->family);return;}+}-priv=dev->nets[channel];+staticvoidkvaser_report_error_event(conststructkvaser_usb*dev,+structkvaser_error_summary*es)+{+structcan_frame*cf;+structsk_buff*skb;+structnet_device_stats*stats;+structkvaser_usb_net_priv*priv;+unsignedintnew_state;++if(es->channel>=dev->nchannels){+dev_err(dev->udev->dev.parent,+"Invalid channel number (%d)\n",es->channel);+return;+}++priv=dev->nets[es->channel];stats=&priv->netdev->stats;-if(status&M16C_STATE_BUS_RESET){+if(es->status&M16C_STATE_BUS_RESET){kvaser_usb_unlink_tx_urbs(priv);return;}
@@ -1601,6 +1968,18 @@ static int kvaser_usb_probe(struct usb_interface *intf,if(!dev)return-ENOMEM;+if(LEAF_PRODUCT_ID(id->idProduct)){+dev->family=KVASER_LEAF;+dev->max_channels=LEAF_MAX_NET_DEVICES;+}elseif(USBCAN_PRODUCT_ID(id->idProduct)){+dev->family=KVASER_USBCAN;+dev->max_channels=USBCAN_MAX_NET_DEVICES;+}else{+dev_err(&intf->dev,"Product ID (%d) does not belong to any "+"known Kvaser USB family",id->idProduct);+return-ENODEV;+}+err=kvaser_usb_get_endpoints(intf,&dev->bulk_in,&dev->bulk_out);if(err){dev_err(&intf->dev,"Cannot get usb endpoint(s)");
On Thu, Dec 25, 2014 at 01:56:44AM +0200, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Note:
Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
is a driver bug in and out of itself: it means that our start/stop
queue flow control is broken.
This patch only fixes the (buggy) error handling code; the root
cause shall be fixed in a later commit.
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
(Marc, Greg, I believe this should also be added to -stable?)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
From: Ahmed S. Darwish <hidden> Date: 2014-12-25 09:39:12
On Wed, Dec 24, 2014 at 06:50:11PM -0800, Greg KH wrote:
On Thu, Dec 25, 2014 at 01:56:44AM +0200, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
> > Flooding the Kvaser CAN to USB dongle with multiple reads and
quoted
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Note:
Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
is a driver bug in and out of itself: it means that our start/stop
queue flow control is broken.
This patch only fixes the (buggy) error handling code; the root
cause shall be fixed in a later commit.
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
(Marc, Greg, I believe this should also be added to -stable?)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
<msg-response>
Note taken. Sorry about that ;-)
</msg-response>
Hello Ahmed,
On Wed, Dec 24, 2014 at 05:04:17PM +0200, Ahmed S. Darwish wrote:
Hi Olivier,
On Wed, Dec 24, 2014 at 01:36:27PM +0100, Olivier Sobrie wrote:
quoted
Hello Ahmed,
On Tue, Dec 23, 2014 at 05:53:11PM +0200, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
Good, thank you :-) I'll try to test the patch during the next
week-end. Small remarks below.
Great! thanks and Merry Christmas :-)
quoted
quoted
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 630 +++++++++++++++++++++++++++++++--------
1 file changed, 505 insertions(+), 125 deletions(-)
(Generated over 3.19.0-rc1 + generic bugfix at
can-kvaser_usb-Don-t-free-packets-when-tight-on-URBs.patch)
Quite true, but it unfortunately fails when the symbol is
used in array size declaration as in below:
struct kvaser_usb {
...
struct kvaser_usb_net_priv *nets[MAX_NET_DEVICES];
...
}
include/linux/kernel.h:713:19: error: braced-group within
expression allowed only inside a function
#define max(x, y) ({ \
^
Just let MAX_NET_DEVICES equals to 3.
quoted
quoted
+
+/*
+ * Kvaser USB CAN dongles are divided into two major families:
+ * - Leaf: Based on Renesas M32C, running firmware labeled as 'filo'
+ * - UsbcanII: Based on Renesas M16C, running firmware labeled as 'helios'
+ */
+enum kvaser_usb_family {
+ KVASER_LEAF,
+ KVASER_USBCAN,
+};
+
#define MAX_TX_URBS 16
#define MAX_RX_URBS 4
#define START_TIMEOUT 1000 /* msecs */
@@ -463,7 +636,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev) if (err) return err;- dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);+ switch (dev->family) {+ case KVASER_LEAF:+ dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);+ break;+ case KVASER_USBCAN:+ dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);+ break;+ default:+ dev_err(dev->udev->dev.parent,+ "Invalid device family (%d)\n", dev->family);+ return -EINVAL;+ } return 0; }
@@ -482,7 +666,7 @@ static int kvaser_usb_get_card_info(struct kvaser_usb *dev) return err; dev->nchannels = msg.u.cardinfo.nchannels;- if (dev->nchannels > MAX_NET_DEVICES)+ if (dev->nchannels > dev->max_channels) return -EINVAL;
IMHO, you can keep MAX_NET_DEVICES here.
The UsbcanII firmware hardcodes a maximum of 2 channels in
its protocol. This is unfortunately due to its inability to
tell whether an error event is from CAN channel 0 or ch 1,
and also due to its error_event format:
struct usbcan_msg_error_event {
u8 tid;
u8 padding;
u8 tx_errors_count_ch0;
u8 rx_errors_count_ch0;
u8 tx_errors_count_ch1;
u8 rx_errors_count_ch1;
u8 status_ch0;
u8 status_ch1;
__le16 time;
} __packed;
But since we have MAX_NET_DEVICES = 3, and given the above,
if the UsbcanII firmware reported to us having more than 2
channels, then it's:
a) most probably a memory corruption bug either in the firmware
or in the driver
b) an updated device/firmware we cannot support yet, since
we cannot arbitrate the origin of error events quite correctly
(especially in the case of CAN_ERR_BUSERROR, where the error
counters stays the same and we have to resort to other hacks.
Kindly check usbcan_report_error_if_applicable().)
So allowing more than 2 channels given the current set of
affairs will really induce correctness problems :-(
@@ -1181,7 +1534,7 @@ static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev) dev->rxbuf[i], dev->rxbuf_dma[i]);- for (i = 0; i < MAX_NET_DEVICES; i++) {+ for (i = 0; i < dev->max_channels; i++) {
here too... or replace it by nchannels.
Yes, indeed. nchannels is the correct choice here, especially since
kvaser_usb_init_one() is called "dev->nchannels" times too.
quoted
quoted
struct kvaser_usb_net_priv *priv = dev->nets[i];
if (priv)
@@ -1286,6 +1639,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, struct kvaser_msg *msg; int i, err; int ret = NETDEV_TX_OK;+ uint8_t *msg_tx_can_flags; bool kfree_skb_on_error = true; if (can_dropped_invalid_skb(netdev, skb))
@@ -1326,7 +1694,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc); if (cf->can_id & CAN_RTR_FLAG)- msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;+ *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME; for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) { if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
@@ -1596,6 +1964,18 @@ static int kvaser_usb_probe(struct usb_interface *intf, if (!dev) return -ENOMEM;+ if (LEAF_PRODUCT_ID(id->idProduct)) {+ dev->family = KVASER_LEAF;+ dev->max_channels = LEAF_MAX_NET_DEVICES;+ } else if (USBCAN_PRODUCT_ID(id->idProduct)) {+ dev->family = KVASER_USBCAN;+ dev->max_channels = USBCAN_MAX_NET_DEVICES;+ } else {+ dev_err(&intf->dev, "Product ID (%d) does not belong to any "+ "known Kvaser USB family", id->idProduct);+ return -ENODEV;+ }+
Is it really required to keep max_channels in the kvaser_usb structure?
If I looked correctly, you use this variable as a replacement for
MAX_NET_DEVICES in the code and MAX_NET_DEVICES is only used in probe
and disconnect functions. I think it can even be replaced by nchannels
in the disconnect path. So I also think that it don't need to be in the
kvaser_usb structure.
hmmm.. given the current state of error arbitration explained
above, where I cannot accept a dev->nchannels > 2, I guess we
have two options:
a) Remove max_channels, and hardcode the channels count
correctness logic as follows:
dev->nchannels = msg.u.cardinfo.nchannels;
if ((dev->family == USBCAN && dev->nchannels > USBCAN_MAX_NET_DEVICES)
|| (dev->family == LEAF && dev->nchannels > LEAF_MAX_NET_DEVICES))
return -EINVAL
b) Leave max_channels in 'struct kvaser_usb' as is.
I personally prefer the solution at 'b)' but I can do it as
in 'a)' if you prefer :-)
Keeping max_channels in the kvaser_usb structure is useless because it
is only used in one function that is called in the probe function.
I would prefer to have:
if (dev->nchannels > MAX_NET_DEVICES)
return -EINVAL
if ((dev->family == USBCAN) &&
(dev->nchannels > MAX_USBCAN_NET_DEVICES))
return -EINVAL
You can remove LEAF_MAX_NET_DEVICES which is not used, keep
MAX_NET_DEVICES equals to 3 and remove the MAX() macro.
The test specific to the USBCAN family can eventually be moved in the
kvaser_usb_probe() function.
quoted
quoted
err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out);
if (err) {
dev_err(&intf->dev, "Cannot get usb endpoint(s)");
@@ -1608,7 +1988,7 @@ static int kvaser_usb_probe(struct usb_interface *intf, usb_set_intfdata(intf, dev);- for (i = 0; i < MAX_NET_DEVICES; i++)+ for (i = 0; i < dev->max_channels; i++) kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, i);
Someone reported me that recent leaf firmwares go in trouble when
you send a command for a channel that does not exist. Instead of
max_channels, you can use nchannels here and move the reset command
in the kvaser_usb_init_one() function.
I've a patch for this but It is not tested yet. I'll send it next week-end after
I did some tests.
Great. I guess I can submit a 3-patch series now
(kfree_skb fix + the above fix + driver).
quoted
quoted
err = kvaser_usb_get_software_info(dev);
Thank you,
Thanks a lot for your review.
P.S. the Gmail mailer you've used messed badly with the patch
code identation; I had to manually restore it back to make the
discussion meaningful for others :-)
Regards,
--
Darwish
On Thu, Dec 25, 2014 at 01:56:44AM +0200, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Note:
Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
is a driver bug in and out of itself: it means that our start/stop
queue flow control is broken.
This patch only fixes the (buggy) error handling code; the root
cause shall be fixed in a later commit.
Signed-off-by: Ahmed S. Darwish <redacted>
Acked-by: Olivier Sobrie <redacted>
quoted hunk
---
drivers/net/can/usb/kvaser_usb.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
(Marc, Greg, I believe this should also be added to -stable?)
On Thu, Dec 25, 2014 at 02:02:56AM +0200, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
"Someone reported me that recent leaf firmwares go in trouble when
you send a command for a channel that does not exist. Instead ...
you can move the reset command to kvaser_usb_init_one() function."
Please adapt the commit log message as follows:
Recent Leaf firmware versions (>= 3.1.557) do not allow to send commands for
non-existing channels. If a command is send for a non-existing channel,
the firmware crashes.
And you can add:
Reported-by: Christopher Storah <redacted>
Signed-off-by: Olivier Sobrie <redacted>
Kr,
Olivier
From: Ahmed S. Darwish <hidden> Date: 2014-12-30 15:38:50
On Sun, Dec 28, 2014 at 10:51:34PM +0100, Olivier Sobrie wrote:
[...]
quoted
quoted
quoted
+ if (LEAF_PRODUCT_ID(id->idProduct)) {
+ dev->family = KVASER_LEAF;
+ dev->max_channels = LEAF_MAX_NET_DEVICES;
+ } else if (USBCAN_PRODUCT_ID(id->idProduct)) {
+ dev->family = KVASER_USBCAN;
+ dev->max_channels = USBCAN_MAX_NET_DEVICES;
+ } else {
+ dev_err(&intf->dev, "Product ID (%d) does not belong to any "
+ "known Kvaser USB family", id->idProduct);
+ return -ENODEV;
+ }
+
Is it really required to keep max_channels in the kvaser_usb structure?
If I looked correctly, you use this variable as a replacement for
MAX_NET_DEVICES in the code and MAX_NET_DEVICES is only used in probe
and disconnect functions. I think it can even be replaced by nchannels
in the disconnect path. So I also think that it don't need to be in the
kvaser_usb structure.
hmmm.. given the current state of error arbitration explained
above, where I cannot accept a dev->nchannels > 2, I guess we
have two options:
a) Remove max_channels, and hardcode the channels count
correctness logic as follows:
dev->nchannels = msg.u.cardinfo.nchannels;
if ((dev->family == USBCAN && dev->nchannels > USBCAN_MAX_NET_DEVICES)
|| (dev->family == LEAF && dev->nchannels > LEAF_MAX_NET_DEVICES))
return -EINVAL
b) Leave max_channels in 'struct kvaser_usb' as is.
I personally prefer the solution at 'b)' but I can do it as
in 'a)' if you prefer :-)
Keeping max_channels in the kvaser_usb structure is useless because it
is only used in one function that is called in the probe function.
I would prefer to have:
if (dev->nchannels > MAX_NET_DEVICES)
return -EINVAL
if ((dev->family == USBCAN) &&
(dev->nchannels > MAX_USBCAN_NET_DEVICES))
return -EINVAL
You can remove LEAF_MAX_NET_DEVICES which is not used, keep
MAX_NET_DEVICES equals to 3 and remove the MAX() macro.
The test specific to the USBCAN family can eventually be moved in the
kvaser_usb_probe() function.
Quite nice, will do it that way in v3.
Regards,
Darwish
On Tue, Dec 30, 2014 at 10:33:26AM -0500, Ahmed S. Darwish wrote:
On Sun, Dec 28, 2014 at 10:51:34PM +0100, Olivier Sobrie wrote:
[...]
quoted
quoted
quoted
quoted
+ if (LEAF_PRODUCT_ID(id->idProduct)) {
+ dev->family = KVASER_LEAF;
+ dev->max_channels = LEAF_MAX_NET_DEVICES;
+ } else if (USBCAN_PRODUCT_ID(id->idProduct)) {
+ dev->family = KVASER_USBCAN;
+ dev->max_channels = USBCAN_MAX_NET_DEVICES;
+ } else {
+ dev_err(&intf->dev, "Product ID (%d) does not belong to any "
+ "known Kvaser USB family", id->idProduct);
+ return -ENODEV;
+ }
+
Is it really required to keep max_channels in the kvaser_usb structure?
If I looked correctly, you use this variable as a replacement for
MAX_NET_DEVICES in the code and MAX_NET_DEVICES is only used in probe
and disconnect functions. I think it can even be replaced by nchannels
in the disconnect path. So I also think that it don't need to be in the
kvaser_usb structure.
hmmm.. given the current state of error arbitration explained
above, where I cannot accept a dev->nchannels > 2, I guess we
have two options:
a) Remove max_channels, and hardcode the channels count
correctness logic as follows:
dev->nchannels = msg.u.cardinfo.nchannels;
if ((dev->family == USBCAN && dev->nchannels > USBCAN_MAX_NET_DEVICES)
|| (dev->family == LEAF && dev->nchannels > LEAF_MAX_NET_DEVICES))
return -EINVAL
b) Leave max_channels in 'struct kvaser_usb' as is.
I personally prefer the solution at 'b)' but I can do it as
in 'a)' if you prefer :-)
Keeping max_channels in the kvaser_usb structure is useless because it
is only used in one function that is called in the probe function.
I would prefer to have:
if (dev->nchannels > MAX_NET_DEVICES)
return -EINVAL
if ((dev->family == USBCAN) &&
(dev->nchannels > MAX_USBCAN_NET_DEVICES))
return -EINVAL
You can remove LEAF_MAX_NET_DEVICES which is not used, keep
MAX_NET_DEVICES equals to 3 and remove the MAX() macro.
The test specific to the USBCAN family can eventually be moved in the
kvaser_usb_probe() function.
Quite nice, will do it that way in v3.
Thank you.
Please also check your patches with scripts/checkpatch.pl.
I see several warnings when I run it. Please fix them.
All the best for New Year's Eve,
--
Olivier
From: Ahmed S. Darwish <hidden> Date: 2015-01-05 17:49:17
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Note:
Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
is a driver bug in and out of itself: it means that our start/stop
queue flow control is broken.
This patch only fixes the (buggy) error handling code; the root
cause shall be fixed in a later commit.
Signed-off-by: Ahmed S. Darwish <redacted>
Acked-by: Olivier Sobrie <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
** V3 Changelog:
- checkpatch.pl suggestions ('net/' commenting style)
@@ -1294,12 +1294,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,if(!urb){netdev_err(netdev,"No memory left for URBs\n");stats->tx_dropped++;-gotonourbmem;+dev_kfree_skb(skb);+returnNETDEV_TX_OK;}buf=kmalloc(sizeof(structkvaser_msg),GFP_ATOMIC);if(!buf){stats->tx_dropped++;+dev_kfree_skb(skb);gotonobufmem;}
@@ -1334,6 +1336,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,}}+/* This should never happen; it implies a flow control bug */if(!context){netdev_warn(netdev,"cannot find free context\n");ret=NETDEV_TX_BUSY;
@@ -1364,9 +1367,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,if(unlikely(err)){can_free_echo_skb(netdev,context->echo_index);-skb=NULL;/* set to NULL to avoid double free in-*dev_kfree_skb(skb)*/-atomic_dec(&priv->active_tx_urbs);usb_unanchor_urb(urb);
From: Ahmed S. Darwish <hidden> Date: 2015-01-05 17:57:21
From: Ahmed S. Darwish <redacted>
Recent Leaf firmware versions (>= 3.1.557) do not allow to send
commands for non-existing channels. If a command is sent for a
non-existing channel, the firmware crashes.
Reported-by: Christopher Storah <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
Signed-off-by: Olivier Sobrie <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
** V3 Changelog:
- Update commit log message per Olivier remarks on v2
From: Ahmed S. Darwish <hidden> Date: 2015-01-05 17:59:40
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in very high frequency (*), closing the CAN channel while
all the transmissions are on (#), opening the device again (@),
then sending a small number of packets would make the driver
enter an almost infinite loop of:
[....]
[15959.853988] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853990] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853991] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853993] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853994] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853995] kvaser_usb 4-3:1.0 can0: cannot find free context
[....]
_dragging the whole system down_ in the process due to the
excessive logging output.
Initially, this has caused random panics in the kernel due to a
buggy error recovery path. That got fixed in an earlier commit.(%)
This patch aims at solving the root cause. -->
16 tx URBs and contexts are allocated per CAN channel per USB
device. Such URBs are protected by:
a) A simple atomic counter, up to a value of MAX_TX_URBS (16)
b) A flag in each URB context, stating if it's free
c) The fact that ndo_start_xmit calls are themselves protected
by the networking layers higher above
After grabbing one of the tx URBs, if the driver noticed that all
of them are now taken, it stops the netif transmission queue.
Such queue is worken up again only if an acknowedgment was received
from the firmware on one of our earlier-sent frames.
Meanwhile, upon channel close (#), the driver sends a CMD_STOP_CHIP
to the firmware, effectively closing all further communication. In
the high traffic case, the atomic counter remains at MAX_TX_URBS,
and all the URB contexts remain marked as active. While opening
the channel again (@), it cannot send any further frames since no
more free tx URB contexts are available.
Reset all tx URB contexts upon CAN channel close.
(*) 50 parallel instances of `cangen0 -g 0 -ix`
(#) `ifconfig can0 down`
(@) `ifconfig can0 up`
(%) "can: kvaser_usb: Don't free packets when tight on URBs"
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 3 +++
1 file changed, 3 insertions(+)
From: Ahmed S. Darwish <hidden> Date: 2015-01-05 18:31:40
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 618 +++++++++++++++++++++++++++++++--------
2 files changed, 503 insertions(+), 123 deletions(-)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
** V3 Changelog:
- Fix padding for the usbcan_msg_tx_acknowledge command
- Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
- Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
- Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
@@ -21,6 +23,15 @@#include<linux/can/dev.h>#include<linux/can/error.h>+/* Kvaser USB CAN dongles are divided into two major families:+*-Leaf:BasedonRenesasM32C,runningfirmwarelabeledas'filo'+*-UsbcanII:BasedonRenesasM16C,runningfirmwarelabeledas'helios'+*/+enumkvaser_usb_family{+KVASER_LEAF,+KVASER_USBCAN,+};+#define MAX_TX_URBS 16#define MAX_RX_URBS 4#define START_TIMEOUT 1000 /* msecs */
@@ -615,37 +799,80 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)priv->tx_contexts[i].echo_index=MAX_TX_URBS;}-staticvoidkvaser_usb_rx_error(conststructkvaser_usb*dev,-conststructkvaser_msg*msg)+staticvoidkvaser_report_error_event(conststructkvaser_usb*dev,+structkvaser_error_summary*es);++/* Report error to userspace iff the controller's errors counter has+*increased,orwe'retheonlychannelseeingthebuserrorstate.+*+*AsreportedbyUSBCANsheets,"the CAN controller has difficulties+*totellwhetheranerrorframearrivedonchannel1oronchannel2."+*Thus,errorcountersarecomparedwiththeirearliervaluesto+*determinewhichchannelwasresponsiblefortheerrorevent.+*/+staticvoidusbcan_report_error_if_applicable(conststructkvaser_usb*dev,+structkvaser_error_summary*es){-structcan_frame*cf;-structsk_buff*skb;-structnet_device_stats*stats;structkvaser_usb_net_priv*priv;-unsignedintnew_state;-u8channel,status,txerr,rxerr,error_factor;+intold_tx_err_count,old_rx_err_count,channel,report_error;++channel=es->channel;+if(channel>=dev->nchannels){+dev_err(dev->udev->dev.parent,+"Invalid channel number (%d)\n",channel);+return;+}++priv=dev->nets[channel];+old_tx_err_count=priv->bec.txerr;+old_rx_err_count=priv->bec.rxerr;++report_error=0;+if(es->txerr>old_tx_err_count){+es->usbcan.error_state|=USBCAN_ERROR_STATE_TX_ERROR;+report_error=1;+}+if(es->rxerr>old_rx_err_count){+es->usbcan.error_state|=USBCAN_ERROR_STATE_RX_ERROR;+report_error=1;+}+if((es->status&M16C_STATE_BUS_ERROR)&&+!(es->usbcan.other_ch_status&M16C_STATE_BUS_ERROR)){+es->usbcan.error_state|=USBCAN_ERROR_STATE_BUSERROR;+report_error=1;+}++if(report_error)+kvaser_report_error_event(dev,es);+}++/* Extract error summary from a Leaf-based device error message */+staticvoidleaf_extract_error_from_msg(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_error_summaryes={0,};switch(msg->id){caseCMD_CAN_ERROR_EVENT:-channel=msg->u.error_event.channel;-status=msg->u.error_event.status;-txerr=msg->u.error_event.tx_errors_count;-rxerr=msg->u.error_event.rx_errors_count;-error_factor=msg->u.error_event.error_factor;+es.channel=msg->u.leaf.error_event.channel;+es.status=msg->u.leaf.error_event.status;+es.txerr=msg->u.leaf.error_event.tx_errors_count;+es.rxerr=msg->u.leaf.error_event.rx_errors_count;+es.leaf.error_factor=msg->u.leaf.error_event.error_factor;break;-caseCMD_LOG_MESSAGE:-channel=msg->u.log_message.channel;-status=msg->u.log_message.data[0];-txerr=msg->u.log_message.data[2];-rxerr=msg->u.log_message.data[3];-error_factor=msg->u.log_message.data[1];+caseCMD_LEAF_LOG_MESSAGE:+es.channel=msg->u.leaf.log_message.channel;+es.status=msg->u.leaf.log_message.data[0];+es.txerr=msg->u.leaf.log_message.data[2];+es.rxerr=msg->u.leaf.log_message.data[3];+es.leaf.error_factor=msg->u.leaf.log_message.data[1];break;caseCMD_CHIP_STATE_EVENT:-channel=msg->u.chip_state_event.channel;-status=msg->u.chip_state_event.status;-txerr=msg->u.chip_state_event.tx_errors_count;-rxerr=msg->u.chip_state_event.rx_errors_count;-error_factor=0;+es.channel=msg->u.leaf.chip_state_event.channel;+es.status=msg->u.leaf.chip_state_event.status;+es.txerr=msg->u.leaf.chip_state_event.tx_errors_count;+es.rxerr=msg->u.leaf.chip_state_event.rx_errors_count;+es.leaf.error_factor=0;break;default:dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",
@@ -653,16 +880,92 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,return;}-if(channel>=dev->nchannels){+kvaser_report_error_event(dev,&es);+}++/* Extract error summary from a USBCANII-based device error message */+staticvoidusbcan_extract_error_from_msg(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_error_summaryes={0,};++switch(msg->id){+/* Sometimes errors are sent as unsolicited chip state events */+caseCMD_CHIP_STATE_EVENT:+es.channel=msg->u.usbcan.chip_state_event.channel;+es.status=msg->u.usbcan.chip_state_event.status;+es.txerr=msg->u.usbcan.chip_state_event.tx_errors_count;+es.rxerr=msg->u.usbcan.chip_state_event.rx_errors_count;+usbcan_report_error_if_applicable(dev,&es);+break;++caseCMD_CAN_ERROR_EVENT:+es.channel=0;+es.status=msg->u.usbcan.error_event.status_ch0;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch0;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch0;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch1;+usbcan_report_error_if_applicable(dev,&es);++/* For error events, the USBCAN firmware does not support+*morethan2channels:ch0,andch1.+*/+if(dev->nchannels>1){+es.channel=1;+es.status=msg->u.usbcan.error_event.status_ch1;+es.txerr=+msg->u.usbcan.error_event.tx_errors_count_ch1;+es.rxerr=+msg->u.usbcan.error_event.rx_errors_count_ch1;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch0;+usbcan_report_error_if_applicable(dev,&es);+}+break;++default:+dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",+msg->id);+}+}++staticvoidkvaser_usb_rx_error(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+switch(dev->family){+caseKVASER_LEAF:+leaf_extract_error_from_msg(dev,msg);+break;+caseKVASER_USBCAN:+usbcan_extract_error_from_msg(dev,msg);+break;+default:dev_err(dev->udev->dev.parent,-"Invalid channel number (%d)\n",channel);+"Invalid device family (%d)\n",dev->family);return;}+}-priv=dev->nets[channel];+staticvoidkvaser_report_error_event(conststructkvaser_usb*dev,+structkvaser_error_summary*es)+{+structcan_frame*cf;+structsk_buff*skb;+structnet_device_stats*stats;+structkvaser_usb_net_priv*priv;+unsignedintnew_state;++if(es->channel>=dev->nchannels){+dev_err(dev->udev->dev.parent,+"Invalid channel number (%d)\n",es->channel);+return;+}++priv=dev->nets[es->channel];stats=&priv->netdev->stats;-if(status&M16C_STATE_BUS_RESET){+if(es->status&M16C_STATE_BUS_RESET){kvaser_usb_unlink_tx_urbs(priv);return;}
@@ -1599,6 +1962,17 @@ static int kvaser_usb_probe(struct usb_interface *intf,if(!dev)return-ENOMEM;+if(LEAF_PRODUCT_ID(id->idProduct)){+dev->family=KVASER_LEAF;+}elseif(USBCAN_PRODUCT_ID(id->idProduct)){+dev->family=KVASER_USBCAN;+}else{+dev_err(&intf->dev,+"Product ID (%d) does not belong to any known Kvaser USB family",+id->idProduct);+return-ENODEV;+}+err=kvaser_usb_get_endpoints(intf,&dev->bulk_in,&dev->bulk_out);if(err){dev_err(&intf->dev,"Cannot get usb endpoint(s)");
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-08 09:59:57
On 01/05/2015 06:49 PM, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Note:
Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
is a driver bug in and out of itself: it means that our start/stop
queue flow control is broken.
This patch only fixes the (buggy) error handling code; the root
cause shall be fixed in a later commit.
Signed-off-by: Ahmed S. Darwish <redacted>
Acked-by: Olivier Sobrie <redacted>
Applied 1-3 to can/master + added stable on Cc.
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 |
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-08 11:54:12
On 01/05/2015 07:31 PM, Ahmed S. Darwish wrote:
quoted hunk
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 618 +++++++++++++++++++++++++++++++--------
2 files changed, 503 insertions(+), 123 deletions(-)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
** V3 Changelog:
- Fix padding for the usbcan_msg_tx_acknowledge command
- Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
- Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
- Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
@@ -21,6 +23,15 @@#include<linux/can/dev.h>#include<linux/can/error.h>+/* Kvaser USB CAN dongles are divided into two major families:+*-Leaf:BasedonRenesasM32C,runningfirmwarelabeledas'filo'+*-UsbcanII:BasedonRenesasM16C,runningfirmwarelabeledas'helios'+*/+enumkvaser_usb_family{+KVASER_LEAF,+KVASER_USBCAN,+};+#define MAX_TX_URBS 16#define MAX_RX_URBS 4#define START_TIMEOUT 1000 /* msecs */
Please rearange your code that forward declarations are not needed (if
possible - I haven't checked, though).
+
+/* Report error to userspace iff the controller's errors counter has
+ * increased, or we're the only channel seeing the bus error state.
+ *
+ * As reported by USBCAN sheets, "the CAN controller has difficulties
+ * to tell whether an error frame arrived on channel 1 or on channel 2."
+ * Thus, error counters are compared with their earlier values to
+ * determine which channel was responsible for the error event.
Your code doesn't match this comment. You compare the error counters
against the old values to tell if it's a rx or tx error, the channel
information from the struct kvaser_error_summary is used directly.
Another patch would be not to touch cf after netif_rx(), please move the stats handling before calling netif_rx(). Same applies to the kvaser_usb_rx_can_msg() function.
@@ -1330,7 +1693,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc); if (cf->can_id & CAN_RTR_FLAG)- msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;+ *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME; for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) { if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
@@ -1599,6 +1962,17 @@ static int kvaser_usb_probe(struct usb_interface *intf, if (!dev) return -ENOMEM;+ if (LEAF_PRODUCT_ID(id->idProduct)) {+ dev->family = KVASER_LEAF;+ } else if (USBCAN_PRODUCT_ID(id->idProduct)) {+ dev->family = KVASER_USBCAN;+ } else {+ dev_err(&intf->dev,+ "Product ID (%d) does not belong to any known Kvaser USB family",+ id->idProduct);+ return -ENODEV;+ }+ err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out); if (err) { dev_err(&intf->dev, "Cannot get usb endpoint(s)");
regards,
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 |
From: Ahmed S. Darwish <hidden> Date: 2015-01-08 15:19:23
Hi Marc,
On Thu, Jan 08, 2015 at 12:53:37PM +0100, Marc Kleine-Budde wrote:
On 01/05/2015 07:31 PM, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
[...]
quoted
+/* Kvaser USB CAN dongles are divided into two major families:
+ * - Leaf: Based on Renesas M32C, running firmware labeled as 'filo'
+ * - UsbcanII: Based on Renesas M16C, running firmware labeled as 'helios'
+ */
+enum kvaser_usb_family {
+ KVASER_LEAF,
+ KVASER_USBCAN,
+};
+
#define MAX_TX_URBS 16
#define MAX_RX_URBS 4
#define START_TIMEOUT 1000 /* msecs */
Can you please convert both *_PRODUCT_ID() macros into static inline
bool functions.
Will do.
[...]
quoted
MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
@@ -463,7 +631,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev) if (err) return err;- dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);+ switch (dev->family) {+ case KVASER_LEAF:+ dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);+ break;+ case KVASER_USBCAN:+ dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);+ break;+ default:+ dev_err(dev->udev->dev.parent,+ "Invalid device family (%d)\n", dev->family);+ return -EINVAL;
The default case should not happen. I think you can remove it.
It's true, it _should_ never happen. But I only add such checks if
the follow-up code critically depends on a certain `dev->family`
behavior. So it's kind of a defensive check against any possible
bug in driver or memory.
What do you think?
quoted
+ }
return 0;
}
@@ -484,6 +663,9 @@ static int kvaser_usb_get_card_info(struct kvaser_usb *dev) dev->nchannels = msg.u.cardinfo.nchannels; if (dev->nchannels > MAX_NET_DEVICES) return -EINVAL;+ if (dev->family == KVASER_USBCAN &&+ dev->nchannels > MAX_USBCAN_NET_DEVICES)+ return -EINVAL;
Nitpick, as the new "if" also does a test on nchannels, why no extend
the existing "if" with an "||"?
Please rearange your code that forward declarations are not needed (if
possible - I haven't checked, though).
I originally did it that way, but it completely messed up with the
patch if I do such rearrangement. A huge block of code gets removed
at top of the patch, and it got added again at the end, making the
actual important lines changed _within_ such big block non-apparent.
Maybe I should do the re-arrangement in a follow-up patch?
quoted
+
+/* Report error to userspace iff the controller's errors counter has
+ * increased, or we're the only channel seeing the bus error state.
+ *
+ * As reported by USBCAN sheets, "the CAN controller has difficulties
+ * to tell whether an error frame arrived on channel 1 or on channel 2."
+ * Thus, error counters are compared with their earlier values to
+ * determine which channel was responsible for the error event.
Your code doesn't match this comment. You compare the error counters
against the old values to tell if it's a rx or tx error, the channel
information from the struct kvaser_error_summary is used directly.
Hmmm, good catch, upon a second look, the code looks a bit deceiving.
The comments, meanwhile, are taken verbatim from the Kvaser sheets:
http://www.kvaser.com/canlib-webhelp/page_hardware_specific_can_controllers.html
Archived at http://www.webcitation.org/6VQd87jIA
So, what happens is that the firmware does not tell us whether the
received error event is for ch0 or ch1. But it gives us the (possibly
new) error counters for both of them:
struct usbcan_msg_error_event {
[..]
u8 tx_errors_count_ch0;
u8 rx_errors_count_ch0;
u8 tx_errors_count_ch1;
u8 rx_errors_count_ch1;
[..]
} __packed;
We loop over each channel, and report an error to userspace if extra
errors were spotted for such channel.
kvaser_error_summary is not a firmware command or response. Since
the wire format for an error event differs between Leaf and Usbcan,
it's just our way to summarize an error whether it's from any of
them, and this is where the conflict stems from:
- If it's a Leaf-device, `error_summary->channel' is the excat
channel reported by the firmware
- If it's a Usbcan device, `error_summary->channel' is just a mark
to check the error counters for such a channel and report error
to userspace iff they've increased.
Thus the raison d'etre for `error_summary' struct is to share
the error handling code between Leaf and UsbcanII devices since
it's quite similar.
So to clear up this conflict, I suggest the following error_summary
layout:
struct kvaser_error_summary {
union {
struct {
u8 channel;
} leaf;
struct {
u8 possible_channel;
} usbcan;
};
/* Rest of layout is left as-is */
}
This way, it's clear that in case of Usbcan, channel is not final
and we need further work to do. What do you think?
(BTW, any better name than "kvaser_error_summary"? It conflicts a
little bit with the namespace used for the packed wire format
structures "kvaser_*")
+
+ switch (msg->id) {
+ /* Sometimes errors are sent as unsolicited chip state events */
+ case CMD_CHIP_STATE_EVENT:
+ es.channel = msg->u.usbcan.chip_state_event.channel;
+ es.status = msg->u.usbcan.chip_state_event.status;
+ es.txerr = msg->u.usbcan.chip_state_event.tx_errors_count;
+ es.rxerr = msg->u.usbcan.chip_state_event.rx_errors_count;
+ usbcan_report_error_if_applicable(dev, &es);
+ break;
+
+ case CMD_CAN_ERROR_EVENT:
+ es.channel = 0;
+ es.status = msg->u.usbcan.error_event.status_ch0;
+ es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch0;
+ es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch0;
+ es.usbcan.other_ch_status =
+ msg->u.usbcan.error_event.status_ch1;
+ usbcan_report_error_if_applicable(dev, &es);
+
+ /* For error events, the USBCAN firmware does not support
+ * more than 2 channels: ch0, and ch1.
+ */
+ if (dev->nchannels > 1) {
+ es.channel = 1;
Why is channel == 1 if the device has more than 1 channel?
This is related to the "kvaser_error_summary" discussion above
where "channel" is only a suggestion for checking the error
counters.
If the Usbcan device has only one channel, then there's no need
to check if the "tx_errors_count_ch1", "rx_errors_count_ch1" has
increased or not. Their values are undefined.
Can you prepare a (seperate) patch that does the stats, even in case of OOM here. Same for kvaser_report_error_event()
Sure.
In kvaser_report_error_event() though, isn't it a little bit tricky?
Specially in fragments as in below:
switch (dev->family) {
case KVASER_LEAF:
if (es->leaf.error_factor) {
priv->can.can_stats.bus_error++;
stats->rx_errors++;
cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
[...]
}
break;
case KVASER_USBCAN:
if (es->usbcan.error_state & USBCAN_ERROR_STATE_TX_ERROR)
stats->tx_errors++;
if (es->usbcan.error_state & USBCAN_ERROR_STATE_RX_ERROR)
stats->rx_errors++;
if (es->usbcan.error_state & USBCAN_ERROR_STATE_BUSERROR) {
priv->can.can_stats.bus_error++;
cf->can_id |= CAN_ERR_BUSERROR;
}
break;
}
IMHO, there will be some duplication of the above fragment. Once
to handle "stats->*", and once to handle packet-related "cf->*" stuff.
Also in the Usbcan case, it will clutter the error_state checks in
different places.
Another patch would be not to touch cf after netif_rx(), please move the stats handling before calling netif_rx(). Same applies to the kvaser_usb_rx_can_msg() function.
Indeed, these can be totally bogus values after netif_rx().
I'll introduce this as a new patch in a new series.
Thanks for your review!
Regards,
Darwish
Another patch would be not to touch cf after netif_rx(), please move the stats handling before calling netif_rx(). Same applies to the kvaser_usb_rx_can_msg() function.
BTW, is it guaranteed from the SocketCAN stack that netif_rx()
will never return NET_RX_DROPPED? Because if no guarantee
exists, I guess below fragment cannot be completely correct?
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
netif_rx(skb);
On the other hand, I don't see evan a single CAN driver checking
netif_rx() return value, so maybe such a check is an overkill...
Thanks,
Darwish
Another patch would be not to touch cf after netif_rx(), please
move the stats handling before calling netif_rx(). Same applies to
the kvaser_usb_rx_can_msg() function.
BTW, is it guaranteed from the SocketCAN stack that netif_rx()
netif_rx() is the generic networking stack already.
will never return NET_RX_DROPPED? Because if no guarantee
exists, I guess below fragment cannot be completely correct?
No, it's not guaranteed...
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
netif_rx(skb);
On the other hand, I don't see evan a single CAN driver checking
netif_rx() return value, so maybe such a check is an overkill...
A quick look shows that almost no ethernet or wireless drivers take care
about the return value. In case of a RX_DROPPED some increase a drop
counter, though.
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 |
From: Ahmed S. Darwish <hidden> Date: 2015-01-11 20:05:51
Hi,
Now since earlier v3 submission patches #1-3 got merged, this
is a new patch series expanding on patch v3 #4: support for
the USBCAN-II family.
A new series is introduced due to the extra additions suggested
by code review, which required being added in their own
self-contained patches.
Thanks,
Darwish
From: Ahmed S. Darwish <hidden> Date: 2015-01-11 20:11:24
From: Ahmed S. Darwish <redacted>
We should not touch the packet after a netif_rx: it might
get freed behind our back.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
From: Ahmed S. Darwish <hidden> Date: 2015-01-11 20:15:27
From: Ahmed S. Darwish <redacted>
Let the error counters be more accurate in case of Out of
Memory conditions.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Ahmed S. Darwish <hidden> Date: 2015-01-11 20:36:21
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 612 ++++++++++++++++++++++++++++++---------
2 files changed, 487 insertions(+), 133 deletions(-)
** V4 Changelog:
- Use type-safe C methods instead of cpp macros
- Further clarify the code and comments on error events channel arbitration
- Remove defensive checks against non-existing families
- Re-order methods to remove forward declarations
- Smaller stuff spotted by earlier review (function prefexes, etc.)
** V3 Changelog:
- Fix padding for the usbcan_msg_tx_acknowledge command
- Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
- Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
- Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
@@ -21,6 +23,15 @@#include<linux/can/dev.h>#include<linux/can/error.h>+/* Kvaser USB CAN dongles are divided into two major families:+*-Leaf:BasedonRenesasM32C,runningfirmwarelabeledas'filo'+*-UsbcanII:BasedonRenesasM16C,runningfirmwarelabeledas'helios'+*/+enumkvaser_usb_family{+KVASER_LEAF,+KVASER_USBCAN,+};+#define MAX_TX_URBS 16#define MAX_RX_URBS 4#define START_TIMEOUT 1000 /* msecs */
@@ -260,19 +392,57 @@ struct kvaser_msg {structkvaser_msg_simplesimple;structkvaser_msg_cardinfocardinfo;structkvaser_msg_cardinfo2cardinfo2;-structkvaser_msg_softinfosoftinfo;structkvaser_msg_busparamsbusparams;++structkvaser_msg_rx_can_headerrx_can_header;+structkvaser_msg_tx_acknowledge_headertx_acknowledge_header;++union{+structleaf_msg_softinfosoftinfo;+structleaf_msg_rx_canrx_can;+structleaf_msg_chip_state_eventchip_state_event;+structleaf_msg_tx_acknowledgetx_acknowledge;+structleaf_msg_error_eventerror_event;+structleaf_msg_log_messagelog_message;+}__packedleaf;++union{+structusbcan_msg_softinfosoftinfo;+structusbcan_msg_rx_canrx_can;+structusbcan_msg_chip_state_eventchip_state_event;+structusbcan_msg_tx_acknowledgetx_acknowledge;+structusbcan_msg_error_eventerror_event;+}__packedusbcan;+structkvaser_msg_tx_cantx_can;-structkvaser_msg_rx_canrx_can;-structkvaser_msg_chip_state_eventchip_state_event;-structkvaser_msg_tx_acknowledgetx_acknowledge;-structkvaser_msg_error_eventerror_event;structkvaser_msg_ctrl_modectrl_mode;structkvaser_msg_flush_queueflush_queue;-structkvaser_msg_log_messagelog_message;}u;}__packed;+/* Summary of a kvaser error event, for a unified Leaf/Usbcan error+*handling.Somediscrepanciesbetweenthetwofamiliesexist:+*+*-USBCANfirmwaredoesnotreportM16C"error factors"+*-USBCANcontrollershasdifficultiesreportingiftheraisederror+*eventisforch0orch1.TheyleavesucharbitrationtotheOS+*driverbylettingitcompareerrorcounterswithpreviousvalues+*anddecidetheerrorevent'schannel.ThusforUSBCAN,thechannel+*fieldisonlyadvisory.+*/+structkvaser_error_summary{+u8channel,status,txerr,rxerr;+union{+struct{+u8error_factor;+}leaf;+struct{+u8other_ch_status;+u8error_state;+}usbcan;+};+};+structkvaser_usb_tx_urb_context{structkvaser_usb_net_priv*priv;u32echo_index;
@@ -775,6 +952,124 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,netif_rx(skb);}+/* For USBCAN, report error to userspace iff the channels's errors counter+*hasincreased,orwe'retheonlychannelseeingabuserrorstate.+*/+staticvoidkvaser_usbcan_conditionally_rx_error(conststructkvaser_usb*dev,+structkvaser_error_summary*es)+{+structkvaser_usb_net_priv*priv;+intchannel;+boolreport_error;++channel=es->channel;+if(channel>=dev->nchannels){+dev_err(dev->udev->dev.parent,+"Invalid channel number (%d)\n",channel);+return;+}++priv=dev->nets[channel];+report_error=false;++if(es->txerr>priv->bec.txerr){+es->usbcan.error_state|=USBCAN_ERROR_STATE_TX_ERROR;+report_error=true;+}+if(es->rxerr>priv->bec.rxerr){+es->usbcan.error_state|=USBCAN_ERROR_STATE_RX_ERROR;+report_error=true;+}+if((es->status&M16C_STATE_BUS_ERROR)&&+!(es->usbcan.other_ch_status&M16C_STATE_BUS_ERROR)){+es->usbcan.error_state|=USBCAN_ERROR_STATE_BUSERROR;+report_error=true;+}++if(report_error)+kvaser_usb_rx_error(dev,es);+}++staticvoidkvaser_usbcan_rx_error(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_error_summaryes={};++switch(msg->id){+/* Sometimes errors are sent as unsolicited chip state events */+caseCMD_CHIP_STATE_EVENT:+es.channel=msg->u.usbcan.chip_state_event.channel;+es.status=msg->u.usbcan.chip_state_event.status;+es.txerr=msg->u.usbcan.chip_state_event.tx_errors_count;+es.rxerr=msg->u.usbcan.chip_state_event.rx_errors_count;+kvaser_usbcan_conditionally_rx_error(dev,&es);+break;++caseCMD_CAN_ERROR_EVENT:+es.channel=0;+es.status=msg->u.usbcan.error_event.status_ch0;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch0;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch0;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch1;+kvaser_usbcan_conditionally_rx_error(dev,&es);++/* The USBCAN firmware does not support more than 2 channels.+*Nowthatch0waschecked,checkifch1hasanyerrors.+*/+if(dev->nchannels==MAX_USBCAN_NET_DEVICES){+es.channel=1;+es.status=msg->u.usbcan.error_event.status_ch1;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch1;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch1;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch0;+kvaser_usbcan_conditionally_rx_error(dev,&es);+}+break;++default:+dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",+msg->id);+}+}++staticvoidkvaser_leaf_rx_error(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_error_summaryes={};++switch(msg->id){+caseCMD_CAN_ERROR_EVENT:+es.channel=msg->u.leaf.error_event.channel;+es.status=msg->u.leaf.error_event.status;+es.txerr=msg->u.leaf.error_event.tx_errors_count;+es.rxerr=msg->u.leaf.error_event.rx_errors_count;+es.leaf.error_factor=msg->u.leaf.error_event.error_factor;+break;+caseCMD_LEAF_LOG_MESSAGE:+es.channel=msg->u.leaf.log_message.channel;+es.status=msg->u.leaf.log_message.data[0];+es.txerr=msg->u.leaf.log_message.data[2];+es.rxerr=msg->u.leaf.log_message.data[3];+es.leaf.error_factor=msg->u.leaf.log_message.data[1];+break;+caseCMD_CHIP_STATE_EVENT:+es.channel=msg->u.leaf.chip_state_event.channel;+es.status=msg->u.leaf.chip_state_event.status;+es.txerr=msg->u.leaf.chip_state_event.tx_errors_count;+es.rxerr=msg->u.leaf.chip_state_event.rx_errors_count;+es.leaf.error_factor=0;+break;+default:+dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",+msg->id);+return;+}++kvaser_usb_rx_error(dev,&es);+}+staticvoidkvaser_usb_rx_can_err(conststructkvaser_usb_net_priv*priv,conststructkvaser_msg*msg){
@@ -1596,6 +1933,17 @@ static int kvaser_usb_probe(struct usb_interface *intf,if(!dev)return-ENOMEM;+if(kvaser_is_leaf(id)){+dev->family=KVASER_LEAF;+}elseif(kvaser_is_usbcan(id)){+dev->family=KVASER_USBCAN;+}else{+dev_err(&intf->dev,+"Product ID (%d) does not belong to any known Kvaser USB family",+id->idProduct);+return-ENODEV;+}+err=kvaser_usb_get_endpoints(intf,&dev->bulk_in,&dev->bulk_out);if(err){dev_err(&intf->dev,"Cannot get usb endpoint(s)");
From: Ahmed S. Darwish <hidden> Date: 2015-01-11 20:45:18
From: Ahmed S. Darwish <redacted>
(This is a draft patch, I'm not sure if this fixes the USB
bug or only its psymptom. Feedback from the linux-usb folks
is really appreciated.)
When plugging the Kvaser USB/CAN dongle the first time, everything
works as expected and all of the transfers from and to the USB
device succeeds.
Meanwhile, after unplugging the device and plugging it again, the
first bulk transfer _always_ returns an -ETIMEDOUT. The following
behaviour was observied:
- Setting higher timeout values for the first bulk transfer never
solved the issue.
- Unloading, then loading, our kvaser_usb module in question
__always__ solved the issue.
- Checking first bulk transfer status, and retry the transfer
again in case of an -ETIMEDOUT also __always__ solved the issue.
This is what the patch below does.
- In the testing done so far, this issue appears only on laptops
but never on PCs (possibly power related?)
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
@@ -1927,7 +1927,7 @@ static int kvaser_usb_probe(struct usb_interface *intf,{structkvaser_usb*dev;interr=-ENOMEM;-inti;+inti,retry=3;dev=devm_kzalloc(&intf->dev,sizeof(*dev),GFP_KERNEL);if(!dev)
@@ -1956,7 +1956,16 @@ static int kvaser_usb_probe(struct usb_interface *intf,usb_set_intfdata(intf,dev);-err=kvaser_usb_get_software_info(dev);+/* On some x86 laptops, plugging a USBCAN device again after+*anunplugmakesthefirmwarealwaysignoretheveryfirst+*command.Forsuchacase,providesomeroomforretries+*insteadofcompletlyexitingthedriver.+*/+while(retry--){+err=kvaser_usb_get_software_info(dev);+if(err!=-ETIMEDOUT)+break;+}if(err){dev_err(&intf->dev,"Cannot get software infos, error %d\n",err);
From: Ahmed S. Darwish <hidden> Date: 2015-01-11 20:50:00
From: Ahmed S. Darwish <redacted>
We should not touch the packet after a netif_rx: it might
get freed behind our back.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
(Resend, fix the garbled subject line. Sorry)
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-11 20:51:25
On 01/11/2015 09:45 PM, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
(This is a draft patch, I'm not sure if this fixes the USB
bug or only its psymptom. Feedback from the linux-usb folks
is really appreciated.)
When plugging the Kvaser USB/CAN dongle the first time, everything
works as expected and all of the transfers from and to the USB
device succeeds.
Meanwhile, after unplugging the device and plugging it again, the
first bulk transfer _always_ returns an -ETIMEDOUT. The following
behaviour was observied:
- Setting higher timeout values for the first bulk transfer never
solved the issue.
- Unloading, then loading, our kvaser_usb module in question
__always__ solved the issue.
- Checking first bulk transfer status, and retry the transfer
again in case of an -ETIMEDOUT also __always__ solved the issue.
This is what the patch below does.
- In the testing done so far, this issue appears only on laptops
but never on PCs (possibly power related?)
Signed-off-by: Ahmed S. Darwish <redacted>
Does this patch apply apply between 3 and 4? If not, please re-arrange
the series. As this is a bug fix, patches 1, 2 and 4 will go via
net/master, 3 will go via net-next/master.
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 |
From: Ahmed S. Darwish <hidden> Date: 2015-01-12 10:14:16
On Sun, Jan 11, 2015 at 09:51:10PM +0100, Marc Kleine-Budde wrote:
On 01/11/2015 09:45 PM, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
(This is a draft patch, I'm not sure if this fixes the USB
bug or only its psymptom. Feedback from the linux-usb folks
is really appreciated.)
When plugging the Kvaser USB/CAN dongle the first time, everything
works as expected and all of the transfers from and to the USB
device succeeds.
Meanwhile, after unplugging the device and plugging it again, the
first bulk transfer _always_ returns an -ETIMEDOUT. The following
behaviour was observied:
- Setting higher timeout values for the first bulk transfer never
solved the issue.
- Unloading, then loading, our kvaser_usb module in question
__always__ solved the issue.
- Checking first bulk transfer status, and retry the transfer
again in case of an -ETIMEDOUT also __always__ solved the issue.
This is what the patch below does.
- In the testing done so far, this issue appears only on laptops
but never on PCs (possibly power related?)
Signed-off-by: Ahmed S. Darwish <redacted>
Does this patch apply apply between 3 and 4? If not, please re-arrange
the series. As this is a bug fix, patches 1, 2 and 4 will go via
net/master, 3 will go via net-next/master.
Since no one complained earlier, I guess this issue only affects
USBCAN devices. That's why I've based it above patch #3: adding
USBCAN hardware support.
Nonetheless, it won't do any harm for the current Leaf-only
driver. So _if_ this is the correct fix, I will update the commit
log, refactor the check into a 'do { } while()' loop, and then
base it above the Leaf-only net/master fixes on patch #1, and #2.
Any feedback on the USB side of things?
Thanks,
Darwish
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-12 10:25:41
On 01/12/2015 11:14 AM, Ahmed S. Darwish wrote:
On Sun, Jan 11, 2015 at 09:51:10PM +0100, Marc Kleine-Budde wrote:
quoted
On 01/11/2015 09:45 PM, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
(This is a draft patch, I'm not sure if this fixes the USB
bug or only its psymptom. Feedback from the linux-usb folks
is really appreciated.)
When plugging the Kvaser USB/CAN dongle the first time, everything
works as expected and all of the transfers from and to the USB
device succeeds.
Meanwhile, after unplugging the device and plugging it again, the
first bulk transfer _always_ returns an -ETIMEDOUT. The following
behaviour was observied:
- Setting higher timeout values for the first bulk transfer never
solved the issue.
- Unloading, then loading, our kvaser_usb module in question
__always__ solved the issue.
- Checking first bulk transfer status, and retry the transfer
again in case of an -ETIMEDOUT also __always__ solved the issue.
This is what the patch below does.
- In the testing done so far, this issue appears only on laptops
but never on PCs (possibly power related?)
Signed-off-by: Ahmed S. Darwish <redacted>
Does this patch apply apply between 3 and 4? If not, please re-arrange
the series. As this is a bug fix, patches 1, 2 and 4 will go via
net/master, 3 will go via net-next/master.
Since no one complained earlier, I guess this issue only affects
USBCAN devices. That's why I've based it above patch #3: adding
USBCAN hardware support.
Nonetheless, it won't do any harm for the current Leaf-only
driver. So _if_ this is the correct fix, I will update the commit
log, refactor the check into a 'do { } while()' loop, and then
base it above the Leaf-only net/master fixes on patch #1, and #2.
Any feedback on the USB side of things?
Maybe you have to change the subject of this patch to be more visible on
the USB list and/or add the right USB people on Cc.
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 |
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-12 11:06:07
On 01/11/2015 09:49 PM, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
We should not touch the packet after a netif_rx: it might
get freed behind our back.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <redacted>
Applied to can/master.
tnx,
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 |
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-12 11:09:42
On 01/11/2015 09:15 PM, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
Let the error counters be more accurate in case of Out of
Memory conditions.
Please have a look at kvaser_usb_rx_error(), the whole state handling is
omitted in case of OOM.
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 |
From: Ahmed S. Darwish <hidden> Date: 2015-01-12 11:20:40
Hi Marc,
On Sun, Jan 11, 2015 at 03:36:12PM -0500, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
Please delay applying this to as I've just discovered that
removal of two of the device family checks introduced two
un-necessary GCC warnings.
Will send and updated version soon.
Thanks,
Darwish
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-12 11:44:11
On 01/11/2015 09:36 PM, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <redacted>
See some minor comments inline.
Marc
quoted hunk
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 612 ++++++++++++++++++++++++++++++---------
2 files changed, 487 insertions(+), 133 deletions(-)
** V4 Changelog:
- Use type-safe C methods instead of cpp macros
- Further clarify the code and comments on error events channel arbitration
- Remove defensive checks against non-existing families
- Re-order methods to remove forward declarations
- Smaller stuff spotted by earlier review (function prefexes, etc.)
** V3 Changelog:
- Fix padding for the usbcan_msg_tx_acknowledge command
- Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
- Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
- Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
@@ -21,6 +23,15 @@#include<linux/can/dev.h>#include<linux/can/error.h>+/* Kvaser USB CAN dongles are divided into two major families:+*-Leaf:BasedonRenesasM32C,runningfirmwarelabeledas'filo'+*-UsbcanII:BasedonRenesasM16C,runningfirmwarelabeledas'helios'+*/+enumkvaser_usb_family{+KVASER_LEAF,+KVASER_USBCAN,+};
@@ -739,34 +902,48 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev, priv->can.can_stats.restarts++; }- if (error_factor) {- priv->can.can_stats.bus_error++;- stats->rx_errors++;-- cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;-- if (error_factor & M16C_EF_ACKE)- cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);- if (error_factor & M16C_EF_CRCE)- cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |- CAN_ERR_PROT_LOC_CRC_DEL);- if (error_factor & M16C_EF_FORME)- cf->data[2] |= CAN_ERR_PROT_FORM;- if (error_factor & M16C_EF_STFE)- cf->data[2] |= CAN_ERR_PROT_STUFF;- if (error_factor & M16C_EF_BITE0)- cf->data[2] |= CAN_ERR_PROT_BIT0;- if (error_factor & M16C_EF_BITE1)- cf->data[2] |= CAN_ERR_PROT_BIT1;- if (error_factor & M16C_EF_TRE)- cf->data[2] |= CAN_ERR_PROT_TX;+ switch (dev->family) {+ case KVASER_LEAF:+ if (es->leaf.error_factor) {+ priv->can.can_stats.bus_error++;+ stats->rx_errors++;++ cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;++ if (es->leaf.error_factor & M16C_EF_ACKE)+ cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);+ if (es->leaf.error_factor & M16C_EF_CRCE)+ cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |+ CAN_ERR_PROT_LOC_CRC_DEL);+ if (es->leaf.error_factor & M16C_EF_FORME)+ cf->data[2] |= CAN_ERR_PROT_FORM;+ if (es->leaf.error_factor & M16C_EF_STFE)+ cf->data[2] |= CAN_ERR_PROT_STUFF;+ if (es->leaf.error_factor & M16C_EF_BITE0)+ cf->data[2] |= CAN_ERR_PROT_BIT0;+ if (es->leaf.error_factor & M16C_EF_BITE1)+ cf->data[2] |= CAN_ERR_PROT_BIT1;+ if (es->leaf.error_factor & M16C_EF_TRE)+ cf->data[2] |= CAN_ERR_PROT_TX;+ }+ break;+ case KVASER_USBCAN:+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_TX_ERROR)+ stats->tx_errors++;+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_RX_ERROR)+ stats->rx_errors++;+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_BUSERROR) {+ priv->can.can_stats.bus_error++;+ cf->can_id |= CAN_ERR_BUSERROR;+ }+ break; }- cf->data[6] = txerr;- cf->data[7] = rxerr;+ cf->data[6] = es->txerr;+ cf->data[7] = es->rxerr;- priv->bec.txerr = txerr;- priv->bec.rxerr = rxerr;+ priv->bec.txerr = es->txerr;+ priv->bec.rxerr = es->rxerr; priv->can.state = new_state;
@@ -775,6 +952,124 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev, netif_rx(skb); }+/* For USBCAN, report error to userspace iff the channels's errors counter+ * has increased, or we're the only channel seeing a bus error state.+ */+static void kvaser_usbcan_conditionally_rx_error(const struct kvaser_usb *dev,+ struct kvaser_error_summary *es)
const struct kvaser_error_summary *es?
+{
+ struct kvaser_usb_net_priv *priv;
+ int channel;
+ bool report_error;
+
+ channel = es->channel;
+ if (channel >= dev->nchannels) {
+ dev_err(dev->udev->dev.parent,
+ "Invalid channel number (%d)\n", channel);
+ return;
+ }
+
+ priv = dev->nets[channel];
+ report_error = false;
+
+ if (es->txerr > priv->bec.txerr) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_TX_ERROR;
+ report_error = true;
+ }
+ if (es->rxerr > priv->bec.rxerr) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_RX_ERROR;
+ report_error = true;
+ }
+ if ((es->status & M16C_STATE_BUS_ERROR) &&
+ !(es->usbcan.other_ch_status & M16C_STATE_BUS_ERROR)) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_BUSERROR;
+ report_error = true;
+ }
+
+ if (report_error)
+ kvaser_usb_rx_error(dev, es);
+}
+
+static void kvaser_usbcan_rx_error(const struct kvaser_usb *dev,
+ const struct kvaser_msg *msg)
+{
+ struct kvaser_error_summary es = { };
+
+ switch (msg->id) {
+ /* Sometimes errors are sent as unsolicited chip state events */
+ case CMD_CHIP_STATE_EVENT:
+ es.channel = msg->u.usbcan.chip_state_event.channel;
+ es.status = msg->u.usbcan.chip_state_event.status;
+ es.txerr = msg->u.usbcan.chip_state_event.tx_errors_count;
+ es.rxerr = msg->u.usbcan.chip_state_event.rx_errors_count;
+ kvaser_usbcan_conditionally_rx_error(dev, &es);
+ break;
+
+ case CMD_CAN_ERROR_EVENT:
+ es.channel = 0;
+ es.status = msg->u.usbcan.error_event.status_ch0;
+ es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch0;
+ es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch0;
+ es.usbcan.other_ch_status =
+ msg->u.usbcan.error_event.status_ch1;
+ kvaser_usbcan_conditionally_rx_error(dev, &es);
+
+ /* The USBCAN firmware does not support more than 2 channels.
Does USBCAN support always 2 channels or are there models with 1
channels, too. I'd rephrase ..."does support up to 2 channels."
@@ -1327,7 +1664,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc); if (cf->can_id & CAN_RTR_FLAG)- msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;+ *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME; for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) { if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
@@ -1596,6 +1933,17 @@ static int kvaser_usb_probe(struct usb_interface *intf, if (!dev) return -ENOMEM;+ if (kvaser_is_leaf(id)) {+ dev->family = KVASER_LEAF;+ } else if (kvaser_is_usbcan(id)) {+ dev->family = KVASER_USBCAN;+ } else {+ dev_err(&intf->dev,+ "Product ID (%d) does not belong to any known Kvaser USB family",+ id->idProduct);+ return -ENODEV;+ }+ err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out); if (err) { dev_err(&intf->dev, "Cannot get usb endpoint(s)");
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 |
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-12 11:52:04
On 01/08/2015 04:19 PM, Ahmed S. Darwish wrote:
[...]
quoted
quoted
MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
@@ -463,7 +631,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev) if (err) return err;- dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);+ switch (dev->family) {+ case KVASER_LEAF:+ dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);+ break;+ case KVASER_USBCAN:+ dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);+ break;+ default:+ dev_err(dev->udev->dev.parent,+ "Invalid device family (%d)\n", dev->family);+ return -EINVAL;
The default case should not happen. I think you can remove it.
It's true, it _should_ never happen. But I only add such checks if
the follow-up code critically depends on a certain `dev->family`
behavior. So it's kind of a defensive check against any possible
bug in driver or memory.
What do you think?
The kernel is full of callback functions, if you have a bit flip there
you're in trouble anyways. A bug in the driver (or other parts of the
kernel) might overwrite the memory of dev->family, but if this happens,
more things will break.
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 |
@@ -21,6 +23,15 @@#include<linux/can/dev.h>#include<linux/can/error.h>+/* Kvaser USB CAN dongles are divided into two major families:+*-Leaf:BasedonRenesasM32C,runningfirmwarelabeledas'filo'+*-UsbcanII:BasedonRenesasM16C,runningfirmwarelabeledas'helios'+*/+enumkvaser_usb_family{+KVASER_LEAF,+KVASER_USBCAN,+};
Can you make "struct kvaser_error_summary *es" const?
Sure.
[...]
quoted
+/* For USBCAN, report error to userspace iff the channels's errors counter
+ * has increased, or we're the only channel seeing a bus error state.
+ */
+static void kvaser_usbcan_conditionally_rx_error(const struct kvaser_usb *dev,
+ struct kvaser_error_summary *es)
const struct kvaser_error_summary *es?
Ditto.
[...]
quoted
+
+ /* The USBCAN firmware does not support more than 2 channels.
Does USBCAN support always 2 channels or are there models with 1
channels, too. I'd rephrase ..."does support up to 2 channels."
Yes, but otherwise I get GCC warnings of 'rx_msg' possibly
being unused. I can add __maybe_unused to rx_msg of course,
but such annotation may hide possible errors in the future.
From: Ahmed S. Darwish <hidden> Date: 2015-01-12 12:26:26
On Mon, Jan 12, 2015 at 12:51:49PM +0100, Marc Kleine-Budde wrote:
On 01/08/2015 04:19 PM, Ahmed S. Darwish wrote:
[...]
quoted
quoted
quoted
MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
@@ -463,7 +631,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev) if (err) return err;- dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);+ switch (dev->family) {+ case KVASER_LEAF:+ dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);+ break;+ case KVASER_USBCAN:+ dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);+ break;+ default:+ dev_err(dev->udev->dev.parent,+ "Invalid device family (%d)\n", dev->family);+ return -EINVAL;
The default case should not happen. I think you can remove it.
quoted
It's true, it _should_ never happen. But I only add such checks if
the follow-up code critically depends on a certain `dev->family`
behavior. So it's kind of a defensive check against any possible
bug in driver or memory.
What do you think?
The kernel is full of callback functions, if you have a bit flip there
you're in trouble anyways. A bug in the driver (or other parts of the
kernel) might overwrite the memory of dev->family, but if this happens,
more things will break.
I see. Thanks for the explanation.
Most of them are now removed in latest submission, except two to
avoid GCC warnings of variables that "may be used uninitialized".
Regards,
Darwish
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-12 12:35:04
On 01/12/2015 01:26 PM, Ahmed S. Darwish wrote:
On Mon, Jan 12, 2015 at 12:51:49PM +0100, Marc Kleine-Budde wrote:
quoted
On 01/08/2015 04:19 PM, Ahmed S. Darwish wrote:
[...]
quoted
quoted
quoted
MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
@@ -463,7 +631,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev) if (err) return err;- dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);+ switch (dev->family) {+ case KVASER_LEAF:+ dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);+ break;+ case KVASER_USBCAN:+ dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);+ break;+ default:+ dev_err(dev->udev->dev.parent,+ "Invalid device family (%d)\n", dev->family);+ return -EINVAL;
The default case should not happen. I think you can remove it.
quoted
It's true, it _should_ never happen. But I only add such checks if
the follow-up code critically depends on a certain `dev->family`
behavior. So it's kind of a defensive check against any possible
bug in driver or memory.
What do you think?
The kernel is full of callback functions, if you have a bit flip there
you're in trouble anyways. A bug in the driver (or other parts of the
kernel) might overwrite the memory of dev->family, but if this happens,
more things will break.
I see. Thanks for the explanation.
Most of them are now removed in latest submission, except two to
avoid GCC warnings of variables that "may be used uninitialized".
Thanks, I'll look at the code later and try to figure out what gcc's
problem might be.
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 |
Yes, but otherwise I get GCC warnings of 'rx_msg' possibly
being unused. I can add __maybe_unused to rx_msg of course,
but such annotation may hide possible errors in the future.
Ah, what I meant is using uninitialized_var() to suppress the
GCC warning. But, really, using that macro has a bad history
of hiding errors in the future.
Kindly check http://lwn.net/Articles/529954/ for context.
Another solution might be initializing rx_msg to NULL.
Hello,
On Mon, Jan 12, 2015 at 05:14:07AM -0500, Ahmed S. Darwish wrote:
On Sun, Jan 11, 2015 at 09:51:10PM +0100, Marc Kleine-Budde wrote:
quoted
On 01/11/2015 09:45 PM, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
(This is a draft patch, I'm not sure if this fixes the USB
bug or only its psymptom. Feedback from the linux-usb folks
is really appreciated.)
When plugging the Kvaser USB/CAN dongle the first time, everything
works as expected and all of the transfers from and to the USB
device succeeds.
Meanwhile, after unplugging the device and plugging it again, the
first bulk transfer _always_ returns an -ETIMEDOUT. The following
behaviour was observied:
- Setting higher timeout values for the first bulk transfer never
solved the issue.
- Unloading, then loading, our kvaser_usb module in question
__always__ solved the issue.
- Checking first bulk transfer status, and retry the transfer
again in case of an -ETIMEDOUT also __always__ solved the issue.
This is what the patch below does.
- In the testing done so far, this issue appears only on laptops
but never on PCs (possibly power related?)
Signed-off-by: Ahmed S. Darwish <redacted>
Does this patch apply apply between 3 and 4? If not, please re-arrange
the series. As this is a bug fix, patches 1, 2 and 4 will go via
net/master, 3 will go via net-next/master.
Since no one complained earlier, I guess this issue only affects
USBCAN devices. That's why I've based it above patch #3: adding
USBCAN hardware support.
Nonetheless, it won't do any harm for the current Leaf-only
driver. So _if_ this is the correct fix, I will update the commit
log, refactor the check into a 'do { } while()' loop, and then
base it above the Leaf-only net/master fixes on patch #1, and #2.
Any feedback on the USB side of things?
Can you take a wireshark capture showing the problem?
It can maybe help people to figure out what happens.
What kind of usbcan device do you use?
Which firmware revision is loaded on the device?
Kr,
--
Olivier
From: Ahmed S. Darwish <hidden> Date: 2015-01-12 13:50:52
On Mon, Jan 12, 2015 at 02:33:30PM +0100, Olivier Sobrie wrote:
Hello,
On Mon, Jan 12, 2015 at 05:14:07AM -0500, Ahmed S. Darwish wrote:
quoted
On Sun, Jan 11, 2015 at 09:51:10PM +0100, Marc Kleine-Budde wrote:
quoted
On 01/11/2015 09:45 PM, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
(This is a draft patch, I'm not sure if this fixes the USB
bug or only its psymptom. Feedback from the linux-usb folks
is really appreciated.)
When plugging the Kvaser USB/CAN dongle the first time, everything
works as expected and all of the transfers from and to the USB
device succeeds.
Meanwhile, after unplugging the device and plugging it again, the
first bulk transfer _always_ returns an -ETIMEDOUT. The following
behaviour was observied:
- Setting higher timeout values for the first bulk transfer never
solved the issue.
- Unloading, then loading, our kvaser_usb module in question
__always__ solved the issue.
- Checking first bulk transfer status, and retry the transfer
again in case of an -ETIMEDOUT also __always__ solved the issue.
This is what the patch below does.
- In the testing done so far, this issue appears only on laptops
but never on PCs (possibly power related?)
Signed-off-by: Ahmed S. Darwish <redacted>
Does this patch apply apply between 3 and 4? If not, please re-arrange
the series. As this is a bug fix, patches 1, 2 and 4 will go via
net/master, 3 will go via net-next/master.
Since no one complained earlier, I guess this issue only affects
USBCAN devices. That's why I've based it above patch #3: adding
USBCAN hardware support.
Nonetheless, it won't do any harm for the current Leaf-only
driver. So _if_ this is the correct fix, I will update the commit
log, refactor the check into a 'do { } while()' loop, and then
base it above the Leaf-only net/master fixes on patch #1, and #2.
Any feedback on the USB side of things?
Can you take a wireshark capture showing the problem?
It can maybe help people to figure out what happens.
Yeah, I'm planning on doing something similar.
What kind of usbcan device do you use?
"Kvaser USBcan II HS/LS"
Which firmware revision is loaded on the device?
The device reports firmware version 2.9.410.
Interesting. The changelog of their latest firmware states
that it "fixed USB configuration issue during USB attach."
That might be the problem.
I have two devices here. I'll update the firmware only for
one of them and see what happens.
Thanks,
Darwish
Hello,
On Sun, Jan 11, 2015 at 03:36:12PM -0500, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <redacted>
Just two small remarks below.
Thanks,
Olivier
quoted hunk
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 612 ++++++++++++++++++++++++++++++---------
2 files changed, 487 insertions(+), 133 deletions(-)
** V4 Changelog:
- Use type-safe C methods instead of cpp macros
- Further clarify the code and comments on error events channel arbitration
- Remove defensive checks against non-existing families
- Re-order methods to remove forward declarations
- Smaller stuff spotted by earlier review (function prefexes, etc.)
** V3 Changelog:
- Fix padding for the usbcan_msg_tx_acknowledge command
- Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
- Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
- Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
@@ -21,6 +23,15 @@#include<linux/can/dev.h>#include<linux/can/error.h>+/* Kvaser USB CAN dongles are divided into two major families:+*-Leaf:BasedonRenesasM32C,runningfirmwarelabeledas'filo'+*-UsbcanII:BasedonRenesasM16C,runningfirmwarelabeledas'helios'+*/+enumkvaser_usb_family{+KVASER_LEAF,+KVASER_USBCAN,+};+#define MAX_TX_URBS 16#define MAX_RX_URBS 4#define START_TIMEOUT 1000 /* msecs */
Is this struct really needed? Can't you simply use
leaf_msg_tx_acknowledge or usbcan_msg_tx_acknowledge
structures to read the header.
Same for kvaser_msg_rx_can_header.
@@ -1327,7 +1664,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb, memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc); if (cf->can_id & CAN_RTR_FLAG)- msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;+ *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME; for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) { if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
@@ -1596,6 +1933,17 @@ static int kvaser_usb_probe(struct usb_interface *intf, if (!dev) return -ENOMEM;+ if (kvaser_is_leaf(id)) {+ dev->family = KVASER_LEAF;+ } else if (kvaser_is_usbcan(id)) {+ dev->family = KVASER_USBCAN;+ } else {+ dev_err(&intf->dev,+ "Product ID (%d) does not belong to any known Kvaser USB family",+ id->idProduct);+ return -ENODEV;+ }+ err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out); if (err) { dev_err(&intf->dev, "Cannot get usb endpoint(s)");
From: Ahmed S. Darwish <hidden> Date: 2015-01-12 20:37:00
On Mon, Jan 12, 2015 at 12:09:32PM +0100, Marc Kleine-Budde wrote:
On 01/11/2015 09:15 PM, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
Let the error counters be more accurate in case of Out of
Memory conditions.
Please have a look at kvaser_usb_rx_error(), the whole state handling is
omitted in case of OOM.
I see. Regarding kvaser_usb_rx_error(), would something like
below patch be acceptable?
Kindly note that separating recording interface state from
error frame packet building leads to duplication of a good
number of if-conditions. Meanwhile, it truly saves _all_
of the possible state before any ENOMEM -- the correct thing
to do.
Another solution was to allocate the can frame on the stack,
and thus avoiding any code duplication. But this only leads
to calls of "kvaser_usb_simple_msg_async", which can fail
with -ENOMEM by itself, returning to the very same problem
again.
If the patch is acceptable, I'll rebase my USBCAN-II driver
above it and re-submit the series (minus the merged patch).
Thanks,
-->
[ Patch is build-tested, but not _fully_ run-time tested.
It's based on linux-can/testing commit d642b49f6d84b94bd
"can: kvaser_usb: Don't dereference skb after a netif_rx" ]
Subject: [PATCH] can: kvaser_usb: Update net interface state
before exiting on OOM
From: Ahmed S. Darwish <redacted>
Let the network interface can bus state and error counters be
more accurate in case of Out of Memory conditions.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 182 +++++++++++++++++++++++----------------
1 file changed, 106 insertions(+), 76 deletions(-)
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-16 14:40:35
On 01/12/2015 09:36 PM, Ahmed S. Darwish wrote:
On Mon, Jan 12, 2015 at 12:09:32PM +0100, Marc Kleine-Budde wrote:
quoted
On 01/11/2015 09:15 PM, Ahmed S. Darwish wrote:
quoted
From: Ahmed S. Darwish <redacted>
Let the error counters be more accurate in case of Out of
Memory conditions.
Please have a look at kvaser_usb_rx_error(), the whole state handling is
omitted in case of OOM.
I see. Regarding kvaser_usb_rx_error(), would something like
below patch be acceptable?
Kindly note that separating recording interface state from
error frame packet building leads to duplication of a good
number of if-conditions. Meanwhile, it truly saves _all_
of the possible state before any ENOMEM -- the correct thing
to do.
Another solution was to allocate the can frame on the stack,
and thus avoiding any code duplication. But this only leads
to calls of "kvaser_usb_simple_msg_async", which can fail
with -ENOMEM by itself, returning to the very same problem
again.
If the patch is acceptable, I'll rebase my USBCAN-II driver
above it and re-submit the series (minus the merged patch).
Looks good from my point of view, stats and state are handled
independent of the error skb.
Andri can you have a look at the state handling itself?
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 |
They were added for completeness: the only gap in our continuous
sequence of command IDs from 12 to 39 ;-) No big deal, to be
removed in the next submission.
...
Is this struct really needed? Can't you simply use
leaf_msg_tx_acknowledge or usbcan_msg_tx_acknowledge
structures to read the header.
Same for kvaser_msg_rx_can_header.
They're added to ensure type-safety throughout the code. Basically
they're the common part of a command that has different wire format
between the Leaf and the USBCan, but share a common header. Such
notation was only added when it was strictly necessary.
For example, there are three functions where 'rx_can_header' is
referenced in the driver, and one function where 'tx_acknowledge_header'
is referenced. Without such header structure, I'll have to sprinkle
3 to 4 extra blocks of:
switch (dev->family) {
case KVASER_LEAF:
case KVASER_USBCAN:
}
which would be _really_ ugly. The *_header notation ensures that, in
the body of each function, we're accessing the fields in a very safe
manner.
Thanks,
Darwish
They were added for completeness: the only gap in our continuous
sequence of command IDs from 12 to 39 ;-) No big deal, to be
removed in the next submission.
...
Is this struct really needed? Can't you simply use
leaf_msg_tx_acknowledge or usbcan_msg_tx_acknowledge
structures to read the header.
Same for kvaser_msg_rx_can_header.
They're added to ensure type-safety throughout the code. Basically
they're the common part of a command that has different wire format
between the Leaf and the USBCan, but share a common header. Such
notation was only added when it was strictly necessary.
For example, there are three functions where 'rx_can_header' is
referenced in the driver, and one function where 'tx_acknowledge_header'
is referenced. Without such header structure, I'll have to sprinkle
3 to 4 extra blocks of:
switch (dev->family) {
case KVASER_LEAF:
case KVASER_USBCAN:
}
which would be _really_ ugly. The *_header notation ensures that, in
the body of each function, we're accessing the fields in a very safe
manner.
+1 Keep it as it is.
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 |
From: Ahmed S. Darwish <hidden> Date: 2015-01-20 21:44:18
From: Ahmed S. Darwish <redacted>
Update all of the can interface's state and error counters before
trying any skb allocation that can actually fail with -ENOMEM.
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 182 +++++++++++++++++++++++----------------
1 file changed, 106 insertions(+), 76 deletions(-)
@@ -653,122 +706,99 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,return;}-if(channel>=dev->nchannels){+if(es.channel>=dev->nchannels){dev_err(dev->udev->dev.parent,-"Invalid channel number (%d)\n",channel);+"Invalid channel number (%d)\n",es.channel);return;}-priv=dev->nets[channel];+priv=dev->nets[es.channel];stats=&priv->netdev->stats;-if(status&M16C_STATE_BUS_RESET){+if(es.status&M16C_STATE_BUS_RESET){kvaser_usb_unlink_tx_urbs(priv);return;}+/* Update all of the can interface's state and error counters before+*tryinganyskballocationthatcanactuallyfailwith-ENOMEM.+*/+old_state=priv->can.state;+kvaser_usb_rx_error_update_can_state(priv,&es);+skb=alloc_can_err_skb(priv->netdev,&cf);if(!skb){stats->rx_dropped++;return;}-new_state=priv->can.state;--netdev_dbg(priv->netdev,"Error status: 0x%02x\n",status);--if(status&M16C_STATE_BUS_OFF){+if(es.status&M16C_STATE_BUS_OFF){cf->can_id|=CAN_ERR_BUSOFF;-priv->can.can_stats.bus_off++;if(!priv->can.restart_ms)kvaser_usb_simple_msg_async(priv,CMD_STOP_CHIP);-netif_carrier_off(priv->netdev);--new_state=CAN_STATE_BUS_OFF;-}elseif(status&M16C_STATE_BUS_PASSIVE){-if(priv->can.state!=CAN_STATE_ERROR_PASSIVE){+}elseif(es.status&M16C_STATE_BUS_PASSIVE){+if(old_state!=CAN_STATE_ERROR_PASSIVE){cf->can_id|=CAN_ERR_CRTL;-if(txerr||rxerr)-cf->data[1]=(txerr>rxerr)+if(es.txerr||es.rxerr)+cf->data[1]=(es.txerr>es.rxerr)?CAN_ERR_CRTL_TX_PASSIVE:CAN_ERR_CRTL_RX_PASSIVE;elsecf->data[1]=CAN_ERR_CRTL_TX_PASSIVE|CAN_ERR_CRTL_RX_PASSIVE;--priv->can.can_stats.error_passive++;}--new_state=CAN_STATE_ERROR_PASSIVE;}-if(status==M16C_STATE_BUS_ERROR){-if((priv->can.state<CAN_STATE_ERROR_WARNING)&&-((txerr>=96)||(rxerr>=96))){+if(es.status==M16C_STATE_BUS_ERROR){+if((old_state<CAN_STATE_ERROR_WARNING)&&+((es.txerr>=96)||(es.rxerr>=96))){cf->can_id|=CAN_ERR_CRTL;-cf->data[1]=(txerr>rxerr)+cf->data[1]=(es.txerr>es.rxerr)?CAN_ERR_CRTL_TX_WARNING:CAN_ERR_CRTL_RX_WARNING;--priv->can.can_stats.error_warning++;-new_state=CAN_STATE_ERROR_WARNING;-}elseif(priv->can.state>CAN_STATE_ERROR_ACTIVE){+}elseif(old_state>CAN_STATE_ERROR_ACTIVE){cf->can_id|=CAN_ERR_PROT;cf->data[2]=CAN_ERR_PROT_ACTIVE;--new_state=CAN_STATE_ERROR_ACTIVE;}}-if(!status){+if(!es.status){cf->can_id|=CAN_ERR_PROT;cf->data[2]=CAN_ERR_PROT_ACTIVE;--new_state=CAN_STATE_ERROR_ACTIVE;}if(priv->can.restart_ms&&-(priv->can.state>=CAN_STATE_BUS_OFF)&&-(new_state<CAN_STATE_BUS_OFF)){+(old_state>=CAN_STATE_BUS_OFF)&&+(priv->can.state<CAN_STATE_BUS_OFF)){cf->can_id|=CAN_ERR_RESTARTED;netif_carrier_on(priv->netdev);--priv->can.can_stats.restarts++;}-if(error_factor){-priv->can.can_stats.bus_error++;-stats->rx_errors++;-+if(es.error_factor){cf->can_id|=CAN_ERR_BUSERROR|CAN_ERR_PROT;-if(error_factor&M16C_EF_ACKE)+if(es.error_factor&M16C_EF_ACKE)cf->data[3]|=(CAN_ERR_PROT_LOC_ACK);-if(error_factor&M16C_EF_CRCE)+if(es.error_factor&M16C_EF_CRCE)cf->data[3]|=(CAN_ERR_PROT_LOC_CRC_SEQ|CAN_ERR_PROT_LOC_CRC_DEL);-if(error_factor&M16C_EF_FORME)+if(es.error_factor&M16C_EF_FORME)cf->data[2]|=CAN_ERR_PROT_FORM;-if(error_factor&M16C_EF_STFE)+if(es.error_factor&M16C_EF_STFE)cf->data[2]|=CAN_ERR_PROT_STUFF;-if(error_factor&M16C_EF_BITE0)+if(es.error_factor&M16C_EF_BITE0)cf->data[2]|=CAN_ERR_PROT_BIT0;-if(error_factor&M16C_EF_BITE1)+if(es.error_factor&M16C_EF_BITE1)cf->data[2]|=CAN_ERR_PROT_BIT1;-if(error_factor&M16C_EF_TRE)+if(es.error_factor&M16C_EF_TRE)cf->data[2]|=CAN_ERR_PROT_TX;}-cf->data[6]=txerr;-cf->data[7]=rxerr;--priv->bec.txerr=txerr;-priv->bec.rxerr=rxerr;--priv->can.state=new_state;+cf->data[6]=es.txerr;+cf->data[7]=es.rxerr;stats->rx_packets++;stats->rx_bytes+=cf->can_dlc;
From: Ahmed S. Darwish <hidden> Date: 2015-01-20 21:45:44
From: Ahmed S. Darwish <redacted>
Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.
Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
1 file changed, 55 insertions(+), 59 deletions(-)
@@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,}/* Update all of the can interface's state and error counters before-*tryinganyskballocationthatcanactuallyfailwith-ENOMEM.+*tryinganymemoryallocationthatcanactuallyfailwith-ENOMEM.+*+*Wesendatemporarystack-allocatederrorcanframeto+*can_change_state()fortheverysamereason.+*+*TODO:Splitcan_change_state()responsibilitybetweenupdatingthe+*caninterface'sstateandcounters,andthesettingupofcanerror+*frameIDanddatatouserspace.Removestackallocationafterwards.*/old_state=priv->can.state;-kvaser_usb_rx_error_update_can_state(priv,&es);+kvaser_usb_rx_error_update_can_state(priv,&es,&tmp_cf);+new_state=priv->can.state;skb=alloc_can_err_skb(priv->netdev,&cf);if(!skb){stats->rx_dropped++;return;}+memcpy(cf,&tmp_cf,sizeof(*cf));-if(es.status&M16C_STATE_BUS_OFF){-cf->can_id|=CAN_ERR_BUSOFF;--if(!priv->can.restart_ms)-kvaser_usb_simple_msg_async(priv,CMD_STOP_CHIP);-netif_carrier_off(priv->netdev);-}elseif(es.status&M16C_STATE_BUS_PASSIVE){-if(old_state!=CAN_STATE_ERROR_PASSIVE){-cf->can_id|=CAN_ERR_CRTL;--if(es.txerr||es.rxerr)-cf->data[1]=(es.txerr>es.rxerr)-?CAN_ERR_CRTL_TX_PASSIVE-:CAN_ERR_CRTL_RX_PASSIVE;-else-cf->data[1]=CAN_ERR_CRTL_TX_PASSIVE|-CAN_ERR_CRTL_RX_PASSIVE;+if(new_state!=old_state){+if(es.status&M16C_STATE_BUS_OFF){+if(!priv->can.restart_ms)+kvaser_usb_simple_msg_async(priv,CMD_STOP_CHIP);+netif_carrier_off(priv->netdev);+}++if(es.status==M16C_STATE_BUS_ERROR){+if((old_state>=CAN_STATE_ERROR_WARNING)||+(es.txerr<96&&es.rxerr<96)){+if(old_state>CAN_STATE_ERROR_ACTIVE){+cf->can_id|=CAN_ERR_PROT;+cf->data[2]=CAN_ERR_PROT_ACTIVE;+}+}}-}-if(es.status==M16C_STATE_BUS_ERROR){-if((old_state<CAN_STATE_ERROR_WARNING)&&-((es.txerr>=96)||(es.rxerr>=96))){-cf->can_id|=CAN_ERR_CRTL;-cf->data[1]=(es.txerr>es.rxerr)-?CAN_ERR_CRTL_TX_WARNING-:CAN_ERR_CRTL_RX_WARNING;-}elseif(old_state>CAN_STATE_ERROR_ACTIVE){+if(!es.status){cf->can_id|=CAN_ERR_PROT;cf->data[2]=CAN_ERR_PROT_ACTIVE;}-}-if(!es.status){-cf->can_id|=CAN_ERR_PROT;-cf->data[2]=CAN_ERR_PROT_ACTIVE;-}--if(priv->can.restart_ms&&-(old_state>=CAN_STATE_BUS_OFF)&&-(priv->can.state<CAN_STATE_BUS_OFF)){-cf->can_id|=CAN_ERR_RESTARTED;-netif_carrier_on(priv->netdev);+if(priv->can.restart_ms&&+(old_state>=CAN_STATE_BUS_OFF)&&+(new_state<CAN_STATE_BUS_OFF)){+cf->can_id|=CAN_ERR_RESTARTED;+netif_carrier_on(priv->netdev);+}}if(es.error_factor){
From: Ahmed S. Darwish <hidden> Date: 2015-01-20 21:47:16
From: Ahmed S. Darwish <redacted>
While being in an ERROR_WARNING state and receiving further
bus error events with error counts in the range of 97-127 inclusive,
the state handling code erroneously reverts back to ERROR_ACTIVE.
As per the CAN standard recommendations, only revert to ERROR_ACTIVE
when the error counters are less than 96.
Moreover, in certain Kvaser models, the BUS_ERROR flag is always set
along with undefined bits in the M16C status register. Thus use
bitwise ops instead of full equality for checking the register
against bus errors.
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
From: Ahmed S. Darwish <hidden> Date: 2015-01-20 21:48:41
From: Ahmed S. Darwish <redacted>
On some x86 laptops, plugging a Kvaser device again after an
unplug makes the firmware always ignore the very first command.
For such a case, provide some room for retries instead of
completly exiting the driver init code.
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
@@ -1614,7 +1614,7 @@ static int kvaser_usb_probe(struct usb_interface *intf,{structkvaser_usb*dev;interr=-ENOMEM;-inti;+inti,retry=3;dev=devm_kzalloc(&intf->dev,sizeof(*dev),GFP_KERNEL);if(!dev)
@@ -1632,7 +1632,15 @@ static int kvaser_usb_probe(struct usb_interface *intf,usb_set_intfdata(intf,dev);-err=kvaser_usb_get_software_info(dev);+/* On some x86 laptops, plugging a Kvaser device again after+*anunplugmakesthefirmwarealwaysignoretheveryfirst+*command.Forsuchacase,providesomeroomforretries+*insteadofcompletlyexitingthedriver.+*/+do{+err=kvaser_usb_get_software_info(dev);+}while(--retry&&err==-ETIMEDOUT);+if(err){dev_err(&intf->dev,"Cannot get software infos, error %d\n",err);
From: Ahmed S. Darwish <hidden> Date: 2015-01-20 21:50:34
From: Ahmed S. Darwish <redacted>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'USBcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the USBcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 598 ++++++++++++++++++++++++++++++---------
2 files changed, 478 insertions(+), 128 deletions(-)
** V5 Changelog:
- Rebase on the new CAN error state changes added for the Leaf driver
- Add minor changes (remove unused commands, constify poniters, etc.)
** V4 Changelog:
- Use type-safe C methods instead of cpp macros
- Remove defensive checks against non-existing families
- Re-order methods to remove forward declarations
- Smaller stuff spotted by earlier review (function prefexes, etc.)
** V3 Changelog:
- Fix padding for the usbcan_msg_tx_acknowledge command
- Remove kvaser_usb->max_channels and the MAX_NET_DEVICES macro
- Rename commands to CMD_LEAF_xxx and CMD_USBCAN_xxx
- Apply checkpatch.pl suggestions ('net/' comments, multi-line strings, etc.)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
@@ -137,9 +171,18 @@#define KVASER_CTRL_MODE_SELFRECEPTION 3#define KVASER_CTRL_MODE_OFF 4-/* log message */+/* Extended CAN identifier flag */#define KVASER_EXTENDED_FRAME BIT(31)+/* Kvaser USB CAN dongles are divided into two major families:+*-Leaf:BasedonRenesasM32C,runningfirmwarelabeledas'filo'+*-UsbcanII:BasedonRenesasM16C,runningfirmwarelabeledas'helios'+*/+enumkvaser_usb_family{+KVASER_LEAF,+KVASER_USBCAN,+};+structkvaser_msg_simple{u8tid;u8channel;
@@ -260,21 +390,55 @@ struct kvaser_msg {structkvaser_msg_simplesimple;structkvaser_msg_cardinfocardinfo;structkvaser_msg_cardinfo2cardinfo2;-structkvaser_msg_softinfosoftinfo;structkvaser_msg_busparamsbusparams;++structkvaser_msg_rx_can_headerrx_can_header;+structkvaser_msg_tx_acknowledge_headertx_acknowledge_header;++union{+structleaf_msg_softinfosoftinfo;+structleaf_msg_rx_canrx_can;+structleaf_msg_chip_state_eventchip_state_event;+structleaf_msg_tx_acknowledgetx_acknowledge;+structleaf_msg_error_eventerror_event;+structleaf_msg_log_messagelog_message;+}__packedleaf;++union{+structusbcan_msg_softinfosoftinfo;+structusbcan_msg_rx_canrx_can;+structusbcan_msg_chip_state_eventchip_state_event;+structusbcan_msg_tx_acknowledgetx_acknowledge;+structusbcan_msg_error_eventerror_event;+}__packedusbcan;+structkvaser_msg_tx_cantx_can;-structkvaser_msg_rx_canrx_can;-structkvaser_msg_chip_state_eventchip_state_event;-structkvaser_msg_tx_acknowledgetx_acknowledge;-structkvaser_msg_error_eventerror_event;structkvaser_msg_ctrl_modectrl_mode;structkvaser_msg_flush_queueflush_queue;-structkvaser_msg_log_messagelog_message;}u;}__packed;+/* Summary of a kvaser error event, for a unified Leaf/Usbcan error+*handling.Somediscrepanciesbetweenthetwofamiliesexist:+*+*-USBCANfirmwaredoesnotreportM16C"error factors"+*-USBCANcontrollershasdifficultiesreportingiftheraisederror+*eventisforch0orch1.TheyleavesucharbitrationtotheOS+*driverbylettingitcompareerrorcounterswithpreviousvalues+*anddecidetheerrorevent'schannel.ThusforUSBCAN,thechannel+*fieldisonlyadvisory.+*/structkvaser_usb_error_summary{-u8channel,status,txerr,rxerr,error_factor;+u8channel,status,txerr,rxerr;+union{+struct{+u8error_factor;+}leaf;+struct{+u8other_ch_status;+u8error_state;+}usbcan;+};};structkvaser_usb_tx_urb_context{
@@ -771,34 +943,161 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,}}-if(es.error_factor){-cf->can_id|=CAN_ERR_BUSERROR|CAN_ERR_PROT;--if(es.error_factor&M16C_EF_ACKE)-cf->data[3]|=(CAN_ERR_PROT_LOC_ACK);-if(es.error_factor&M16C_EF_CRCE)-cf->data[3]|=(CAN_ERR_PROT_LOC_CRC_SEQ|-CAN_ERR_PROT_LOC_CRC_DEL);-if(es.error_factor&M16C_EF_FORME)-cf->data[2]|=CAN_ERR_PROT_FORM;-if(es.error_factor&M16C_EF_STFE)-cf->data[2]|=CAN_ERR_PROT_STUFF;-if(es.error_factor&M16C_EF_BITE0)-cf->data[2]|=CAN_ERR_PROT_BIT0;-if(es.error_factor&M16C_EF_BITE1)-cf->data[2]|=CAN_ERR_PROT_BIT1;-if(es.error_factor&M16C_EF_TRE)-cf->data[2]|=CAN_ERR_PROT_TX;+switch(dev->family){+caseKVASER_LEAF:+if(es->leaf.error_factor){+cf->can_id|=CAN_ERR_BUSERROR|CAN_ERR_PROT;++if(es->leaf.error_factor&M16C_EF_ACKE)+cf->data[3]|=(CAN_ERR_PROT_LOC_ACK);+if(es->leaf.error_factor&M16C_EF_CRCE)+cf->data[3]|=(CAN_ERR_PROT_LOC_CRC_SEQ|+CAN_ERR_PROT_LOC_CRC_DEL);+if(es->leaf.error_factor&M16C_EF_FORME)+cf->data[2]|=CAN_ERR_PROT_FORM;+if(es->leaf.error_factor&M16C_EF_STFE)+cf->data[2]|=CAN_ERR_PROT_STUFF;+if(es->leaf.error_factor&M16C_EF_BITE0)+cf->data[2]|=CAN_ERR_PROT_BIT0;+if(es->leaf.error_factor&M16C_EF_BITE1)+cf->data[2]|=CAN_ERR_PROT_BIT1;+if(es->leaf.error_factor&M16C_EF_TRE)+cf->data[2]|=CAN_ERR_PROT_TX;+}+break;+caseKVASER_USBCAN:+if(es->usbcan.error_state&USBCAN_ERROR_STATE_BUSERROR){+cf->can_id|=CAN_ERR_BUSERROR;+}+break;}-cf->data[6]=es.txerr;-cf->data[7]=es.rxerr;+cf->data[6]=es->txerr;+cf->data[7]=es->rxerr;stats->rx_packets++;stats->rx_bytes+=cf->can_dlc;netif_rx(skb);}+/* For USBCAN, report error to userspace iff the channels's errors counter+*hasincreased,orwe'retheonlychannelseeingabuserrorstate.+*/+staticvoidkvaser_usbcan_conditionally_rx_error(conststructkvaser_usb*dev,+structkvaser_usb_error_summary*es)+{+structkvaser_usb_net_priv*priv;+intchannel;+boolreport_error;++channel=es->channel;+if(channel>=dev->nchannels){+dev_err(dev->udev->dev.parent,+"Invalid channel number (%d)\n",channel);+return;+}++priv=dev->nets[channel];+report_error=false;++if(es->txerr>priv->bec.txerr){+es->usbcan.error_state|=USBCAN_ERROR_STATE_TX_ERROR;+report_error=true;+}+if(es->rxerr>priv->bec.rxerr){+es->usbcan.error_state|=USBCAN_ERROR_STATE_RX_ERROR;+report_error=true;+}+if((es->status&M16C_STATE_BUS_ERROR)&&+!(es->usbcan.other_ch_status&M16C_STATE_BUS_ERROR)){+es->usbcan.error_state|=USBCAN_ERROR_STATE_BUSERROR;+report_error=true;+}++if(report_error)+kvaser_usb_rx_error(dev,es);+}++staticvoidkvaser_usbcan_rx_error(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_usb_error_summaryes={};++switch(msg->id){+/* Sometimes errors are sent as unsolicited chip state events */+caseCMD_CHIP_STATE_EVENT:+es.channel=msg->u.usbcan.chip_state_event.channel;+es.status=msg->u.usbcan.chip_state_event.status;+es.txerr=msg->u.usbcan.chip_state_event.tx_errors_count;+es.rxerr=msg->u.usbcan.chip_state_event.rx_errors_count;+kvaser_usbcan_conditionally_rx_error(dev,&es);+break;++caseCMD_CAN_ERROR_EVENT:+es.channel=0;+es.status=msg->u.usbcan.error_event.status_ch0;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch0;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch0;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch1;+kvaser_usbcan_conditionally_rx_error(dev,&es);++/* The USBCAN firmware supports up to 2 channels.+*Nowthatch0waschecked,checkifch1hasanyerrors.+*/+if(dev->nchannels==MAX_USBCAN_NET_DEVICES){+es.channel=1;+es.status=msg->u.usbcan.error_event.status_ch1;+es.txerr=msg->u.usbcan.error_event.tx_errors_count_ch1;+es.rxerr=msg->u.usbcan.error_event.rx_errors_count_ch1;+es.usbcan.other_ch_status=+msg->u.usbcan.error_event.status_ch0;+kvaser_usbcan_conditionally_rx_error(dev,&es);+}+break;++default:+dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",+msg->id);+}+}++staticvoidkvaser_leaf_rx_error(conststructkvaser_usb*dev,+conststructkvaser_msg*msg)+{+structkvaser_usb_error_summaryes={};++switch(msg->id){+caseCMD_CAN_ERROR_EVENT:+es.channel=msg->u.leaf.error_event.channel;+es.status=msg->u.leaf.error_event.status;+es.txerr=msg->u.leaf.error_event.tx_errors_count;+es.rxerr=msg->u.leaf.error_event.rx_errors_count;+es.leaf.error_factor=msg->u.leaf.error_event.error_factor;+break;+caseCMD_LEAF_LOG_MESSAGE:+es.channel=msg->u.leaf.log_message.channel;+es.status=msg->u.leaf.log_message.data[0];+es.txerr=msg->u.leaf.log_message.data[2];+es.rxerr=msg->u.leaf.log_message.data[3];+es.leaf.error_factor=msg->u.leaf.log_message.data[1];+break;+caseCMD_CHIP_STATE_EVENT:+es.channel=msg->u.leaf.chip_state_event.channel;+es.status=msg->u.leaf.chip_state_event.status;+es.txerr=msg->u.leaf.chip_state_event.tx_errors_count;+es.rxerr=msg->u.leaf.chip_state_event.rx_errors_count;+es.leaf.error_factor=0;+break;+default:+dev_err(dev->udev->dev.parent,"Invalid msg id (%d)\n",+msg->id);+return;+}++kvaser_usb_rx_error(dev,&es);+}+staticvoidkvaser_usb_rx_can_err(conststructkvaser_usb_net_priv*priv,conststructkvaser_msg*msg){
@@ -1620,6 +1953,17 @@ static int kvaser_usb_probe(struct usb_interface *intf,if(!dev)return-ENOMEM;+if(kvaser_is_leaf(id)){+dev->family=KVASER_LEAF;+}elseif(kvaser_is_usbcan(id)){+dev->family=KVASER_USBCAN;+}else{+dev_err(&intf->dev,+"Product ID (%d) does not belong to any known Kvaser USB family",+id->idProduct);+return-ENODEV;+}+err=kvaser_usb_get_endpoints(intf,&dev->bulk_in,&dev->bulk_out);if(err){dev_err(&intf->dev,"Cannot get usb endpoint(s)");
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-21 10:45:13
On 01/21/2015 11:33 AM, Andri Yngvason wrote:
Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
quoted
From: Ahmed S. Darwish <redacted>
Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.
Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
1 file changed, 55 insertions(+), 59 deletions(-)
Is this an Acked-by?
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 |
From: Ahmed S. Darwish <redacted>
Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.
Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
1 file changed, 55 insertions(+), 59 deletions(-)
@@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,}/* Update all of the can interface's state and error counters before-*tryinganyskballocationthatcanactuallyfailwith-ENOMEM.+*tryinganymemoryallocationthatcanactuallyfailwith-ENOMEM.+*+*Wesendatemporarystack-allocatederrorcanframeto+*can_change_state()fortheverysamereason.+*+*TODO:Splitcan_change_state()responsibilitybetweenupdatingthe+*caninterface'sstateandcounters,andthesettingupofcanerror+*frameIDanddatatouserspace.Removestackallocationafterwards.*/old_state=priv->can.state;-kvaser_usb_rx_error_update_can_state(priv,&es);+kvaser_usb_rx_error_update_can_state(priv,&es,&tmp_cf);+new_state=priv->can.state;skb=alloc_can_err_skb(priv->netdev,&cf);if(!skb){stats->rx_dropped++;return;}+memcpy(cf,&tmp_cf,sizeof(*cf));-if(es.status&M16C_STATE_BUS_OFF){-cf->can_id|=CAN_ERR_BUSOFF;--if(!priv->can.restart_ms)-kvaser_usb_simple_msg_async(priv,CMD_STOP_CHIP);-netif_carrier_off(priv->netdev);-}elseif(es.status&M16C_STATE_BUS_PASSIVE){-if(old_state!=CAN_STATE_ERROR_PASSIVE){-cf->can_id|=CAN_ERR_CRTL;--if(es.txerr||es.rxerr)-cf->data[1]=(es.txerr>es.rxerr)-?CAN_ERR_CRTL_TX_PASSIVE-:CAN_ERR_CRTL_RX_PASSIVE;-else-cf->data[1]=CAN_ERR_CRTL_TX_PASSIVE|-CAN_ERR_CRTL_RX_PASSIVE;+if(new_state!=old_state){+if(es.status&M16C_STATE_BUS_OFF){+if(!priv->can.restart_ms)+kvaser_usb_simple_msg_async(priv,CMD_STOP_CHIP);+netif_carrier_off(priv->netdev);+}++if(es.status==M16C_STATE_BUS_ERROR){+if((old_state>=CAN_STATE_ERROR_WARNING)||+(es.txerr<96&&es.rxerr<96)){+if(old_state>CAN_STATE_ERROR_ACTIVE){+cf->can_id|=CAN_ERR_PROT;+cf->data[2]=CAN_ERR_PROT_ACTIVE;+}+}}-}-if(es.status==M16C_STATE_BUS_ERROR){-if((old_state<CAN_STATE_ERROR_WARNING)&&-((es.txerr>=96)||(es.rxerr>=96))){-cf->can_id|=CAN_ERR_CRTL;-cf->data[1]=(es.txerr>es.rxerr)-?CAN_ERR_CRTL_TX_WARNING-:CAN_ERR_CRTL_RX_WARNING;-}elseif(old_state>CAN_STATE_ERROR_ACTIVE){+if(!es.status){cf->can_id|=CAN_ERR_PROT;cf->data[2]=CAN_ERR_PROT_ACTIVE;}-}-if(!es.status){-cf->can_id|=CAN_ERR_PROT;-cf->data[2]=CAN_ERR_PROT_ACTIVE;-}--if(priv->can.restart_ms&&-(old_state>=CAN_STATE_BUS_OFF)&&-(priv->can.state<CAN_STATE_BUS_OFF)){-cf->can_id|=CAN_ERR_RESTARTED;-netif_carrier_on(priv->netdev);+if(priv->can.restart_ms&&+(old_state>=CAN_STATE_BUS_OFF)&&+(new_state<CAN_STATE_BUS_OFF)){+cf->can_id|=CAN_ERR_RESTARTED;+netif_carrier_on(priv->netdev);+}}if(es.error_factor){
From: Ahmed S. Darwish <redacted>
Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.
Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
1 file changed, 55 insertions(+), 59 deletions(-)
From: Wolfgang Grandegger <hidden> Date: 2015-01-21 12:03:09
On Wed, 21 Jan 2015 10:33:19 +0000, Andri Yngvason
[off-list ref] wrote:
Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
quoted
From: Ahmed S. Darwish <redacted>
Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.
Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 114
+++++++++++++++++++--------------------
1 file changed, 55 insertions(+), 59 deletions(-)
diff --git a/drivers/net/can/usb/kvaser_usb.c
b/drivers/net/can/usb/kvaser_usb.c
index 971c5f9..0386d3f 100644
Would be nice to see some "candump" traces as well. Ahmed, could you
please generate such traces doing:
1. Execute in a session:
# candump -t d -e any,0:0,#FFFFFFFF
2. Execute in another session:
# cangen -g 10 -D i can0
3. Disconnect the CAN cable
4. After a while reconnect the CAN cable
5. Stop candump and save the trace.
and to test bus-off:
1. Execute in a session:
# candump -t d -e any,0:0,#FFFFFFFF
2. Execute in another session:
# cangen -g 10 -D i can0
3. Short-circuit the CAN low and high wires of the CAN cable
4. After a while remove the short-circuit.
5. Stop candump and save the trace.
Thanks,
Wolfgang.
From: Sergei Shtylyov <hidden> Date: 2015-01-21 12:24:56
Hello.
On 1/21/2015 12:48 AM, Ahmed S. Darwish wrote:
From: Ahmed S. Darwish <redacted>
On some x86 laptops, plugging a Kvaser device again after an
unplug makes the firmware always ignore the very first command.
For such a case, provide some room for retries instead of
completly exiting the driver init code.
Completely.
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
@@ -1632,7 +1632,15 @@ static int kvaser_usb_probe(struct usb_interface *intf, usb_set_intfdata(intf, dev);- err = kvaser_usb_get_software_info(dev);+ /* On some x86 laptops, plugging a Kvaser device again after+ * an unplug makes the firmware always ignore the very first+ * command. For such a case, provide some room for retries+ * instead of completly exiting the driver.
From: Ahmed S. Darwish <hidden> Date: 2015-01-21 14:43:36
Hi!
On Wed, Jan 21, 2015 at 12:53:58PM +0100, Wolfgang Grandegger wrote:
On Wed, 21 Jan 2015 10:33:19 +0000, Andri Yngvason
[off-list ref] wrote:
quoted
Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
quoted
From: Ahmed S. Darwish <redacted>
Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.
Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 114
+++++++++++++++++++--------------------
1 file changed, 55 insertions(+), 59 deletions(-)
diff --git a/drivers/net/can/usb/kvaser_usb.c
b/drivers/net/can/usb/kvaser_usb.c
index 971c5f9..0386d3f 100644
Hi!
On Wed, Jan 21, 2015 at 12:53:58PM +0100, Wolfgang Grandegger wrote:
quoted
On Wed, 21 Jan 2015 10:33:19 +0000, Andri Yngvason
[off-list ref] wrote:
quoted
Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
quoted
From: Ahmed S. Darwish <redacted>
Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.
Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 114
+++++++++++++++++++--------------------
1 file changed, 55 insertions(+), 59 deletions(-)
diff --git a/drivers/net/can/usb/kvaser_usb.c
b/drivers/net/can/usb/kvaser_usb.c
index 971c5f9..0386d3f 100644
Would be nice to see some "candump" traces as well.
Sure. The USBCan-II device trace below is generated after applying
all patches in the series, especially patch #3, which fixes some
some invalid CAN state transitions logic in the original driver.
bus-error
error-counter-tx-rx{{128}{0}}
(000.001868) can0 20000080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(000.001982) can0 20000080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(( Then a continous flood, exactly similar to the above packet, appears.
Unfortunately this flooding is a firmware problem. ))
<-- Replug the cable, after a good amount of time -->
Bus-off seems OK. You could have just short circuited them without
disconnecting.
Reverse state transitions are missing from the logs. See comments above.
--
Andri
(000.001868) can0 20000080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(000.001982) can0 20000080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(( Then a continous flood, exactly similar to the above packet, appears.
Unfortunately this flooding is a firmware problem. ))
<-- Replug the cable, after a good amount of time -->
Where are the reverse state transitions?
quoted
Hmmm...
[ ... ]
Reverse state transitions are missing from the logs. See comments above.
When the device is on the _receiving_ end, and I unplug the CAN cable after
introducing some noise to the level of reaching WARNING or PASSIVE, I
receive a BUS_ERROR event with the rxerr count reset back to 0 or 1. In
that case, the driver correctly transitions back the state to ERROR_ACTIVE
and candump produces something similar to:
(000.000362) can0 2000008C [8] 00 40 40 00 00 00 00 01 ERRORFRAME
controller-problem{}
protocol-violation{{back-to-error-active}{}}
bus-error
error-counter-tx-rx{{0}{1}}
which is, AFAIK, the correct behaviour from the driver side.
Meanwhile, when the device is on the _sending_ end and I re-plug the CAN
cable again. Sometimes I receive events with txerr reset to 0 or 1, and
the driver correctly reverts back to ERROR_ACTIVE in that case. But on
another times like the quoted case above, I don't receive any events
resetting txerr back -- only data packets on the bus.
So, What can the driver do given the above?
Thanks,
Darwish
P.S. just in case, I'll also re-check now if the driver unintentionally
drops any important events resetting the txerr count back after a CAN
cable replug -- preventing the code from returning to ERROR_ACTIVE in
the process.
(000.001868) can0 20000080 [8] 00 00 00 00 00 00 80 00
ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(000.001982) can0 20000080 [8] 00 00 00 00 00 00 80 00
ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(( Then a continous flood, exactly similar to the above packet,
appears.
Unfortunately this flooding is a firmware problem. ))
<-- Replug the cable, after a good amount of time -->
Where are the reverse state transitions?
quoted
Hmmm...
[ ... ]
quoted
Reverse state transitions are missing from the logs. See comments
above.
quoted
When the device is on the _receiving_ end, and I unplug the CAN cable
after
introducing some noise to the level of reaching WARNING or PASSIVE, I
receive a BUS_ERROR event with the rxerr count reset back to 0 or 1. In
that case, the driver correctly transitions back the state to
ERROR_ACTIVE
and candump produces something similar to:
(000.000362) can0 2000008C [8] 00 40 40 00 00 00 00 01
ERRORFRAME
controller-problem{}
protocol-violation{{back-to-error-active}{}}
bus-error
error-counter-tx-rx{{0}{1}}
which is, AFAIK, the correct behaviour from the driver side.
Meanwhile, when the device is on the _sending_ end and I re-plug the CAN
cable again. Sometimes I receive events with txerr reset to 0 or 1, and
the driver correctly reverts back to ERROR_ACTIVE in that case. But on
another times like the quoted case above, I don't receive any events
resetting txerr back -- only data packets on the bus.
Well, the firmware seems to report *only* bus-errors via
CMD_CAN_ERROR_EVENT
messages, also carrying the new state, but no CMD_CHIP_STATE_EVENT just
for
the state changes.
So, What can the driver do given the above?
Little if the notification does not come.
Wolfgang.
From: Ahmed S. Darwish <redacted>
Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.
Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
1 file changed, 55 insertions(+), 59 deletions(-)
@@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev, } /* Update all of the can interface's state and error counters before- * trying any skb allocation that can actually fail with -ENOMEM.+ * trying any memory allocation that can actually fail with -ENOMEM.+ *+ * We send a temporary stack-allocated error can frame to+ * can_change_state() for the very same reason.+ *+ * TODO: Split can_change_state() responsibility between updating the+ * can interface's state and counters, and the setting up of can error+ * frame ID and data to userspace. Remove stack allocation afterwards. */ old_state = priv->can.state;- kvaser_usb_rx_error_update_can_state(priv, &es);+ kvaser_usb_rx_error_update_can_state(priv, &es, &tmp_cf);+ new_state = priv->can.state; skb = alloc_can_err_skb(priv->netdev, &cf); if (!skb) { stats->rx_dropped++; return; }+ memcpy(cf, &tmp_cf, sizeof(*cf));- if (es.status & M16C_STATE_BUS_OFF) {- cf->can_id |= CAN_ERR_BUSOFF;-- if (!priv->can.restart_ms)- kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);- netif_carrier_off(priv->netdev);- } else if (es.status & M16C_STATE_BUS_PASSIVE) {- if (old_state != CAN_STATE_ERROR_PASSIVE) {- cf->can_id |= CAN_ERR_CRTL;-- if (es.txerr || es.rxerr)- cf->data[1] = (es.txerr > es.rxerr)- ? CAN_ERR_CRTL_TX_PASSIVE- : CAN_ERR_CRTL_RX_PASSIVE;- else- cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |- CAN_ERR_CRTL_RX_PASSIVE;+ if (new_state != old_state) {+ if (es.status & M16C_STATE_BUS_OFF) {+ if (!priv->can.restart_ms)+ kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);+ netif_carrier_off(priv->netdev);+ }+
This block is wrong. The usage of PROT_ACTIVE is based on a misunderstanding.
It's used in some drivers to signify back-to-error-active but its original
meaning is something completely different, AFAIK.
This is handled in can_change_state() using a new CTRL message; namely:
CAN_ERR_CTRL_ACTIVE. The newest version of can-utils is up to date with this.
Looking over the patch again, I've noticed that there are a few things that are
not quite right.
Marc, could you merge the "move bus_off++" patch before you merge this so that I
won't have to incorporate this patch-set into it?
--
Andri
(000.001868) can0 20000080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(000.001982) can0 20000080 [8] 00 00 00 00 00 00 80 00 ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(( Then a continous flood, exactly similar to the above packet, appears.
Unfortunately this flooding is a firmware problem. ))
<-- Replug the cable, after a good amount of time -->
Where are the reverse state transitions?
quoted
Hmmm...
[ ... ]
quoted
Reverse state transitions are missing from the logs. See comments above.
When the device is on the _receiving_ end, and I unplug the CAN cable after
introducing some noise to the level of reaching WARNING or PASSIVE, I
receive a BUS_ERROR event with the rxerr count reset back to 0 or 1. In
that case, the driver correctly transitions back the state to ERROR_ACTIVE
and candump produces something similar to:
(000.000362) can0 2000008C [8] 00 40 40 00 00 00 00 01 ERRORFRAME
controller-problem{}
protocol-violation{{back-to-error-active}{}}
bus-error
error-counter-tx-rx{{0}{1}}
which is, AFAIK, the correct behaviour from the driver side.
Meanwhile, when the device is on the _sending_ end and I re-plug the CAN
cable again. Sometimes I receive events with txerr reset to 0 or 1, and
the driver correctly reverts back to ERROR_ACTIVE in that case. But on
another times like the quoted case above, I don't receive any events
resetting txerr back -- only data packets on the bus.
So, What can the driver do given the above?
So what you're telling us is that the state does not got back to error-active
unless there is something else transmitting on the bus?
If that's the case, it's almost definitely because state changes aren't
triggering interrupts.
An rx event will give you an interrupt which yields a napi poll which means that
the state will be polled, so in that case you don't need the interrupts.
Look for "Consolidate and unify state change handling" on gmane. There was a lot
of discussion about this kind of issues, especially regarding FlexCAN.
Best regards,
Andri
From: Marc Kleine-Budde <mkl@pengutronix.de> Date: 2015-01-21 22:59:38
On 01/21/2015 05:20 PM, Andri Yngvason wrote:
Marc, could you merge the "move bus_off++" patch before you merge this so that I
won't have to incorporate this patch-set into it?
...included in the lastest pull-request to David. Use
tags/linux-can-next-for-3.20-20150121 of the can-next repo as you new base.
regards,
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 |
For a bus-errors I would also expcect some more information in the
data[2..3] fields. But these are always zero.
M16C error factors made it possible to report things like
CAN_ERR_PROT_FORM/STUFF/BIT0/BIT1/TX in data[2], and
CAN_ERR_PROT_LOC_ACK/CRC_DEL in data[3].
Unfortunately such error factors are only reported in Leaf, but
not in USBCan-II due to the wire format change in the error event:
struct leaf_msg_error_event {
u8 tid;
u8 flags;
__le16 time[3];
u8 channel;
u8 padding;
u8 tx_errors_count;
u8 rx_errors_count;
u8 status;
u8 error_factor;
} __packed;
struct usbcan_msg_error_event {
u8 tid;
u8 padding;
u8 tx_errors_count_ch0;
u8 rx_errors_count_ch0;
u8 tx_errors_count_ch1;
u8 rx_errors_count_ch1;
u8 status_ch0;
u8 status_ch1;
__le16 time;
} __packed;
I speculate that the wire format was changed due to controller
bugs in the USBCan-II, which was slightly mentioned in their
data sheets here:
http://www.kvaser.com/canlib-webhelp/page_hardware_specific_can_controllers.html
So it seems there's really no way for filling such bus error
info given the very limited amount of data exported :-(
The issue of incomplete data does not even stop here, kindly
check below notes regarding reverse state transitions:
(000.001868) can0 20000080 [8] 00 00 00 00 00 00 80 00
ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(000.001982) can0 20000080 [8] 00 00 00 00 00 00 80 00
ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(( Then a continous flood, exactly similar to the above packet,
appears.
Unfortunately this flooding is a firmware problem. ))
<-- Replug the cable, after a good amount of time -->
Where are the reverse state transitions?
quoted
Hmmm...
[ ... ]
quoted
Reverse state transitions are missing from the logs. See comments
above.
quoted
quoted
When the device is on the _receiving_ end, and I unplug the CAN cable
after
quoted
introducing some noise to the level of reaching WARNING or PASSIVE, I
receive a BUS_ERROR event with the rxerr count reset back to 0 or 1. In
that case, the driver correctly transitions back the state to
ERROR_ACTIVE
quoted
and candump produces something similar to:
(000.000362) can0 2000008C [8] 00 40 40 00 00 00 00 01
ERRORFRAME
controller-problem{}
protocol-violation{{back-to-error-active}{}}
bus-error
error-counter-tx-rx{{0}{1}}
which is, AFAIK, the correct behaviour from the driver side.
Meanwhile, when the device is on the _sending_ end and I re-plug the CAN
cable again. Sometimes I receive events with txerr reset to 0 or 1, and
the driver correctly reverts back to ERROR_ACTIVE in that case. But on
another times like the quoted case above, I don't receive any events
resetting txerr back -- only data packets on the bus.
Well, the firmware seems to report *only* bus-errors via
CMD_CAN_ERROR_EVENT messages, also carrying the new state, but no
CMD_CHIP_STATE_EVENT just for the state changes.
I've dumped _every_ message I receive from the firmware while
disconnecting the CAN bus, waiting a while, and connecting it again.
I really received _nothing_ from the firmware when the CAN bus was
reconnected and the data packets were flowing again. Not even a
single CHIP_STATE_EVENT, even after waiting for a long time.
So it's basically:
...
ERR EVENT, txerr=128, rxerr=0
ERR EVENT, txerr=128, rxerr=0
ERR EVENT, txerr=128, rxerr=0
...
then complete silence, except the data frames. I've even tried with
different versions of the firmware, but the same behaviour persisted.
quoted
So, What can the driver do given the above?
Little if the notification does not come.
We can poll the state by sending CMD_GET_CHIP_STATE to the firmware,
and it will hopefully reply with a CHIP_STATE_EVENT response
containing the new txerr and rxerr values that we can use for
reverse state transitions.
But do we _really_ want to go through the path? I feel that it will
open some cans of worms w.r.t. concurrent access to both the netdev
and USB stacks from a single driver.
A possible solution can be setting up a kernel thread that queries
for a CHIP_STATE_EVENT every second?
Your inputs on this is appreciated.
For a bus-errors I would also expcect some more information in the
data[2..3] fields. But these are always zero.
M16C error factors made it possible to report things like
CAN_ERR_PROT_FORM/STUFF/BIT0/BIT1/TX in data[2], and
CAN_ERR_PROT_LOC_ACK/CRC_DEL in data[3].
Unfortunately such error factors are only reported in Leaf, but
not in USBCan-II due to the wire format change in the error event:
struct leaf_msg_error_event {
u8 tid;
u8 flags;
__le16 time[3];
u8 channel;
u8 padding;
u8 tx_errors_count;
u8 rx_errors_count;
u8 status;
u8 error_factor;
} __packed;
struct usbcan_msg_error_event {
u8 tid;
u8 padding;
u8 tx_errors_count_ch0;
u8 rx_errors_count_ch0;
u8 tx_errors_count_ch1;
u8 rx_errors_count_ch1;
u8 status_ch0;
u8 status_ch1;
__le16 time;
} __packed;
I speculate that the wire format was changed due to controller
bugs in the USBCan-II, which was slightly mentioned in their
data sheets here:
http://www.kvaser.com/canlib-webhelp/page_hardware_specific_can_controllers.html
So it seems there's really no way for filling such bus error
info given the very limited amount of data exported :-(
We experienced similar problems with FlexCAN.
The issue of incomplete data does not even stop here, kindly
check below notes regarding reverse state transitions:
quoted
quoted
quoted
quoted
ERRORFRAME
bus-error
error-counter-tx-rx{{16}{0}}
[...]
[...]
quoted
quoted
quoted
quoted
ERRORFRAME
bus-error
error-counter-tx-rx{{128}{0}}
(( Then a continous flood, exactly similar to the above packet,
appears.
Unfortunately this flooding is a firmware problem. ))
<-- Replug the cable, after a good amount of time -->
Where are the reverse state transitions?
quoted
Hmmm...
[ ... ]
quoted
Reverse state transitions are missing from the logs. See comments
above.
quoted
quoted
When the device is on the _receiving_ end, and I unplug the CAN cable
after
quoted
introducing some noise to the level of reaching WARNING or PASSIVE, I
receive a BUS_ERROR event with the rxerr count reset back to 0 or 1. In
that case, the driver correctly transitions back the state to
ERROR_ACTIVE
quoted
and candump produces something similar to:
(000.000362) can0 2000008C [8] 00 40 40 00 00 00 00 01
ERRORFRAME
controller-problem{}
protocol-violation{{back-to-error-active}{}}
bus-error
error-counter-tx-rx{{0}{1}}
which is, AFAIK, the correct behaviour from the driver side.
Meanwhile, when the device is on the _sending_ end and I re-plug the CAN
cable again. Sometimes I receive events with txerr reset to 0 or 1, and
the driver correctly reverts back to ERROR_ACTIVE in that case. But on
another times like the quoted case above, I don't receive any events
resetting txerr back -- only data packets on the bus.
Well, the firmware seems to report *only* bus-errors via
CMD_CAN_ERROR_EVENT messages, also carrying the new state, but no
CMD_CHIP_STATE_EVENT just for the state changes.
I've dumped _every_ message I receive from the firmware while
disconnecting the CAN bus, waiting a while, and connecting it again.
I really received _nothing_ from the firmware when the CAN bus was
reconnected and the data packets were flowing again. Not even a
single CHIP_STATE_EVENT, even after waiting for a long time.
So it's basically:
...
ERR EVENT, txerr=128, rxerr=0
ERR EVENT, txerr=128, rxerr=0
ERR EVENT, txerr=128, rxerr=0
...
then complete silence, except the data frames. I've even tried with
different versions of the firmware, but the same behaviour persisted.
quoted
quoted
So, What can the driver do given the above?
Little if the notification does not come.
We can poll the state by sending CMD_GET_CHIP_STATE to the firmware,
and it will hopefully reply with a CHIP_STATE_EVENT response
containing the new txerr and rxerr values that we can use for
reverse state transitions.
But do we _really_ want to go through the path? I feel that it will
open some cans of worms w.r.t. concurrent access to both the netdev
and USB stacks from a single driver.
Honestly, I don't know.
A possible solution can be setting up a kernel thread that queries
for a CHIP_STATE_EVENT every second?
Have you considered polling in kvaser_usb_tx_acknowledge? You could do something
like:
if(unlikely(dev->can.state != CAN_STATE_ERROR_ACTIVE))
{
request_state();
}
I don't think that anything beyond that would be worth pursuing.
Best regards,
Andri
For a bus-errors I would also expcect some more information in the
data[2..3] fields. But these are always zero.
M16C error factors made it possible to report things like
CAN_ERR_PROT_FORM/STUFF/BIT0/BIT1/TX in data[2], and
CAN_ERR_PROT_LOC_ACK/CRC_DEL in data[3].
Unfortunately such error factors are only reported in Leaf, but
not in USBCan-II due to the wire format change in the error event:
struct leaf_msg_error_event {
u8 tid;
u8 flags;
__le16 time[3];
u8 channel;
u8 padding;
u8 tx_errors_count;
u8 rx_errors_count;
u8 status;
u8 error_factor;
} __packed;
struct usbcan_msg_error_event {
u8 tid;
u8 padding;
u8 tx_errors_count_ch0;
u8 rx_errors_count_ch0;
u8 tx_errors_count_ch1;
u8 rx_errors_count_ch1;
u8 status_ch0;
u8 status_ch1;
__le16 time;
} __packed;
I speculate that the wire format was changed due to controller
bugs in the USBCan-II, which was slightly mentioned in their
data sheets here:
http://www.kvaser.com/canlib-webhelp/page_hardware_specific_can_controllers.html
So it seems there's really no way for filling such bus error
info given the very limited amount of data exported :-(
We experienced similar problems with FlexCAN.
Hmm, I'll have a look there then...
Although my initial instincts imply that the FlexCAN driver has
access to the raw CAN registers, something I'm unable to do here.
But maybe there's some black magic I'm missing :-)
[...]
quoted
I've dumped _every_ message I receive from the firmware while
disconnecting the CAN bus, waiting a while, and connecting it again.
I really received _nothing_ from the firmware when the CAN bus was
reconnected and the data packets were flowing again. Not even a
single CHIP_STATE_EVENT, even after waiting for a long time.
So it's basically:
...
ERR EVENT, txerr=128, rxerr=0
ERR EVENT, txerr=128, rxerr=0
ERR EVENT, txerr=128, rxerr=0
...
then complete silence, except the data frames. I've even tried with
different versions of the firmware, but the same behaviour persisted.
quoted
quoted
So, What can the driver do given the above?
Little if the notification does not come.
We can poll the state by sending CMD_GET_CHIP_STATE to the firmware,
and it will hopefully reply with a CHIP_STATE_EVENT response
containing the new txerr and rxerr values that we can use for
reverse state transitions.
But do we _really_ want to go through the path? I feel that it will
open some cans of worms w.r.t. concurrent access to both the netdev
and USB stacks from a single driver.
Honestly, I don't know.
quoted
A possible solution can be setting up a kernel thread that queries
for a CHIP_STATE_EVENT every second?
Have you considered polling in kvaser_usb_tx_acknowledge? You could do something
like:
if(unlikely(dev->can.state != CAN_STATE_ERROR_ACTIVE))
{
request_state();
}
OK, I have four important updates on this issue:
a) My initial testing was done on high-speed channel, at a bitrate
of 50K. After setting the bus to a more reasonable bitrate 500K
or 1M, I was _consistently_ able to receive CHIP_STATE_EVENTs
when plugging the CAN connector again after an unplug.
b) The error counters on this device do not get reset on plugging
after an unplug. I've setup a kernel thread [2] that queries
the chip state event every second, and the error counters stays
the same all the time. [1]
c) There's a single case when the erro counters do indeed get
reversed, and it happens only when introducing some noise in
the bus after the re-plug. In that case, the new error events
get raised with new error counters starting from 0/1 again.
d) I've discovered a bug that forbids the CAN state from
returning to ERROR_ACTIVE in case of the error counters
numbers getting decreased. But independent from that bug, the
verbose debugging messages clearly imply that we only get the
error counters decreased in the case mentioned at `c)' above.
So from [1] and [2], it's now clear that the device do not reset
these counters back in the re-plug case. I'll give a check to
flexcan as advised, but unfortunately I don't really think there's
much I can do about this.
[1]
[ 877.207082] CAN_ERROR_: channel=0, txerr=88, rxerr=0
[ 877.207090] CAN_ERROR_: channel=0, txerr=136, rxerr=0
[ 877.207094] CAN_ERROR_: channel=0, txerr=144, rxerr=0
[ 877.207098] CAN_ERROR_: channel=0, txerr=152, rxerr=0
[ 877.207100] CAN_ERROR_: channel=0, txerr=160, rxerr=0
[ 877.207102] CAN_ERROR_: channel=0, txerr=168, rxerr=0
[ 877.208075] CAN_ERROR_: channel=0, txerr=200, rxerr=0
(( The above error event, staying the same at txerr=200 keeps
flooding the bus until the CAN cable is re-plugged ))
[ 878.225116] CHIP_STATE: channel=0, txerr=200, rxerr=0
[ 878.225143] CHIP_STATE: channel=1, txerr=0, rxerr=0
[ 879.265167] CHIP_STATE: channel=0, txerr=200, rxerr=0
[ 879.267152] CHIP_STATE: channel=1, txerr=0, rxerr=0
[ 879.265167] CHIP_STATE: channel=0, txerr=200, rxerr=0
[ 879.267152] CHIP_STATE: channel=1, txerr=0, rxerr=0
(( The same counters get repeated every second ))
[2] State was polled using:
static int kvaser_usb_poll_chip_state(void *vpriv) {
struct kvaser_usb_net_priv *priv = vpriv;
while (!kthread_should_stop()) {
kvaser_usb_simple_msg_async(priv, CMD_GET_CHIP_STATE);
ssleep(1);
}
return 0;
}
I don't think that anything beyond that would be worth pursuing.
I agree, but given the new input, it seems that our problem
extends to the error counters themselves not getting decreased
on re-plug. So, even polling will not solve the issue: we'll
get the same txerr/rxerr values again and again :-(
From: Ahmed S. Darwish <hidden> Date: 2015-01-25 02:49:09
On Thu, Jan 22, 2015 at 10:14:47AM +0000, Andri Yngvason wrote:
Quoting Marc Kleine-Budde (2015-01-21 22:59:23)
quoted
On 01/21/2015 05:20 PM, Andri Yngvason wrote:
quoted
Marc, could you merge the "move bus_off++" patch before you merge this so that I
won't have to incorporate this patch-set into it?
...included in the lastest pull-request to David. Use
tags/linux-can-next-for-3.20-20150121 of the can-next repo as you new base.
Thanks!
I guess I'll re-base my next submission over this tag too.
Nothing in the new 5 patches is substantial enough to be
included in the current kernel release.
Thanks!
Darwish