Re: [PATCH v4 0/7] can: refactoring of can-dev module and of Kbuild

[Vincent MAILHOL]

On Sat. 4 juin 2022 at 20:46, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> Hello Vincent,
>
> wow! This is a great series which addresses a lot of long outstanding
> issues. Great work!

Thanks.

> As this cover letter brings so much additional information I'll ask
> Jakub and David if they take pull request from me, which itself have
> merges. This cover letter would be part of my merge. If I get the OK,
> can you provide this series as a tag (ideally GPG signed) that I can
> pull?

Fine, but I need a bit of guidance here. To provide a tag, I need to
have my own git repository hosted online, right? Is GitHub OK or
should I create one on https://git.kernel.org/?

> regards,
> Marc
>
> On 03.06.2022 19:28:41, Vincent Mailhol wrote:
> > Aside of calc_bittiming.o which can be configured with
> > CAN_CALC_BITTIMING, all objects from drivers/net/can/dev/ get linked
> > unconditionally to can-dev.o even if not needed by the user.
> >
> > This series first goal it to split the can-dev modules so that the
> > user can decide which features get built in during
> > compilation. Additionally, the CAN Device Drivers menu is moved from
> > the "Networking support" category to the "Device Drivers" category
> > (where all drivers are supposed to be).
> >
> > Below diagrams illustrate the changes made.
> > The arrow symbol "x --> y" denotes that "y depends on x".
> >
> > * menu before this series *
> >
> > CAN bus subsystem support
> >   symbol: CONFIG_CAN
> >   |
> >   +-> CAN Device Drivers
> >       (no symbol)
> >       |
> >       +-> software/virtual CAN device drivers
> >       |   (at time of writing: slcan, vcan, vxcan)
> >       |
> >       +-> Platform CAN drivers with Netlink support
> >           symbol: CONFIG_CAN_DEV
> >         |
> >           +-> CAN bit-timing calculation  (optional for hardware drivers)
> >           |   symbol: CONFIG_CAN_BITTIMING
> >         |
> >         +-> All other CAN devices
> >
> > * menu after this series *
> >
> > Network device support
> >   symbol: CONFIG_NETDEVICES
> >   |
> >   +-> CAN Device Drivers
> >       symbol: CONFIG_CAN_DEV
> >       |
> >       +-> software/virtual CAN device drivers
> >       |   (at time of writing: slcan, vcan, vxcan)
> >       |
> >       +-> CAN device drivers with Netlink support
> >           symbol: CONFIG_CAN_NETLINK (matches previous CONFIG_CAN_DEV)
> >           |
> >           +-> CAN bit-timing calculation (optional for all drivers)
> >           |   symbol: CONFIG_CAN_BITTIMING
> >         |
> >         +-> All other CAN devices not relying on RX offload
> >           |
> >           +-> CAN rx offload
> >               symbol: CONFIG_CAN_RX_OFFLOAD
> >               |
> >               +-> CAN devices relying on rx offload
> >                   (at time of writing: flexcan, ti_hecc and mcp251xfd)
> >
> > Patches 1 to 5 of this series do above modification.
> >
> > The last two patches add a check toward CAN_CTRLMODE_LISTENONLY in
> > can_dropped_invalid_skb() to discard tx skb (such skb can potentially
> > reach the driver if injected via the packet socket). In more details,
> > patch 6 moves can_dropped_invalid_skb() from skb.h to skb.o and patch
> > 7 is the actual change.
> >
> > Those last two patches are actually connected to the first five ones:
> > because slcan and v(x)can requires can_dropped_invalid_skb(), it was
> > necessary to add those three devices to the scope of can-dev before
> > moving the function to skb.o.
> >
> >
> > ** N.B. **
> >
> > This design results from the lengthy discussion in [1].
> >
> > I did one change from Oliver's suggestions in [2]. The initial idea
> > was that the first two config symbols should be respectively
> > CAN_DEV_SW and CAN_DEV instead of CAN_DEV and CAN_NETLINK as proposed
> > in this series.
> >
> >   * First symbol is changed from CAN_DEV_SW to CAN_DEV. The rationale
> >     is that it is this entry that will trigger the build of can-dev.o
> >     and it makes more sense for me to name the symbol share the same
> >     name as the module. Furthermore, this allows to create a menuentry
> >     with an explicit name that will cover both the virtual and
> >     physical devices (naming the menuentry "CAN Device Software" would
> >     be inconsistent with the fact that physical devices would also be
> >     present in a sub menu). And not using menuentry complexifies the
> >     menu.
> >
> >   * Second symbol is renamed from CAN_DEV to CAN_NETLINK because
> >     CAN_DEV is now taken by the previous menuconfig and netlink is the
> >     predominant feature added at this level. I am opened to other
> >     naming suggestion (CAN_DEV_NETLINK, CAN_DEV_HW...?).
> >
> > [1] https://lore.kernel.org/linux-can/20220514141650.1109542-1-mailhol.vincent@xxxxxxxxxx/
> > [2] https://lore.kernel.org/linux-can/22590a57-c7c6-39c6-06d5-11c6e4e1534b@xxxxxxxxxxxx/
> >
> >
> > ** Changelog **
> >
> > v3 -> v4:
> >
> >   * Five additional patches added to split can-dev module and refactor
> >     Kbuild. c.f. below (lengthy) thread:
> >     https://lore.kernel.org/linux-can/20220514141650.1109542-1-mailhol.vincent@xxxxxxxxxx/
> >
> >
> > v2 -> v3:
> >
> >   * Apply can_dropped_invalid_skb() to slcan.
> >
> >   * Make vcan, vxcan and slcan dependent of CONFIG_CAN_DEV by
> >     modifying Kbuild.
> >
> >   * fix small typos.
> >
> > v1 -> v2:
> >
> >   * move can_dropped_invalid_skb() to skb.c instead of dev.h
> >
> >   * also move can_skb_headroom_valid() to skb.c
> >
> > Vincent Mailhol (7):
> >   can: Kbuild: rename config symbol CAN_DEV into CAN_NETLINK
> >   can: Kconfig: turn menu "CAN Device Drivers" into a menuconfig using
> >     CAN_DEV
> >   can: bittiming: move bittiming calculation functions to
> >     calc_bittiming.c
> >   can: Kconfig: add CONFIG_CAN_RX_OFFLOAD
> >   net: Kconfig: move the CAN device menu to the "Device Drivers" section
> >   can: skb: move can_dropped_invalid_skb() and can_skb_headroom_valid()
> >     to skb.c
> >   can: skb: drop tx skb if in listen only mode
> >
> >  drivers/net/Kconfig                   |   2 +
> >  drivers/net/can/Kconfig               |  66 +++++++--
> >  drivers/net/can/dev/Makefile          |  20 ++-
> >  drivers/net/can/dev/bittiming.c       | 197 -------------------------
> >  drivers/net/can/dev/calc_bittiming.c  | 202 ++++++++++++++++++++++++++
> >  drivers/net/can/dev/dev.c             |   9 +-
> >  drivers/net/can/dev/skb.c             |  72 +++++++++
> >  drivers/net/can/spi/mcp251xfd/Kconfig |   1 +
> >  include/linux/can/skb.h               |  59 +-------
> >  net/can/Kconfig                       |   5 +-
> >  10 files changed, 351 insertions(+), 282 deletions(-)
> >  create mode 100644 drivers/net/can/dev/calc_bittiming.c
> >
> > --
> > 2.35.1
> >
> >
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |