can: kvaser_usb: Update stats and state even if alloc_can_err_skb() fails

Ensure statistics, error counters, and CAN state are updated consistently,
even when alloc_can_err_skb() fails during state changes or error message
frame reception.

Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
Link: https://patch.msgid.link/20241230142645.128244-1-extja@kvaser.com
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index 3764b263add3a89064ed7c25ad10921ff8efc1ed..5ec8b300bebf87762c64ed47648d238a40af814d 100644 (file)
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -926,6 +926,42 @@ kvaser_usb_hydra_bus_status_to_can_state(const struct kvaser_usb_net_priv *priv,
        }
 }
 
+static void kvaser_usb_hydra_change_state(struct kvaser_usb_net_priv *priv,
+                                         const struct can_berr_counter *bec,
+                                         struct can_frame *cf,
+                                         enum can_state new_state)
+{
+       struct net_device *netdev = priv->netdev;
+       enum can_state old_state = priv->can.state;
+       enum can_state tx_state, rx_state;
+
+       tx_state = (bec->txerr >= bec->rxerr) ?
+                               new_state : CAN_STATE_ERROR_ACTIVE;
+       rx_state = (bec->txerr <= bec->rxerr) ?
+                               new_state : CAN_STATE_ERROR_ACTIVE;
+       can_change_state(netdev, cf, tx_state, rx_state);
+
+       if (new_state == CAN_STATE_BUS_OFF && old_state < CAN_STATE_BUS_OFF) {
+               if (priv->can.restart_ms == 0)
+                       kvaser_usb_hydra_send_simple_cmd_async(priv, CMD_STOP_CHIP_REQ);
+
+               can_bus_off(netdev);
+       }
+
+       if (priv->can.restart_ms &&
+           old_state >= CAN_STATE_BUS_OFF &&
+           new_state < CAN_STATE_BUS_OFF) {
+               priv->can.can_stats.restarts++;
+               if (cf)
+                       cf->can_id |= CAN_ERR_RESTARTED;
+       }
+       if (cf && new_state != CAN_STATE_BUS_OFF) {
+               cf->can_id |= CAN_ERR_CNT;
+               cf->data[6] = bec->txerr;
+               cf->data[7] = bec->rxerr;
+       }
+}
+
 static void kvaser_usb_hydra_update_state(struct kvaser_usb_net_priv *priv,
                                          u8 bus_status,
                                          const struct can_berr_counter *bec)
@@ -951,41 +987,11 @@ static void kvaser_usb_hydra_update_state(struct kvaser_usb_net_priv *priv,
                return;
 
        skb = alloc_can_err_skb(netdev, &cf);
-       if (skb) {
-               enum can_state tx_state, rx_state;
-
-               tx_state = (bec->txerr >= bec->rxerr) ?
-                                       new_state : CAN_STATE_ERROR_ACTIVE;
-               rx_state = (bec->txerr <= bec->rxerr) ?
-                                       new_state : CAN_STATE_ERROR_ACTIVE;
-               can_change_state(netdev, cf, tx_state, rx_state);
-       }
-
-       if (new_state == CAN_STATE_BUS_OFF && old_state < CAN_STATE_BUS_OFF) {
-               if (!priv->can.restart_ms)
-                       kvaser_usb_hydra_send_simple_cmd_async
-                                               (priv, CMD_STOP_CHIP_REQ);
-
-               can_bus_off(netdev);
-       }
-
-       if (!skb) {
+       kvaser_usb_hydra_change_state(priv, bec, cf, new_state);
+       if (skb)
+               netif_rx(skb);
+       else
                netdev_warn(netdev, "No memory left for err_skb\n");
-               return;
-       }
-
-       if (priv->can.restart_ms &&
-           old_state >= CAN_STATE_BUS_OFF &&
-           new_state < CAN_STATE_BUS_OFF)
-               priv->can.can_stats.restarts++;
-
-       if (new_state != CAN_STATE_BUS_OFF) {
-               cf->can_id |= CAN_ERR_CNT;
-               cf->data[6] = bec->txerr;
-               cf->data[7] = bec->rxerr;
-       }
-
-       netif_rx(skb);
 }
 
 static void kvaser_usb_hydra_state_event(const struct kvaser_usb *dev,
@@ -1080,7 +1086,6 @@ kvaser_usb_hydra_error_frame(struct kvaser_usb_net_priv *priv,
        struct net_device_stats *stats = &netdev->stats;
        struct can_frame *cf;
        struct sk_buff *skb;
-       struct skb_shared_hwtstamps *shhwtstamps;
        struct can_berr_counter bec;
        enum can_state new_state, old_state;
        u8 bus_status;
@@ -1097,51 +1102,22 @@ kvaser_usb_hydra_error_frame(struct kvaser_usb_net_priv *priv,
                                                 &new_state);
 
        skb = alloc_can_err_skb(netdev, &cf);
+       if (new_state != old_state)
+               kvaser_usb_hydra_change_state(priv, &bec, cf, new_state);
 
-       if (new_state != old_state) {
-               if (skb) {
-                       enum can_state tx_state, rx_state;
-
-                       tx_state = (bec.txerr >= bec.rxerr) ?
-                                       new_state : CAN_STATE_ERROR_ACTIVE;
-                       rx_state = (bec.txerr <= bec.rxerr) ?
-                                       new_state : CAN_STATE_ERROR_ACTIVE;
-
-                       can_change_state(netdev, cf, tx_state, rx_state);
-
-                       if (priv->can.restart_ms &&
-                           old_state >= CAN_STATE_BUS_OFF &&
-                           new_state < CAN_STATE_BUS_OFF)
-                               cf->can_id |= CAN_ERR_RESTARTED;
-               }
-
-               if (new_state == CAN_STATE_BUS_OFF) {
-                       if (!priv->can.restart_ms)
-                               kvaser_usb_hydra_send_simple_cmd_async
-                                               (priv, CMD_STOP_CHIP_REQ);
-
-                       can_bus_off(netdev);
-               }
-       }
-
-       if (!skb) {
-               stats->rx_dropped++;
-               netdev_warn(netdev, "No memory left for err_skb\n");
-               return;
-       }
-
-       shhwtstamps = skb_hwtstamps(skb);
-       shhwtstamps->hwtstamp = hwtstamp;
+       if (skb) {
+               struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
 
-       cf->can_id |= CAN_ERR_BUSERROR;
-       if (new_state != CAN_STATE_BUS_OFF) {
-               cf->can_id |= CAN_ERR_CNT;
+               shhwtstamps->hwtstamp = hwtstamp;
+               cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_CNT;
                cf->data[6] = bec.txerr;
                cf->data[7] = bec.rxerr;
+               netif_rx(skb);
+       } else {
+               stats->rx_dropped++;
+               netdev_warn(netdev, "No memory left for err_skb\n");
        }
 
-       netif_rx(skb);
-
        priv->bec.txerr = bec.txerr;
        priv->bec.rxerr = bec.rxerr;
 }

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 6b9122ab1464f9c85e5d0e320efa3501c4bc3912..0491b4a6d8e88e980085f9bf265ca9655238eabe 100644 (file)
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -1121,8 +1121,6 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
                                     const struct kvaser_usb_err_summary *es)
 {
        struct can_frame *cf;
-       struct can_frame tmp_cf = { .can_id = CAN_ERR_FLAG,
-                                   .len = CAN_ERR_DLC };
        struct sk_buff *skb;
        struct net_device_stats *stats;
        struct kvaser_usb_net_priv *priv;
@@ -1143,18 +1141,9 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
        if (!netif_running(priv->netdev))
                return;
 
-       /* Update all of the CAN interface's state and error counters before
-        * 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_leaf_rx_error_update_can_state(priv, es, &tmp_cf);
+       skb = alloc_can_err_skb(priv->netdev, &cf);
+       kvaser_usb_leaf_rx_error_update_can_state(priv, es, cf);
        new_state = priv->can.state;
 
        /* If there are errors, request status updates periodically as we do
@@ -1168,13 +1157,6 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
                schedule_delayed_work(&leaf->chip_state_req_work,
                                      msecs_to_jiffies(500));
 
-       skb = alloc_can_err_skb(priv->netdev, &cf);
-       if (!skb) {
-               stats->rx_dropped++;
-               return;
-       }
-       memcpy(cf, &tmp_cf, sizeof(*cf));
-
        if (new_state != old_state) {
                if (es->status &
                    (M16C_STATE_BUS_OFF | M16C_STATE_BUS_RESET)) {
@@ -1187,11 +1169,18 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
                if (priv->can.restart_ms &&
                    old_state == CAN_STATE_BUS_OFF &&
                    new_state < CAN_STATE_BUS_OFF) {
-                       cf->can_id |= CAN_ERR_RESTARTED;
+                       if (cf)
+                               cf->can_id |= CAN_ERR_RESTARTED;
                        netif_carrier_on(priv->netdev);
                }
        }
 
+       if (!skb) {
+               stats->rx_dropped++;
+               netdev_warn(priv->netdev, "No memory left for err_skb\n");
+               return;
+       }
+
        switch (dev->driver_info->family) {
        case KVASER_LEAF:
                if (es->leaf.error_factor) {