On Fri. 3 Dec 2021 at 08:35, Jimmy Assarsson <extja@xxxxxxxxxx> wrote: > On 2021-11-28 13:37, Vincent Mailhol wrote: > > The CAN error message frames (i.e. error skb) are an interface > > specific to socket CAN. The payload of the CAN error message frames > > does not correspond to any actual data sent on the wire. Only an error > > flag and a delimiter are transmitted when an error occurs (c.f. ISO > > 11898-1 section 10.4.4.2 "Error flag"). > > > > For this reason, it makes no sense to increment the tx_packets and > > tx_bytes fields of struct net_device_stats when sending an error > > message frame because no actual payload will be transmitted on the > > wire. > > > > N.B. Sending error message frames is a very specific feature which, at > > the moment, is only supported by the Kvaser Hydra hardware. Please > > refer to [1] for more details on the topic. > > > > [1] https://lore.kernel.org/linux-can/CAMZ6RqK0rTNg3u3mBpZOoY51jLZ-et-J01tY6-+mWsM4meVw-A@xxxxxxxxxxxxxx/t/#u > > > > CC: Jimmy Assarsson <extja@xxxxxxxxxx> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > Hi Vincent! > > Thanks for the patch. > There are flags in the TX ACK package, which makes it possible to > determine if it was an error frame or not. So we don't need to get > the original CAN frame to determine this. > I suggest the following change: This is a great suggestion. I was not a fan of getting the original CAN frame, this TX ACK solves the issue. > --- > .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 25 ++++++++++++------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > 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 3398da323126..01b076f04e26 100644 > --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c > +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c > @@ -295,6 +295,7 @@ struct kvaser_cmd { > #define KVASER_USB_HYDRA_CF_FLAG_OVERRUN BIT(1) > #define KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME BIT(4) > #define KVASER_USB_HYDRA_CF_FLAG_EXTENDED_ID BIT(5) > +#define KVASER_USB_HYDRA_CF_FLAG_TX_ACK BIT(6) > /* CAN frame flags. Used in ext_rx_can and ext_tx_can */ > #define KVASER_USB_HYDRA_CF_FLAG_OSM_NACK BIT(12) > #define KVASER_USB_HYDRA_CF_FLAG_ABL BIT(13) > @@ -1112,7 +1113,9 @@ static void kvaser_usb_hydra_tx_acknowledge(const > struct kvaser_usb *dev, > struct kvaser_usb_tx_urb_context *context; > struct kvaser_usb_net_priv *priv; > unsigned long irq_flags; > + unsigned int len; > bool one_shot_fail = false; > + bool is_err_frame = false; > u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd); > > priv = kvaser_usb_hydra_net_priv_from_cmd(dev, cmd); > @@ -1131,24 +1134,28 @@ static void > kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev, > kvaser_usb_hydra_one_shot_fail(priv, cmd_ext); > one_shot_fail = true; > } > - } > - > - context = &priv->tx_contexts[transid % dev->max_tx_urbs]; > - if (!one_shot_fail) { > - struct net_device_stats *stats = &priv->netdev->stats; > - > - stats->tx_packets++; > - stats->tx_bytes += can_fd_dlc2len(context->dlc); > + if (flags & KVASER_USB_HYDRA_CF_FLAG_TX_ACK && > + flags & KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME) > + is_err_frame = true; Nitpick, but I prefer to write: + is_err_frame = flags & KVASER_USB_HYDRA_CF_FLAG_TX_ACK && + flags & KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME; > } > > spin_lock_irqsave(&priv->tx_contexts_lock, irq_flags); > > - can_get_echo_skb(priv->netdev, context->echo_index, NULL); > + context = &priv->tx_contexts[transid % dev->max_tx_urbs]; > + len = can_get_echo_skb(priv->netdev, context->echo_index, NULL); This line is related to the tx RTR. I will rebase this into "can: do not increase rx_bytes statistics for RTR frames" (patch 4/5). > + > context->echo_index = dev->max_tx_urbs; > --priv->active_tx_contexts; > netif_wake_queue(priv->netdev); > > spin_unlock_irqrestore(&priv->tx_contexts_lock, irq_flags); > + > + if (!one_shot_fail && !is_err_frame) { > + struct net_device_stats *stats = &priv->netdev->stats; > + > + stats->tx_packets++; > + stats->tx_bytes += len; > + } Same here, there is no need anymore to move this block *in this patch*, will rebase it. > } > > static void kvaser_usb_hydra_rx_msg_std(const struct kvaser_usb *dev, This patch ("can: kvaser_usb: do not increase tx statistics when sending error message frames") will become: 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 3398da323126..75009d38f8e3 100644 --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c @@ -295,6 +295,7 @@ struct kvaser_cmd { #define KVASER_USB_HYDRA_CF_FLAG_OVERRUN BIT(1) #define KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME BIT(4) #define KVASER_USB_HYDRA_CF_FLAG_EXTENDED_ID BIT(5) +#define KVASER_USB_HYDRA_CF_FLAG_TX_ACK BIT(6) /* CAN frame flags. Used in ext_rx_can and ext_tx_can */ #define KVASER_USB_HYDRA_CF_FLAG_OSM_NACK BIT(12) #define KVASER_USB_HYDRA_CF_FLAG_ABL BIT(13) @@ -1113,6 +1114,7 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev, struct kvaser_usb_net_priv *priv; unsigned long irq_flags; bool one_shot_fail = false; + bool is_err_frame = false; u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd); priv = kvaser_usb_hydra_net_priv_from_cmd(dev, cmd); @@ -1131,10 +1133,13 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev, kvaser_usb_hydra_one_shot_fail(priv, cmd_ext); one_shot_fail = true; } + + is_err_frame = flags & KVASER_USB_HYDRA_CF_FLAG_TX_ACK && + flags & KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME; } context = &priv->tx_contexts[transid % dev->max_tx_urbs]; - if (!one_shot_fail) { + if (!one_shot_fail && !is_err_frame) { struct net_device_stats *stats = &priv->netdev->stats; stats->tx_packets++; And patch 5/5 ("can: do not increase tx_bytes statistics for RTR frames") becomes: 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 75009d38f8e3..2cb35bd162a4 100644 --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c @@ -1113,6 +1113,7 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev, struct kvaser_usb_tx_urb_context *context; struct kvaser_usb_net_priv *priv; unsigned long irq_flags; + unsigned int len; bool one_shot_fail = false; bool is_err_frame = false; u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd); @@ -1139,21 +1140,23 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev, } context = &priv->tx_contexts[transid % dev->max_tx_urbs]; - if (!one_shot_fail && !is_err_frame) { - struct net_device_stats *stats = &priv->netdev->stats; - - stats->tx_packets++; - stats->tx_bytes += can_fd_dlc2len(context->dlc); - } spin_lock_irqsave(&priv->tx_contexts_lock, irq_flags); - can_get_echo_skb(priv->netdev, context->echo_index, NULL); + len = can_get_echo_skb(priv->netdev, context->echo_index, NULL); context->echo_index = dev->max_tx_urbs; --priv->active_tx_contexts; netif_wake_queue(priv->netdev); spin_unlock_irqrestore(&priv->tx_contexts_lock, irq_flags); + + if (!one_shot_fail && !is_err_frame) { + struct net_device_stats *stats = &priv->netdev->stats; + + stats->tx_packets++; + stats->tx_bytes += len; + } } Does this look good to you? If so, can I add these tags to patch 2/5? Co-developed-by: Jimmy Assarsson <extja@xxxxxxxxxx> Signed-off-by: Jimmy Assarsson <extja@xxxxxxxxxx> Also, can I add your tested-by to patches 1/5, 4/5 and 5/5? Tested-by: Jimmy Assarsson <extja@xxxxxxxxxx> Yours sincerely, Vincent Mailhol
