lkml.org 
[lkml]   [2021]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 5/5] can: do not increase tx_bytes statistics for RTR frames
    On Fri. 3 Dec. 2021 at 08:35, Jimmy Assarsson <extja@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

    \
     
     \ /
      Last update: 2021-12-03 02:06    [W:2.182 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site