Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220513142355.250389-1-mailhol.vincent@wanadoo.fr>
 <20220514141650.1109542-1-mailhol.vincent@wanadoo.fr> <20220514141650.1109542-4-mailhol.vincent@wanadoo.fr>
 <7b1644ad-c117-881e-a64f-35b8d8b40ef7@hartkopp.net>
In-Reply-To: <7b1644ad-c117-881e-a64f-35b8d8b40ef7@hartkopp.net>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Tue, 17 May 2022 10:50:16 +0900
Message-ID: <CAMZ6RqKZMHXB7rQ70GrXcVE7x7kytAAGfE+MOpSgWgWgp0gD2g@mail.gmail.com>
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and
 can_skb_headroom_valid to skb.c
To:     Oliver Hartkopp <socketcan@hartkopp.net>
Cc:     Marc Kleine-Budde <mkl@pengutronix.de>, linux-can@vger.kernel.org,
        linux-kernel@vger.kernel.org, Max Staudt <max@enpas.org>,
        netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Oliver,

On Mon. 16 May 2022 at 04:17, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Vincent,
>
> On 14.05.22 16:16, Vincent Mailhol wrote:
> > The functions can_dropped_invalid_skb() and can_skb_headroom_valid()
> > grew a lot over the years to a point which it does not make much sense
> > to have them defined as static inline in header files. Move those two
> > functions to the .c counterpart of skb.h.
> >
> > can_skb_headroom_valid() only caller being can_dropped_invalid_skb(),
> > the declaration is removed from the header. Only
> > can_dropped_invalid_skb() gets its symbol exported.
>
> I can see your point but the need for can-dev was always given for
> hardware specific stuff like bitrates, TDC, transceivers, whatever.

I also see your point :)
Actually, I raised the exact same idea in a previous message:
https://lore.kernel.org/linux-can/CAMZ6RqLU-Wg0Cau3cM=QsU-t-7Lyzmo1nJ_VAA4Mbw3u0jnNtw@mail.gmail.com/

But you were not in CC and it seems that there is a lot of congestion
recently on the mailing list so I wouldn’t be surprised if you tell me
you did not receive it.

> As there would be more things in slcan (e.g. dev_alloc_skb() could be
> unified with alloc_can_skb())

And also the can_{put,get}_echo_skb() I guess.

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


Yours sincerely,
Vincent Mailhol