Re: [PATCH 0/4] can: automate IFF_ECHO flag for generic echo skbs

-CC [email protected]

On 05.08.26 23:06, Vincent Mailhol wrote:
> On 05/08/2026 at 18:17, Oliver Hartkopp wrote:

>> IMO it's the right approach that alloc_candev_mqs() sets the IFF_ECHO
>> flag and the default queue len.
> 
> For IFF_ECHO, this is exactly what this series does!

I just wanted to second you. This does not mean that I fully support the 
way it is implemented.

> For the default queue len, why not. I have not study this particular
> topic. But I think the IFF_ECHO and the queue len should be in separate
> series.

My patch does not even compile. I just wanted to lead the dicsussion 
into a direction to find a more versatile solution that covers virtual 
CAN interfaces, non-echo CAN interfaces and full featured (echo'ing) CAn 
interfaces.

>> What puzzles me is that the slcan driver is something in between which
>> is neither a real CAN hardware nor a virtual CAN interface.
> 
> My understanding it that devices which do not have a TX completion
> handler (like slcan or can327) have no benefits to implement the
> echo_skb framework and can instead simply rely on the PF_CAN core.

Right.

>> My idea would be to use alloc_candev() (-> alloc_candev_mqs()) only for
>> real CAN hardware devices and open code slcan and the virtual CAN
>> drivers ... which goes into the direction below.
>>
>> Any thoughts?
> 
> The logic I tried to follow in this series is that alloc_candev{,_mqs}()
> has two arguments:
> 
>    1. one for the priv structure
> 
>    2. one for the number of echo_skb
> 
> But then, when 2. is zero:
> 
>    alloc_candev{,_mqs}(..., 0)
> 
> means to me: give me all the features expect from the echo_skb.
> 
> With the above, there is no anomalies to see the slcan do:
> 
>    dev = alloc_candev(sizeof(*sl), 0);
> 
> So I don't see the point to open code the allocations in slcan. After
> patch #1 which corrects the echo skb count, the code describes correctly
> the behaviour.

To me ", 0);" is a silent switch which does not make clear that slcan 
and can327 do something different here.

We have 4 features:

- support of IFF_ECHO mode using echo_skb's
- support setting of bitrates via netlink
- support setting of whatever via ethtool
- use of TX queues (tx_queue_len != 0)

And I would like these features to be separately selected to be 
transparent about what the CAN driver needs and supports.

E.g. by defining a wrapper/define

dev = alloc_non_echo_candev(sizeof(*sl));

which calls

dev = alloc_candev(sizeof(*sl), 0);

And the same applies to the other features.

>> +    dev->tx_queue_len = CAN_TX_QUEUE_LEN;
> 
> 
> 
>> +    dev->flags |= IFF_ECHO;
> 
> I really prefer to have the IFF_ECHO gated under the
> 
> 	if (echo_skb_max) {
> 
> because it is tightly linked to the echo skb framework.

Definitely not. This is not what I meant with transparency.

> And yes, there are a couple drivers here and there which set IFF_ECHO
> without using the echo skb framework. But these are the drivers which
> implements their own custom echo skb logic. So it makes sense to have
> them open code the IFF_ECHO because they are also open coding the rest
> of the echo skb logic.
> 
> This goes back to my previous point that:
> 
>    alloc_candev{,_mqs}(..., 0)
> 
> means that the drivers do not use the framework echo skb. Such drivers
> fall in two categories:
> 
>    - No echo skb at all (e.g. slcan or can327): no IFF_ECHO
> 
>    - custom echo skb (e.g. grcan, janz-ican3): everything is open coded
>      -> explicit IFF_ECHO flag
> 

And that's why I would like to split these things up - at least by 
naming them differently.

>>       if (echo_skb_max) {
>>           priv->echo_skb_max = echo_skb_max;
>>           priv->echo_skb = (void *)priv +
>>               (size - echo_skb_max * sizeof(struct sk_buff *));
>>       }
>> diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
>> index 76e6b7b5c6a1..70263813ec40 100644
>> --- a/drivers/net/can/vcan.c
>> +++ b/drivers/net/can/vcan.c
>> @@ -167,16 +167,15 @@ static const struct ethtool_ops vcan_ethtool_ops = {
>>       .get_ts_info = ethtool_op_get_ts_info,
>>   };
>>
>>   static void vcan_setup(struct net_device *dev)
>>   {
>> -    dev->type        = ARPHRD_CAN;
>> -    dev->mtu        = CANXL_MTU;
>> -    dev->hard_header_len    = 0;
>> -    dev->addr_len        = 0;
>> -    dev->tx_queue_len    = 0;
>> -    dev->flags        = IFF_NOARP;
>> +    can_setup(dev);
>> +    dev->tx_queue_len = 0;
>> +    dev->mtu = CANXL_MTU;
>> +    dev->min_mtu = CAN_MTU;
>> +    dev->max_mtu = CANXL_MTU;
>>       can_set_ml_priv(dev, netdev_priv(dev));
>>       vcan_set_cap_info(dev);
> 
> In such example, please don't add parasite white space changes. It makes
> it hard to grasp what you are actually modifying.

Agreed. As I wrote above - it does not even compile and was never 
intended to be used as upstream code.

(..)

>> -void can_setup(struct net_device *dev);
>> +void can_setup(struct net_device *dev)
>> +{
>> +    dev->type = ARPHRD_CAN;
>> +    dev->mtu = CAN_MTU;
>> +    dev->min_mtu = CAN_MTU;
>> +    dev->max_mtu = CAN_MTU;
>> +    dev->hard_header_len = 0;
>> +    dev->addr_len = 0;
>> +
>> +    /* New-style flags. */
>> +    dev->flags = IFF_NOARP;
>> +    dev->features = NETIF_F_HW_CSUM;
>> +}
> 
> It is strange to have a non static inline function in a header. What was
> the motivation for pulling this out of dev.c?

Yeah. My thought was that we might reduce code duplication when we 
provice more granularity in those helper functions.

I moved it to dev.h to avoid the building of dev.c for the virtual CAN 
interfaces.

Best regards,
Oliver