diff mbox series

[7/7] can: f81604: fix {rx,tx}_errors statistics

Message ID 20241116180301.3935879-8-dario.binacchi@amarulasolutions.com
State New
Headers
Series Fix {rx,tx}_errors CAN statistics |

Commit Message

Dario Binacchi Nov. 16, 2024, 6:02 p.m. UTC 
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(-)

Comments

'Abel Vesa' via Amarula Linux Nov. 18, 2024, 8:39 a.m. UTC | #1 
-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.
Marc Kleine-Budde Nov. 18, 2024, 8:50 a.m. UTC | #2 
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 mbox series

Patch

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);