by Jimmy Assarsson

[permalink] [raw]

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

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/[email protected]

Best regards,
jimmy

2022-06-06 15:52:18

by Jimmy Assarsson

[permalink] [raw]

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

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 <[email protected]>

Looks good to me.
Tested-by: Jimmy Assarsson <[email protected]>

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