Re: [PATCH v6 0/7] can: refactoring of can-dev module and of Kbuild - Marc Kleine-Budde

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Cc: linux-can@vger.kernel.org, linux-kernel@vger.kernel.org,
	Max Staudt <max@enpas.org>,
	Oliver Hartkopp <socketcan@hartkopp.net>,
	netdev@vger.kernel.org, Geert Uytterhoeven <geert@linux-m68k.org>,
	Jakub Kicinski <kuba@kernel.org>
Subject: Re: [PATCH v6 0/7] can: refactoring of can-dev module and of Kbuild
Date: Sat, 11 Jun 2022 17:17:54 +0200	[thread overview]
Message-ID: <20220611151754.2agcczimjcgr25xl@pengutronix.de> (raw)
In-Reply-To: <20220610143009.323579-1-mailhol.vincent@wanadoo.fr>

[-- Attachment #1: Type: text/plain, Size: 4611 bytes --]

On 10.06.2022 23:30:02, 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
> only the needed 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).
> 
> 
> * 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_CALC_BITTIMING
>           |
>           +-> All other CAN devices drivers
> 
> * 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_CALC_BITTIMING
>           |
>           +-> All other CAN devices drivers
>               (some may select CONFIG_CAN_RX_OFFLOAD)
>               |
>               +-> CAN rx offload (automatically selected by some drivers)
>                   (hidden symbol: CONFIG_CAN_RX_OFFLOAD)
> 
> 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.
> 
> This design results from the lengthy discussion in [1].
> 
> [1] https://lore.kernel.org/linux-can/20220514141650.1109542-1-mailhol.vincent@wanadoo.fr/
> 
> 
> ** Changelog **
> 
> v5 -> v6:
> 
>   * fix typo in patch #1's title: Kbuild -> Kconfig.
> 
>   * make CONFIG_RX_CAN an hidden config symbol and modify the diagram
>     in the cover letter accordingly.
> 
>     @Oliver, with CONFIG_CAN_RX_OFFLOAD now being an hidden config,
>     that option fully depends on the drivers. So contrary to your
>     suggestion, I put CONFIG_CAN_RX_OFFLOAD below the device drivers
>     in the diagram.
> 
>   * fix typo in cover letter: CONFIG_CAN_BITTIMING -> CONFIG_CAN_CALC_BITTIMING.
> 
> v4 -> v5:
> 
>   * m_can is also requires RX offload. Add the "select CAN_RX_OFFLOAD"
>     to its Makefile.
> 
>   * Reorder the lines of drivers/net/can/dev/Makefile.
> 
>   * Remove duplicated rx-offload.o target in drivers/net/can/dev/Makefile
> 
>   * Remove the Nota Bene in the cover letter.
> 
> 
> 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@wanadoo.fr/
> 
> 
> 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

Applied to can-next/master....as a merge with the above message!
Congrats on this series and the first ever merge to the linux-can
branch!

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]