Hi Marc,
On Wed. 10 Mar 2021 at 06:19, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
> 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?
Yours sincerely,
Vincent
