| Message ID | 20220604163000.211077-5-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: refactoring of can-dev module and of Kbuild | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject, async |
Hi Vincent, On Sun, Jun 5, 2022 at 12:25 AM Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > Only a few drivers rely on the CAN rx offload framework (as of the > writing of this patch, only four: flexcan, m_can, mcp251xfd and > ti_hecc). Give the option to the user to deselect this features during > compilation. Thanks for your patch! > The drivers relying on CAN rx offload are in different sub > folders. All of these drivers get tagged with "select CAN_RX_OFFLOAD" > so that the option is automatically enabled whenever one of those > driver is chosen. Great! But... > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -102,6 +102,20 @@ config CAN_CALC_BITTIMING > > If unsure, say Y. > > +config CAN_RX_OFFLOAD > + bool "CAN RX offload" > + default y ... then why does this default to "y"? > + help > + Framework to offload the controller's RX FIFO during one > + interrupt. The CAN frames of the FIFO are read and put into a skb > + queue during that interrupt and transmitted afterwards in a NAPI > + context. > + > + The additional features selected by this option will be added to the > + can-dev module. > + > + If unsure, say Y. ... and do you suggest to enable this? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Tue. 7 Jun 2022 at 17:43, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Vincent, > > On Sun, Jun 5, 2022 at 12:25 AM Vincent Mailhol > <mailhol.vincent@wanadoo.fr> wrote: > > Only a few drivers rely on the CAN rx offload framework (as of the > > writing of this patch, only four: flexcan, m_can, mcp251xfd and > > ti_hecc). Give the option to the user to deselect this features during > > compilation. > > Thanks for your patch! Thank you too, happy to see the warm feedback from all of you. > > The drivers relying on CAN rx offload are in different sub > > folders. All of these drivers get tagged with "select CAN_RX_OFFLOAD" > > so that the option is automatically enabled whenever one of those > > driver is chosen. The "select CAN_RX_OFFLOAD" is to make it dummy proof for the user who will deselect CAN_RX_OFFLOAD can still see the menu entries for all drivers. I think it is better than a "depends on" which would hide the rx offload devices. > Great! But... > > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > > > --- a/drivers/net/can/Kconfig > > +++ b/drivers/net/can/Kconfig > > @@ -102,6 +102,20 @@ config CAN_CALC_BITTIMING > > > > If unsure, say Y. > > > > +config CAN_RX_OFFLOAD > > + bool "CAN RX offload" > > + default y > > ... then why does this default to "y"? > > > + help > > + Framework to offload the controller's RX FIFO during one > > + interrupt. The CAN frames of the FIFO are read and put into a skb > > + queue during that interrupt and transmitted afterwards in a NAPI > > + context. > > + > > + The additional features selected by this option will be added to the > > + can-dev module. > > + > > + If unsure, say Y. > > ... and do you suggest to enable this? Several reasons. First, *before* this series, the help menu for "Platform CAN drivers with Netlink support" (old CAN_DEV) had the "default y" and said: "if unsure, say Y." CAN_RX_OFFLOAD was part of it so, I am just maintaining the status quo. Second, and regardless of the above, I really think that it makes sense to have everything built in can-dev.ko by default. If someone does a binary release of can-dev.ko in which the rx offload is deactivated, end users would get really confused. Having a can-dev module stripped down is an expert setting. The average user which does not need CAN can deselect CONFIG_CAN and be happy. The average hobbyist who wants to do some CAN hacking will activate CONFIG_CAN and will automatically have the prerequisites in can-dev for any type of device drivers (after that just need to select the actual device drivers). The advanced user who actually read all the help menus will know that he should rather keep those to "yes" throughout the "if unsure, say Y" comment. Finally, the experts can fine tune their configuration by deselecting the pieces they did not wish for. Honestly, I am totally happy to have the "default y" tag, the "if unsure, say Y" comment and the "select CAN_RX_OFFLOAD" all together. Unless I am violating some kind of best practices, I prefer to keep it as-is. Hope this makes sense. Yours sincerely, Vincent Mailhol
On Tue, 7 Jun 2022 18:27:55 +0900 Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote: > Second, and regardless of the above, I really think that it makes > sense to have everything built in can-dev.ko by default. If someone > does a binary release of can-dev.ko in which the rx offload is > deactivated, end users would get really confused. > > Having a can-dev module stripped down is an expert setting. The > average user which does not need CAN can deselect CONFIG_CAN and be > happy. The average hobbyist who wants to do some CAN hacking will > activate CONFIG_CAN and will automatically have the prerequisites in > can-dev for any type of device drivers (after that just need to select > the actual device drivers). The advanced user who actually read all > the help menus will know that he should rather keep those to "yes" > throughout the "if unsure, say Y" comment. Finally, the experts can > fine tune their configuration by deselecting the pieces they did not > wish for. > > Honestly, I am totally happy to have the "default y" tag, the "if > unsure, say Y" comment and the "select CAN_RX_OFFLOAD" all together. > > Unless I am violating some kind of best practices, I prefer to keep it > as-is. Hope this makes sense. I wholeheartedly agree with Vincent's decision. One example case would be users of my can327 driver, as long as it is not upstream yet. They need to have RX_OFFLOAD built into their distribution's can_dev.ko, otherwise they will have no choice but to build their own kernel. Max
On Tue, 7 Jun 2022 18:22:16 +0200 Max Staudt wrote: > > Honestly, I am totally happy to have the "default y" tag, the "if > > unsure, say Y" comment and the "select CAN_RX_OFFLOAD" all together. > > > > Unless I am violating some kind of best practices, I prefer to keep it > > as-is. Hope this makes sense. AFAIU Linus likes for everything that results in code being added to the kernel to default to n. If the drivers hard-select that Kconfig why bother user with the question at all? My understanding is that Linus also likes to keep Kconfig as simple as possible. > I wholeheartedly agree with Vincent's decision. > > One example case would be users of my can327 driver, as long as it is > not upstream yet. They need to have RX_OFFLOAD built into their > distribution's can_dev.ko, otherwise they will have no choice but to > build their own kernel. Upstream mentioning out-of-tree modules may have the opposite effect to what you intend :( Forgive my ignorance, what's the reason to keep the driver out of tree?
On Wed. 8 Jun 2022 à 07:06, Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 7 Jun 2022 18:22:16 +0200 Max Staudt wrote: > > > Honestly, I am totally happy to have the "default y" tag, the "if > > > unsure, say Y" comment and the "select CAN_RX_OFFLOAD" all together. > > > > > > Unless I am violating some kind of best practices, I prefer to keep it > > > as-is. Hope this makes sense. > > AFAIU Linus likes for everything that results in code being added to > the kernel to default to n. A "make defconfig" would not select CONFIG_CAN (on which CAN_RX_OFFLOAD indirectly depends) and so by default this code is not added to the kernel. > If the drivers hard-select that Kconfig > why bother user with the question at all? My understanding is that > Linus also likes to keep Kconfig as simple as possible. I do not think that this is so convoluted. What would bother me is that RX offload is not a new feature. Before this series, RX offload is built-in the can-dev.o by default. If this new CAN_RX_OFFLOAD does not default to yes, then the default features built-in can-dev.o would change before and after this series. But you being one of the maintainers, if you insist I will go in your direction. So will removing the "default yes" and the comment "If unsure, say yes" from the CAN_RX_OFFLOAD satisfy you? > > I wholeheartedly agree with Vincent's decision. > > > > One example case would be users of my can327 driver, as long as it is > > not upstream yet. They need to have RX_OFFLOAD built into their > > distribution's can_dev.ko, otherwise they will have no choice but to > > build their own kernel. > > Upstream mentioning out-of-tree modules may have the opposite effect > to what you intend :( Forgive my ignorance, what's the reason to keep > the driver out of tree? I can answer for Max. The can327 patch is under review with the clear intent to have it upstream. c.f.: https://lore.kernel.org/linux-can/20220602213544.68273-1-max@enpas.org/ But until the patch gets accepted, it is defacto an out of tree module. Yours sincerely, Vincent Mailhol
On Tue, 7 Jun 2022 15:06:14 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 7 Jun 2022 18:22:16 +0200 Max Staudt wrote: > > > Honestly, I am totally happy to have the "default y" tag, the "if > > > unsure, say Y" comment and the "select CAN_RX_OFFLOAD" all > > > together. > > > > > > Unless I am violating some kind of best practices, I prefer to > > > keep it as-is. Hope this makes sense. > > AFAIU Linus likes for everything that results in code being added to > the kernel to default to n. If the drivers hard-select that Kconfig > why bother user with the question at all? My understanding is that > Linus also likes to keep Kconfig as simple as possible. > > > I wholeheartedly agree with Vincent's decision. > > > > One example case would be users of my can327 driver, as long as it > > is not upstream yet. They need to have RX_OFFLOAD built into their > > distribution's can_dev.ko, otherwise they will have no choice but to > > build their own kernel. > > Upstream mentioning out-of-tree modules may have the opposite effect > to what you intend :( Forgive my ignorance, what's the reason to keep > the driver out of tree? None, it's being upstreamed. But even with the best of intentions, it has been in this process for a long time, and it's still going on! For some reason, upstream tends to forget about everything that is not upstream *yet*. I've also convinced Greg K-H to include the N_DEVELOPMENT ldisc number for this very reason: To allow new drivers (ldiscs in this case) to be developed comfortably out-of-tree before they are upstreamed (and then assigned their own ldisc number). It seems strange to me to magically build some extra features into can_dev.ko, depending on whether some other .ko files are built in that very same moment, or not. By "magically", I mean an invisible Kconfig option. This is why I think Vincent's approach is best here, by making the drivers a clearly visible subset of the RX_OFFLOAD option in Kconfig, and RX_OFFLOAD user-selectable. How about making RX_OFFLOAD a separate .ko file, so we don't have various possible versions of can_dev.ko? @Vincent, I think you suggested that some time ago, IIRC? (I know, I was against a ton of little modules, but I'm changing my ways here now since it seems to help...) Max
On Wed, 8 Jun 2022 08:40:19 +0900 Vincent MAILHOL wrote: > On Wed. 8 Jun 2022 à 07:06, Jakub Kicinski <kuba@kernel.org> wrote: > > AFAIU Linus likes for everything that results in code being added to > > the kernel to default to n. > > A "make defconfig" would not select CONFIG_CAN (on which > CAN_RX_OFFLOAD indirectly depends) and so by default this code is not > added to the kernel. > > > If the drivers hard-select that Kconfig > > why bother user with the question at all? My understanding is that > > Linus also likes to keep Kconfig as simple as possible. > > I do not think that this is so convoluted. What would bother me is > that RX offload is not a new feature. Before this series, RX offload > is built-in the can-dev.o by default. If this new CAN_RX_OFFLOAD does > not default to yes, then the default features built-in can-dev.o would > change before and after this series. > > But you being one of the maintainers, if you insist I will go in your > direction. So will removing the "default yes" and the comment "If > unsure, say yes" from the CAN_RX_OFFLOAD satisfy you? I'm mostly trying to make sure Linus won't complain and block the entire net-next PR. Unfortunately I don't think the rules are written down anywhere. I could well be missing some CAN-specific context here but I see no practical benefit to exposing a knob for enabling driver framework and then selecting that framework in drivers as well. The only beneficiary I can think of is out-of-tree code. If the framework is optional (covers only parts of the driver's functionality) we make the knob configurable and drivers should work with or without it (e.g. PTP). If the framework is important / fundamental - hide it completely from the user / Kconfig and have the drivers 'select' it as a dependency (e.g. DEVLINK, PAGE_POOL). I'm not familiar with examples of the middle ground where we'd both expose the Kconfig, _and_ select in the drivers. Are there any? I don't want you to rage-quit over this tho, so we can merge as is and deal with the consequences. > > Upstream mentioning out-of-tree modules may have the opposite effect > > to what you intend :( Forgive my ignorance, what's the reason to keep > > the driver out of tree? > > I can answer for Max. The can327 patch is under review with the clear > intent to have it upstream. c.f.: > https://lore.kernel.org/linux-can/20220602213544.68273-1-max@enpas.org/ > > But until the patch gets accepted, it is defacto an out of tree module. Good to hear, then it will get upstream in due course, and the problem will disappear, no?
On Wed, 8 Jun 2022 01:43:54 +0200 Max Staudt wrote: > It seems strange to me to magically build some extra features into > can_dev.ko, depending on whether some other .ko files are built in that > very same moment, or not. By "magically", I mean an invisible Kconfig > option. This is why I think Vincent's approach is best here, by making > the drivers a clearly visible subset of the RX_OFFLOAD option in > Kconfig, and RX_OFFLOAD user-selectable. Sorry for a chunked response, vger becoming unresponsive the week after the merge window seems to become a tradition :/ We have a ton of "magical" / hidden Kconfigs in networking, take a look at net/Kconfig. Quick grep, likely not very accurate but FWIW: # not-hidden $ git grep -c -E '(bool|tristate)..' net/Kconfig net/Kconfig:23 # hidden $ git grep -c -E '(bool|tristate)$' net/Kconfig net/Kconfig:20 > How about making RX_OFFLOAD a separate .ko file, so we don't have > various possible versions of can_dev.ko? > > @Vincent, I think you suggested that some time ago, IIRC? > > (I know, I was against a ton of little modules, but I'm changing my > ways here now since it seems to help...) A separate module wouldn't help with my objections, I don't think.
On Tue, 7 Jun 2022 17:14:55 -0700 Jakub Kicinski <kuba@kernel.org> wrote: > We have a ton of "magical" / hidden Kconfigs in networking, take a > look at net/Kconfig. Quick grep, likely not very accurate but FWIW: Fair enough. Thinking about it, I've grepped my distro's kernel config for features more than just a handful of times... > > How about making RX_OFFLOAD a separate .ko file, so we don't have > > various possible versions of can_dev.ko? > > > > @Vincent, I think you suggested that some time ago, IIRC? > > > > (I know, I was against a ton of little modules, but I'm changing my > > ways here now since it seems to help...) > > A separate module wouldn't help with my objections, I don't think. In a system where the CAN stack is compiled as modules (i.e. a regular desktop distribution), the feature's presence/absence would be easily visible via the .ko file's presence/absence. Then again, I have to agree, distributing a system where RX_OFFLOAD is present, but no drivers using it whatsoever, seems... strange. I guess I got lost in my thinking there, with my out of tree development and all. Sorry for the noise. Max
On Wed. 8 juin 2022 at 09:14, Jakub Kicinski <kuba@kernel.org> wrote: > On Wed, 8 Jun 2022 01:43:54 +0200 Max Staudt wrote: > > It seems strange to me to magically build some extra features into > > can_dev.ko, depending on whether some other .ko files are built in that > > very same moment, or not. By "magically", I mean an invisible Kconfig > > option. This is why I think Vincent's approach is best here, by making > > the drivers a clearly visible subset of the RX_OFFLOAD option in > > Kconfig, and RX_OFFLOAD user-selectable. > > Sorry for a chunked response, vger becoming unresponsive the week after > the merge window seems to become a tradition :/ > > We have a ton of "magical" / hidden Kconfigs in networking, take a look > at net/Kconfig. Quick grep, likely not very accurate but FWIW: > > # not-hidden > $ git grep -c -E '(bool|tristate)..' net/Kconfig > net/Kconfig:23 > > # hidden > $ git grep -c -E '(bool|tristate)$' net/Kconfig > net/Kconfig:20 OK. So we have a proposal to make CAN_RX_OFFLOAD an hidden configuration. I did not consider this approach before because the CAN subsystem *never* relies on this and I did not really explore other Kbuild files. | $ git grep -c -E '(bool|tristate)$' net/can/Kconfig | <no output> Before pushing my driver upstream, it was also an out of tree module for about one year and I relate a lot to what Max said. But Jakub explanations are consistent and reflect the best practices of the kernel development. Also, mainstream distribution would do an allyesconfig and ship the can-dev.ko with everything built in. So the lambda user would still have everything built-in. I will let people continue to comment for a couple of days before making the final choice and sending the next version. But so far, I am leading toward Jakub’s idea to make it a hidden feature. > > How about making RX_OFFLOAD a separate .ko file, so we don't have > > various possible versions of can_dev.ko? > > > > @Vincent, I think you suggested that some time ago, IIRC? > > > > (I know, I was against a ton of little modules, but I'm changing my > > ways here now since it seems to help...) > > A separate module wouldn't help with my objections, I don't think.
diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index 87470feae6b1..91e4af727d1f 100644 --- a/drivers/net/can/Kconfig +++ b/drivers/net/can/Kconfig @@ -102,6 +102,20 @@ config CAN_CALC_BITTIMING If unsure, say Y. +config CAN_RX_OFFLOAD + bool "CAN RX offload" + default y + help + Framework to offload the controller's RX FIFO during one + interrupt. The CAN frames of the FIFO are read and put into a skb + queue during that interrupt and transmitted afterwards in a NAPI + context. + + The additional features selected by this option will be added to the + can-dev module. + + If unsure, say Y. + config CAN_AT91 tristate "Atmel AT91 onchip CAN controller" depends on (ARCH_AT91 || COMPILE_TEST) && HAS_IOMEM @@ -113,6 +127,7 @@ config CAN_FLEXCAN tristate "Support for Freescale FLEXCAN based chips" depends on OF || COLDFIRE || COMPILE_TEST depends on HAS_IOMEM + select CAN_RX_OFFLOAD help Say Y here if you want to support for Freescale FlexCAN. @@ -162,6 +177,7 @@ config CAN_SUN4I config CAN_TI_HECC depends on ARM tristate "TI High End CAN Controller" + select CAN_RX_OFFLOAD help Driver for TI HECC (High End CAN Controller) module found on many TI devices. The device specifications are available from www.ti.com diff --git a/drivers/net/can/dev/Makefile b/drivers/net/can/dev/Makefile index 791e6b297ea3..633687d6b6c0 100644 --- a/drivers/net/can/dev/Makefile +++ b/drivers/net/can/dev/Makefile @@ -9,4 +9,4 @@ can-dev-$(CONFIG_CAN_NETLINK) += bittiming.o can-dev-$(CONFIG_CAN_NETLINK) += dev.o can-dev-$(CONFIG_CAN_NETLINK) += length.o can-dev-$(CONFIG_CAN_NETLINK) += netlink.o -can-dev-$(CONFIG_CAN_NETLINK) += rx-offload.o +can-dev-$(CONFIG_CAN_RX_OFFLOAD) += rx-offload.o diff --git a/drivers/net/can/m_can/Kconfig b/drivers/net/can/m_can/Kconfig index 45ad1b3f0cd0..fc2afab36279 100644 --- a/drivers/net/can/m_can/Kconfig +++ b/drivers/net/can/m_can/Kconfig @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only menuconfig CAN_M_CAN tristate "Bosch M_CAN support" + select CAN_RX_OFFLOAD help Say Y here if you want support for Bosch M_CAN controller framework. This is common support for devices that embed the Bosch M_CAN IP. diff --git a/drivers/net/can/spi/mcp251xfd/Kconfig b/drivers/net/can/spi/mcp251xfd/Kconfig index dd0fc0a54be1..877e4356010d 100644 --- a/drivers/net/can/spi/mcp251xfd/Kconfig +++ b/drivers/net/can/spi/mcp251xfd/Kconfig @@ -2,6 +2,7 @@ config CAN_MCP251XFD tristate "Microchip MCP251xFD SPI CAN controllers" + select CAN_RX_OFFLOAD select REGMAP select WANT_DEV_COREDUMP help
Only a few drivers rely on the CAN rx offload framework (as of the writing of this patch, only four: flexcan, m_can, mcp251xfd and ti_hecc). Give the option to the user to deselect this features during compilation. The drivers relying on CAN rx offload are in different sub folders. All of these drivers get tagged with "select CAN_RX_OFFLOAD" so that the option is automatically enabled whenever one of those driver is chosen. Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- drivers/net/can/Kconfig | 16 ++++++++++++++++ drivers/net/can/dev/Makefile | 2 +- drivers/net/can/m_can/Kconfig | 1 + drivers/net/can/spi/mcp251xfd/Kconfig | 1 + 4 files changed, 19 insertions(+), 1 deletion(-)