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: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>,
	linux-can@vger.kernel.org, linux-kernel@vger.kernel.org,
	Max Staudt <max@enpas.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: Tue, 17 May 2022 16:04:53 +0900	[thread overview]
Message-ID: <CAMZ6RqJ3sXYUOpw7hEfDzj14H-vXK_i+eYojBk2Lq=h=7cm7Jg@mail.gmail.com> (raw)
In-Reply-To: <20220517060821.akuqbqxro34tj7x6@pengutronix.de>

On Tue. 17 May 2022 at 15:08, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 17.05.2022 10:50:16, Vincent MAILHOL wrote:
> > > would it probably make sense to
> > > introduce a new can-skb module that could be used by all CAN
> > > virtual/software interfaces?
> > >
> > > Or some other split-up ... any idea?
> >
> > My concern is: what would be the merrit? If we do not split, the users
> > of slcan and v(x)can would have to load the can-dev module which will
> > be slightly bloated for their use, but is this really an issue?
>
> If you use modprobe all required modules are loaded automatically.

Yes, this, now I know. In the past, when I started developing
etas_es58x as an out of tree module (which I inserted using insmod),
it took me a little time to figure out the dependencies tree and which
module I actually had to modprobe separately.

> > I do
> > not see how this can become a performance bottleneck, so what is the
> > problem?
> > I could also argue that most of the devices do not depend on
> > rx-offload.o. So should we also split this one out of can-dev on the
> > same basis and add another module dependency?
>
> We can add a non user visible Kconfig symbol for rx-offload and let the
> drivers that need it do a "select" on it. If selected the rx-offload
> would be compiled into to can-dev module.

Yes, and this remark also applies to can-skb I think.

So slcan, v(x)can and can-dev will select can-skb, and some of the
hardware drivers (still have to figure out the list) will select
can-rx-offload (other dependencies will stay as it is today).

I think that splitting the current can-dev into can-skb + can-dev +
can-rx-offload is enough. Please let me know if you see a need for
more.

> > The benefit (not having to load a bloated module for three drivers)
> > does not outweigh the added complexity: all hardware modules will have
> > one additional modprobe dependency on the tiny can-skb module.
> >
> > But as said above, I am not fully opposed to the split, I am just
> > strongly divided. If we go for the split, creating a can-skb module is
> > the natural and only option I see.
> > If the above argument does not convince you, I will send a v3 with that split.

If both you and Oliver prefer the split, then split it would be!

Because this topic is related to Kbuild, there is actually one thing
which bothered me but which I never asked: 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? I can do it while working on the v3.