[PATCH v3 5/5] can: do not increase tx_bytes statistics for RTR frames

Vincent MAILHOL mailhol.vincent at wanadoo.fr
Thu Dec 2 17:05:18 PST 2021


On Fri. 3 Dec. 2021 at 08:35, Jimmy Assarsson <extja at kvaser.com> wrote:
> On 2021-11-28 13:37, Vincent Mailhol wrote:
> > The actual payload length of the CAN Remote Transmission Request (RTR)
> > frames is always 0, i.e. nothing is transmitted on the wire. However,
> > those RTR frames still use the DLC to indicate the length of the
> > requested frame.
> >
> > As such, net_device_stats:tx_bytes should not be increased when
> > sending RTR frames.
> >
> > The function can_get_echo_skb() already returns the correct length,
> > even for RTR frames (c.f. [1]). However, for historical reasons, the
> > drivers do not use can_get_echo_skb()'s return value and instead, most
> > of them store a temporary length (or dlc) in some local structure or
> > array. Using the return value of can_get_echo_skb() solves the
> > issue. After doing this, such length/dlc fields become unused and so
> > this patch does the adequate cleaning when needed.
> >
> > This patch fixes all the CAN drivers.
> >
> > Finally, can_get_echo_skb() is decorated with the __must_check
> > attribute in order to force future drivers to correctly use its return
> > value (else the compiler would emit a warning).
> >
> > [1] commit ed3320cec279 ("can: dev: __can_get_echo_skb():
> > fix real payload length return value for RTR frames")
>
> Hi Vincent!
>
> Thanks for the patch!
> I've reviewed and tested the changes affecting kvaser_usb.
> Looks good to me, only a minor nitpick inline :)
>

[...]

> > diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> > index 390b6bde883c..3a49257f9fa6 100644
> > --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> > +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> > @@ -77,7 +77,6 @@ struct kvaser_usb_dev_card_data {
> >   struct kvaser_usb_tx_urb_context {
> >       struct kvaser_usb_net_priv *priv;
> >       u32 echo_index;
> > -     int dlc;
> >   };
> >
> >   struct kvaser_usb {
> > @@ -162,8 +161,8 @@ struct kvaser_usb_dev_ops {
> >       void (*dev_read_bulk_callback)(struct kvaser_usb *dev, void *buf,
> >                                      int len);
> >       void *(*dev_frame_to_cmd)(const struct kvaser_usb_net_priv *priv,
> > -                               const struct sk_buff *skb, int *frame_len,
> > -                               int *cmd_len, u16 transid);
> > +                               const struct sk_buff *skb, int *cmd_len,
> > +                               u16 transid);
> >   };
> >
> >   struct kvaser_usb_dev_cfg {
> > diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> > index 3e682ef43f8e..c4b4d3d0a387 100644
> > --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> > +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> > @@ -565,7 +565,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> >               goto freeurb;
> >       }
> >
> > -     buf = dev->ops->dev_frame_to_cmd(priv, skb, &context->dlc, &cmd_len,
> > +     buf = dev->ops->dev_frame_to_cmd(priv, skb, &cmd_len,
> >                                        context->echo_index);
> >       if (!buf) {
> >               stats->tx_dropped++;
> > 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 17fabd3d0613..9f423a5fb63f 100644
> > --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> > +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> > @@ -1113,7 +1113,7 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
> >       struct kvaser_usb_net_priv *priv;
> >       struct can_frame *cf;
> >       unsigned long irq_flags;
> > -     int len;
> > +     unsigned int len;
> >       bool one_shot_fail = false, is_err_frame = false;
> >       u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd);
> >
> > @@ -1136,7 +1136,6 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
> >       }
> >
> >       context = &priv->tx_contexts[transid % dev->max_tx_urbs];
> > -     len = context->dlc;
> >
> >       spin_lock_irqsave(&priv->tx_contexts_lock, irq_flags);
> >
> > @@ -1144,7 +1143,8 @@ static void kvaser_usb_hydra_tx_acknowledge(const struct kvaser_usb *dev,
> >       if (cf)
> >               is_err_frame = !!(cf->can_id & CAN_RTR_FLAG);
> >
> > -     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);
> > @@ -1375,8 +1375,8 @@ static void kvaser_usb_hydra_handle_cmd(const struct kvaser_usb *dev,
> >
> >   static void *
> >   kvaser_usb_hydra_frame_to_cmd_ext(const struct kvaser_usb_net_priv *priv,
> > -                               const struct sk_buff *skb, int *frame_len,
> > -                               int *cmd_len, u16 transid)
> > +                               const struct sk_buff *skb, int *cmd_len,
> > +                               u16 transid)
> >   {
> >       struct kvaser_usb *dev = priv->dev;
> >       struct kvaser_cmd_ext *cmd;
> > @@ -1388,8 +1388,6 @@ kvaser_usb_hydra_frame_to_cmd_ext(const struct kvaser_usb_net_priv *priv,
> >       u32 kcan_id;
> >       u32 kcan_header;
> >
> > -     *frame_len = nbr_of_bytes;
> > -
> >       cmd = kcalloc(1, sizeof(struct kvaser_cmd_ext), GFP_ATOMIC);
> >       if (!cmd)
> >               return NULL;
> > @@ -1455,8 +1453,8 @@ kvaser_usb_hydra_frame_to_cmd_ext(const struct kvaser_usb_net_priv *priv,
> >
> >   static void *
> >   kvaser_usb_hydra_frame_to_cmd_std(const struct kvaser_usb_net_priv *priv,
> > -                               const struct sk_buff *skb, int *frame_len,
> > -                               int *cmd_len, u16 transid)
> > +                               const struct sk_buff *skb, int *cmd_len,
> > +                               u16 transid)
> >   {
> >       struct kvaser_usb *dev = priv->dev;
> >       struct kvaser_cmd *cmd;
> > @@ -1464,8 +1462,6 @@ kvaser_usb_hydra_frame_to_cmd_std(const struct kvaser_usb_net_priv *priv,
> >       u32 flags;
> >       u32 id;
> >
> > -     *frame_len = cf->len;
> > -
> >       cmd = kcalloc(1, sizeof(struct kvaser_cmd), GFP_ATOMIC);
> >       if (!cmd)
> >               return NULL;
> > @@ -1493,13 +1489,13 @@ kvaser_usb_hydra_frame_to_cmd_std(const struct kvaser_usb_net_priv *priv,
> >       if (cf->can_id & CAN_RTR_FLAG)
> >               flags |= KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME;
> >
> > -     flags |= (cf->can_id & CAN_ERR_FLAG ?
> > -               KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME : 0);
> > +     if (cf->can_id & CAN_ERR_FLAG)
> > +             flags |= KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME;
>
> This has nothing to do with RTR. Maybe put it in a separate patch?

Arg... You are right. This should not be here. I saw it in my
final check, removed it in my tree and forgot to redo a "git
format-patch".

This is some leftover of a previous version in which I did more
heavy changes to kvaser_usb_hydra_frame_to_cmd_std(). This is
purely cosmetic though. I am not willing to go into a clean up
crusade of all CAN drivers so I will just leave the ternary
operator untouched. Free to you to reuse it if you want to do a
clean up later on.

> >
> >       cmd->tx_can.id = cpu_to_le32(id);
> >       cmd->tx_can.flags = flags;
> >
> > -     memcpy(cmd->tx_can.data, cf->data, *frame_len);
> > +     memcpy(cmd->tx_can.data, cf->data, cf->len);
> >
> >       return cmd;
> >   }
> > @@ -2007,17 +2003,17 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
> >
> >   static void *
> >   kvaser_usb_hydra_frame_to_cmd(const struct kvaser_usb_net_priv *priv,
> > -                           const struct sk_buff *skb, int *frame_len,
> > -                           int *cmd_len, u16 transid)
> > +                           const struct sk_buff *skb, int *cmd_len,
> > +                           u16 transid)
> >   {
> >       void *buf;
> >
> >       if (priv->dev->card_data.capabilities & KVASER_USB_HYDRA_CAP_EXT_CMD)
> > -             buf = kvaser_usb_hydra_frame_to_cmd_ext(priv, skb, frame_len,
> > -                                                     cmd_len, transid);
> > +             buf = kvaser_usb_hydra_frame_to_cmd_ext(priv, skb, cmd_len,
> > +                                                     transid);
> >       else
> > -             buf = kvaser_usb_hydra_frame_to_cmd_std(priv, skb, frame_len,
> > -                                                     cmd_len, transid);
> > +             buf = kvaser_usb_hydra_frame_to_cmd_std(priv, skb, cmd_len,
> > +                                                     transid);
> >
> >       return buf;
> >   }
> > 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 14b445643554..47fa7f5a11c6 100644
> > --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> > +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> > @@ -342,16 +342,14 @@ struct kvaser_usb_err_summary {
> >
> >   static void *
> >   kvaser_usb_leaf_frame_to_cmd(const struct kvaser_usb_net_priv *priv,
> > -                          const struct sk_buff *skb, int *frame_len,
> > -                          int *cmd_len, u16 transid)
> > +                          const struct sk_buff *skb, int *cmd_len,
> > +                          u16 transid)
> >   {
> >       struct kvaser_usb *dev = priv->dev;
> >       struct kvaser_cmd *cmd;
> >       u8 *cmd_tx_can_flags = NULL;            /* GCC */
> >       struct can_frame *cf = (struct can_frame *)skb->data;
> >
> > -     *frame_len = cf->len;
> > -
> >       cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> >       if (cmd) {
> >               cmd->u.tx_can.tid = transid & 0xff;
> > @@ -587,12 +585,11 @@ static void kvaser_usb_leaf_tx_acknowledge(const struct kvaser_usb *dev,
> >               priv->can.state = CAN_STATE_ERROR_ACTIVE;
> >       }
> >
> > -     stats->tx_packets++;
> > -     stats->tx_bytes += context->dlc;
> > -
> >       spin_lock_irqsave(&priv->tx_contexts_lock, flags);
> >
> > -     can_get_echo_skb(priv->netdev, context->echo_index, NULL);
> > +     stats->tx_packets++;
> > +     stats->tx_bytes += 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);

[...]

Yours sincerely,
Vincent Mailhol



More information about the linux-arm-kernel mailing list