Re: [PATCH can-next] can: dev: always create TX echo skb

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

On Thur. 18 Mar 2021 at 19:21, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> Hi Vincent,
>
> On 18.03.21 11:03, Vincent MAILHOL wrote:
> > Hi Marc,
> >
> > On Wed. 10 Mar 2021 at 06:19, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>
> >> So far the creation of the TX echo skb was optional and can be
> >> controlled by the local sender of a CAN frame.
> >>
> >> It turns out that the TX echo CAN skb can be piggybacked to carry
> >> information in the driver from the TX- to the TX-complete handler.
> >>
> >> Several drivers already use the return value of
> >> can_get_echo_skb() (which is the length of the data field in the CAN
> >> frame) for their number of transferred bytes statistics. The
> >> statistics are not working if CAN echo skbs are disabled.
> >>
> >> Another use case is to calculate and set the CAN frame length on the
> >> wire, which is needed for BQL support in both the TX and TX-completion
> >> handler.
> >>
> >> For now in can_put_echo_skb(), which is called from the TX handler,
> >> the skb carrying the CAN frame is discarded if no TX echo is
> >> requested, leading to the above illustrated problems.
> >>
> >> This patch changes the can_put_echo_skb() function, so that the echo
> >> skb is always generated. If the sender requests no echo, the echo skb
> >> is consumed in __can_get_echo_skb() without being passed into the RX
> >> handler of the networking stack, but the CAN data length and CAN frame
> >> length information is properly returned.
> >>
> >> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >> ---
> >>   drivers/net/can/dev/skb.c | 10 ++++++++--
> >>   1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> >> index 6a64fe410987..22b0472a5fad 100644
> >> --- a/drivers/net/can/dev/skb.c
> >> +++ b/drivers/net/can/dev/skb.c
> >> @@ -45,7 +45,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> >>          BUG_ON(idx >= priv->echo_skb_max);
> >>
> >>          /* check flag whether this packet has to be looped back */
> >> -       if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK ||
> >> +       if (!(dev->flags & IFF_ECHO) ||
> >>              (skb->protocol != htons(ETH_P_CAN) &&
> >>               skb->protocol != htons(ETH_P_CANFD))) {
> >>                  kfree_skb(skb);
> >> @@ -58,7 +58,6 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> >>                          return -ENOMEM;
> >>
> >>                  /* make settings for echo to reduce code in irq context */
> >> -               skb->pkt_type = PACKET_BROADCAST;
> >>                  skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>                  skb->dev = dev;
> >>
> >> @@ -111,6 +110,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
> >>
> >>                  priv->echo_skb[idx] = NULL;
> >>
> >> +               if (skb->pkt_type == PACKET_LOOPBACK) {
> >> +                       skb->pkt_type = PACKET_BROADCAST;
> >> +               } else {
> >> +                       dev_consume_skb_any(skb);
> >> +                       return NULL;
> >> +               }
> >> +
> >>                  return skb;
> >>          }
> >
> > I do see any particular issues on this patch at the moment,
> > however, while looking at the TX echo functionality, it reminded
> > me of a point which has always been a bit unclear to me: the
> > CAN_CTRLMODE_LOOPBACK. So let me go a bit off topic.
> >
> > Like all other controller's mode, I would expect the
> > CAN_CTRLMODE_LOOPBACK flag to do two things:
> >    - Announce that the device is capable of doing loopback
> >    - Control this feature (enable/disable it)
> >
> > But, by default, this flag is set to 0 unless the user
> > explicitly passes the "loopback on" argument when configuring
> > the device.
> >
> > So isn't this supposed to be an issue for all the drivers which
> > expect to get a TX loopback in order to trigger the TX completion
> > handler?
> >
> > Personally, for my driver, I would like to use the
> > can_set_static_ctrlmode() to force CAN_CTRLMODE_LOOPBACK so that
> > I do not need a different TX completion logic when
> > CAN_CTRLMODE_LOOPBACK is off.
> >
> > The issue is that because CAN_CTRLMODE_LOOPBACK is per default
> > off, doing:
> >    can_set_static_ctrlmode(netdev, CAN_CTRLMODE_LOOPBACK);
> > would lead to a
> >    RTNETLINK answers: Operation not supported
> > when configuring the device unless "loopback on" is explicitly
> > passed on the command line.
> >
> > At the moment, I have the feeling that many drivers just ignore
> > the value of this flag and activate the loopback regardless.
> >
> > Do you think that it would make sense to set
> > CAN_CTRLMODE_LOOPBACK by default to on?
>
> Definitely not.
>
> CAN_CTRLMODE_LOOPBACK defines that the CAN controller establishes a
> shortcut below the rx/tx bitstream engines on chip level. So the
> attached CAN transceiver is not in operation anymore.
>
> Please do not mix up CAN_CTRLMODE_LOOPBACK with the IFF_ECHO flag that
> echoes CAN frames in the sending host to reflect the correct CAN traffic
> e.g. in candump.

OK. I indeed totally misunderstood the meaning of that
CAN_CTRLMODE_LOOPBACK.

Thanks for the clarification. Now, I understand the meaning.


Yours sincerely,
Vincent