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 <[email protected]>
---
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(-)
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 http://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
--
2.35.1
Hi Geert,
On Tue. 7 Jun 2022 at 17:43, Geert Uytterhoeven <[email protected]> wrote:
> Hi Vincent,
>
> On Sun, Jun 5, 2022 at 12:25 AM Vincent Mailhol
> <[email protected]> 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 <[email protected]>
>
> > --- 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 <[email protected]> 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
Hi Vincent,
On Sun, Jun 5, 2022 at 12:25 AM Vincent Mailhol
<[email protected]> 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 <[email protected]>
> --- 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 -- [email protected]
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
On Wed. 8 Jun 2022 à 07:06, Jakub Kicinski <[email protected]> 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/[email protected]/
But until the patch gets accepted, it is defacto an out of tree module.
Yours sincerely,
Vincent Mailhol
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 08:40:19 +0900 Vincent MAILHOL wrote:
> On Wed. 8 Jun 2022 à 07:06, Jakub Kicinski <[email protected]> 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/[email protected]/
>
> 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 <[email protected]> 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 <[email protected]> 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.