Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <bbfaad00-4332-1236-081a-8b8c9e1edc58@kvaser.com>
Date:   Mon, 6 Jun 2022 17:32:17 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH 11/12] can: kvaser_usb_leaf: Ignore stale bus-off after
 start
Content-Language: en-US
To:     Anssi Hannula <anssi.hannula@bitwise.fi>
Cc:     linux-can@vger.kernel.org, Marc Kleine-Budde <mkl@pengutronix.de>,
        linux-kernel@vger.kernel.org
References: <20220516134748.3724796-1-anssi.hannula@bitwise.fi>
 <20220516134748.3724796-12-anssi.hannula@bitwise.fi>
From:   Jimmy Assarsson <extja@kvaser.com>
In-Reply-To: <20220516134748.3724796-12-anssi.hannula@bitwise.fi>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;