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@xxxxxxxxxx>

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

> ---
>   .../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;