On 12.02.2025 10:49:43, Ming Yu wrote:

quoted

quoted

+static void nct6694_can_handle_state_errors(struct net_device *ndev, u8 status)
+{
+     struct nct6694_can_priv *priv = netdev_priv(ndev);

It seems you don't have dedicated RX and TX states, so call
nct6694_can_get_berr_counter() and use can_state_get_by_berr_counter()
to get the states. Then basically do that what what
mcp251xfd_handle_cerrif() does, starting with "new_state = max(tx_state, rx_state);"

Excuse me, do you mean that nct6694_can_handle_state_change() the flow
should like v5?
https://lore.kernel.org/linux-can/CAMZ6RqLHEoukxDfV33iDWXjM1baK922QnWSkOP01VzZ0S_9H8g@mail.gmail.com/ (local)

The handling of
CAMZ6RqLHEoukxDfV33iDWXjM1baK922QnWSkOP01VzZ0S_9H8g@mail.gmail.com in v5
was better, but there were some questions by Vincent...

So let's continue the discussion from v5 here:

quoted

+static void nct6694_can_handle_state_change(struct net_device *ndev,
+                                           u8 status)
+{
+       struct nct6694_can_priv *priv = netdev_priv(ndev);
+       enum can_state new_state = priv->can.state;
+       enum can_state rx_state, tx_state;
+       struct can_berr_counter bec;
+       struct can_frame *cf;
+       struct sk_buff *skb;
+
+       nct6694_can_get_berr_counter(ndev, &bec);
+       can_state_get_by_berr_counter(ndev, &bec, &tx_state, &rx_state);

Here, you set up tx_state and rx_state...

remove the switch (status)...

quoted

+       switch (status) {
+       case NCT6694_CAN_EVT_STS_ERROR_ACTIVE:
+               new_state = CAN_STATE_ERROR_ACTIVE;
+               break;
+       case NCT6694_CAN_EVT_STS_ERROR_PASSIVE:
+               new_state = CAN_STATE_ERROR_PASSIVE;
+               break;
+       case NCT6694_CAN_EVT_STS_BUS_OFF:
+               new_state = CAN_STATE_BUS_OFF;
+               break;
+       case NCT6694_CAN_EVT_STS_WARNING:
+               new_state = CAN_STATE_ERROR_WARNING;
+               break;
+       default:
+               netdev_err(ndev, "Receive unknown CAN status event.\n");
+               return;
+       }

replace it by:

	new_state = max(tx_state, rx_state);

quoted

+
+       /* state hasn't changed */
+       if (new_state == priv->can.state)
+               return;
+
+       skb = alloc_can_err_skb(ndev, &cf);
+

remove this VVV

quoted

+       tx_state = bec.txerr >= bec.rxerr ? new_state : 0;
+       rx_state = bec.txerr <= bec.rxerr ? new_state : 0;

            ^^^

... but you never used the values returned by
can_state_get_by_berr_counter() and just overwrote the tx and rx
state.

What is the logic here? Why do you need to manually adjust those two
values? Isn't the logic in can_change_state() sufficient?

quoted

+       can_change_state(ndev, cf, tx_state, rx_state);
+
+       if (new_state == CAN_STATE_BUS_OFF) {

if (priv->can.state == CAN_STATE_BUS_OFF) {

Same for the new_state. The function can_change_state() calculate the
new state from tx_state and rx_state and save it under
can_priv->state. But here, you do your own calculation.

Only keep one of the two. If your device already tells you the state,
then fine! Just use the information from your device and do not use
can_change_state(). Here, you are doing double work resulting in a
weird mix.

what does your device do when it goes into bus off?

quoted

+               can_bus_off(ndev);
+       } else if (skb) {
+               cf->can_id |= CAN_ERR_CNT;
+               cf->data[6] = bec.txerr;
+               cf->data[7] = bec.rxerr;
+       }
+

if (skb)

quoted

+       nct6694_can_rx_offload(&priv->offload, skb);
+}

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |