Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling

Hi!

On Wed, Jan 21, 2015 at 04:20:25PM +0000, Andri Yngvason wrote:

Quoting Ahmed S. Darwish (2015-01-20 21:45:37)

quoted

From: Ahmed S. Darwish <redacted>

Replace most of the can interface's state and error counters
handling with the new can-dev can_change_state() mechanism.

Suggested-by: Andri Yngvason <redacted>
Signed-off-by: Ahmed S. Darwish <redacted>
---
 drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
 1 file changed, 55 insertions(+), 59 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 971c5f9..0386d3f 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c

@@ -620,40 +620,43 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
 }
 
 static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
-                                                const struct kvaser_usb_error_summary *es)
+                                                const struct kvaser_usb_error_summary *es,
+                                                struct can_frame *cf)
 {
        struct net_device_stats *stats;
-       enum can_state new_state;
-
-       stats = &priv->netdev->stats;
-       new_state = priv->can.state;
+       enum can_state cur_state, new_state, tx_state, rx_state;
 
        netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
 
-       if (es->status & M16C_STATE_BUS_OFF) {
-               priv->can.can_stats.bus_off++;
+       stats = &priv->netdev->stats;
+       new_state = cur_state = priv->can.state;
+
+       if (es->status & M16C_STATE_BUS_OFF)
                new_state = CAN_STATE_BUS_OFF;
-       } else if (es->status & M16C_STATE_BUS_PASSIVE) {
-               if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
-                       priv->can.can_stats.error_passive++;
+       else if (es->status & M16C_STATE_BUS_PASSIVE)
                new_state = CAN_STATE_ERROR_PASSIVE;
-       }
 
        if (es->status == M16C_STATE_BUS_ERROR) {
-               if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
-                   ((es->txerr >= 96) || (es->rxerr >= 96))) {
-                       priv->can.can_stats.error_warning++;
+               if ((cur_state < CAN_STATE_ERROR_WARNING) &&
+                   ((es->txerr >= 96) || (es->rxerr >= 96)))
                        new_state = CAN_STATE_ERROR_WARNING;
-               } else if (priv->can.state > CAN_STATE_ERROR_ACTIVE) {
+               else if (cur_state > CAN_STATE_ERROR_ACTIVE)
                        new_state = CAN_STATE_ERROR_ACTIVE;
-               }
        }
 
        if (!es->status)
                new_state = CAN_STATE_ERROR_ACTIVE;
 
+       if (new_state != cur_state) {
+               tx_state = (es->txerr >= es->rxerr) ? new_state : 0;
+               rx_state = (es->txerr <= es->rxerr) ? new_state : 0;
+
+               can_change_state(priv->netdev, cf, tx_state, rx_state);

This (below) is redundant. It doesn't harm but at this point can_change_state
has set priv->can.state to new_state.

quoted

+               new_state = priv->can.state;
+       }
+

Correct; I will remove it.

quoted

        if (priv->can.restart_ms &&
-           (priv->can.state >= CAN_STATE_BUS_OFF) &&
+           (cur_state >= CAN_STATE_BUS_OFF) &&
            (new_state < CAN_STATE_BUS_OFF)) {
                priv->can.can_stats.restarts++;
        }

@@ -665,18 +668,17 @@ static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *pri
 
        priv->bec.txerr = es->txerr;
        priv->bec.rxerr = es->rxerr;
-       priv->can.state = new_state;
 }
 
 static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
                                const struct kvaser_msg *msg)
 {
-       struct can_frame *cf;
+       struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc = CAN_ERR_DLC };
        struct sk_buff *skb;
        struct net_device_stats *stats;
        struct kvaser_usb_net_priv *priv;
        struct kvaser_usb_error_summary es = { };
-       enum can_state old_state;
+       enum can_state old_state, new_state;
 
        switch (msg->id) {
        case CMD_CAN_ERROR_EVENT:

@@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
        }
 
        /* Update all of the can interface's state and error counters before
-        * trying any skb allocation that can actually fail with -ENOMEM.
+        * trying any memory allocation that can actually fail with -ENOMEM.
+        *
+        * We send a temporary stack-allocated error can frame to
+        * can_change_state() for the very same reason.
+        *
+        * TODO: Split can_change_state() responsibility between updating the
+        * can interface's state and counters, and the setting up of can error
+        * frame ID and data to userspace. Remove stack allocation afterwards.
         */
        old_state = priv->can.state;
-       kvaser_usb_rx_error_update_can_state(priv, &es);
+       kvaser_usb_rx_error_update_can_state(priv, &es, &tmp_cf);
+       new_state = priv->can.state;
 
        skb = alloc_can_err_skb(priv->netdev, &cf);
        if (!skb) {
                stats->rx_dropped++;
                return;
        }
+       memcpy(cf, &tmp_cf, sizeof(*cf));
 
-       if (es.status & M16C_STATE_BUS_OFF) {
-               cf->can_id |= CAN_ERR_BUSOFF;
-
-               if (!priv->can.restart_ms)
-                       kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
-               netif_carrier_off(priv->netdev);
-       } else if (es.status & M16C_STATE_BUS_PASSIVE) {
-               if (old_state != CAN_STATE_ERROR_PASSIVE) {
-                       cf->can_id |= CAN_ERR_CRTL;
-
-                       if (es.txerr || es.rxerr)
-                               cf->data[1] = (es.txerr > es.rxerr)
-                                               ? CAN_ERR_CRTL_TX_PASSIVE
-                                               : CAN_ERR_CRTL_RX_PASSIVE;
-                       else
-                               cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
-                                             CAN_ERR_CRTL_RX_PASSIVE;
+       if (new_state != old_state) {
+               if (es.status & M16C_STATE_BUS_OFF) {
+                       if (!priv->can.restart_ms)
+                               kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
+                       netif_carrier_off(priv->netdev);
+               }
+

This block [below] is wrong. The usage of PROT_ACTIVE is based on a misunderstanding.
It's used in some drivers to signify back-to-error-active but its original
meaning is something completely different, AFAIK.
This is handled in can_change_state() using a new CTRL message; namely:
CAN_ERR_CTRL_ACTIVE. The newest version of can-utils is up to date with this.

quoted

+               if (es.status == M16C_STATE_BUS_ERROR) {
+                       if ((old_state >= CAN_STATE_ERROR_WARNING) ||
+                           (es.txerr < 96 && es.rxerr < 96)) {
+                               if (old_state > CAN_STATE_ERROR_ACTIVE) {
+                                       cf->can_id |= CAN_ERR_PROT;
+                                       cf->data[2] = CAN_ERR_PROT_ACTIVE;
+                               }
+                       }
                }

So I would just make the new_state equals CAN_STATE_ERROR_ACTIVE,
and then can_change_state() will do the right thing? Excellent!!
This means the entire block above can be removed ;-)

[ On another note, the block's if conditions above are faulty and
  fixed in patch #3. This patch (#2) used can_change_state()
  without changing any of that faulty logic to ease any future
  bisection. ]

quoted

-       }
 
-       if (es.status == M16C_STATE_BUS_ERROR) {
-               if ((old_state < CAN_STATE_ERROR_WARNING) &&
-                   ((es.txerr >= 96) || (es.rxerr >= 96))) {
-                       cf->can_id |= CAN_ERR_CRTL;
-                       cf->data[1] = (es.txerr > es.rxerr)
-                                       ? CAN_ERR_CRTL_TX_WARNING
-                                       : CAN_ERR_CRTL_RX_WARNING;
-               } else if (old_state > CAN_STATE_ERROR_ACTIVE) {

This is also redundant, and wrong:

quoted

+               if (!es.status) {
                        cf->can_id |= CAN_ERR_PROT;
                        cf->data[2] = CAN_ERR_PROT_ACTIVE;
                }
-       }

As in the above; extra code to be removed, yay! ;-)

quoted

 
-       if (!es.status) {
-               cf->can_id |= CAN_ERR_PROT;
-               cf->data[2] = CAN_ERR_PROT_ACTIVE;
-       }
-
-       if (priv->can.restart_ms &&
-           (old_state >= CAN_STATE_BUS_OFF) &&
-           (priv->can.state < CAN_STATE_BUS_OFF)) {
-               cf->can_id |= CAN_ERR_RESTARTED;
-               netif_carrier_on(priv->netdev);
+               if (priv->can.restart_ms &&
+                   (old_state >= CAN_STATE_BUS_OFF) &&
+                   (new_state < CAN_STATE_BUS_OFF)) {
+                       cf->can_id |= CAN_ERR_RESTARTED;
+                       netif_carrier_on(priv->netdev);
+               }
        }
 
        if (es.error_factor) {
-- 
1.9.1

Looking over the patch again, I've noticed that there
are a few things that are not quite right.

The state-handlig code has become much simpler since your
reveiw and Kleine-Budde's one. Thanks a lot for all the effort.

Regards,
Darwish