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

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Max Staudt <max@enpas.org>,
	Marc Kleine-Budde <mkl@pengutronix.de>,
	linux-can@vger.kernel.org, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom_valid to skb.c
Date: Wed, 18 May 2022 21:03:37 +0900	[thread overview]
Message-ID: <CAMZ6RqJ0iCsHT-D5VuYQ9fk42ZEjHStU1yW0RfX1zuJpk5rVtQ@mail.gmail.com> (raw)
In-Reply-To: <0b505b1f-1ee4-5a2c-3bbf-6e9822f78817@hartkopp.net>

I didn't think this would trigger such a passionate discussion!

On Tue. 17 mai 2022 at 22:35, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 5/17/22 14:39, Max Staudt wrote:
> > On Tue, 17 May 2022 14:21:53 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >
> >> On 17.05.2022 14:14:04, Max Staudt wrote:
> >>>> After looking through drivers/net/can/Kconfig I would probably
> >>>> phrase it like this:
> >>>>
> >>>> Select CAN devices (hw/sw) -> we compile a can_dev module. E.g.
> >>>> to handle the skb stuff for vcan's.
> >>>>
> >>>> Select hardware CAN devices -> we compile the netlink stuff into
> >>>> can_dev and offer CAN_CALC_BITTIMING and CAN_LEDS to be compiled
> >>>> into can_dev too.
> >>>>
> >>>> In the latter case: The selection of flexcan, ti_hecc and
> >>>> mcp251xfd automatically selects CAN_RX_OFFLOAD which is then also
> >>>> compiled into can_dev.
> >>>>
> >>>> Would that fit in terms of complexity?
> >>>
> >>> IMHO these should always be compiled into can-dev. Out of tree
> >>> drivers are fairly common here, and having to determine which kind
> >>> of can-dev (stripped or not) the user has on their system is a
> >>> nightmare waiting to happen.
> >>
> >> I personally don't care about out-of-tree drivers.
> >
> > I know that this is the official stance in the kernel.
> >
> > But out-of-tree drivers do happen on a regular basis, even when
> > developing with the aim of upstreaming. And if a developer builds a
> > minimal kernel to host a CAN driver, without building in-tree hardware
> > CAN drivers, then can-dev will be there but behave differently from
> > can-dev in a full distro. Leading to heisenbugs and wasting time. The
> > source of heisenbugs really are the suggested *hidden* Kconfigs.
> >
> >
> > On another note, is the module accounting overhead in the kernel for
> > two new modules with relatively little code in each, code that almost
> > always is loaded when CAN is used, really worth it?
>
> Oh, I didn't want to introduce two new kernel modules but to have
> can_dev in different 'feature levels'.
>
> I would assume a distro kernel to have everything enabled with a full
> featured can_dev - which is likely the base for out-of-tree drivers too.
>
> 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.

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

On a different topic, why are all the CAN devices
under "Networking support" and not "Device Drivers" in menuconfig
like everything else? Would it make sense to move our devices
under the "Device Drivers" section?