[PATCH 00/12] can: kvaser_usb: Various fixes

[PATCH 00/12] can: kvaser_usb: Various fixes

[Anssi Hannula]

Hi all,

Here's a set of fixes for issues I found while testing kvaser_usb as we
are preparing to start using it in production (with 0bfd:0124).

The biggest caveat is that I only have two devices to test with [1] and I
don't have HW documentation, so there is a possibility that some of the
fixes might not work properly on all HW variants.
Hopefully Jimmy can confirm they look OK, or suggest alternatives.

[1] Tested devices:
- 0bfd:0017 Kvaser Memorator Professional HS/HS FW 2.0.50
- 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778

-- 
Anssi Hannula / Bitwise Oy
+358503803997

[PATCH 01/12] can: kvaser_usb_leaf: Fix overread with an invalid command

[Anssi Hannula]

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.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

---

A simpler option would be to just zero-pad the data into a
stack-allocated struct kvaser_cmd without knowledge of the needed
minimum size (so no tables needed), though that would mean incomplete
commands would get passed on silently.

 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index c805b999c543..d9f40b9702a5 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -320,6 +320,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:
  *
@@ -387,6 +419,43 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_32mhz = {
 	.bittiming_const = &kvaser_usb_leaf_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->card_data.leaf.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,
@@ -492,6 +561,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;
 }
 
@@ -1113,6 +1185,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);
-- 
2.34.1

Re: [PATCH 01/12] can: kvaser_usb_leaf: Fix overread with an invalid command

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.
> 
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>


> ---
> 
> A simpler option would be to just zero-pad the data into a
> stack-allocated struct kvaser_cmd without knowledge of the needed
> minimum size (so no tables needed), though that would mean incomplete
> commands would get passed on silently.
> 
>   .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 75 +++++++++++++++++++
>   1 file changed, 75 insertions(+)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index c805b999c543..d9f40b9702a5 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -320,6 +320,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:
>    *
> @@ -387,6 +419,43 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_32mhz = {
>   	.bittiming_const = &kvaser_usb_leaf_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->card_data.leaf.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,
> @@ -492,6 +561,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;
>   }
>   
> @@ -1113,6 +1185,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);

[PATCH 02/12] can: kvaser_usb: Fix use of uninitialized completion

[Anssi Hannula]

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.

Fixes: aec5fb2268b7 ("can: kvaser_usb: Add support for Kvaser USB hydra family")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 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(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index e67658b53d02..47bff40c36b6 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -673,6 +673,7 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev,
 	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;
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index a26823c5b62a..4e90a4b7f95a 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -1910,7 +1910,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);
-- 
2.34.1

Re: [PATCH 02/12] can: kvaser_usb: Fix use of uninitialized completion

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.
> 
> Fixes: aec5fb2268b7 ("can: kvaser_usb: Add support for Kvaser USB hydra family")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>


> ---
>   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(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> index e67658b53d02..47bff40c36b6 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> @@ -673,6 +673,7 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev,
>   	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;
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> index a26823c5b62a..4e90a4b7f95a 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> @@ -1910,7 +1910,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);

[PATCH 03/12] can: kvaser_usb: Fix possible completions during init_completion

[Anssi Hannula]

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.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 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(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index 4e90a4b7f95a..b0094f162964 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -1869,7 +1869,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);
@@ -1887,7 +1887,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()
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index d9f40b9702a5..09ade66256b2 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -1300,7 +1300,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);
@@ -1318,7 +1318,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);
-- 
2.34.1

Re: [PATCH 03/12] can: kvaser_usb: Fix possible completions during init_completion

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.
> 
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>

> ---
>   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(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> index 4e90a4b7f95a..b0094f162964 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> @@ -1869,7 +1869,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);
> @@ -1887,7 +1887,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()
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index d9f40b9702a5..09ade66256b2 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -1300,7 +1300,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);
> @@ -1318,7 +1318,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);

[PATCH 04/12] can: kvaser_usb: Mark Mini PCIe 2xHS as supporting error counters

[Anssi Hannula]

The 0bfd:0124 Kvaser Mini PCI Express 2xHS (FW 4.18.778) seems to support
TX/RX error counters in exactly the same way (via unsolicited cmd 106 on
bus errors and via cmd 20 when queried with cmd 19) as 0bfd:0017 Kvaser
Memorator Professional HS/HS (FW 2.0.50), but only the latter has
KVASER_USB_HAS_TXRX_ERRORS set to enable do_get_berr_counter().

Enable error counter retrieval for Kvaser Mini PCI Express 2xHS, too.

Fixes: 71873a9b38d1 ("can: kvaser_usb: Add support for more Kvaser Leaf v2 devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

---

I'm not really sure what KVASER_USB_HAS_TXRX_ERRORS means, exactly,
w.r.t. device behavior, though, i.e. how does a device without it behave.


 drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index 47bff40c36b6..7388fdca9079 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -165,7 +165,8 @@ static const struct usb_device_id kvaser_usb_table[] = {
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_HS_PRODUCT_ID) },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LIGHT_HS_V2_OEM_PRODUCT_ID) },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN_LIGHT_2HS_PRODUCT_ID) },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_2HS_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_2HS_PRODUCT_ID),
+		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN_R_V2_PRODUCT_ID) },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LIGHT_R_V2_PRODUCT_ID) },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LIGHT_HS_V2_OEM2_PRODUCT_ID) },
-- 
2.34.1

Re: [PATCH 04/12] can: kvaser_usb: Mark Mini PCIe 2xHS as supporting error counters

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> The 0bfd:0124 Kvaser Mini PCI Express 2xHS (FW 4.18.778) seems to support
> TX/RX error counters in exactly the same way (via unsolicited cmd 106 on
> bus errors and via cmd 20 when queried with cmd 19) as 0bfd:0017 Kvaser
> Memorator Professional HS/HS (FW 2.0.50), but only the latter has
> KVASER_USB_HAS_TXRX_ERRORS set to enable do_get_berr_counter().
> 
> Enable error counter retrieval for Kvaser Mini PCI Express 2xHS, too.
> 
> Fixes: 71873a9b38d1 ("can: kvaser_usb: Add support for more Kvaser Leaf v2 devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> 
> ---
> 
> I'm not really sure what KVASER_USB_HAS_TXRX_ERRORS means, exactly,
> w.r.t. device behavior, though, i.e. how does a device without it behave.

Devices without KVASER_USB_HAS_TXRX_ERRORS, firmware will always report
zero for the Rx and Tx error counters.

>   drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> index 47bff40c36b6..7388fdca9079 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> @@ -165,7 +165,8 @@ static const struct usb_device_id kvaser_usb_table[] = {
>   	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_HS_PRODUCT_ID) },
>   	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LIGHT_HS_V2_OEM_PRODUCT_ID) },
>   	{ USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN_LIGHT_2HS_PRODUCT_ID) },
> -	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_2HS_PRODUCT_ID) },
> +	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_2HS_PRODUCT_ID),
> +		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
>   	{ USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN_R_V2_PRODUCT_ID) },
>   	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LIGHT_R_V2_PRODUCT_ID) },
>   	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LIGHT_HS_V2_OEM2_PRODUCT_ID) },



It's possible to query the device for specific capabilities.
i.e. Kvaser Mini PCI Express 2xHS does also got support for silent mode.

I want to replace this patch with the one below:

 From fbf1c02e5f7860be9bdafd1c9b4f01c903dd9258 Mon Sep 17 00:00:00 2001
From: Jimmy Assarsson <extja@kvaser.com>
Date: Wed, 25 May 2022 20:21:19 +0200
Subject: [PATCH 04/13] can: kvaser_usb: kvaser_usb_leaf: Get 
capabilities from
  device

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

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB 
devices")
Reported-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
  .../net/can/usb/kvaser_usb/kvaser_usb_core.c  |  61 ++------
  .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 146 +++++++++++++++++-
  2 files changed, 162 insertions(+), 45 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c 
b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index 47bff40c36b6..247caf0982bc 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -116,51 +116,24 @@ static const struct usb_device_id 
kvaser_usb_table[] = {
  	/* Leaf USB product IDs */
  	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
  	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LS_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_SWC_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LIN_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_LS_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_SWC_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_DEVEL_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSHS_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_UPRO_HSHS_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LS_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_SWC_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LIN_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_LS_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_SWC_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_DEVEL_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSHS_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_UPRO_HSHS_PRODUCT_ID) },
  	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_GI_PRODUCT_ID) },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_OBDII_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS |
-			       KVASER_USB_HAS_SILENT_MODE },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSLS_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_CH_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_BLACKBIRD_SPRO_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_MERCURY_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_LEAF_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
-	{ USB_DEVICE(KVASER_VENDOR_ID, USB_CAN_R_PRODUCT_ID),
-		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_OBDII_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSLS_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_CH_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_BLACKBIRD_SPRO_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_MERCURY_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_LEAF_PRODUCT_ID) },
+	{ USB_DEVICE(KVASER_VENDOR_ID, USB_CAN_R_PRODUCT_ID) },
  	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_V2_PRODUCT_ID) },
  	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_HS_PRODUCT_ID) },
  	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LIGHT_HS_V2_OEM_PRODUCT_ID) },
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c 
b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 09ade66256b2..aee2dae67459 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -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)
@@ -288,6 +292,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;
@@ -305,6 +331,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 {
@@ -334,6 +362,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,
  };
@@ -596,6 +625,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;
+
  	switch (sw_options & KVASER_USB_LEAF_SWOPTION_FREQ_MASK) {
  	case KVASER_USB_LEAF_SWOPTION_FREQ_16_MHZ_CLK:
  		dev->cfg = &kvaser_usb_leaf_dev_cfg_16mhz;
@@ -676,6 +708,118 @@ 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 = kcalloc(1, sizeof(struct kvaser_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->card_data.leaf.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)
  {
@@ -1462,7 +1606,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,
-- 
2.36.1

Re: [PATCH 04/12] can: kvaser_usb: Mark Mini PCIe 2xHS as supporting error counters

[Anssi Hannula]

On 28.5.2022 10.37, Jimmy Assarsson wrote:
> On 5/16/22 15:47, Anssi Hannula wrote:
>> The 0bfd:0124 Kvaser Mini PCI Express 2xHS (FW 4.18.778) seems to support
>> TX/RX error counters in exactly the same way (via unsolicited cmd 106 on
>> bus errors and via cmd 20 when queried with cmd 19) as 0bfd:0017 Kvaser
>> Memorator Professional HS/HS (FW 2.0.50), but only the latter has
>> KVASER_USB_HAS_TXRX_ERRORS set to enable do_get_berr_counter().
>>
>> Enable error counter retrieval for Kvaser Mini PCI Express 2xHS, too.
>>
>> Fixes: 71873a9b38d1 ("can: kvaser_usb: Add support for more Kvaser Leaf v2 devices")
>> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
>>
>> ---
>>
>> I'm not really sure what KVASER_USB_HAS_TXRX_ERRORS means, exactly,
>> w.r.t. device behavior, though, i.e. how does a device without it behave.
> Devices without KVASER_USB_HAS_TXRX_ERRORS, firmware will always report
> zero for the Rx and Tx error counters.
>
> It's possible to query the device for specific capabilities.
> i.e. Kvaser Mini PCI Express 2xHS does also got support for silent mode.
> I want to replace this patch with the one below:

Sounds good!
A couple of minor style comments below. I didn't test the code yet.

>  From fbf1c02e5f7860be9bdafd1c9b4f01c903dd9258 Mon Sep 17 00:00:00 2001
> From: Jimmy Assarsson <extja@kvaser.com>
> Date: Wed, 25 May 2022 20:21:19 +0200
> Subject: [PATCH 04/13] can: kvaser_usb: kvaser_usb_leaf: Get 
> capabilities from
>   device
>
> Use 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.
>
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB 
> devices")
> Reported-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>   .../net/can/usb/kvaser_usb/kvaser_usb_core.c  |  61 ++------
>   .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 146 +++++++++++++++++-
>   2 files changed, 162 insertions(+), 45 deletions(-)
[...]
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c 
> b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 09ade66256b2..aee2dae67459 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
[...]
> +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 = kcalloc(1, sizeof(struct kvaser_cmd), GFP_KERNEL);

kzalloc can be used here, and prefer sizeof(*ptr) to avoid the risk of
type mismatch:

cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);


[...]
> +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);

I believe kernel style is to keep the opening parenthesis on the same
line with the function name here.

> +	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);


Ditto.

[...]

-- 
Anssi Hannula / Bitwise Oy
+358 503803997

[PATCH 05/12] can: kvaser_usb_leaf: Set Warning state even without bus errors

[Anssi Hannula]

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.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 09ade66256b2..d7f2d64a8083 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -775,20 +775,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;
-- 
2.34.1

Re: [PATCH 05/12] can: kvaser_usb_leaf: Set Warning state even without bus errors

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.
> 
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>

> ---
>   .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 20 ++++++++-----------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 09ade66256b2..d7f2d64a8083 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -775,20 +775,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;

[PATCH 06/12] can: kvaser_usb_leaf: Fix TX queue out of sync after restart

[Anssi Hannula]

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.

Also, add an explicit queue flush in case some hardware versions do not
do that implicitly.

Tested with 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 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 | 6 ++++++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
index 3a49257f9fa6..f1bea13a3829 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
@@ -175,6 +175,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);
 
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index 7388fdca9079..a8d72fb8291a 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -440,7 +440,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);
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index d7f2d64a8083..2d30a662edb5 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -1402,6 +1402,12 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev,
 
 	switch (mode) {
 	case CAN_MODE_START:
+		err = kvaser_usb_leaf_flush_queue(priv);
+		if (err)
+			return err;
+
+		kvaser_usb_unlink_tx_urbs(priv);
+
 		err = kvaser_usb_leaf_simple_cmd_async(priv, CMD_START_CHIP);
 		if (err)
 			return err;
-- 
2.34.1

Re: [PATCH 06/12] can: kvaser_usb_leaf: Fix TX queue out of sync after restart

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.
> 
> Also, add an explicit queue flush in case some hardware versions do not
> do that implicitly.

See comment below regarding this.

> Tested with 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778.
> 
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> ---
>   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 | 6 ++++++
>   3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> index 3a49257f9fa6..f1bea13a3829 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> @@ -175,6 +175,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);
>   
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> index 7388fdca9079..a8d72fb8291a 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> @@ -440,7 +440,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);
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index d7f2d64a8083..2d30a662edb5 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -1402,6 +1402,12 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev,
>   
>   	switch (mode) {
>   	case CAN_MODE_START:
> +		err = kvaser_usb_leaf_flush_queue(priv);

All affected devices, leaf and usbcanII, will flush the Tx queue when
receiving CMD_START_CHIP.
So this is superfluous, and can be removed.

> +		if (err)
> +			return err;
> +
> +		kvaser_usb_unlink_tx_urbs(priv);
> +
>   		err = kvaser_usb_leaf_simple_cmd_async(priv, CMD_START_CHIP);
>   		if (err)
>   			return err;

[PATCH 07/12] can: kvaser_usb_leaf: Fix CAN state after restart

[Anssi Hannula]

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.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 2d30a662edb5..5f27c00179c1 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -1411,6 +1411,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;
-- 
2.34.1

Re: [PATCH 07/12] can: kvaser_usb_leaf: Fix CAN state after restart

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.
> 
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>


Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>

> ---
>   drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 2d30a662edb5..5f27c00179c1 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -1411,6 +1411,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;

[PATCH 08/12] can: kvaser_usb_leaf: Fix improved state not being reported

[Anssi Hannula]

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 the driver seems to be prepared for non-error-counter devices
(!KVASER_USB_HAS_TXRX_ERRORS) as well, also always poll in ERROR_PASSIVE
even if the counters show zero.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |  7 +++
 .../net/can/usb/kvaser_usb/kvaser_usb_core.c  | 18 +++++-
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 58 +++++++++++++++++++
 3 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
index f1bea13a3829..70aa7a9ed35b 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
@@ -107,6 +107,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;
@@ -128,6 +131,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
@@ -149,6 +154,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);
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index a8d72fb8291a..6a1ebdd9ba85 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -645,6 +645,9 @@ static void kvaser_usb_remove_interfaces(struct kvaser_usb *dev)
 		if (!dev->nets[i])
 			continue;
 
+		if (dev->ops->dev_remove_channel)
+			dev->ops->dev_remove_channel(dev->nets[i]);
+
 		free_candev(dev->nets[i]->netdev);
 	}
 }
@@ -712,17 +715,26 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev,
 
 	dev->nets[channel] = priv;
 
+	if (dev->ops->dev_init_channel) {
+		err = dev->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,
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 5f27c00179c1..abb681808a28 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -21,6 +21,7 @@
 #include <linux/types.h>
 #include <linux/units.h>
 #include <linux/usb.h>
+#include <linux/workqueue.h>
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
@@ -56,6 +57,7 @@
 #define CMD_RX_EXT_MESSAGE		14
 #define CMD_TX_EXT_MESSAGE		15
 #define CMD_SET_BUS_PARAMS		16
+#define CMD_GET_CHIP_STATE		19
 #define CMD_CHIP_STATE_EVENT		20
 #define CMD_SET_CTRL_MODE		21
 #define CMD_RESET_CHIP			24
@@ -375,6 +377,12 @@ struct kvaser_usb_err_summary {
 	};
 };
 
+struct kvaser_usb_net_leaf_priv {
+	struct kvaser_usb_net_priv *net;
+
+	struct delayed_work chip_state_req_work;
+};
+
 static const struct can_bittiming_const kvaser_usb_leaf_bittiming_const = {
 	.name = "kvaser_usb",
 	.tseg1_min = KVASER_USB_TSEG1_MIN,
@@ -757,6 +765,16 @@ static int kvaser_usb_leaf_simple_cmd_async(struct kvaser_usb_net_priv *priv,
 	return err;
 }
 
+static void kvaser_usb_leaf_chip_state_req_work(struct work_struct *work)
+{
+	struct kvaser_usb_net_leaf_priv *leaf =
+		container_of(work, struct kvaser_usb_net_leaf_priv,
+			     chip_state_req_work.work);
+	struct kvaser_usb_net_priv *priv = leaf->net;
+
+	kvaser_usb_leaf_simple_cmd_async(priv, CMD_GET_CHIP_STATE);
+}
+
 static void
 kvaser_usb_leaf_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
 					const struct kvaser_usb_err_summary *es,
@@ -828,6 +846,7 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
 	struct sk_buff *skb;
 	struct net_device_stats *stats;
 	struct kvaser_usb_net_priv *priv;
+	struct kvaser_usb_net_leaf_priv *leaf;
 	enum can_state old_state, new_state;
 
 	if (es->channel >= dev->nchannels) {
@@ -837,6 +856,7 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
 	}
 
 	priv = dev->nets[es->channel];
+	leaf = priv->sub_priv;
 	stats = &priv->netdev->stats;
 
 	/* Update all of the CAN interface's state and error counters before
@@ -853,6 +873,14 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
 	kvaser_usb_leaf_rx_error_update_can_state(priv, es, &tmp_cf);
 	new_state = priv->can.state;
 
+	/* If there are errors, request status updates periodically as we do
+	 * not get automatic notifications of improved state.
+	 */
+	if (new_state < CAN_STATE_BUS_OFF &&
+	    (es->rxerr || es->txerr || new_state == CAN_STATE_ERROR_PASSIVE))
+		schedule_delayed_work(&leaf->chip_state_req_work,
+				      msecs_to_jiffies(500));
+
 	skb = alloc_can_err_skb(priv->netdev, &cf);
 	if (!skb) {
 		stats->rx_dropped++;
@@ -1312,10 +1340,13 @@ static int kvaser_usb_leaf_start_chip(struct kvaser_usb_net_priv *priv)
 
 static int kvaser_usb_leaf_stop_chip(struct kvaser_usb_net_priv *priv)
 {
+	struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;
 	int err;
 
 	reinit_completion(&priv->stop_comp);
 
+	cancel_delayed_work(&leaf->chip_state_req_work);
+
 	err = kvaser_usb_leaf_send_simple_cmd(priv->dev, CMD_STOP_CHIP,
 					      priv->channel);
 	if (err)
@@ -1362,6 +1393,31 @@ static int kvaser_usb_leaf_init_card(struct kvaser_usb *dev)
 	return 0;
 }
 
+static int kvaser_usb_leaf_init_channel(struct kvaser_usb_net_priv *priv)
+{
+	struct kvaser_usb_net_leaf_priv *leaf;
+
+	leaf = devm_kzalloc(&priv->dev->intf->dev, sizeof(*leaf), GFP_KERNEL);
+	if (!leaf)
+		return -ENOMEM;
+
+	leaf->net = priv;
+	INIT_DELAYED_WORK(&leaf->chip_state_req_work,
+			  kvaser_usb_leaf_chip_state_req_work);
+
+	priv->sub_priv = leaf;
+
+	return 0;
+}
+
+static void kvaser_usb_leaf_remove_channel(struct kvaser_usb_net_priv *priv)
+{
+	struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;
+
+	if (leaf)
+		cancel_delayed_work_sync(&leaf->chip_state_req_work);
+}
+
 static int kvaser_usb_leaf_set_bittiming(struct net_device *netdev)
 {
 	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
@@ -1463,6 +1519,8 @@ const struct kvaser_usb_dev_ops kvaser_usb_leaf_dev_ops = {
 	.dev_get_berr_counter = kvaser_usb_leaf_get_berr_counter,
 	.dev_setup_endpoints = kvaser_usb_leaf_setup_endpoints,
 	.dev_init_card = kvaser_usb_leaf_init_card,
+	.dev_init_channel = kvaser_usb_leaf_init_channel,
+	.dev_remove_channel = kvaser_usb_leaf_remove_channel,
 	.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,
-- 
2.34.1

Re: [PATCH 08/12] can: kvaser_usb_leaf: Fix improved state not being reported

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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 the driver seems to be prepared for non-error-counter devices
> (!KVASER_USB_HAS_TXRX_ERRORS) as well, also always poll in ERROR_PASSIVE
> even if the counters show zero.
> 
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>

> ---
>   drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |  7 +++
>   .../net/can/usb/kvaser_usb/kvaser_usb_core.c  | 18 +++++-
>   .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 58 +++++++++++++++++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> index f1bea13a3829..70aa7a9ed35b 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> @@ -107,6 +107,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;
> @@ -128,6 +131,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
> @@ -149,6 +154,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);
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> index a8d72fb8291a..6a1ebdd9ba85 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> @@ -645,6 +645,9 @@ static void kvaser_usb_remove_interfaces(struct kvaser_usb *dev)
>   		if (!dev->nets[i])
>   			continue;
>   
> +		if (dev->ops->dev_remove_channel)
> +			dev->ops->dev_remove_channel(dev->nets[i]);
> +
>   		free_candev(dev->nets[i]->netdev);
>   	}
>   }
> @@ -712,17 +715,26 @@ static int kvaser_usb_init_one(struct kvaser_usb *dev,
>   
>   	dev->nets[channel] = priv;
>   
> +	if (dev->ops->dev_init_channel) {
> +		err = dev->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,
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 5f27c00179c1..abb681808a28 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -21,6 +21,7 @@
>   #include <linux/types.h>
>   #include <linux/units.h>
>   #include <linux/usb.h>
> +#include <linux/workqueue.h>
>   
>   #include <linux/can.h>
>   #include <linux/can/dev.h>
> @@ -56,6 +57,7 @@
>   #define CMD_RX_EXT_MESSAGE		14
>   #define CMD_TX_EXT_MESSAGE		15
>   #define CMD_SET_BUS_PARAMS		16
> +#define CMD_GET_CHIP_STATE		19
>   #define CMD_CHIP_STATE_EVENT		20
>   #define CMD_SET_CTRL_MODE		21
>   #define CMD_RESET_CHIP			24
> @@ -375,6 +377,12 @@ struct kvaser_usb_err_summary {
>   	};
>   };
>   
> +struct kvaser_usb_net_leaf_priv {
> +	struct kvaser_usb_net_priv *net;
> +
> +	struct delayed_work chip_state_req_work;
> +};
> +
>   static const struct can_bittiming_const kvaser_usb_leaf_bittiming_const = {
>   	.name = "kvaser_usb",
>   	.tseg1_min = KVASER_USB_TSEG1_MIN,
> @@ -757,6 +765,16 @@ static int kvaser_usb_leaf_simple_cmd_async(struct kvaser_usb_net_priv *priv,
>   	return err;
>   }
>   
> +static void kvaser_usb_leaf_chip_state_req_work(struct work_struct *work)
> +{
> +	struct kvaser_usb_net_leaf_priv *leaf =
> +		container_of(work, struct kvaser_usb_net_leaf_priv,
> +			     chip_state_req_work.work);
> +	struct kvaser_usb_net_priv *priv = leaf->net;
> +
> +	kvaser_usb_leaf_simple_cmd_async(priv, CMD_GET_CHIP_STATE);
> +}
> +
>   static void
>   kvaser_usb_leaf_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
>   					const struct kvaser_usb_err_summary *es,
> @@ -828,6 +846,7 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
>   	struct sk_buff *skb;
>   	struct net_device_stats *stats;
>   	struct kvaser_usb_net_priv *priv;
> +	struct kvaser_usb_net_leaf_priv *leaf;
>   	enum can_state old_state, new_state;
>   
>   	if (es->channel >= dev->nchannels) {
> @@ -837,6 +856,7 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
>   	}
>   
>   	priv = dev->nets[es->channel];
> +	leaf = priv->sub_priv;
>   	stats = &priv->netdev->stats;
>   
>   	/* Update all of the CAN interface's state and error counters before
> @@ -853,6 +873,14 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
>   	kvaser_usb_leaf_rx_error_update_can_state(priv, es, &tmp_cf);
>   	new_state = priv->can.state;
>   
> +	/* If there are errors, request status updates periodically as we do
> +	 * not get automatic notifications of improved state.
> +	 */
> +	if (new_state < CAN_STATE_BUS_OFF &&
> +	    (es->rxerr || es->txerr || new_state == CAN_STATE_ERROR_PASSIVE))
> +		schedule_delayed_work(&leaf->chip_state_req_work,
> +				      msecs_to_jiffies(500));
> +
>   	skb = alloc_can_err_skb(priv->netdev, &cf);
>   	if (!skb) {
>   		stats->rx_dropped++;
> @@ -1312,10 +1340,13 @@ static int kvaser_usb_leaf_start_chip(struct kvaser_usb_net_priv *priv)
>   
>   static int kvaser_usb_leaf_stop_chip(struct kvaser_usb_net_priv *priv)
>   {
> +	struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;
>   	int err;
>   
>   	reinit_completion(&priv->stop_comp);
>   
> +	cancel_delayed_work(&leaf->chip_state_req_work);
> +
>   	err = kvaser_usb_leaf_send_simple_cmd(priv->dev, CMD_STOP_CHIP,
>   					      priv->channel);
>   	if (err)
> @@ -1362,6 +1393,31 @@ static int kvaser_usb_leaf_init_card(struct kvaser_usb *dev)
>   	return 0;
>   }
>   
> +static int kvaser_usb_leaf_init_channel(struct kvaser_usb_net_priv *priv)
> +{
> +	struct kvaser_usb_net_leaf_priv *leaf;
> +
> +	leaf = devm_kzalloc(&priv->dev->intf->dev, sizeof(*leaf), GFP_KERNEL);
> +	if (!leaf)
> +		return -ENOMEM;
> +
> +	leaf->net = priv;
> +	INIT_DELAYED_WORK(&leaf->chip_state_req_work,
> +			  kvaser_usb_leaf_chip_state_req_work);
> +
> +	priv->sub_priv = leaf;
> +
> +	return 0;
> +}
> +
> +static void kvaser_usb_leaf_remove_channel(struct kvaser_usb_net_priv *priv)
> +{
> +	struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;
> +
> +	if (leaf)
> +		cancel_delayed_work_sync(&leaf->chip_state_req_work);
> +}
> +
>   static int kvaser_usb_leaf_set_bittiming(struct net_device *netdev)
>   {
>   	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> @@ -1463,6 +1519,8 @@ const struct kvaser_usb_dev_ops kvaser_usb_leaf_dev_ops = {
>   	.dev_get_berr_counter = kvaser_usb_leaf_get_berr_counter,
>   	.dev_setup_endpoints = kvaser_usb_leaf_setup_endpoints,
>   	.dev_init_card = kvaser_usb_leaf_init_card,
> +	.dev_init_channel = kvaser_usb_leaf_init_channel,
> +	.dev_remove_channel = kvaser_usb_leaf_remove_channel,
>   	.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,

[PATCH 09/12] can: kvaser_usb_leaf: Fix silently failing bus params setup

[Anssi Hannula]

The device may reject bus params with cmd 45, which is an unhandled
error event that does not contain any channel reference.

In such cases set_bittiming() callback returns 0 so upper levels will
think setting bitrate succeeded and proceed with starting the interface
with wrong parameters, causing bus errors (the kernel log will have
"Unhandled command (45)", though).

Fix that by verifying the bus params took hold by reading them back.

Also, add a handler for cmd 45 that simply prints out the error.

This condition can be triggered on 0bfd:0124 Kvaser Mini PCI Express
2xHS FW 4.18.778 by trying to set bitrate 1300000 and on 0bfd:0124
Kvaser Mini PCI Express 2xHS FW 4.18.778 by trying to set bitrate
1000000.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

---

Not sure about the best/correct naming for cmd 45. kvaser_usb_hydra
calls it CMD_ERROR_EVENT but kvaser_usb_leaf already has
CMD_CAN_ERROR_EVENT (kvaser_cmd.u.leaf.error_event) so I made it
ctrl_error_event to clearly differentiate it.


 drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |   2 +
 .../net/can/usb/kvaser_usb/kvaser_usb_core.c  |   2 +-
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 108 ++++++++++++++++++
 3 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
index 70aa7a9ed35b..b618ce299dbc 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
@@ -182,6 +182,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;
 
+int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev);
+
 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,
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index 6a1ebdd9ba85..ff0119c74b49 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -326,7 +326,7 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
 	}
 }
 
-static int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev)
+int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev)
 {
 	int i, err = 0;
 
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index abb681808a28..7ed2ced8ba08 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -57,6 +57,8 @@
 #define CMD_RX_EXT_MESSAGE		14
 #define CMD_TX_EXT_MESSAGE		15
 #define CMD_SET_BUS_PARAMS		16
+#define CMD_GET_BUS_PARAMS		17
+#define CMD_GET_BUS_PARAMS_REPLY	18
 #define CMD_GET_CHIP_STATE		19
 #define CMD_CHIP_STATE_EVENT		20
 #define CMD_SET_CTRL_MODE		21
@@ -72,6 +74,7 @@
 #define CMD_GET_CARD_INFO_REPLY		35
 #define CMD_GET_SOFTWARE_INFO		38
 #define CMD_GET_SOFTWARE_INFO_REPLY	39
+#define CMD_CTRL_ERROR_EVENT		45
 #define CMD_FLUSH_QUEUE			48
 #define CMD_TX_ACKNOWLEDGE		50
 #define CMD_CAN_ERROR_EVENT		51
@@ -290,6 +293,15 @@ struct leaf_cmd_log_message {
 	u8 data[8];
 } __packed;
 
+struct kvaser_cmd_ctrl_error_event {
+	u8 unknown[2];
+	__le16 timestamp[3];
+	u8 reserved;
+	u8 error_code;
+	__le16 info1;
+	__le16 info2;
+} __packed;
+
 struct kvaser_cmd {
 	u8 len;
 	u8 id;
@@ -319,6 +331,7 @@ struct kvaser_cmd {
 		struct kvaser_cmd_tx_can tx_can;
 		struct kvaser_cmd_ctrl_mode ctrl_mode;
 		struct kvaser_cmd_flush_queue flush_queue;
+		struct kvaser_cmd_ctrl_error_event ctrl_error_event;
 	} u;
 } __packed;
 
@@ -336,6 +349,8 @@ 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_BUS_PARAMS_REPLY]	= kvaser_fsize(u.busparams),
+	[CMD_CTRL_ERROR_EVENT]		= kvaser_fsize(u.ctrl_error_event),
 	/* ignored events: */
 	[CMD_FLUSH_QUEUE_REPLY]		= CMD_SIZE_ANY,
 };
@@ -350,6 +365,8 @@ 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.error_event),
+	[CMD_GET_BUS_PARAMS_REPLY]	= kvaser_fsize(u.busparams),
+	[CMD_CTRL_ERROR_EVENT]		= kvaser_fsize(u.ctrl_error_event),
 	/* ignored events: */
 	[CMD_USBCAN_CLOCK_OVERFLOW_EVENT] = CMD_SIZE_ANY,
 };
@@ -380,6 +397,9 @@ struct kvaser_usb_err_summary {
 struct kvaser_usb_net_leaf_priv {
 	struct kvaser_usb_net_priv *net;
 
+	struct completion get_bus_params_comp;
+	struct kvaser_cmd_busparams params_response;
+
 	struct delayed_work chip_state_req_work;
 };
 
@@ -1206,6 +1226,44 @@ static void kvaser_usb_leaf_stop_chip_reply(const struct kvaser_usb *dev,
 	complete(&priv->stop_comp);
 }
 
+static void kvaser_usb_leaf_get_bus_params_reply(const struct kvaser_usb *dev,
+						 const struct kvaser_cmd *cmd)
+{
+	struct kvaser_usb_net_leaf_priv *leaf;
+	u8 channel = cmd->u.busparams.channel;
+
+	if (channel >= dev->nchannels) {
+		dev_err(&dev->intf->dev,
+			"Invalid channel number (%d)\n", channel);
+		return;
+	}
+
+	leaf = dev->nets[channel]->sub_priv;
+	memcpy(&leaf->params_response, &cmd->u.busparams, sizeof(leaf->params_response));
+
+	complete(&leaf->get_bus_params_comp);
+}
+
+static void kvaser_usb_leaf_ctrl_error_event(const struct kvaser_usb *dev,
+					     const struct kvaser_cmd *cmd)
+{
+	const char *desc = "";
+
+	if (cmd->u.ctrl_error_event.error_code == 0x00 &&
+	    cmd->u.ctrl_error_event.info1 == CMD_SET_BUS_PARAMS)
+		desc = "bus params not accepted";
+
+	dev_err_ratelimited(&dev->intf->dev,
+			    "received error (cmd 45)%s%s: %02x %02x, code 0x%02x, info1 %u info2 %u\n",
+			    desc ? ", " : "",
+			    desc,
+			    cmd->u.ctrl_error_event.unknown[0],
+			    cmd->u.ctrl_error_event.unknown[1],
+			    cmd->u.ctrl_error_event.error_code,
+			    le16_to_cpu(cmd->u.ctrl_error_event.info1),
+			    le16_to_cpu(cmd->u.ctrl_error_event.info2));
+}
+
 static void kvaser_usb_leaf_handle_command(const struct kvaser_usb *dev,
 					   const struct kvaser_cmd *cmd)
 {
@@ -1244,6 +1302,14 @@ static void kvaser_usb_leaf_handle_command(const struct kvaser_usb *dev,
 		kvaser_usb_leaf_tx_acknowledge(dev, cmd);
 		break;
 
+	case CMD_GET_BUS_PARAMS_REPLY:
+		kvaser_usb_leaf_get_bus_params_reply(dev, cmd);
+		break;
+
+	case CMD_CTRL_ERROR_EVENT:
+		kvaser_usb_leaf_ctrl_error_event(dev, cmd);
+		break;
+
 	/* Ignored commands */
 	case CMD_USBCAN_CLOCK_OVERFLOW_EVENT:
 		if (dev->card_data.leaf.family != KVASER_USBCAN)
@@ -1402,6 +1468,7 @@ static int kvaser_usb_leaf_init_channel(struct kvaser_usb_net_priv *priv)
 		return -ENOMEM;
 
 	leaf->net = priv;
+	init_completion(&leaf->get_bus_params_comp);
 	INIT_DELAYED_WORK(&leaf->chip_state_req_work,
 			  kvaser_usb_leaf_chip_state_req_work);
 
@@ -1418,9 +1485,34 @@ static void kvaser_usb_leaf_remove_channel(struct kvaser_usb_net_priv *priv)
 		cancel_delayed_work_sync(&leaf->chip_state_req_work);
 }
 
+static int kvaser_usb_leaf_get_bus_params(struct kvaser_usb_net_priv *priv)
+{
+	struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;
+	int err;
+
+	/* we need RX urbs enabled to get the reply */
+	err = kvaser_usb_setup_rx_urbs(priv->dev);
+	if (err)
+		return err;
+
+	reinit_completion(&leaf->get_bus_params_comp);
+
+	err = kvaser_usb_leaf_send_simple_cmd(priv->dev, CMD_GET_BUS_PARAMS,
+					      priv->channel);
+	if (err)
+		return err;
+
+	if (!wait_for_completion_timeout(&leaf->get_bus_params_comp,
+					 msecs_to_jiffies(KVASER_USB_TIMEOUT)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
 static int kvaser_usb_leaf_set_bittiming(struct net_device *netdev)
 {
 	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
+	struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;
 	struct can_bittiming *bt = &priv->can.bittiming;
 	struct kvaser_usb *dev = priv->dev;
 	struct kvaser_cmd *cmd;
@@ -1446,6 +1538,22 @@ static int kvaser_usb_leaf_set_bittiming(struct net_device *netdev)
 
 	rc = kvaser_usb_send_cmd(dev, cmd, cmd->len);
 
+	if (rc < 0)
+		goto out;
+
+	/* check that the parameters were accepted */
+
+	rc = kvaser_usb_leaf_get_bus_params(priv);
+	if (rc < 0)
+		goto out;
+
+	if (memcmp(&leaf->params_response, &cmd->u.busparams,
+		   sizeof(leaf->params_response)) != 0) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+out:
 	kfree(cmd);
 	return rc;
 }
-- 
2.34.1

Re: [PATCH 09/12] can: kvaser_usb_leaf: Fix silently failing bus params setup

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> The device may reject bus params with cmd 45, which is an unhandled
> error event that does not contain any channel reference.
> 
> In such cases set_bittiming() callback returns 0 so upper levels will
> think setting bitrate succeeded and proceed with starting the interface
> with wrong parameters, causing bus errors (the kernel log will have
> "Unhandled command (45)", though).
> 
> Fix that by verifying the bus params took hold by reading them back.
> 
> Also, add a handler for cmd 45 that simply prints out the error.
> 
> This condition can be triggered on 0bfd:0124 Kvaser Mini PCI Express
> 2xHS FW 4.18.778 by trying to set bitrate 1300000 and on 0bfd:0124
> Kvaser Mini PCI Express 2xHS FW 4.18.778 by trying to set bitrate
> 1000000.

The device will send cmd CMD_ERROR_EVENT (45) when invalid data is
received from the driver and should never occur. CMD_ERROR_EVENT
indicates a misbehaving driver.

For the Kvaser Mini PCI Express 2xHS case, the problem is settings
resulting in prescaler equal to 1 are not accepted. This is a bug in the
firmware and will be fixed in our next release.

And for Kvaser Memorator Professional HS/HS, the can_bittiming_const
limits are not correct.
I'll have to look into this a bit more. I'm pretty sure we endup with
three different can_bittiming_const in kvaser_usb_leaf, depedning on
firmware/microcontroller.

I created new patches for the CMD_ERROR_EVENT, based on your patch.
See at end of this mail.

I prefere if we can avoid the paramter readback and
kvaser_usb_setup_rx_urbs() in kvaser_usb_leaf_set_bittiming().
With correct can_bittiming_const limits this should not be an issue.
On the otherhand, since there are existing devices with a bug that will
reject parameters that are within the correct limits, and the result
silently failing calls, I cannot see any alternative...

If it ok with you I'll create a new patch based on your readback fix,
and also implement this in kvaser_usb_hydra?


> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
> 
> ---
> 
> Not sure about the best/correct naming for cmd 45. kvaser_usb_hydra
> calls it CMD_ERROR_EVENT but kvaser_usb_leaf already has
> CMD_CAN_ERROR_EVENT (kvaser_cmd.u.leaf.error_event) so I made it
> ctrl_error_event to clearly differentiate it.
> 
> 
>   drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |   2 +
>   .../net/can/usb/kvaser_usb/kvaser_usb_core.c  |   2 +-
>   .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 108 ++++++++++++++++++
>   3 files changed, 111 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> index 70aa7a9ed35b..b618ce299dbc 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> @@ -182,6 +182,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;
>   
> +int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev);
> +
>   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,
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> index 6a1ebdd9ba85..ff0119c74b49 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> @@ -326,7 +326,7 @@ static void kvaser_usb_read_bulk_callback(struct urb *urb)
>   	}
>   }
>   
> -static int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev)
> +int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev)
>   {
>   	int i, err = 0;
>   
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index abb681808a28..7ed2ced8ba08 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -57,6 +57,8 @@
>   #define CMD_RX_EXT_MESSAGE		14
>   #define CMD_TX_EXT_MESSAGE		15
>   #define CMD_SET_BUS_PARAMS		16
> +#define CMD_GET_BUS_PARAMS		17
> +#define CMD_GET_BUS_PARAMS_REPLY	18
>   #define CMD_GET_CHIP_STATE		19
>   #define CMD_CHIP_STATE_EVENT		20
>   #define CMD_SET_CTRL_MODE		21
> @@ -72,6 +74,7 @@
>   #define CMD_GET_CARD_INFO_REPLY		35
>   #define CMD_GET_SOFTWARE_INFO		38
>   #define CMD_GET_SOFTWARE_INFO_REPLY	39
> +#define CMD_CTRL_ERROR_EVENT		45
>   #define CMD_FLUSH_QUEUE			48
>   #define CMD_TX_ACKNOWLEDGE		50
>   #define CMD_CAN_ERROR_EVENT		51
> @@ -290,6 +293,15 @@ struct leaf_cmd_log_message {
>   	u8 data[8];
>   } __packed;
>   
> +struct kvaser_cmd_ctrl_error_event {
> +	u8 unknown[2];
> +	__le16 timestamp[3];
> +	u8 reserved;
> +	u8 error_code;
> +	__le16 info1;
> +	__le16 info2;
> +} __packed;
> +
>   struct kvaser_cmd {
>   	u8 len;
>   	u8 id;
> @@ -319,6 +331,7 @@ struct kvaser_cmd {
>   		struct kvaser_cmd_tx_can tx_can;
>   		struct kvaser_cmd_ctrl_mode ctrl_mode;
>   		struct kvaser_cmd_flush_queue flush_queue;
> +		struct kvaser_cmd_ctrl_error_event ctrl_error_event;
>   	} u;
>   } __packed;
>   
> @@ -336,6 +349,8 @@ 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_BUS_PARAMS_REPLY]	= kvaser_fsize(u.busparams),
> +	[CMD_CTRL_ERROR_EVENT]		= kvaser_fsize(u.ctrl_error_event),
>   	/* ignored events: */
>   	[CMD_FLUSH_QUEUE_REPLY]		= CMD_SIZE_ANY,
>   };
> @@ -350,6 +365,8 @@ 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.error_event),
> +	[CMD_GET_BUS_PARAMS_REPLY]	= kvaser_fsize(u.busparams),
> +	[CMD_CTRL_ERROR_EVENT]		= kvaser_fsize(u.ctrl_error_event),
>   	/* ignored events: */
>   	[CMD_USBCAN_CLOCK_OVERFLOW_EVENT] = CMD_SIZE_ANY,
>   };
> @@ -380,6 +397,9 @@ struct kvaser_usb_err_summary {
>   struct kvaser_usb_net_leaf_priv {
>   	struct kvaser_usb_net_priv *net;
>   
> +	struct completion get_bus_params_comp;
> +	struct kvaser_cmd_busparams params_response;
> +
>   	struct delayed_work chip_state_req_work;
>   };
>   
> @@ -1206,6 +1226,44 @@ static void kvaser_usb_leaf_stop_chip_reply(const struct kvaser_usb *dev,
>   	complete(&priv->stop_comp);
>   }
>   
> +static void kvaser_usb_leaf_get_bus_params_reply(const struct kvaser_usb *dev,
> +						 const struct kvaser_cmd *cmd)
> +{
> +	struct kvaser_usb_net_leaf_priv *leaf;
> +	u8 channel = cmd->u.busparams.channel;
> +
> +	if (channel >= dev->nchannels) {
> +		dev_err(&dev->intf->dev,
> +			"Invalid channel number (%d)\n", channel);
> +		return;
> +	}
> +
> +	leaf = dev->nets[channel]->sub_priv;
> +	memcpy(&leaf->params_response, &cmd->u.busparams, sizeof(leaf->params_response));
> +
> +	complete(&leaf->get_bus_params_comp);
> +}
> +
> +static void kvaser_usb_leaf_ctrl_error_event(const struct kvaser_usb *dev,
> +					     const struct kvaser_cmd *cmd)
> +{
> +	const char *desc = "";
> +
> +	if (cmd->u.ctrl_error_event.error_code == 0x00 &&
> +	    cmd->u.ctrl_error_event.info1 == CMD_SET_BUS_PARAMS)
> +		desc = "bus params not accepted";
> +
> +	dev_err_ratelimited(&dev->intf->dev,
> +			    "received error (cmd 45)%s%s: %02x %02x, code 0x%02x, info1 %u info2 %u\n",
> +			    desc ? ", " : "",
> +			    desc,
> +			    cmd->u.ctrl_error_event.unknown[0],
> +			    cmd->u.ctrl_error_event.unknown[1],
> +			    cmd->u.ctrl_error_event.error_code,
> +			    le16_to_cpu(cmd->u.ctrl_error_event.info1),
> +			    le16_to_cpu(cmd->u.ctrl_error_event.info2));
> +}
> +
>   static void kvaser_usb_leaf_handle_command(const struct kvaser_usb *dev,
>   					   const struct kvaser_cmd *cmd)
>   {
> @@ -1244,6 +1302,14 @@ static void kvaser_usb_leaf_handle_command(const struct kvaser_usb *dev,
>   		kvaser_usb_leaf_tx_acknowledge(dev, cmd);
>   		break;
>   
> +	case CMD_GET_BUS_PARAMS_REPLY:
> +		kvaser_usb_leaf_get_bus_params_reply(dev, cmd);
> +		break;
> +
> +	case CMD_CTRL_ERROR_EVENT:
> +		kvaser_usb_leaf_ctrl_error_event(dev, cmd);
> +		break;
> +
>   	/* Ignored commands */
>   	case CMD_USBCAN_CLOCK_OVERFLOW_EVENT:
>   		if (dev->card_data.leaf.family != KVASER_USBCAN)
> @@ -1402,6 +1468,7 @@ static int kvaser_usb_leaf_init_channel(struct kvaser_usb_net_priv *priv)
>   		return -ENOMEM;
>   
>   	leaf->net = priv;
> +	init_completion(&leaf->get_bus_params_comp);
>   	INIT_DELAYED_WORK(&leaf->chip_state_req_work,
>   			  kvaser_usb_leaf_chip_state_req_work);
>   
> @@ -1418,9 +1485,34 @@ static void kvaser_usb_leaf_remove_channel(struct kvaser_usb_net_priv *priv)
>   		cancel_delayed_work_sync(&leaf->chip_state_req_work);
>   }
>   
> +static int kvaser_usb_leaf_get_bus_params(struct kvaser_usb_net_priv *priv)
> +{
> +	struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;
> +	int err;
> +
> +	/* we need RX urbs enabled to get the reply */
> +	err = kvaser_usb_setup_rx_urbs(priv->dev);
> +	if (err)
> +		return err;
> +
> +	reinit_completion(&leaf->get_bus_params_comp);
> +
> +	err = kvaser_usb_leaf_send_simple_cmd(priv->dev, CMD_GET_BUS_PARAMS,
> +					      priv->channel);
> +	if (err)
> +		return err;
> +
> +	if (!wait_for_completion_timeout(&leaf->get_bus_params_comp,
> +					 msecs_to_jiffies(KVASER_USB_TIMEOUT)))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
>   static int kvaser_usb_leaf_set_bittiming(struct net_device *netdev)
>   {
>   	struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> +	struct kvaser_usb_net_leaf_priv *leaf = priv->sub_priv;
>   	struct can_bittiming *bt = &priv->can.bittiming;
>   	struct kvaser_usb *dev = priv->dev;
>   	struct kvaser_cmd *cmd;
> @@ -1446,6 +1538,22 @@ static int kvaser_usb_leaf_set_bittiming(struct net_device *netdev)
>   
>   	rc = kvaser_usb_send_cmd(dev, cmd, cmd->len);
>   
> +	if (rc < 0)
> +		goto out;
> +
> +	/* check that the parameters were accepted */
> +
> +	rc = kvaser_usb_leaf_get_bus_params(priv);
> +	if (rc < 0)
> +		goto out;
> +
> +	if (memcmp(&leaf->params_response, &cmd->u.busparams,
> +		   sizeof(leaf->params_response)) != 0) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +out:
>   	kfree(cmd);
>   	return rc;
>   }


 From db14e82d4cf8c29f2f1b225c2d89418980787df8 Mon Sep 17 00:00:00 2001
From: Jimmy Assarsson <extja@kvaser.com>
Date: Wed, 25 May 2022 20:42:03 +0200
Subject: [PATCH 05/13] can: kvaser_usb: kvaser_usb_leaf: Rename
  {leaf,usbcan}_cmd_error_event to {leaf,usbcan}_cmd_can_error_event

Prepare for handling CMD_ERROR_EVENT. Rename struct
{leaf,usbcan}_cmd_error_event to {leaf,usbcan}_cmd_can_error_event.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB 
devices")
Reported-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
  .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 38 +++++++++----------
  1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c 
b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index aee2dae67459..420343e512b4 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -244,7 +244,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];
@@ -256,7 +256,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;
@@ -329,7 +329,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;
@@ -339,7 +339,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;
@@ -361,7 +361,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,
@@ -376,7 +376,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,
  };
@@ -1114,11 +1114,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.
@@ -1126,13 +1126,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;
@@ -1149,11 +1149,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;
-- 
2.36.1





 From 33720111c80f23021c1b37deab78e6a10672f81c Mon Sep 17 00:00:00 2001
From: Jimmy Assarsson <extja@kvaser.com>
Date: Wed, 25 May 2022 21:08:32 +0200
Subject: [PATCH 06/13] can: kvaser_usb: kvaser_usb_leaf: Handle
  CMD_ERROR_EVENT

The device will send an error event command, to indicate certain errors.
This indicates a misbehaving driver, and should never occur.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB 
devices")
Co-developed-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
  .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 99 +++++++++++++++++++
  1 file changed, 99 insertions(+)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c 
b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 420343e512b4..5fd4a6133787 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -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
@@ -268,6 +269,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;
@@ -331,6 +354,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;
@@ -340,6 +364,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;
@@ -363,6 +388,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,
  };
@@ -377,6 +403,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,
  };
@@ -1286,6 +1313,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->card_data.leaf.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->card_data.leaf.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)
  {
@@ -1364,6 +1459,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->card_data.leaf.family != KVASER_USBCAN)
-- 
2.36.1

Re: [PATCH 09/12] can: kvaser_usb_leaf: Fix silently failing bus params setup

[Anssi Hannula]

On 28.5.2022 10.37, Jimmy Assarsson wrote:
> On 5/16/22 15:47, Anssi Hannula wrote:
>> The device may reject bus params with cmd 45, which is an unhandled
>> error event that does not contain any channel reference.
>>
>> In such cases set_bittiming() callback returns 0 so upper levels will
>> think setting bitrate succeeded and proceed with starting the interface
>> with wrong parameters, causing bus errors (the kernel log will have
>> "Unhandled command (45)", though).
>>
>> Fix that by verifying the bus params took hold by reading them back.
>>
>> Also, add a handler for cmd 45 that simply prints out the error.
>>
>> This condition can be triggered on 0bfd:0124 Kvaser Mini PCI Express
>> 2xHS FW 4.18.778 by trying to set bitrate 1300000 and on 0bfd:0124
>> Kvaser Mini PCI Express 2xHS FW 4.18.778 by trying to set bitrate
>> 1000000.
> The device will send cmd CMD_ERROR_EVENT (45) when invalid data is
> received from the driver and should never occur. CMD_ERROR_EVENT
> indicates a misbehaving driver.
>
> For the Kvaser Mini PCI Express 2xHS case, the problem is settings
> resulting in prescaler equal to 1 are not accepted. This is a bug in the
> firmware and will be fixed in our next release.
>
> And for Kvaser Memorator Professional HS/HS, the can_bittiming_const
> limits are not correct.
> I'll have to look into this a bit more. I'm pretty sure we endup with
> three different can_bittiming_const in kvaser_usb_leaf, depedning on
> firmware/microcontroller.
>
> I created new patches for the CMD_ERROR_EVENT, based on your patch.
> See at end of this mail.
>
> I prefere if we can avoid the paramter readback and
> kvaser_usb_setup_rx_urbs() in kvaser_usb_leaf_set_bittiming().
> With correct can_bittiming_const limits this should not be an issue.
> On the otherhand, since there are existing devices with a bug that will
> reject parameters that are within the correct limits, and the result
> silently failing calls, I cannot see any alternative...
>
> If it ok with you I'll create a new patch based on your readback fix,
> and also implement this in kvaser_usb_hydra?

All OK for me.

-- 
Anssi Hannula / Bitwise Oy
+358 503803997

[PATCH 10/12] can: kvaser_usb_leaf: Fix wrong CAN state after stopping

[Anssi Hannula]

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.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 7ed2ced8ba08..742626e69dd8 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -879,6 +879,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.
 	 *
-- 
2.34.1

Re: [PATCH 10/12] can: kvaser_usb_leaf: Fix wrong CAN state after stopping

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.
> 
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>

> ---
>   drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 7ed2ced8ba08..742626e69dd8 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -879,6 +879,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.
>   	 *

[PATCH 11/12] can: kvaser_usb_leaf: Ignore stale bus-off after start

[Anssi Hannula]

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.

Fixes: 71873a9b38d1 ("can: kvaser_usb: Add support for more Kvaser Leaf v2 devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 31 ++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 742626e69dd8..4125074c7066 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -401,6 +401,9 @@ struct kvaser_usb_net_leaf_priv {
 	struct kvaser_cmd_busparams params_response;
 
 	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_bittiming_const = {
@@ -800,6 +803,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;
@@ -824,6 +828,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;
@@ -899,9 +919,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));
 
@@ -1392,8 +1415,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,
@@ -1566,6 +1592,7 @@ 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) {
@@ -1576,6 +1603,8 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev,
 
 		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;
-- 
2.34.1

Re: [PATCH 11/12] can: kvaser_usb_leaf: Ignore stale bus-off after start

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.

I'm able to reproduce this and it definitely looks like a bug in
firmware. I've created a case for this in our backlog, but I don't know
which priority it will get.

> 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.
> 
> Fixes: 71873a9b38d1 ("can: kvaser_usb: Add support for more Kvaser Leaf v2 devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>

> ---
>   .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 31 ++++++++++++++++++-
>   1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 742626e69dd8..4125074c7066 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -401,6 +401,9 @@ struct kvaser_usb_net_leaf_priv {
>   	struct kvaser_cmd_busparams params_response;
>   
>   	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_bittiming_const = {
> @@ -800,6 +803,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;
> @@ -824,6 +828,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;
> @@ -899,9 +919,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));
>   
> @@ -1392,8 +1415,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,
> @@ -1566,6 +1592,7 @@ 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) {
> @@ -1576,6 +1603,8 @@ static int kvaser_usb_leaf_set_mode(struct net_device *netdev,
>   
>   		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;

[PATCH 12/12] can: kvaser_usb_leaf: Fix bogus restart events

[Anssi Hannula]

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.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 4125074c7066..b280d315673f 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -735,7 +735,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;
 
@@ -852,7 +852,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++;
 
@@ -945,7 +945,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);
-- 
2.34.1

Re: [PATCH 12/12] can: kvaser_usb_leaf: Fix bogus restart events

[Jimmy Assarsson]

On 5/16/22 15:47, Anssi Hannula wrote:
> 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.
> 
> Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
> Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>

Looks good to me.
Tested-by: Jimmy Assarsson <extja@kvaser.com>

> ---
>   drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index 4125074c7066..b280d315673f 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -735,7 +735,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;
>   
> @@ -852,7 +852,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++;
>   
> @@ -945,7 +945,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);

Re: [PATCH 00/12] can: kvaser_usb: Various fixes

[Jimmy Assarsson]

On 2022-05-16 15:47, Anssi Hannula wrote:
> 
> Hi all,
> 
> Here's a set of fixes for issues I found while testing kvaser_usb as we
> are preparing to start using it in production (with 0bfd:0124).

Hi Anssi,

Thanks for the patches!
I will review and test your fixes before the weekend.

Best regards,
jimmy


> The biggest caveat is that I only have two devices to test with [1] and I
> don't have HW documentation, so there is a possibility that some of the
> fixes might not work properly on all HW variants.
> Hopefully Jimmy can confirm they look OK, or suggest alternatives.
> 
> [1] Tested devices:
> - 0bfd:0017 Kvaser Memorator Professional HS/HS FW 2.0.50
> - 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778

Re: [PATCH 00/12] can: kvaser_usb: Various fixes

[Jimmy Assarsson]

On 5/17/22 10:41, Jimmy Assarsson wrote:
> On 2022-05-16 15:47, Anssi Hannula wrote:
>>
>> Hi all,
>>
>> Here's a set of fixes for issues I found while testing kvaser_usb as we
>> are preparing to start using it in production (with 0bfd:0124).
> 
> Hi Anssi,
> 
> Thanks for the patches!
> I will review and test your fixes before the weekend.
> 
> Best regards,
> jimmy

Sorry for the delay!

To summarize the status.

These patches look good:
[PATCH 01/12] can: kvaser_usb_leaf: Fix overread with an invalid command
[PATCH 02/12] can: kvaser_usb: Fix use of uninitialized completion
[PATCH 03/12] can: kvaser_usb: Fix possible completions during 
init_completion
[PATCH 05/12] can: kvaser_usb_leaf: Set Warning state even without bus 
errors
[PATCH 10/12] can: kvaser_usb_leaf: Fix wrong CAN state after stopping
[PATCH 12/12] can: kvaser_usb_leaf: Fix bogus restart events

This looks good, but see comment regarding explicit queue flush:
[PATCH 06/12] can: kvaser_usb_leaf: Fix TX queue out of sync after restart

I still need some more time looking into:
PATCH 07/12] can: kvaser_usb_leaf: Fix CAN state after restart
PATCH 08/12] can: kvaser_usb_leaf: Fix improved state not being reported
PATCH 11/12] can: kvaser_usb_leaf: Ignore stale bus-off after start

I want to replace
[PATCH 04/12] can: kvaser_usb: Mark Mini PCIe 2xHS as supporting error 
counters
with a new patch
"can: kvaser_usb: kvaser_usb_leaf: Get capabilities from device"

I want to split the handling of CMD_ERROR_EVENT and the readback
functionality. I also want to add parameter readback for
kvaser_usb_hydra. I need more time to look over the can_bittiming_const
in kvaser_usb_leaf for the different supported firmware.
[PATCH 09/12] can: kvaser_usb_leaf: Fix silently failing bus params setup


I would like to create a V2 series, including my patches, if you are
okay with it?


Best regards,
jimmy



>> The biggest caveat is that I only have two devices to test with [1] and I
>> don't have HW documentation, so there is a possibility that some of the
>> fixes might not work properly on all HW variants.
>> Hopefully Jimmy can confirm they look OK, or suggest alternatives.
>>
>> [1] Tested devices:
>> - 0bfd:0017 Kvaser Memorator Professional HS/HS FW 2.0.50
>> - 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778

Re: [PATCH 00/12] can: kvaser_usb: Various fixes

[Anssi Hannula]

On 28.5.2022 10.42, Jimmy Assarsson wrote:
> On 5/17/22 10:41, Jimmy Assarsson wrote:
>> On 2022-05-16 15:47, Anssi Hannula wrote:
>>> Hi all,
>>>
>>> Here's a set of fixes for issues I found while testing kvaser_usb as we
>>> are preparing to start using it in production (with 0bfd:0124).
>> Hi Anssi,
>>
>> Thanks for the patches!
>> I will review and test your fixes before the weekend.
>>
>> Best regards,
>> jimmy
> Sorry for the delay!

No problem!

> To summarize the status.
>
> These patches look good:
> [PATCH 01/12] can: kvaser_usb_leaf: Fix overread with an invalid command
> [PATCH 02/12] can: kvaser_usb: Fix use of uninitialized completion
> [PATCH 03/12] can: kvaser_usb: Fix possible completions during 
> init_completion
> [PATCH 05/12] can: kvaser_usb_leaf: Set Warning state even without bus 
> errors
> [PATCH 10/12] can: kvaser_usb_leaf: Fix wrong CAN state after stopping
> [PATCH 12/12] can: kvaser_usb_leaf: Fix bogus restart events
>
> This looks good, but see comment regarding explicit queue flush:
> [PATCH 06/12] can: kvaser_usb_leaf: Fix TX queue out of sync after restart

Feel free to drop the flush, or let me know if you want me to resend it
without it.

> I still need some more time looking into:
> PATCH 07/12] can: kvaser_usb_leaf: Fix CAN state after restart
> PATCH 08/12] can: kvaser_usb_leaf: Fix improved state not being reported
> PATCH 11/12] can: kvaser_usb_leaf: Ignore stale bus-off after start
>
> I want to replace
> [PATCH 04/12] can: kvaser_usb: Mark Mini PCIe 2xHS as supporting error 
> counters
> with a new patch
> "can: kvaser_usb: kvaser_usb_leaf: Get capabilities from device"
>
> I want to split the handling of CMD_ERROR_EVENT and the readback
> functionality. I also want to add parameter readback for
> kvaser_usb_hydra. I need more time to look over the can_bittiming_const
> in kvaser_usb_leaf for the different supported firmware.
> [PATCH 09/12] can: kvaser_usb_leaf: Fix silently failing bus params setup
>
>
> I would like to create a V2 series, including my patches, if you are
> okay with it?


Yes, that's fine. I can test your series on my setup as well.

>
> Best regards,
> jimmy
>
>
>
>>> The biggest caveat is that I only have two devices to test with [1] and I
>>> don't have HW documentation, so there is a possibility that some of the
>>> fixes might not work properly on all HW variants.
>>> Hopefully Jimmy can confirm they look OK, or suggest alternatives.
>>>
>>> [1] Tested devices:
>>> - 0bfd:0017 Kvaser Memorator Professional HS/HS FW 2.0.50
>>> - 0bfd:0124 Kvaser Mini PCI Express 2xHS FW 4.18.778


-- 
Anssi Hannula / Bitwise Oy
+358 503803997

Re: [PATCH 00/12] can: kvaser_usb: Various fixes

[Jimmy Assarsson]

On 5/30/22 12:56, Anssi Hannula wrote:
> On 28.5.2022 10.42, Jimmy Assarsson wrote:
>> On 5/17/22 10:41, Jimmy Assarsson wrote:
>>> On 2022-05-16 15:47, Anssi Hannula wrote:
>>>> Hi all,
>>>>
>>>> Here's a set of fixes for issues I found while testing kvaser_usb as we
>>>> are preparing to start using it in production (with 0bfd:0124).
>>> Hi Anssi,
>>>
>>> Thanks for the patches!
>>> I will review and test your fixes before the weekend.
>>>
>>> Best regards,
>>> jimmy
>> Sorry for the delay!
> 
> No problem!
> 
>> To summarize the status.
>>
>> These patches look good:
>> [PATCH 01/12] can: kvaser_usb_leaf: Fix overread with an invalid command
>> [PATCH 02/12] can: kvaser_usb: Fix use of uninitialized completion
>> [PATCH 03/12] can: kvaser_usb: Fix possible completions during
>> init_completion
>> [PATCH 05/12] can: kvaser_usb_leaf: Set Warning state even without bus
>> errors
>> [PATCH 10/12] can: kvaser_usb_leaf: Fix wrong CAN state after stopping
>> [PATCH 12/12] can: kvaser_usb_leaf: Fix bogus restart events
>>
>> This looks good, but see comment regarding explicit queue flush:
>> [PATCH 06/12] can: kvaser_usb_leaf: Fix TX queue out of sync after restart
> 
> Feel free to drop the flush, or let me know if you want me to resend it
> without it.
> 
>> I still need some more time looking into:
>> PATCH 07/12] can: kvaser_usb_leaf: Fix CAN state after restart
>> PATCH 08/12] can: kvaser_usb_leaf: Fix improved state not being reported
>> PATCH 11/12] can: kvaser_usb_leaf: Ignore stale bus-off after start
>>
>> I want to replace
>> [PATCH 04/12] can: kvaser_usb: Mark Mini PCIe 2xHS as supporting error
>> counters
>> with a new patch
>> "can: kvaser_usb: kvaser_usb_leaf: Get capabilities from device"
>>
>> I want to split the handling of CMD_ERROR_EVENT and the readback
>> functionality. I also want to add parameter readback for
>> kvaser_usb_hydra. I need more time to look over the can_bittiming_const
>> in kvaser_usb_leaf for the different supported firmware.
>> [PATCH 09/12] can: kvaser_usb_leaf: Fix silently failing bus params setup
>>
>>
>> I would like to create a V2 series, including my patches, if you are
>> okay with it?
> 
> 
> Yes, that's fine. I can test your series on my setup as well.

I'll rebase and send V2 of this series, once "can: kvaser_usb: CAN clock 
frequency regression" [2], is sorted out.
That would be great :)


[2] 
https://lore.kernel.org/linux-can/20220603083820.800246-1-extja@kvaser.com

Best regards,
jimmy