On 1/10/21 7:52 AM, Vincent MAILHOL wrote:
[...]

quoted

@@ -118,7 +123,7 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
        struct sk_buff *skb;
        u8 len;

-       skb = __can_get_echo_skb(dev, idx, &len);
+       skb = __can_get_echo_skb(dev, idx, &len, NULL);
        if (!skb)
                return 0;

The can_skb_priv->frame_len is smart. Nice change :)

tnx :)

I have one knit pick concerning the symmetry between
can_put_echo_skb() and can_get_echo_skb().

My current understanding is that:
  * In the tx branch, we need to manually set can_skb_priv->frame_len. Example:
        can_skb_prv(skb)->frame_len = can_skb_get_frame_len(skb);
        can_put_echo_skb(skb, netdev, skb_idx);

ack

  * In the rx branch, it is accessed through the function can_get_echo_skb():
        unsigned int frame_len;
        can_get_echo_skb(skb, netdev, skb_idx, &frame_len);

ack

Please correct me if my understanding is wrong.

I think that you did not modify can_put_echo_skb() so that the
drivers which do not implement the BQL would not have to call
can_skb_get_frame_len(skb) and thus saving computing resources. I
also understand that the motivation to modify can_put_echo_skb()
is to factorise the code.

But the absence of symmetry in the final result bothers me a
bit. Reading manually can_skb_prv(skb)->frame_len by hand would
look as below, which I think is short enough not to be
factorized within can_get_echo_skb():
        struct sk_buff *skb = priv->echo_skb[skb_idx];
        unsigned int frame_len = can_skb_prv(skb)->frame_len;
        can_get_echo_skb(skb, netdev, skb_idx);

So at the end, I would suggest not to modify can_get_echo_skb()
so that it is a better "mirror" of can_put_echo_skb().

That is the logical next step, which I didn't take :)

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |