Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 18.05.22 16:07, Vincent MAILHOL wrote:

>>> It does. I've seen CAN setups (really more than one or two!) in VMs and
>>> container environments that are fed by Ethernet tunnels - sometimes also in
>>> embedded devices.
> 
> Are those VM running custom builds of the kernel in which all the CAN
> hardware devices have been removed? Also, isn't it hard to keep those
> up to date with all the kernel security patches?

AFAIK all kind of BSPs create custom configured kernels. And to remove 
attack surfaces the idea is to minimize the code size. That's not only 
to save some flash space.

>>>> A two level split (with or without rx-offload) is what makes the most
>>>> sense to me.
>>>>
>>>> Regardless, having the three level split is not harmful. And because
>>>> there seems to be a consensus on that, I am fine to continue in this
>>>> direction.
>>>
>>> Thanks!
>>>
>>> Should we remove the extra option for the bitrate calculation then?
>>
>> +1
> 
> I can imagine people wanting to ship a product with the bitrate
> calculation removed. For example, an infotainment unit designed for
> one specific vehicle platform (i.e. baudrate is fixed). In that case,
> the integrator might decide to remove bittiming calculation and
> hardcode all hand calculated bittiming parameters instead.
> 
> So that one, I prefer to keep it. I just didn't mention it in my
> previous message because it was already splitted out.

Ok. Interesting that we have such different expectations.

>>> And what about the LEDS support depending on BROKEN?
>>> That was introduced by commit 30f3b42147ba6f ("can: mark led trigger as
>>> broken") from Uwe as it seems there were some changes in 2018.
>>
>> There's a proper generic LED trigger now for network devices. So remove
>> LED triggers, too.
> 
> Yes, the broken LED topic also popped-up last year:
> 
> https://lore.kernel.org/linux-can/20210906143057.zrpor5fkh67uqwi2@pengutronix.de/ > I am OK to add one patch to the series for LED removal.

Hm. We have several drivers that support LED triggers:

$ git grep led.h
at91_can.c:#include <linux/can/led.h>
c_can/c_can_main.c:#include <linux/can/led.h>
ctucanfd/ctucanfd_base.c:#include <linux/can/led.h>
dev/dev.c:#include <linux/can/led.h>
flexcan/flexcan-core.c:#include <linux/can/led.h>
led.c:#include <linux/can/led.h>
m_can/m_can.h:#include <linux/can/led.h>
rcar/rcar_can.c:#include <linux/can/led.h>
rcar/rcar_canfd.c:#include <linux/can/led.h>
sja1000/sja1000.c:#include <linux/can/led.h>
spi/hi311x.c:#include <linux/can/led.h>
spi/mcp251x.c:#include <linux/can/led.h>
sun4i_can.c:#include <linux/can/led.h>
ti_hecc.c:#include <linux/can/led.h>
usb/mcba_usb.c:#include <linux/can/led.h>
usb/usb_8dev.c:#include <linux/can/led.h>
xilinx_can.c:#include <linux/can/led.h>

And I personally like the ability to be able to fire some LEDS (either 
as GPIO or probably in a window manager).

I would suggest to remove the Kconfig entry but not all the code inside 
the drivers, so that a volunteer can convert the LED support based on 
the existing trigger points in the drivers code later.

Our would it make sense to just leave some comment at those places like:

/* LED TX trigger here */

??

> Just some
> heads-up: I will take my time, it will definitely not be ready for the
> v5.19 merge window. And I also expect that there will be at least one
> more round of discussion.
> 
> While I am at it, anything else to add to the wish list before I start
> to working it?

Not really. IMO we already share a common picture now. Thanks for 
picking this up!

Best regards,
Oliver