Re: [PATCH v5 4/7] can: Kconfig: add CONFIG_CAN_RX_OFFLOAD

[Vincent MAILHOL]

Hi Geert,

On Tue. 7 Jun 2022 at 17:43, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> Hi Vincent,
>
> On Sun, Jun 5, 2022 at 12:25 AM Vincent Mailhol
> <mailhol.vincent@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> > --- 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