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

[Vincent MAILHOL]

On Wed. 18 May 2022 à 22:32, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> On 18.05.2022 15:10:44, Oliver Hartkopp wrote:
> > On 18.05.22 14:03, Vincent MAILHOL wrote:
> > > I didn't think this would trigger such a passionate discussion!
> >
> > :-D
> >
> > Maybe your change was the drop that let the bucket run over ;-)
>
> It's so trivial that everybody feels the urge to say something. :D
>
> > > > But e.g. the people that are running Linux instances in a cloud only
> > > > using vcan and vxcan would not need to carry the entire infrastructure
> > > > of CAN hardware support and rx-offload.
> > >
> > > Are there really some people running custom builds of the Linux kernel
> > > in a cloud environment? The benefit of saving a few kilobytes by not
> > > having to carry the entire CAN hardware infrastructure is blown away
> > > by the cost of having to maintain a custom build.
> >
> > When looking to the current Kconfig and Makefile content in
> > drivers/net/can(/dev) there is also some CONFIG_CAN_LEDS which "depends on
> > BROKEN" and builds a leds.o from a non existing leds.c ?!?
> >
> > Oh leds.c is in drivers/net/can/leds.c but not in drivers/net/can/dev/leds.c
> > where it could build ... ?
> >
> > So what I would suggest is that we always build a can-dev.ko when a CAN
> > driver is needed.
> >
> > Then we have different options that may be built-in:
> >
> > 1. netlink hw config interface
> > 2. bitrate calculation
> > 3. rx-offload
> > 4. leds
> >
> > E.g. having the netlink interface without bitrate calculation does not make
> > sense to me too.
>
> ACK
>
> > > I perfectly follow the idea to split rx-offload. Integrators building
> > > some custom firmware for an embedded device might want to strip out
> > > any unneeded piece. But I am not convinced by this same argument when
> > > applied to v(x)can.
> >
> > 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?

> > > 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.

> > 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@xxxxxxxxxxxxxx/

I am OK to add one patch to the series for LED removal. 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?