| Message ID | 20241116180301.3935879-8-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
-netdev ML Hello Dario, thanks for taking a look on the error statistics. As an example for my following remark I picked this patch. On 16.11.24 19:02, Dario Binacchi wrote: > The f81604_handle_can_bus_errors() function only incremented the receive > error counter and never the transmit error counter, even if the ECC_DIR > flag reported that an error had occurred during transmission. The patch > increments the receive/transmit error counter based on the value of the > ECC_DIR flag. > > Fixes: 88da17436973 ("can: usb: f81604: add Fintek F81604 support") > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > --- > > drivers/net/can/usb/f81604.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c > index bc0c8903fe77..8463e00517c9 100644 > --- a/drivers/net/can/usb/f81604.c > +++ b/drivers/net/can/usb/f81604.c > @@ -526,7 +526,6 @@ static void f81604_handle_can_bus_errors(struct f81604_port_priv *priv, > netdev_dbg(netdev, "bus error interrupt\n"); > > priv->can.can_stats.bus_error++; > - stats->rx_errors++; > > if (skb) { The statistics are updated only if we successfully allocated a skbuff for the error message frame. > cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > @@ -550,8 +549,12 @@ static void f81604_handle_can_bus_errors(struct f81604_port_priv *priv, > cf->data[3] = data->ecc & F81604_SJA1000_ECC_SEG; > > /* Error occurred during transmission? */ > - if ((data->ecc & F81604_SJA1000_ECC_DIR) == 0) > + if ((data->ecc & F81604_SJA1000_ECC_DIR) == 0) { > cf->data[2] |= CAN_ERR_PROT_TX; > + stats->tx_errors++; > + } else { > + stats->rx_errors++; > + } > } > > set_bit(F81604_CLEAR_ECC, &priv->clear_flags); Is this a good approach or should we always update the statistics independently of the potential failure of the skbuff allocation? Best regards, Oliver To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On 18.11.2024 09:39:27, Oliver Hartkopp wrote: > -netdev ML > > Hello Dario, > > thanks for taking a look on the error statistics. > > As an example for my following remark I picked this patch. > > On 16.11.24 19:02, Dario Binacchi wrote: > > The f81604_handle_can_bus_errors() function only incremented the receive > > error counter and never the transmit error counter, even if the ECC_DIR > > flag reported that an error had occurred during transmission. The patch > > increments the receive/transmit error counter based on the value of the > > ECC_DIR flag. "Increment" the receive/transmit error counter instead of "The patch increments". > > > > Fixes: 88da17436973 ("can: usb: f81604: add Fintek F81604 support") > > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > > > --- > > > > drivers/net/can/usb/f81604.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c > > index bc0c8903fe77..8463e00517c9 100644 > > --- a/drivers/net/can/usb/f81604.c > > +++ b/drivers/net/can/usb/f81604.c > > @@ -526,7 +526,6 @@ static void f81604_handle_can_bus_errors(struct f81604_port_priv *priv, > > netdev_dbg(netdev, "bus error interrupt\n"); > > priv->can.can_stats.bus_error++; > > - stats->rx_errors++; > > if (skb) { > > The statistics are updated only if we successfully allocated a skbuff for > the error message frame. > > > cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; > > @@ -550,8 +549,12 @@ static void f81604_handle_can_bus_errors(struct f81604_port_priv *priv, > > cf->data[3] = data->ecc & F81604_SJA1000_ECC_SEG; > > /* Error occurred during transmission? */ > > - if ((data->ecc & F81604_SJA1000_ECC_DIR) == 0) > > + if ((data->ecc & F81604_SJA1000_ECC_DIR) == 0) { > > cf->data[2] |= CAN_ERR_PROT_TX; > > + stats->tx_errors++; > > + } else { > > + stats->rx_errors++; > > + } > > } > > set_bit(F81604_CLEAR_ECC, &priv->clear_flags); > > Is this a good approach or should we always update the statistics > independently of the potential failure of the skbuff allocation? Yes, we always want to update the statistics. If the error skb allocation fails due to OOM, we want at least accurate statistics. regards, Marc
diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c index bc0c8903fe77..8463e00517c9 100644 --- a/drivers/net/can/usb/f81604.c +++ b/drivers/net/can/usb/f81604.c @@ -526,7 +526,6 @@ static void f81604_handle_can_bus_errors(struct f81604_port_priv *priv, netdev_dbg(netdev, "bus error interrupt\n"); priv->can.can_stats.bus_error++; - stats->rx_errors++; if (skb) { cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR; @@ -550,8 +549,12 @@ static void f81604_handle_can_bus_errors(struct f81604_port_priv *priv, cf->data[3] = data->ecc & F81604_SJA1000_ECC_SEG; /* Error occurred during transmission? */ - if ((data->ecc & F81604_SJA1000_ECC_DIR) == 0) + if ((data->ecc & F81604_SJA1000_ECC_DIR) == 0) { cf->data[2] |= CAN_ERR_PROT_TX; + stats->tx_errors++; + } else { + stats->rx_errors++; + } } set_bit(F81604_CLEAR_ECC, &priv->clear_flags);
The f81604_handle_can_bus_errors() function only incremented the receive error counter and never the transmit error counter, even if the ECC_DIR flag reported that an error had occurred during transmission. The patch increments the receive/transmit error counter based on the value of the ECC_DIR flag. Fixes: 88da17436973 ("can: usb: f81604: add Fintek F81604 support") Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- drivers/net/can/usb/f81604.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)