On 1/10/21 7:52 AM, Vincent MAILHOL wrote: [...] >> @@ -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 |
Attachment:
signature.asc
Description: OpenPGP digital signature
