can: kvaser_usb: Various fixes
Subsystems (2)
v3 cover can: kvaser_usb: Various fixes
This patch series was originally posted by Anssi Hannula [1]. In v2 I rebased and updated some of the patches [2].
Changes in v3:
- Rebase on top of commit
1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add Tested-by: Anssi Hannula
- Add [email protected] to CC.
- Add my S-o-b to all patches
- Fix regression introduced in
[PATCH v2 04/15] can: kvaser_usb: kvaser_usb_leaf: Get capabilities from device, found by Anssi Hannula [3]
[1]
https://lore.kernel.org/linux-can/[email protected]
[2]
https://lore.kernel.org/linux-can/[email protected]
[3]
https://lore.kernel.org/linux-can/[email protected]
Anssi Hannula (10):
can: kvaser_usb_leaf: Fix overread with an invalid command
can: kvaser_usb: Fix use of uninitialized completion
can: kvaser_usb: Fix possible completions during init_completion
can: kvaser_usb_leaf: Set Warning state even without bus errors
can: kvaser_usb_leaf: Fix TX queue out of sync after restart
can: kvaser_usb_leaf: Fix CAN state after restart
can: kvaser_usb_leaf: Fix improved state not being reported
can: kvaser_usb_leaf: Fix wrong CAN state after stopping
can: kvaser_usb_leaf: Ignore stale bus-off after start
can: kvaser_usb_leaf: Fix bogus restart events
Jimmy Assarsson (5):
can: kvaser_usb: kvaser_usb_leaf: Get capabilities from device
can: kvaser_usb: kvaser_usb_leaf: Rename {leaf,usbcan}_cmd_error_event
to {leaf,usbcan}_cmd_can_error_event
can: kvaser_usb: kvaser_usb_leaf: Handle CMD_ERROR_EVENT
can: kvaser_usb: Add struct kvaser_usb_busparams
can: kvaser_usb: Compare requested bittiming parameters with actual
parameters in do_set_{,data}_bittiming
drivers/net/can/usb/kvaser_usb/kvaser_usb.h | 32 +- .../net/can/usb/kvaser_usb/kvaser_usb_core.c | 118 +++- .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 166 ++++-- .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 543 ++++++++++++++++-- 4 files changed, 764 insertions(+), 95 deletions(-)
Signature · 1 line
2.37.3
PATCH v3 1/15 can: kvaser_usb_leaf: Fix overread with an invalid command
From: Anssi Hannula <[email protected]>
For command events read from the device, kvaser_usb_leaf_read_bulk_callback() verifies that cmd->len does not exceed the size of the received data, but the actual kvaser_cmd handlers will happily read any kvaser_cmd fields without checking for cmd->len.
This can cause an overread if the last cmd in the buffer is shorter than expected for the command type (with cmd->len showing the actual short size).
Maximum overread seems to be 22 bytes (CMD_LEAF_LOG_MESSAGE), some of which are delivered to userspace as-is.
Fix that by verifying the length of command before handling it.
This issue can only occur after RX URBs have been set up, i.e. the interface has been opened at least once.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +75 -0
@@ -310,6 +310,38 @@ struct kvaser_cmd { } u; } __packed; +#define CMD_SIZE_ANY 0xff +#define kvaser_fsize (field) sizeof_field(struct kvaser_cmd, field) + +static const u8 kvaser_usb_leaf_cmd_sizes_leaf[] = { + [CMD_START_CHIP_REPLY ] =kvaser_fsize (u.simple), + [CMD_STOP_CHIP_REPLY ] =kvaser_fsize (u.simple), + [CMD_GET_CARD_INFO_REPLY ] =kvaser_fsize (u.cardinfo), + [CMD_TX_ACKNOWLEDGE ] =kvaser_fsize (u.tx_acknowledge_header), + [CMD_GET_SOFTWARE_INFO_REPLY ] =kvaser_fsize (u.leaf.softinfo), + [CMD_RX_STD_MESSAGE ] =kvaser_fsize (u.leaf.rx_can), + [CMD_RX_EXT_MESSAGE ] =kvaser_fsize (u.leaf.rx_can), + [CMD_LEAF_LOG_MESSAGE ] =kvaser_fsize (u.leaf.log_message), + [CMD_CHIP_STATE_EVENT ] =kvaser_fsize (u.leaf.chip_state_event), + [CMD_CAN_ERROR_EVENT ] =kvaser_fsize (u.leaf.error_event), +/* ignored events: */ + [CMD_FLUSH_QUEUE_REPLY ] =CMD_SIZE_ANY , +}; + +static const u8 kvaser_usb_leaf_cmd_sizes_usbcan[] = { + [CMD_START_CHIP_REPLY ] =kvaser_fsize (u.simple), + [CMD_STOP_CHIP_REPLY ] =kvaser_fsize (u.simple), + [CMD_GET_CARD_INFO_REPLY ] =kvaser_fsize (u.cardinfo), + [CMD_TX_ACKNOWLEDGE ] =kvaser_fsize (u.tx_acknowledge_header), + [CMD_GET_SOFTWARE_INFO_REPLY ] =kvaser_fsize (u.usbcan.softinfo), + [CMD_RX_STD_MESSAGE ] =kvaser_fsize (u.usbcan.rx_can), + [CMD_RX_EXT_MESSAGE ] =kvaser_fsize (u.usbcan.rx_can), + [CMD_CHIP_STATE_EVENT ] =kvaser_fsize (u.usbcan.chip_state_event), + [CMD_CAN_ERROR_EVENT ] =kvaser_fsize (u.usbcan.error_event), +/* ignored events: */ + [CMD_USBCAN_CLOCK_OVERFLOW_EVENT ] =CMD_SIZE_ANY , +}; +/* Summary of a kvaser error event, for a unified Leaf/Usbcan error * handling. Some discrepancies between the two families exist: *
@@ -397,6 +429,43 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_imx_dev_cfg_32mhz = { .bittiming_const = &kvaser_usb_flexc_bittiming_const, }; +static int kvaser_usb_leaf_verify_size (const struct kvaser_usb *dev, +const struct kvaser_cmd *cmd) +{ +/* buffer size >= cmd->len ensured by caller */ +u8 min_size =0 ; + +switch (dev->driver_info->family) { +case KVASER_LEAF : +if (cmd->id <ARRAY_SIZE (kvaser_usb_leaf_cmd_sizes_leaf)) + min_size = kvaser_usb_leaf_cmd_sizes_leaf[cmd->id]; +break ; +case KVASER_USBCAN : +if (cmd->id <ARRAY_SIZE (kvaser_usb_leaf_cmd_sizes_usbcan)) + min_size = kvaser_usb_leaf_cmd_sizes_usbcan[cmd->id]; +break ; + } + +if (min_size ==CMD_SIZE_ANY ) +return 0 ; + +if (min_size) { + min_size +=CMD_HEADER_LEN ; +if (cmd->len >= min_size) +return 0 ; + +dev_err_ratelimited (&dev->intf->dev, +"Received command %u too short (size %u, needed %u)" , + cmd->id, cmd->len, min_size); +return -EIO ; + } + +dev_warn_ratelimited (&dev->intf->dev, +"Unhandled command (%d, size %d)\n" , + cmd->id, cmd->len); +return -EINVAL ; +} +static void * kvaser_usb_leaf_frame_to_cmd(const struct kvaser_usb_net_priv *priv, const struct sk_buff *skb, int *cmd_len,
@@ -502,6 +571,9 @@ static int kvaser_usb_leaf_wait_cmd(const struct kvaser_usb *dev, u8 id, end:kfree (buf); +if (err ==0 ) + err =kvaser_usb_leaf_verify_size (dev, cmd); +return err; }
@@ -1133,6 +1205,9 @@ static void kvaser_usb_leaf_stop_chip_reply(const struct kvaser_usb *dev,static void kvaser_usb_leaf_handle_command (const struct kvaser_usb *dev,const struct kvaser_cmd *cmd) { +if (kvaser_usb_leaf_verify_size (dev, cmd) <0 ) +return ; +switch (cmd->id) {case CMD_START_CHIP_REPLY :kvaser_usb_leaf_start_chip_reply (dev, cmd);
Signature · 1 line
2.37.3
PATCH v3 2/15 can: kvaser_usb: Fix use of uninitialized completion
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
flush_comp is initialized when CMD_FLUSH_QUEUE is sent to the device and completed when the device sends CMD_FLUSH_QUEUE_RESP.
This causes completion of uninitialized completion if the device sends CMD_FLUSH_QUEUE_RESP before CMD_FLUSH_QUEUE is ever sent (e.g. as a response to a flush by a previously bound driver, or a misbehaving device).
Fix that by initializing flush_comp in kvaser_usb_init_one() like the other completions.
This issue is only triggerable after RX URBs have been set up, i.e. the interface has been opened at least once.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 1 + drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c +1 -0
@@ -729,6 +729,7 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev, int channel)init_usb_anchor (&priv->tx_submitted);init_completion (&priv->start_comp);init_completion (&priv->stop_comp); +init_completion (&priv->flush_comp); priv->can.ctrlmode_supported =0 ; priv->dev = dev;
drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c +1 -1
@@ -1916,7 +1916,7 @@ static int kvaser_usb_hydra_flush_queue(struct kvaser_usb_net_priv *priv) {int err; -init_completion (&priv->flush_comp); +reinit_completion (&priv->flush_comp); err =kvaser_usb_hydra_send_simple_cmd (priv->dev,CMD_FLUSH_QUEUE , priv->channel);
Signature · 1 line
2.37.3
PATCH v3 3/15 can: kvaser_usb: Fix possible completions during init_completion
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
kvaser_usb uses completions to signal when a response event is received for outgoing commands.
However, it uses init_completion() to reinitialize the start_comp and stop_comp completions before sending the start/stop commands.
In case the device sends the corresponding response just before the actual command is sent, complete() may be called concurrently with init_completion() which is not safe.
This might be triggerable even with a properly functioning device by stopping the interface (CMD_STOP_CHIP) just after it goes bus-off (which also causes the driver to send CMD_STOP_CHIP when restart-ms is off), but that was not tested.
Fix the issue by using reinit_completion() instead.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 4 ++-- drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c +2 -2
@@ -1875,7 +1875,7 @@ static int kvaser_usb_hydra_start_chip(struct kvaser_usb_net_priv *priv) {int err; -init_completion (&priv->start_comp); +reinit_completion (&priv->start_comp); err =kvaser_usb_hydra_send_simple_cmd (priv->dev,CMD_START_CHIP_REQ , priv->channel);
@@ -1893,7 +1893,7 @@ static int kvaser_usb_hydra_stop_chip(struct kvaser_usb_net_priv *priv) {int err; -init_completion (&priv->stop_comp); +reinit_completion (&priv->stop_comp);/* Make sure we do not report invalid BUS_OFF from CMD_CHIP_STATE_EVENT * see comment in kvaser_usb_hydra_update_state()
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +2 -2
@@ -1320,7 +1320,7 @@ static int kvaser_usb_leaf_start_chip(struct kvaser_usb_net_priv *priv) {int err; -init_completion (&priv->start_comp); +reinit_completion (&priv->start_comp); err =kvaser_usb_leaf_send_simple_cmd (priv->dev,CMD_START_CHIP , priv->channel);
@@ -1338,7 +1338,7 @@ static int kvaser_usb_leaf_stop_chip(struct kvaser_usb_net_priv *priv) {int err; -init_completion (&priv->stop_comp); +reinit_completion (&priv->stop_comp); err =kvaser_usb_leaf_send_simple_cmd (priv->dev,CMD_STOP_CHIP , priv->channel);
Signature · 1 line
2.37.3
PATCH v3 4/15 can: kvaser_usb: kvaser_usb_leaf: Get capabilities from device
In reply to Jimmy AssarssonUse the CMD_GET_CAPABILITIES_REQ command to query the device for certain capabilities. We are only interested in LISTENONLY mode and wither the device reports CAN error counters.
And remove hard coded capabilities for all Leaf devices.
---
Changes in v3
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Re-add hard coded capabilities for Leaf M32C devices, to fix regression
found by Anssi Hannula in v2 [1].
Changes in v2:
- New in v2. Replaces [PATCH 04/12] can: kvaser_usb: Mark Mini PCIe 2xHS as supporting
error counters
- Fixed Anssi's comments; https://lore.kernel.org/linux-can/[email protected]/
[1] https://lore.kernel.org/linux-can/[email protected]/
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 144 +++++++++++++++++- 1 file changed, 143 insertions(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +143 -1
@@ -74,6 +74,8 @@#define CMD_TX_ACKNOWLEDGE 50#define CMD_CAN_ERROR_EVENT 51#define CMD_FLUSH_QUEUE_REPLY 68 +#define CMD_GET_CAPABILITIES_REQ 95 +#define CMD_GET_CAPABILITIES_RESP 96#define CMD_LEAF_LOG_MESSAGE 106
@@ -83,6 +85,8 @@#define KVASER_USB_LEAF_SWOPTION_FREQ_32_MHZ_CLK BIT(5)#define KVASER_USB_LEAF_SWOPTION_FREQ_24_MHZ_CLK BIT(6) +#define KVASER_USB_LEAF_SWOPTION_EXT_CAP BIT(12) +/* error factors */ #define M16C_EF_ACKE BIT(0)#define M16C_EF_CRCE BIT(1)
@@ -278,6 +282,28 @@ struct leaf_cmd_log_message {u8 data[8 ]; } __packed; +/* Sub commands for cap_req and cap_res */ +#define KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE 0x02 +#define KVASER_USB_LEAF_CAP_CMD_ERR_REPORT 0x05 +struct kvaser_cmd_cap_req { +__le16 padding0; +__le16 cap_cmd; +__le16 padding1; +__le16 channel; +} __packed; + +/* Status codes for cap_res */ +#define KVASER_USB_LEAF_CAP_STAT_OK 0x00 +#define KVASER_USB_LEAF_CAP_STAT_NOT_IMPL 0x01 +#define KVASER_USB_LEAF_CAP_STAT_UNAVAIL 0x02 +struct kvaser_cmd_cap_res { +__le16 padding; +__le16 cap_cmd; +__le16 status; +__le32 mask; +__le32 value; +} __packed; +struct kvaser_cmd {u8 len;u8 id;
@@ -295,6 +321,8 @@ struct kvaser_cmd {struct leaf_cmd_chip_state_event chip_state_event;struct leaf_cmd_error_event error_event;struct leaf_cmd_log_message log_message; +struct kvaser_cmd_cap_req cap_req; +struct kvaser_cmd_cap_res cap_res; }__packed leaf;union {
@@ -324,6 +352,7 @@ static const u8 kvaser_usb_leaf_cmd_sizes_leaf[] = { [CMD_LEAF_LOG_MESSAGE] = kvaser_fsize(u.leaf.log_message), [CMD_CHIP_STATE_EVENT] = kvaser_fsize(u.leaf.chip_state_event), [CMD_CAN_ERROR_EVENT] = kvaser_fsize(u.leaf.error_event), + [CMD_GET_CAPABILITIES_RESP] = kvaser_fsize(u.leaf.cap_res),/* ignored events: */ [CMD_FLUSH_QUEUE_REPLY] = CMD_SIZE_ANY, };
@@ -606,6 +635,9 @@ static void kvaser_usb_leaf_get_software_info_leaf(struct kvaser_usb *dev, dev->fw_version =le32_to_cpu (softinfo->fw_version); dev->max_tx_urbs =le16_to_cpu (softinfo->max_outstanding_tx); +if (sw_options &KVASER_USB_LEAF_SWOPTION_EXT_CAP ) + dev->card_data.capabilities |=KVASER_USB_CAP_EXT_CAP ; +if (dev->driver_info->quirks &KVASER_USB_QUIRK_IGNORE_CLK_FREQ ) {/* Firmware expects bittiming parameters calculated for 16MHz * clock, regardless of the actual clock
@@ -693,6 +725,116 @@ static int kvaser_usb_leaf_get_card_info(struct kvaser_usb *dev)return 0 ; } +static int kvaser_usb_leaf_get_single_capability (struct kvaser_usb *dev, +u16 cap_cmd_req,u16 *status) +{ +struct kvaser_usb_dev_card_data *card_data = &dev->card_data; +struct kvaser_cmd *cmd; +u32 value =0 ; +u32 mask =0 ; +u16 cap_cmd_res; +int err; +int i; + + cmd =kzalloc (sizeof (*cmd),GFP_KERNEL ); +if (!cmd) +return -ENOMEM ; + + cmd->id =CMD_GET_CAPABILITIES_REQ ; + cmd->u.leaf.cap_req.cap_cmd =cpu_to_le16 (cap_cmd_req); + cmd->len =CMD_HEADER_LEN +sizeof (struct kvaser_cmd_cap_req ); + + err =kvaser_usb_send_cmd (dev, cmd, cmd->len); +if (err) + goto end; + + err =kvaser_usb_leaf_wait_cmd (dev,CMD_GET_CAPABILITIES_RESP , cmd); +if (err) + goto end; + + *status =le16_to_cpu (cmd->u.leaf.cap_res.status); + +if (*status !=KVASER_USB_LEAF_CAP_STAT_OK ) + goto end; + + cap_cmd_res =le16_to_cpu (cmd->u.leaf.cap_res.cap_cmd); +switch (cap_cmd_res) { +case KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE : +case KVASER_USB_LEAF_CAP_CMD_ERR_REPORT : + value =le32_to_cpu (cmd->u.leaf.cap_res.value); + mask =le32_to_cpu (cmd->u.leaf.cap_res.mask); +break ; +default : +dev_warn (&dev->intf->dev,"Unknown capability command %u\n" , + cap_cmd_res); +break ; + } + +for (i =0 ; i < dev->nchannels; i++) { +if (BIT (i) & (value & mask)) { +switch (cap_cmd_res) { +case KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE : + card_data->ctrlmode_supported |= +CAN_CTRLMODE_LISTENONLY ; +break ; +case KVASER_USB_LEAF_CAP_CMD_ERR_REPORT : + card_data->capabilities |= +KVASER_USB_CAP_BERR_CAP ; +break ; + } + } + } + +end: +kfree (cmd); + +return err; +} + +static int kvaser_usb_leaf_get_capabilities_leaf (struct kvaser_usb *dev) +{ +int err; +u16 status; + +if (!(dev->card_data.capabilities &KVASER_USB_CAP_EXT_CAP )) { +dev_info (&dev->intf->dev, +"No extended capability support. Upgrade device firmware.\n" ); +return 0 ; + } + + err =kvaser_usb_leaf_get_single_capability (dev, +KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE , + &status); +if (err) +return err; +if (status) +dev_info (&dev->intf->dev, +"KVASER_USB_LEAF_CAP_CMD_LISTEN_MODE failed %u\n" , + status); + + err =kvaser_usb_leaf_get_single_capability (dev, +KVASER_USB_LEAF_CAP_CMD_ERR_REPORT , + &status); +if (err) +return err; +if (status) +dev_info (&dev->intf->dev, +"KVASER_USB_LEAF_CAP_CMD_ERR_REPORT failed %u\n" , + status); + +return 0 ; +} + +static int kvaser_usb_leaf_get_capabilities (struct kvaser_usb *dev) +{ +int err =0 ; + +if (dev->driver_info->family ==KVASER_LEAF ) + err =kvaser_usb_leaf_get_capabilities_leaf (dev); + +return err; +} +static void kvaser_usb_leaf_tx_acknowledge (const struct kvaser_usb *dev,const struct kvaser_cmd *cmd) {
@@ -1482,7 +1624,7 @@ const struct kvaser_usb_dev_ops kvaser_usb_leaf_dev_ops = { .dev_get_software_info = kvaser_usb_leaf_get_software_info, .dev_get_software_details = NULL, .dev_get_card_info = kvaser_usb_leaf_get_card_info, - .dev_get_capabilities = NULL, + .dev_get_capabilities = kvaser_usb_leaf_get_capabilities, .dev_set_opt_mode = kvaser_usb_leaf_set_opt_mode, .dev_start_chip = kvaser_usb_leaf_start_chip, .dev_stop_chip = kvaser_usb_leaf_stop_chip,
Signature · 1 line
2.37.3
Re: [PATCH v3 04/15] can: kvaser_usb: kvaser_usb_leaf: Get capabilities from device
Hi,
Quoted 5 lines · Jimmy Assarsson
Use the
CMD_GET_CAPABILITIES_REQcommand to query the device for certain capabilities. We are only interested in LISTENONLY mode and wither the device reports CAN error counters.And remove hard coded capabilities for all Leaf devices.
I think the second paragraph is no longer accurate.
But the patch itself works for me now with no regressions that I can see.
Thanks for taking care of the patchset!
Quoted 21 lines
Fixes 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")Reported-by Anssi Hannula <[email protected]>Signed-off-by Jimmy Assarsson <[email protected]>---
Changes in v3
- Rebased on1d5eeda23f36("can:kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Re-add hard coded capabilities for Leaf M32C devices, to fix regression
found by Anssi Hannula in v2 [1].Changes in v2:
- New in v2. Replaces [PATCH 04/12] can:kvaser_usb: Mark Mini PCIe 2xHS as supporting
error counters
- Fixed Anssi's comments; https://lore.kernel.org/linux-can/[email protected]/[1] https://lore.kernel.org/linux-can/[email protected]/
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 144 +++++++++++++++++- 1 file changed, 143 insertions(+), 1 deletion(-)
[...]
Signature · 2 lines
Anssi Hannula / Bitwise Oy +358 503803997
Re: [PATCH v3 04/15] can: kvaser_usb: kvaser_usb_leaf: Get capabilities from device
Quoted 10 lines · Anssi Hannula
Hi,
Quoted 5 lines · Jimmy Assarsson
Use the
CMD_GET_CAPABILITIES_REQcommand to query the device for certain capabilities. We are only interested in LISTENONLY mode and wither the device reports CAN error counters.And remove hard coded capabilities for all Leaf devices.
I think the second paragraph is no longer accurate.
Oops, will fix this in v4.
Quoted 5 lines
But the patch itself works for me now with no regressions that I can see.
Tested-by Anssi Hannula <[email protected]>Thanks for taking care of the patchset!
Thanks again for testing!
Quoted 22 lines
Quoted 21 lines
Fixes 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")Reported-by Anssi Hannula <[email protected]>Signed-off-by Jimmy Assarsson <[email protected]>---
Changes in v3
- Rebased on1d5eeda23f36("can:kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Re-add hard coded capabilities for Leaf M32C devices, to fix regression
found by Anssi Hannula in v2 [1].Changes in v2:
- New in v2. Replaces [PATCH 04/12] can:kvaser_usb: Mark Mini PCIe 2xHS as supporting
error counters
- Fixed Anssi's comments; https://lore.kernel.org/linux-can/[email protected]/[1] https://lore.kernel.org/linux-can/[email protected]/
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 144 +++++++++++++++++- 1 file changed, 143 insertions(+), 1 deletion(-)[...]
PATCH v3 5/15 can: kvaser_usb: kvaser_usb_leaf: Rename {leaf,usbcan}_cmd_error_event to {leaf,usbcan}_cmd_can_error_event
In reply to Jimmy AssarssonPrepare for handling CMD_ERROR_EVENT. Rename struct {leaf,usbcan}_cmd_error_event to {leaf,usbcan}_cmd_can_error_event.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add Tested-by Anssi Hannula
Changes in v2:
- New in v2.
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +19 -19
@@ -234,7 +234,7 @@ struct kvaser_cmd_tx_acknowledge_header {u8 tid; } __packed; -struct leaf_cmd_error_event { +struct leaf_cmd_can_error_event {u8 tid;u8 flags;__le16 time[3 ];
@@ -246,7 +246,7 @@ struct leaf_cmd_error_event {u8 error_factor; } __packed; -struct usbcan_cmd_error_event { +struct usbcan_cmd_can_error_event {u8 tid;u8 padding;u8 tx_errors_count_ch0;
@@ -319,7 +319,7 @@ struct kvaser_cmd {struct leaf_cmd_softinfo softinfo;struct leaf_cmd_rx_can rx_can;struct leaf_cmd_chip_state_event chip_state_event; -struct leaf_cmd_error_event error_event; +struct leaf_cmd_can_error_event can_error_event;struct leaf_cmd_log_message log_message;struct kvaser_cmd_cap_req cap_req;struct kvaser_cmd_cap_res cap_res;
@@ -329,7 +329,7 @@ struct kvaser_cmd {struct usbcan_cmd_softinfo softinfo;struct usbcan_cmd_rx_can rx_can;struct usbcan_cmd_chip_state_event chip_state_event; -struct usbcan_cmd_error_event error_event; +struct usbcan_cmd_can_error_event can_error_event; }__packed usbcan;struct kvaser_cmd_tx_can tx_can;
@@ -351,7 +351,7 @@ static const u8 kvaser_usb_leaf_cmd_sizes_leaf[] = { [CMD_RX_EXT_MESSAGE] = kvaser_fsize(u.leaf.rx_can), [CMD_LEAF_LOG_MESSAGE] = kvaser_fsize(u.leaf.log_message), [CMD_CHIP_STATE_EVENT] = kvaser_fsize(u.leaf.chip_state_event), - [CMD_CAN_ERROR_EVENT] = kvaser_fsize(u.leaf.error_event), + [CMD_CAN_ERROR_EVENT] = kvaser_fsize(u.leaf.can_error_event), [CMD_GET_CAPABILITIES_RESP] = kvaser_fsize(u.leaf.cap_res),/* ignored events: */ [CMD_FLUSH_QUEUE_REPLY] = CMD_SIZE_ANY,
@@ -366,7 +366,7 @@ static const u8 kvaser_usb_leaf_cmd_sizes_usbcan[] = { [CMD_RX_STD_MESSAGE] = kvaser_fsize(u.usbcan.rx_can), [CMD_RX_EXT_MESSAGE] = kvaser_fsize(u.usbcan.rx_can), [CMD_CHIP_STATE_EVENT] = kvaser_fsize(u.usbcan.chip_state_event), - [CMD_CAN_ERROR_EVENT] = kvaser_fsize(u.usbcan.error_event), + [CMD_CAN_ERROR_EVENT] = kvaser_fsize(u.usbcan.can_error_event),/* ignored events: */ [CMD_USBCAN_CLOCK_OVERFLOW_EVENT] = CMD_SIZE_ANY, };
@@ -1132,11 +1132,11 @@ static void kvaser_usb_leaf_usbcan_rx_error(const struct kvaser_usb *dev,case CMD_CAN_ERROR_EVENT : es.channel =0 ; - es.status = cmd->u.usbcan.error_event.status_ch0; - es.txerr = cmd->u.usbcan.error_event.tx_errors_count_ch0; - es.rxerr = cmd->u.usbcan.error_event.rx_errors_count_ch0; + es.status = cmd->u.usbcan.can_error_event.status_ch0; + es.txerr = cmd->u.usbcan.can_error_event.tx_errors_count_ch0; + es.rxerr = cmd->u.usbcan.can_error_event.rx_errors_count_ch0; es.usbcan.other_ch_status = - cmd->u.usbcan.error_event.status_ch1; + cmd->u.usbcan.can_error_event.status_ch1;kvaser_usb_leaf_usbcan_conditionally_rx_error (dev, &es);/* The USBCAN firmware supports up to 2 channels.
@@ -1144,13 +1144,13 @@ static void kvaser_usb_leaf_usbcan_rx_error(const struct kvaser_usb *dev,*/ if (dev->nchannels ==MAX_USBCAN_NET_DEVICES ) { es.channel =1 ; - es.status = cmd->u.usbcan.error_event.status_ch1; + es.status = cmd->u.usbcan.can_error_event.status_ch1; es.txerr = - cmd->u.usbcan.error_event.tx_errors_count_ch1; + cmd->u.usbcan.can_error_event.tx_errors_count_ch1; es.rxerr = - cmd->u.usbcan.error_event.rx_errors_count_ch1; + cmd->u.usbcan.can_error_event.rx_errors_count_ch1; es.usbcan.other_ch_status = - cmd->u.usbcan.error_event.status_ch0; + cmd->u.usbcan.can_error_event.status_ch0;kvaser_usb_leaf_usbcan_conditionally_rx_error (dev, &es); }break ;
@@ -1167,11 +1167,11 @@ static void kvaser_usb_leaf_leaf_rx_error(const struct kvaser_usb *dev,switch (cmd->id) {case CMD_CAN_ERROR_EVENT : - es.channel = cmd->u.leaf.error_event.channel; - es.status = cmd->u.leaf.error_event.status; - es.txerr = cmd->u.leaf.error_event.tx_errors_count; - es.rxerr = cmd->u.leaf.error_event.rx_errors_count; - es.leaf.error_factor = cmd->u.leaf.error_event.error_factor; + es.channel = cmd->u.leaf.can_error_event.channel; + es.status = cmd->u.leaf.can_error_event.status; + es.txerr = cmd->u.leaf.can_error_event.tx_errors_count; + es.rxerr = cmd->u.leaf.can_error_event.rx_errors_count; + es.leaf.error_factor = cmd->u.leaf.can_error_event.error_factor;break ;case CMD_LEAF_LOG_MESSAGE : es.channel = cmd->u.leaf.log_message.channel;
Signature · 1 line
2.37.3
PATCH v3 6/15 can: kvaser_usb: kvaser_usb_leaf: Handle CMD_ERROR_EVENT
In reply to Jimmy AssarssonThe device will send an error event command, to indicate certain errors. This indicates a misbehaving driver, and should never occur.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add Tested-by Anssi Hannula
Changes in v2:
- New in v2. Replaces parts of [PATCH 09/12] can: kvaser_usb_leaf: Fix silently failing bus params
setup
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +99 -0
@@ -70,6 +70,7 @@#define CMD_GET_CARD_INFO_REPLY 35#define CMD_GET_SOFTWARE_INFO 38#define CMD_GET_SOFTWARE_INFO_REPLY 39 +#define CMD_ERROR_EVENT 45#define CMD_FLUSH_QUEUE 48#define CMD_TX_ACKNOWLEDGE 50#define CMD_CAN_ERROR_EVENT 51
@@ -258,6 +259,28 @@ struct usbcan_cmd_can_error_event {__le16 time; } __packed; +/* CMD_ERROR_EVENT error codes */ +#define KVASER_USB_LEAF_ERROR_EVENT_TX_QUEUE_FULL 0x8 +#define KVASER_USB_LEAF_ERROR_EVENT_PARAM 0x9 + +struct leaf_cmd_error_event { +u8 tid; +u8 error_code; +__le16 timestamp[3 ]; +__le16 padding; +__le16 info1; +__le16 info2; +} __packed; + +struct usbcan_cmd_error_event { +u8 tid; +u8 error_code; +__le16 info1; +__le16 info2; +__le16 timestamp; +__le16 padding; +} __packed; +struct kvaser_cmd_ctrl_mode {u8 tid;u8 channel;
@@ -321,6 +344,7 @@ struct kvaser_cmd {struct leaf_cmd_chip_state_event chip_state_event;struct leaf_cmd_can_error_event can_error_event;struct leaf_cmd_log_message log_message; +struct leaf_cmd_error_event error_event;struct kvaser_cmd_cap_req cap_req;struct kvaser_cmd_cap_res cap_res; }__packed leaf;
@@ -330,6 +354,7 @@ struct kvaser_cmd {struct usbcan_cmd_rx_can rx_can;struct usbcan_cmd_chip_state_event chip_state_event;struct usbcan_cmd_can_error_event can_error_event; +struct usbcan_cmd_error_event error_event; }__packed usbcan;struct kvaser_cmd_tx_can tx_can;
@@ -353,6 +378,7 @@ static const u8 kvaser_usb_leaf_cmd_sizes_leaf[] = { [CMD_CHIP_STATE_EVENT] = kvaser_fsize(u.leaf.chip_state_event), [CMD_CAN_ERROR_EVENT] = kvaser_fsize(u.leaf.can_error_event), [CMD_GET_CAPABILITIES_RESP] = kvaser_fsize(u.leaf.cap_res), + [CMD_ERROR_EVENT] = kvaser_fsize(u.leaf.error_event),/* ignored events: */ [CMD_FLUSH_QUEUE_REPLY] = CMD_SIZE_ANY, };
@@ -367,6 +393,7 @@ static const u8 kvaser_usb_leaf_cmd_sizes_usbcan[] = { [CMD_RX_EXT_MESSAGE] = kvaser_fsize(u.usbcan.rx_can), [CMD_CHIP_STATE_EVENT] = kvaser_fsize(u.usbcan.chip_state_event), [CMD_CAN_ERROR_EVENT] = kvaser_fsize(u.usbcan.can_error_event), + [CMD_ERROR_EVENT] = kvaser_fsize(u.usbcan.error_event),/* ignored events: */ [CMD_USBCAN_CLOCK_OVERFLOW_EVENT] = CMD_SIZE_ANY, };
@@ -1304,6 +1331,74 @@ static void kvaser_usb_leaf_rx_can_msg(const struct kvaser_usb *dev,netif_rx (skb); } +static void kvaser_usb_leaf_error_event_parameter (const struct kvaser_usb *dev, +const struct kvaser_cmd *cmd) +{ +u16 info1 =0 ; + +switch (dev->driver_info->family) { +case KVASER_LEAF : + info1 =le16_to_cpu (cmd->u.leaf.error_event.info1); +break ; +case KVASER_USBCAN : + info1 =le16_to_cpu (cmd->u.usbcan.error_event.info1); +break ; + } + +/* info1 will contain the offending cmd_no */ +switch (info1) { +case CMD_SET_CTRL_MODE : +dev_warn (&dev->intf->dev, +"CMD_SET_CTRL_MODE error in parameter\n" ); +break ; + +case CMD_SET_BUS_PARAMS : +dev_warn (&dev->intf->dev, +"CMD_SET_BUS_PARAMS error in parameter\n" ); +break ; + +default : +dev_warn (&dev->intf->dev, +"Unhandled parameter error event cmd_no (%u)\n" , + info1); +break ; + } +} + +static void kvaser_usb_leaf_error_event (const struct kvaser_usb *dev, +const struct kvaser_cmd *cmd) +{ +u8 error_code =0 ; + +switch (dev->driver_info->family) { +case KVASER_LEAF : + error_code = cmd->u.leaf.error_event.error_code; +break ; +case KVASER_USBCAN : + error_code = cmd->u.usbcan.error_event.error_code; +break ; + } + +switch (error_code) { +case KVASER_USB_LEAF_ERROR_EVENT_TX_QUEUE_FULL : +/* Received additional CAN message, when firmware TX queue is +* already full. Something is wrong with the driver. +* This should never happen! +*/ +dev_err (&dev->intf->dev, +"Received error event TX_QUEUE_FULL\n" ); +break ; +case KVASER_USB_LEAF_ERROR_EVENT_PARAM : +kvaser_usb_leaf_error_event_parameter (dev, cmd); +break ; + +default : +dev_warn (&dev->intf->dev, +"Unhandled error event (%d)\n" , error_code); +break ; + } +} +static void kvaser_usb_leaf_start_chip_reply (const struct kvaser_usb *dev,const struct kvaser_cmd *cmd) {
@@ -1382,6 +1477,10 @@ static void kvaser_usb_leaf_handle_command(const struct kvaser_usb *dev,kvaser_usb_leaf_tx_acknowledge (dev, cmd);break ; +case CMD_ERROR_EVENT : +kvaser_usb_leaf_error_event (dev, cmd); +break ; +/* Ignored commands */ case CMD_USBCAN_CLOCK_OVERFLOW_EVENT :if (dev->driver_info->family !=KVASER_USBCAN )
Signature · 1 line
2.37.3
PATCH v3 7/15 can: kvaser_usb_leaf: Set Warning state even without bus errors
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
kvaser_usb_leaf_rx_error_update_can_state() sets error state according to error counters when the hardware does not indicate a specific state directly.
However, this is currently gated behind a check for M16C_STATE_BUS_ERROR which does not always seem to be set when error counters are increasing, and may not be set when error counters are decreasing.
This causes the CAN_STATE_ERROR_WARNING state to not be set in some cases even when appropriate.
Change the code to set error state from counters even without M16C_STATE_BUS_ERROR.
The Error-Passive case seems superfluous as it is already set via M16C_STATE_BUS_PASSIVE flag above, but it is kept for now.
Tested with 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +8 -12
@@ -961,20 +961,16 @@ kvaser_usb_leaf_rx_error_update_can_state(struct kvaser_usb_net_priv *priv, new_state =CAN_STATE_BUS_OFF ; }else if (es->status &M16C_STATE_BUS_PASSIVE ) { new_state =CAN_STATE_ERROR_PASSIVE ; - }else if (es->status &M16C_STATE_BUS_ERROR ) { + }else if ((es->status &M16C_STATE_BUS_ERROR ) && + cur_state >=CAN_STATE_BUS_OFF ) {/* Guard against spurious error events after a busoff */ -if (cur_state <CAN_STATE_BUS_OFF ) { -if (es->txerr >=128 || es->rxerr >=128 ) - new_state =CAN_STATE_ERROR_PASSIVE ; -else if (es->txerr >=96 || es->rxerr >=96 ) - new_state =CAN_STATE_ERROR_WARNING ; -else if (cur_state >CAN_STATE_ERROR_ACTIVE ) - new_state =CAN_STATE_ERROR_ACTIVE ; - } - } - -if (!es->status) + }else if (es->txerr >=128 || es->rxerr >=128 ) { + new_state =CAN_STATE_ERROR_PASSIVE ; + }else if (es->txerr >=96 || es->rxerr >=96 ) { + new_state =CAN_STATE_ERROR_WARNING ; + }else { new_state =CAN_STATE_ERROR_ACTIVE ; + }if (new_state != cur_state) { tx_state = (es->txerr >= es->rxerr) ? new_state :0 ;
Signature · 1 line
2.37.3
PATCH v3 8/15 can: kvaser_usb_leaf: Fix TX queue out of sync after restart
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
The TX queue seems to be implicitly flushed by the hardware during bus-off or bus-off recovery, but the driver does not reset the TX bookkeeping.
Despite not resetting TX bookkeeping the driver still re-enables TX queue unconditionally, leading to "cannot find free context" / NETDEV_TX_BUSY errors if the TX queue was full at bus-off time.
Fix that by resetting TX bookkeeping on CAN restart.
Tested with 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
- Removed explicit queue flush.
drivers/net/can/usb/kvaser_usb/kvaser_usb.h | 2 ++ drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 2 +- drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb.h +2 -0
@@ -178,6 +178,8 @@ struct kvaser_usb_dev_cfg {extern const struct kvaser_usb_dev_ops kvaser_usb_hydra_dev_ops;extern const struct kvaser_usb_dev_ops kvaser_usb_leaf_dev_ops; +void kvaser_usb_unlink_tx_urbs (struct kvaser_usb_net_priv *priv); +int kvaser_usb_recv_cmd (const struct kvaser_usb *dev,void *cmd,int len,int *actual_len);
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c +1 -1
@@ -477,7 +477,7 @@ static void kvaser_usb_reset_tx_urb_contexts(struct kvaser_usb_net_priv *priv)/* This method might sleep. Do not call it in the atomic context * of URB completions. */ -static void kvaser_usb_unlink_tx_urbs (struct kvaser_usb_net_priv *priv) +void kvaser_usb_unlink_tx_urbs (struct kvaser_usb_net_priv *priv) {usb_kill_anchored_urbs (&priv->tx_submitted);kvaser_usb_reset_tx_urb_contexts (priv);
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +2 -0
@@ -1663,6 +1663,8 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev,switch (mode) {case CAN_MODE_START : +kvaser_usb_unlink_tx_urbs (priv); + err =kvaser_usb_leaf_simple_cmd_async (priv,CMD_START_CHIP );if (err)return err;
Signature · 1 line
2.37.3
PATCH v3 9/15 can: kvaser_usb_leaf: Fix CAN state after restart
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
can_restart() expects CMD_START_CHIP to set the error state to ERROR_ACTIVE as it calls netif_carrier_on() immediately afterwards.
Otherwise the user may immediately trigger restart again and hit a BUG_ON() in can_restart().
Fix kvaser_usb_leaf set_mode(CMD_START_CHIP) to set the expected state.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +2 -0
@@ -1668,6 +1668,8 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev, err =kvaser_usb_leaf_simple_cmd_async (priv,CMD_START_CHIP );if (err)return err; + + priv->can.state =CAN_STATE_ERROR_ACTIVE ;break ;default :return -EOPNOTSUPP ;
Signature · 1 line
2.37.3
PATCH v3 10/15 can: kvaser_usb_leaf: Fix improved state not being reported
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
The tested 0bfd:0017 Kvaser Memorator Professional HS/HS FW 2.0.50 and 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778 do not seem to send any unsolicited events when error counters decrease or when the device transitions from ERROR_PASSIVE to ERROR_ACTIVE (or WARNING).
This causes the interface to e.g. indefinitely stay in the ERROR_PASSIVE state.
Fix that by asking for chip state (inc. counters) event every 0.5 secs when error counters are non-zero.
Since there are non-error-counter devices, also always poll in ERROR_PASSIVE even if the counters show zero.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
- Rephrase commit message regarding non-error-counter devices.
drivers/net/can/usb/kvaser_usb/kvaser_usb.h | 7 +++ .../net/can/usb/kvaser_usb/kvaser_usb_core.c | 19 +++++- .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 58 +++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb.h +7 -0
@@ -104,6 +104,9 @@ struct kvaser_usb_net_priv {struct can_priv can;struct can_berr_counter bec; +/* subdriver-specific data */ +void *sub_priv; +struct kvaser_usb *dev;struct net_device *netdev;int channel;
@@ -125,6 +128,8 @@ struct kvaser_usb_net_priv {* * @dev_setup_endpoints: setup USB in and out endpoints * @dev_init_card: initialize card +* @dev_init_channel: initialize channel +* @dev_remove_channel: uninitialize channel * @dev_get_software_info: get software info * @dev_get_software_details: get software details * @dev_get_card_info: get card info
@@ -146,6 +151,8 @@ struct kvaser_usb_dev_ops { struct can_berr_counter *bec);int (*dev_setup_endpoints)(struct kvaser_usb *dev);int (*dev_init_card)(struct kvaser_usb *dev); +int (*dev_init_channel)(struct kvaser_usb_net_priv *priv); +void (*dev_remove_channel)(struct kvaser_usb_net_priv *priv);int (*dev_get_software_info)(struct kvaser_usb *dev);int (*dev_get_software_details)(struct kvaser_usb *dev);int (*dev_get_card_info)(struct kvaser_usb *dev);
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c +16 -3
@@ -684,6 +684,7 @@ static const struct ethtool_ops kvaser_usb_ethtool_ops_hwts = {static void kvaser_usb_remove_interfaces (struct kvaser_usb *dev) { +const struct kvaser_usb_dev_ops *ops = dev->driver_info->ops;int i;for (i =0 ; i < dev->nchannels; i++) {
@@ -699,6 +700,9 @@ static void kvaser_usb_remove_interfaces(struct kvaser_usb *dev)if (!dev->nets[i])continue ; +if (ops->dev_remove_channel) + ops->dev_remove_channel (dev->nets[i]); +free_candev (dev->nets[i]->netdev); } }
@@ -772,17 +776,26 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev, int channel) dev->nets[channel] = priv; +if (ops->dev_init_channel) { + err = ops->dev_init_channel (priv); +if (err) + goto err; + } + err =register_candev (netdev);if (err) {dev_err (&dev->intf->dev,"Failed to register CAN device\n" ); -free_candev (netdev); - dev->nets[channel] =NULL ; -return err; + goto err; }netdev_dbg (netdev,"device registered\n" );return 0 ; + +err: +free_candev (netdev); + dev->nets[channel] =NULL ; +return err; } static int kvaser_usb_probe(struct usb_interface *intf,
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +58 -0
122 lines not on this page · raw message
Signature · 1 line
2.37.3
PATCH v3 11/15 can: kvaser_usb_leaf: Fix wrong CAN state after stopping
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778 sends a CMD_CHIP_STATE_EVENT indicating bus-off after stopping the device, causing a stopped device to appear as CAN_STATE_BUS_OFF instead of CAN_STATE_STOPPED.
Fix that by not handling error events on stopped devices.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +4 -0
@@ -1045,6 +1045,10 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev, leaf = priv->sub_priv; stats = &priv->netdev->stats; +/* Ignore e.g. state change to bus-off reported just after stopping */ +if (!netif_running (priv->netdev)) +return ; +/* Update all of the CAN interface's state and error counters before * trying any memory allocation that can actually fail with -ENOMEM. *
Signature · 1 line
2.37.3
PATCH v3 12/15 can: kvaser_usb_leaf: Ignore stale bus-off after start
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
With 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778 it was observed that if the device was bus-off when stopped, at next start (either via interface down/up or manual bus-off restart) the initial CMD_CHIP_STATE_EVENT received just after CMD_START_CHIP_REPLY will have the M16C_STATE_BUS_OFF bit still set, causing the interface to immediately go bus-off again.
The bit seems to internally clear quickly afterwards but we do not get another CMD_CHIP_STATE_EVENT.
Fix the issue by ignoring any initial bus-off state until we see at least one bus-on state. Also, poll the state periodically until that occurs.
It is possible we lose one actual immediately occurring bus-off event here in which case the HW will auto-recover and we see the recovery event. We will then catch the next bus-off event, if any.
This issue did not reproduce with 0bfd:0017 Kvaser Memorator Professional HS/HS FW 2.0.50.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
.../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +30 -1
@@ -427,6 +427,9 @@ struct kvaser_usb_net_leaf_priv {struct kvaser_usb_net_priv *net;struct delayed_work chip_state_req_work; + +/* started but not reported as bus-on yet */ +bool joining_bus; };static const struct can_bittiming_const kvaser_usb_leaf_m16c_bittiming_const = {
@@ -966,6 +969,7 @@ kvaser_usb_leaf_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,const struct kvaser_usb_err_summary *es, struct can_frame *cf) { + struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;struct kvaser_usb *dev = priv->dev;struct net_device_stats *stats = &priv->netdev->stats;enum can_state cur_state, new_state, tx_state, rx_state;
@@ -990,6 +994,22 @@ kvaser_usb_leaf_rx_error_update_can_state(struct kvaser_usb_net_priv *priv, new_state =CAN_STATE_ERROR_ACTIVE ; } +/* 0bfd:0124 FW 4.18.778 was observed to send the initial +* CMD_CHIP_STATE_EVENT after CMD_START_CHIP with M16C_STATE_BUS_OFF +* bit set if the channel was bus-off when it was last stopped (even +* across chip resets). This bit will clear shortly afterwards, without +* triggering a second unsolicited chip state event. +* Ignore this initial bus-off. +*/ +if (leaf->joining_bus) { +if (new_state ==CAN_STATE_BUS_OFF ) { +netdev_dbg (priv->netdev,"ignoring bus-off during startup" ); + new_state = cur_state; + }else { + leaf->joining_bus = false; + } + } +if (new_state != cur_state) { tx_state = (es->txerr >= es->rxerr) ? new_state :0 ; rx_state = (es->txerr <= es->rxerr) ? new_state :0 ;
@@ -1065,9 +1085,12 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,/* If there are errors, request status updates periodically as we do * not get automatic notifications of improved state. +* Also request updates if we saw a stale BUS_OFF during startup +* (joining_bus). */ if (new_state <CAN_STATE_BUS_OFF && - (es->rxerr || es->txerr || new_state ==CAN_STATE_ERROR_PASSIVE )) + (es->rxerr || es->txerr || new_state ==CAN_STATE_ERROR_PASSIVE || + leaf->joining_bus))schedule_delayed_work (&leaf->chip_state_req_work,msecs_to_jiffies (500 ));
@@ -1587,8 +1610,11 @@ static int kvaser_usb_leaf_set_opt_mode(const struct kvaser_usb_net_priv *priv)static int kvaser_usb_leaf_start_chip (struct kvaser_usb_net_priv *priv) { +struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;int err; + leaf->joining_bus = true; +reinit_completion (&priv->start_comp); err = kvaser_usb_leaf_send_simple_cmd(priv->dev, CMD_START_CHIP,
@@ -1719,12 +1745,15 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev, enum can_mode mode) {struct kvaser_usb_net_priv *priv =netdev_priv (netdev); +struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;int err;switch (mode) {case CAN_MODE_START :kvaser_usb_unlink_tx_urbs (priv); + leaf->joining_bus = true; + err =kvaser_usb_leaf_simple_cmd_async (priv,CMD_START_CHIP );if (err)return err;
Signature · 1 line
2.37.3
PATCH v3 13/15 can: kvaser_usb_leaf: Fix bogus restart events
In reply to Jimmy AssarssonFrom: Anssi Hannula <[email protected]>
When auto-restart is enabled, the kvaser_usb_leaf driver considers transition from any state >= CAN_STATE_BUS_OFF as a bus-off recovery event (restart).
However, these events may occur at interface startup time before kvaser_usb_open() has set the state to CAN_STATE_ERROR_ACTIVE, causing restarts counter to increase and CAN_ERR_RESTARTED to be sent despite no actual restart having occurred.
Fix that by making the auto-restart condition checks more strict so that they only trigger when the interface was actually in the BUS_OFF state.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add S-o-b
Changes in v2:
- Rebased on b3b6df2c56d8 ("can: kvaser_usb: kvaser_usb_leaf: fix bittiming limits")
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +3 -3
@@ -901,7 +901,7 @@ static void kvaser_usb_leaf_tx_acknowledge(const struct kvaser_usb *dev, context = &priv->tx_contexts[tid % dev->max_tx_urbs];/* Sometimes the state change doesn't come after a bus-off event */ -if (priv->can.restart_ms && priv->can.state >=CAN_STATE_BUS_OFF ) { +if (priv->can.restart_ms && priv->can.state ==CAN_STATE_BUS_OFF ) {struct sk_buff *skb;struct can_frame *cf;
@@ -1018,7 +1018,7 @@ kvaser_usb_leaf_rx_error_update_can_state(struct kvaser_usb_net_priv *priv, }if (priv->can.restart_ms && - cur_state >=CAN_STATE_BUS_OFF && + cur_state ==CAN_STATE_BUS_OFF && new_state <CAN_STATE_BUS_OFF ) priv->can.can_stats.restarts++;
@@ -1111,7 +1111,7 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev, }if (priv->can.restart_ms && - old_state >=CAN_STATE_BUS_OFF && + old_state ==CAN_STATE_BUS_OFF && new_state <CAN_STATE_BUS_OFF ) { cf->can_id |=CAN_ERR_RESTARTED ;netif_carrier_on (priv->netdev);
Signature · 1 line
2.37.3
PATCH v3 14/15 can: kvaser_usb: Add struct kvaser_usb_busparams
In reply to Jimmy AssarssonAdd struct kvaser_usb_busparams containing the busparameters used in CMD_{SET,GET}_BUSPARAMS* commands.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add Tested-by Anssi Hannula
Changes in v2:
- New in v2.
drivers/net/can/usb/kvaser_usb/kvaser_usb.h | 8 +++++ .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 32 +++++++------------ .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 18 ++++------- 3 files changed, 27 insertions(+), 31 deletions(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb.h +8 -0
@@ -76,6 +76,14 @@ struct kvaser_usb_tx_urb_context {u32 echo_index; }; +struct kvaser_usb_busparams { +__le32 bitrate; +u8 tseg1; +u8 tseg2; +u8 sjw; +u8 nsamples; +}; +struct kvaser_usb {struct usb_device *udev;struct usb_interface *intf;
drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c +12 -20
@@ -196,17 +196,9 @@ struct kvaser_cmd_chip_state_event {#define KVASER_USB_HYDRA_BUS_MODE_CANFD_ISO 0x01#define KVASER_USB_HYDRA_BUS_MODE_NONISO 0x02struct kvaser_cmd_set_busparams { -__le32 bitrate; -u8 tseg1; -u8 tseg2; -u8 sjw; -u8 nsamples; +struct kvaser_usb_busparams busparams_arb;u8 reserved0[4 ]; -__le32 bitrate_d; -u8 tseg1_d; -u8 tseg2_d; -u8 sjw_d; -u8 nsamples_d; +struct kvaser_usb_busparams busparams_data;u8 canfd_mode;u8 reserved1[7 ]; } __packed;
@@ -1538,11 +1530,11 @@ static int kvaser_usb_hydra_set_bittiming(struct net_device *netdev)return -ENOMEM ; cmd->header.cmd_no =CMD_SET_BUSPARAMS_REQ ; - cmd->set_busparams_req.bitrate =cpu_to_le32 (bt->bitrate); - cmd->set_busparams_req.sjw = (u8 )sjw; - cmd->set_busparams_req.tseg1 = (u8 )tseg1; - cmd->set_busparams_req.tseg2 = (u8 )tseg2; - cmd->set_busparams_req.nsamples =1 ; + cmd->set_busparams_req.busparams_arb.bitrate =cpu_to_le32 (bt->bitrate); + cmd->set_busparams_req.busparams_arb.sjw = (u8 )sjw; + cmd->set_busparams_req.busparams_arb.tseg1 = (u8 )tseg1; + cmd->set_busparams_req.busparams_arb.tseg2 = (u8 )tseg2; + cmd->set_busparams_req.busparams_arb.nsamples =1 ;kvaser_usb_hydra_set_cmd_dest_he (cmd, dev->card_data.hydra.channel_to_he[priv->channel]);
@@ -1572,11 +1564,11 @@ static int kvaser_usb_hydra_set_data_bittiming(struct net_device *netdev)return -ENOMEM ; cmd->header.cmd_no =CMD_SET_BUSPARAMS_FD_REQ ; - cmd->set_busparams_req.bitrate_d =cpu_to_le32 (dbt->bitrate); - cmd->set_busparams_req.sjw_d = (u8 )sjw; - cmd->set_busparams_req.tseg1_d = (u8 )tseg1; - cmd->set_busparams_req.tseg2_d = (u8 )tseg2; - cmd->set_busparams_req.nsamples_d =1 ; + cmd->set_busparams_req.busparams_data.bitrate =cpu_to_le32 (dbt->bitrate); + cmd->set_busparams_req.busparams_data.sjw = (u8 )sjw; + cmd->set_busparams_req.busparams_data.tseg1 = (u8 )tseg1; + cmd->set_busparams_req.busparams_data.tseg2 = (u8 )tseg2; + cmd->set_busparams_req.busparams_data.nsamples =1 ;if (priv->can.ctrlmode &CAN_CTRLMODE_FD ) {if (priv->can.ctrlmode &CAN_CTRLMODE_FD_NON_ISO )
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +7 -11
@@ -164,11 +164,7 @@ struct usbcan_cmd_softinfo {struct kvaser_cmd_busparams {u8 tid;u8 channel; -__le32 bitrate; -u8 tseg1; -u8 tseg2; -u8 sjw; -u8 no_samp; +struct kvaser_usb_busparams busparams; } __packed;struct kvaser_cmd_tx_can {
@@ -1725,15 +1721,15 @@ static int kvaser_usb_leaf_set_bittiming(struct net_device *netdev) cmd->len =CMD_HEADER_LEN +sizeof (struct kvaser_cmd_busparams ); cmd->u.busparams.channel = priv->channel; cmd->u.busparams.tid =0xff ; - cmd->u.busparams.bitrate =cpu_to_le32 (bt->bitrate); - cmd->u.busparams.sjw = bt->sjw; - cmd->u.busparams.tseg1 = bt->prop_seg + bt->phase_seg1; - cmd->u.busparams.tseg2 = bt->phase_seg2; + cmd->u.busparams.busparams.bitrate =cpu_to_le32 (bt->bitrate); + cmd->u.busparams.busparams.sjw = bt->sjw; + cmd->u.busparams.busparams.tseg1 = bt->prop_seg + bt->phase_seg1; + cmd->u.busparams.busparams.tseg2 = bt->phase_seg2;if (priv->can.ctrlmode &CAN_CTRLMODE_3_SAMPLES ) - cmd->u.busparams.no_samp =3 ; + cmd->u.busparams.busparams.nsamples =3 ;else - cmd->u.busparams.no_samp =1 ; + cmd->u.busparams.busparams.nsamples =1 ; rc =kvaser_usb_send_cmd (dev, cmd, cmd->len);
Signature · 1 line
2.37.3
PATCH v3 15/15 can: kvaser_usb: Compare requested bittiming parameters with actual parameters in do_set_{,data}_bittiming
In reply to Jimmy AssarssonThe device will respond with a CMD_ERROR_EVENT command, with error_code KVASER_USB_{LEAF,HYDRA}_ERROR_EVENT_PARAM, if the CMD_SET_BUSPARAMS_REQ contains invalid bittiming parameters.
However, this command does not contain any channel reference.
To check if the CMD_SET_BUSPARAMS_REQ was successful, redback and compare the requested bittiming parameters with the device reported parameters.
---
Changes in v3:
- Rebased on 1d5eeda23f36 ("can: kvaser_usb: advertise timestamping capabilities and add ioctl support")
- Add stable to CC
- Add Tested-by Anssi Hannula
Changes in v2:
- New in v2. Replaces parts of [PATCH 09/12] can: kvaser_usb_leaf: Fix silently failing bus params
setup
drivers/net/can/usb/kvaser_usb/kvaser_usb.h | 15 +- .../net/can/usb/kvaser_usb/kvaser_usb_core.c | 96 ++++++++++- .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 150 +++++++++++++++--- .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 64 ++++++-- 4 files changed, 284 insertions(+), 41 deletions(-)
drivers/net/can/usb/kvaser_usb/kvaser_usb.h +12 -3
@@ -119,9 +119,12 @@ struct kvaser_usb_net_priv {struct net_device *netdev;int channel; -struct completion start_comp, stop_comp, flush_comp; +struct completion start_comp, stop_comp, flush_comp, + get_busparams_comp;struct usb_anchor tx_submitted; +struct kvaser_usb_busparams busparams_nominal, busparams_data; +spinlock_t tx_contexts_lock;/* lock for active_tx_contexts */ int active_tx_contexts;struct kvaser_usb_tx_urb_context tx_contexts[];
@@ -131,7 +134,9 @@ struct kvaser_usb_net_priv {* struct kvaser_usb_dev_ops - Device specific functions * @dev_set_mode: used for can.do_set_mode * @dev_set_bittiming: used for can.do_set_bittiming +* @dev_get_busparams: readback arbitration busparams * @dev_set_data_bittiming: used for can.do_set_data_bittiming +* @dev_get_data_busparams: readback data busparams * @dev_get_berr_counter: used for can.do_get_berr_counter * * @dev_setup_endpoints: setup USB in and out endpoints
@@ -153,8 +158,12 @@ struct kvaser_usb_net_priv {*/ struct kvaser_usb_dev_ops {int (*dev_set_mode)(struct net_device *netdev,enum can_mode mode); -int (*dev_set_bittiming)(struct net_device *netdev); -int (*dev_set_data_bittiming)(struct net_device *netdev); +int (*dev_set_bittiming)(const struct net_device *netdev, +const struct kvaser_usb_busparams *busparams); +int (*dev_get_busparams)(struct kvaser_usb_net_priv *priv); +int (*dev_set_data_bittiming)(const struct net_device *netdev, +const struct kvaser_usb_busparams *busparams); +int (*dev_get_data_busparams)(struct kvaser_usb_net_priv *priv);int (*dev_get_berr_counter)(const struct net_device *netdev,struct can_berr_counter *bec);int (*dev_setup_endpoints)(struct kvaser_usb *dev);
drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c +90 -6
126 lines not on this page · raw message
drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c +129 -21
240 lines not on this page · raw message
drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c +53 -11
115 lines not on this page · raw message
Signature · 1 line
2.37.3