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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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.

Best,
Oliver