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 |
