can: xilinx_can: avoid non-requested bus error frames

Userspace can signal with CAN_CTRLMODE_BERR_REPORTING whether they need
reporting of bus errors (CAN_ERR_BUSERROR) or not.

However, xilinx_can driver currently always sends CAN_ERR_BUSERROR
frames to userspace on bus errors.

To improve performance on error conditions when bus error reporting is
not needed, avoid sending CAN_ERR_BUSERROR frames unless requested via
CAN_CTRLMODE_BERR_REPORTING.

The error interrupt is still kept enabled as there is no dedicated state
transition interrupt, but just disabling error frame submission still
yields a significant performance improvement. In a simple test with
continuous bus errors and no userspace programs reading/writing CAN I
saw system CPU load reduced by 1/3.

Tested on a ZynqMP board with CAN-FD v1.0.

Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
index b6fa735e47f1439644f9064aed9302b096650cb8..f338633790361cdfd2caef28b4ebe13a55182a52 100644 (file)
--- a/drivers/net/can/xilinx_can.c
+++ b/drivers/net/can/xilinx_can.c
@@ -470,7 +470,13 @@ static int xcan_chip_start(struct net_device *ndev)
        if (err < 0)
                return err;
 
-       /* Enable interrupts */
+       /* Enable interrupts
+        *
+        * We enable the ERROR interrupt even with
+        * CAN_CTRLMODE_BERR_REPORTING disabled as there is no
+        * dedicated interrupt for a state change to
+        * ERROR_WARNING/ERROR_PASSIVE.
+        */
        ier = XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |
                XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK |
                XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
@@ -980,12 +986,9 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
 {
        struct xcan_priv *priv = netdev_priv(ndev);
        struct net_device_stats *stats = &ndev->stats;
-       struct can_frame *cf;
-       struct sk_buff *skb;
+       struct can_frame cf = { };
        u32 err_status;
 
-       skb = alloc_can_err_skb(ndev, &cf);
-
        err_status = priv->read_reg(priv, XCAN_ESR_OFFSET);
        priv->write_reg(priv, XCAN_ESR_OFFSET, err_status);
 
@@ -995,32 +998,27 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
                /* Leave device in Config Mode in bus-off state */
                priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
                can_bus_off(ndev);
-               if (skb)
-                       cf->can_id |= CAN_ERR_BUSOFF;
+               cf.can_id |= CAN_ERR_BUSOFF;
        } else {
                enum can_state new_state = xcan_current_error_state(ndev);
 
                if (new_state != priv->can.state)
-                       xcan_set_error_state(ndev, new_state, skb ? cf : NULL);
+                       xcan_set_error_state(ndev, new_state, &cf);
        }
 
        /* Check for Arbitration lost interrupt */
        if (isr & XCAN_IXR_ARBLST_MASK) {
                priv->can.can_stats.arbitration_lost++;
-               if (skb) {
-                       cf->can_id |= CAN_ERR_LOSTARB;
-                       cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
-               }
+               cf.can_id |= CAN_ERR_LOSTARB;
+               cf.data[0] = CAN_ERR_LOSTARB_UNSPEC;
        }
 
        /* Check for RX FIFO Overflow interrupt */
        if (isr & XCAN_IXR_RXOFLW_MASK) {
                stats->rx_over_errors++;
                stats->rx_errors++;
-               if (skb) {
-                       cf->can_id |= CAN_ERR_CRTL;
-                       cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
-               }
+               cf.can_id |= CAN_ERR_CRTL;
+               cf.data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
        }
 
        /* Check for RX Match Not Finished interrupt */
@@ -1028,68 +1026,77 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
                stats->rx_dropped++;
                stats->rx_errors++;
                netdev_err(ndev, "RX match not finished, frame discarded\n");
-               if (skb) {
-                       cf->can_id |= CAN_ERR_CRTL;
-                       cf->data[1] |= CAN_ERR_CRTL_UNSPEC;
-               }
+               cf.can_id |= CAN_ERR_CRTL;
+               cf.data[1] |= CAN_ERR_CRTL_UNSPEC;
        }
 
        /* Check for error interrupt */
        if (isr & XCAN_IXR_ERROR_MASK) {
-               if (skb)
-                       cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+               bool berr_reporting = false;
+
+               if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
+                       berr_reporting = true;
+                       cf.can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+               }
 
                /* Check for Ack error interrupt */
                if (err_status & XCAN_ESR_ACKER_MASK) {
                        stats->tx_errors++;
-                       if (skb) {
-                               cf->can_id |= CAN_ERR_ACK;
-                               cf->data[3] = CAN_ERR_PROT_LOC_ACK;
+                       if (berr_reporting) {
+                               cf.can_id |= CAN_ERR_ACK;
+                               cf.data[3] = CAN_ERR_PROT_LOC_ACK;
                        }
                }
 
                /* Check for Bit error interrupt */
                if (err_status & XCAN_ESR_BERR_MASK) {
                        stats->tx_errors++;
-                       if (skb) {
-                               cf->can_id |= CAN_ERR_PROT;
-                               cf->data[2] = CAN_ERR_PROT_BIT;
+                       if (berr_reporting) {
+                               cf.can_id |= CAN_ERR_PROT;
+                               cf.data[2] = CAN_ERR_PROT_BIT;
                        }
                }
 
                /* Check for Stuff error interrupt */
                if (err_status & XCAN_ESR_STER_MASK) {
                        stats->rx_errors++;
-                       if (skb) {
-                               cf->can_id |= CAN_ERR_PROT;
-                               cf->data[2] = CAN_ERR_PROT_STUFF;
+                       if (berr_reporting) {
+                               cf.can_id |= CAN_ERR_PROT;
+                               cf.data[2] = CAN_ERR_PROT_STUFF;
                        }
                }
 
                /* Check for Form error interrupt */
                if (err_status & XCAN_ESR_FMER_MASK) {
                        stats->rx_errors++;
-                       if (skb) {
-                               cf->can_id |= CAN_ERR_PROT;
-                               cf->data[2] = CAN_ERR_PROT_FORM;
+                       if (berr_reporting) {
+                               cf.can_id |= CAN_ERR_PROT;
+                               cf.data[2] = CAN_ERR_PROT_FORM;
                        }
                }
 
                /* Check for CRC error interrupt */
                if (err_status & XCAN_ESR_CRCER_MASK) {
                        stats->rx_errors++;
-                       if (skb) {
-                               cf->can_id |= CAN_ERR_PROT;
-                               cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
+                       if (berr_reporting) {
+                               cf.can_id |= CAN_ERR_PROT;
+                               cf.data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
                        }
                }
                priv->can.can_stats.bus_error++;
        }
 
-       if (skb) {
-               stats->rx_packets++;
-               stats->rx_bytes += cf->can_dlc;
-               netif_rx(skb);
+       if (cf.can_id) {
+               struct can_frame *skb_cf;
+               struct sk_buff *skb = alloc_can_err_skb(ndev, &skb_cf);
+
+               if (skb) {
+                       skb_cf->can_id |= cf.can_id;
+                       memcpy(skb_cf->data, cf.data, CAN_ERR_DLC);
+                       stats->rx_packets++;
+                       stats->rx_bytes += CAN_ERR_DLC;
+                       netif_rx(skb);
+               }
        }
 
        netdev_dbg(ndev, "%s: error status register:0x%x\n",