Le dim. 10 janv. 2021 à 15:52, Vincent MAILHOL
<mailhol.vincent@xxxxxxxxxx> a écrit :
>
> On Sun. 10 Jan 2021 at 02:40, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> >
> > In order to implement byte queue limits (bql) in CAN drivers, the length of the
> > CAN frame needs to be passed into the networking stack after queueing and after
> > transmission completion.
> >
> > To avoid to calculate this length twice, extend the struct can_skb_priv to hold
> > the length of the CAN frame and extend __can_get_echo_skb() to return that
> > value.
> >
> > Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> > ---
> > drivers/net/can/dev/rx-offload.c | 2 +-
> > drivers/net/can/dev/skb.c | 9 +++++++--
> > include/linux/can/skb.h | 4 +++-
> > 3 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> > index 3c1912c0430b..6a26b5282df1 100644
> > --- a/drivers/net/can/dev/rx-offload.c
> > +++ b/drivers/net/can/dev/rx-offload.c
> > @@ -271,7 +271,7 @@ unsigned int can_rx_offload_get_echo_skb(struct can_rx_offload *offload,
> > u8 len;
> > int err;
> >
> > - skb = __can_get_echo_skb(dev, idx, &len);
> > + skb = __can_get_echo_skb(dev, idx, &len, NULL);
> > if (!skb)
> > return 0;
> >
> > diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> > index 26cd597ff771..24f782a23409 100644
> > --- a/drivers/net/can/dev/skb.c
> > +++ b/drivers/net/can/dev/skb.c
> > @@ -76,7 +76,8 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> > EXPORT_SYMBOL_GPL(can_put_echo_skb);
> >
> > struct sk_buff *
> > -__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
> > +__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
> > + unsigned int *frame_len_ptr)
> > {
> > struct can_priv *priv = netdev_priv(dev);
> >
> > @@ -91,6 +92,7 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
> > * length is supported on both CAN and CANFD frames.
> > */
> > struct sk_buff *skb = priv->echo_skb[idx];
> > + struct can_skb_priv *can_skb_priv = can_skb_prv(skb);
> > struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> >
> > /* get the real payload length for netdev statistics */
> > @@ -99,6 +101,9 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr)
> > else
> > *len_ptr = cf->len;
> >
> > + if (frame_len_ptr)
> > + *frame_len_ptr = can_skb_priv->frame_len;
> > +
> > priv->echo_skb[idx] = NULL;
> >
> > return skb;
> > @@ -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 :)
>
> 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);
> * 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);
>
> 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().
Or another option is to change can_put_echo_skb to:
int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
unsigned int idx, unsigned int frame_len)
Drivers which implement BQL call it with the output of
can_skb_get_frame_len(skb) and drivers which do not simply pass
zero as an input (in the same way that NULL would be given to
can_get_echo_skb()). This way, we have a nice symmetry between
the two echo functions.
>
> > diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> > index c90ebbd3008c..5db9da30843c 100644
> > --- a/include/linux/can/skb.h
> > +++ b/include/linux/can/skb.h
> > @@ -20,7 +20,7 @@ void can_flush_echo_skb(struct net_device *dev);
> > int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> > unsigned int idx);
> > struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx,
> > - u8 *len_ptr);
> > + u8 *len_ptr, unsigned int *frame_len_ptr);
> > unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
> > void can_free_echo_skb(struct net_device *dev, unsigned int idx);
> > struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
> > @@ -42,11 +42,13 @@ struct sk_buff *alloc_can_err_skb(struct net_device *dev,
> > * struct can_skb_priv - private additional data inside CAN sk_buffs
> > * @ifindex: ifindex of the first interface the CAN frame appeared on
> > * @skbcnt: atomic counter to have an unique id together with skb pointer
> > + * @frame_len: length of CAN frame in data link layer
> > * @cf: align to the following CAN frame at skb->data
> > */
> > struct can_skb_priv {
> > int ifindex;
> > int skbcnt;
> > + unsigned int frame_len;
> > struct can_frame cf[];
> > };
> >
> > --
> > 2.29.2
> >
> >
