mbox series

[v5,0/7] can: refactoring of can-dev module and of Kbuild

Message ID 20220604163000.211077-1-mailhol.vincent@wanadoo.fr (mailing list archive)
Headers show
Series can: refactoring of can-dev module and of Kbuild | expand

Message

Vincent Mailhol June 4, 2022, 4:29 p.m. UTC
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, m_can, mcp251xfd and ti_hecc)

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

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

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          |  17 ++-
 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/m_can/Kconfig         |   1 +
 drivers/net/can/spi/mcp251xfd/Kconfig |   1 +
 include/linux/can/skb.h               |  59 +-------
 net/can/Kconfig                       |   5 +-
 11 files changed, 349 insertions(+), 282 deletions(-)
 create mode 100644 drivers/net/can/dev/calc_bittiming.c

Comments

Max Staudt June 5, 2022, 5:23 p.m. UTC | #1
Thanks Vincent for this cleanup!

Since I am upstreaming a driver that may (?) not fit the proposed
structure, one question below.


On Sun,  5 Jun 2022 01:29:53 +0900
Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:

> * 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, m_can, mcp251xfd and
> ti_hecc)


This seemingly splits drivers into "things that speak to hardware" and
"things that don't". Except... slcan really does speak to hardware. It
just so happens to not use any of BITTIMING or RX_OFFLOAD. However, my
can327 (formerly elmcan) driver, which is an ldisc just like slcan,
*does* use RX_OFFLOAD, so where to I put it? Next to flexcan, m_can,
mcp251xfd and ti_hecc?

Is it really just a split by features used in drivers, and no longer a
split by virtual/real?


Max
Marc Kleine-Budde June 5, 2022, 6:06 p.m. UTC | #2
On 05.06.2022 19:23:47, Max Staudt wrote:
> Thanks Vincent for this cleanup!
> 
> Since I am upstreaming a driver that may (?) not fit the proposed
> structure, one question below.
> 
> 
> On Sun,  5 Jun 2022 01:29:53 +0900
> Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> 
> > * 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, m_can, mcp251xfd and
> > ti_hecc)
> 
> This seemingly splits drivers into "things that speak to hardware" and
> "things that don't". Except... slcan really does speak to hardware. It
> just so happens to not use any of BITTIMING or RX_OFFLOAD. However, my
> can327 (formerly elmcan) driver, which is an ldisc just like slcan,
> *does* use RX_OFFLOAD, so where to I put it? Next to flexcan, m_can,
> mcp251xfd and ti_hecc?
> 
> Is it really just a split by features used in drivers, and no longer a
> split by virtual/real?

We can move RX_OFFLOAD out of the "if CAN_NETLINK". Who wants to create
an incremental patch against can-next/master?

Marc
Max Staudt June 5, 2022, 8:46 p.m. UTC | #3
On Sun, 5 Jun 2022 20:06:41 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 05.06.2022 19:23:47, Max Staudt wrote:
> > This seemingly splits drivers into "things that speak to hardware"
> > and "things that don't". Except... slcan really does speak to
> > hardware. It just so happens to not use any of BITTIMING or
> > RX_OFFLOAD. However, my can327 (formerly elmcan) driver, which is
> > an ldisc just like slcan, *does* use RX_OFFLOAD, so where to I put
> > it? Next to flexcan, m_can, mcp251xfd and ti_hecc?
> > 
> > Is it really just a split by features used in drivers, and no
> > longer a split by virtual/real?  
> 
> We can move RX_OFFLOAD out of the "if CAN_NETLINK". Who wants to
> create an incremental patch against can-next/master?

Yes, this may be useful in the future. But for now, I think I can
answer my question myself :)

My driver does support Netlink to set CAN link parameters. So I'll just
drop it into the CAN_NETLINK -> RX_OFFLOAD category in Kconfig, unless
anyone objects.


I just got confused because in my mind, I'm still comparing it to
slcan, whereas in reality, it's now functionally closer to all the other
hardware drivers. Netlink and all.

Apologies for the noise! 


Max
Vincent Mailhol June 6, 2022, 12:24 a.m. UTC | #4
On Mon. 6 Jun. 2022, at 05:46, Max Staudt <max@enpas.org> wrote:
>
> On Sun, 5 Jun 2022 20:06:41 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> > On 05.06.2022 19:23:47, Max Staudt wrote:
> > > This seemingly splits drivers into "things that speak to hardware"
> > > and "things that don't". Except... slcan really does speak to
> > > hardware.

slcan is just an oddity in this regard because all the netlink logic
is done in userspace using slcand. I think that it would really
benefit to be rewritten using the features under CAN_NETLINK.

This way, it could for example benefit from can_priv::bitrate_const to
manage the bitrates via iproute2 instead of relying on slcand c.f.:
https://elinux.org/Bringing_CAN_interface_up#SLCAN_based_Interfaces

Similarly, it doesn't seem that slcan loopbacks the TX frames which,
in some way, violates one of the core concepts of SocketCAN:

https://docs.kernel.org/networking/can.html#local-loopback-of-sent-frames

You did a great job by putting all the logic in your can327 driver
instead of requiring a userland tool and I think slcan merits to have
your can327 improvements backported to him.

> It just so happens to not use any of BITTIMING or
> > > RX_OFFLOAD. However, my can327 (formerly elmcan) driver, which is
> > > an ldisc just like slcan, *does* use RX_OFFLOAD, so where to I put
> > > it? Next to flexcan, m_can, mcp251xfd and ti_hecc?
> > >
> > > Is it really just a split by features used in drivers, and no
> > > longer a split by virtual/real?
> >
> > We can move RX_OFFLOAD out of the "if CAN_NETLINK". Who wants to
> > create an incremental patch against can-next/master?
>
> Yes, this may be useful in the future. But for now, I think I can
> answer my question myself :)

I was about to answer you, but you corrected the shot before I had
time to do so :)

> My driver does support Netlink to set CAN link parameters. So I'll just
> drop it into the CAN_NETLINK -> RX_OFFLOAD category in Kconfig, unless
> anyone objects.

This is the correct approach (and the only one). Try to maintain the
alphabetical order of the menu when you add it.

> I just got confused because in my mind, I'm still comparing it to
> slcan, whereas in reality, it's now functionally closer to all the other
> hardware drivers. Netlink and all.
>
> Apologies for the noise!

No problem!


Yours sincerely,
Vincent Mailhol
Oliver Hartkopp June 6, 2022, 7:24 p.m. UTC | #5
Hi Vincent,

great work!

On 04.06.22 18:29, Vincent Mailhol wrote:

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

Is this still true in patch series 5?

If I understood it correctly CONFIG_CAN_BITTIMING and 
CONFIG_CAN_RX_OFFLOAD can be enabled by the user and 
(alternatively/additionally) the selection of "flexcan, m_can, mcp251xfd 
and ti_hecc" enables CONFIG_CAN_RX_OFFLOAD too.

Right?

>                |
>                +-> CAN devices relying on rx offload
>                    (at time of writing: flexcan, m_can, mcp251xfd and ti_hecc)

Best regards,
Oliver
Vincent Mailhol June 7, 2022, 2:49 a.m. UTC | #6
On Tue. 7 Jun. 2022 at 04:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> Hi Vincent,
>
> great work!

Thanks!

> On 04.06.22 18:29, Vincent Mailhol wrote:
>
> > * 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
                           ^^^^^^^^^^^^^^^^^^^^
Typo: the symbol is CONFIG_CAN_*CALC*_BITTIMING. I made that typo
twice in the cover letter (once in each diagram). The patches and
their comments remain correct.

> >            |
> >            +-> All other CAN devices not relying on RX offload
> >            |
> >            +-> CAN rx offload
> >                symbol: CONFIG_CAN_RX_OFFLOAD
>
> Is this still true in patch series 5?
>
> If I understood it correctly CONFIG_CAN_BITTIMING and
> CONFIG_CAN_RX_OFFLOAD can be enabled by the user and
> (alternatively/additionally) the selection of "flexcan, m_can, mcp251xfd
> and ti_hecc" enables CONFIG_CAN_RX_OFFLOAD too.
>
> Right?

Yes, this is correct. Maybe what troubles you is the meaning of the
"x --> y" arrow in the graph. I said it denotes that "y depends on x".
Here "depends on" has a loose meaning. It translates to either:
  * Feature Y is encapsulated in Kbuild by some "if X/endif" and won't
show up unless X is selected.
  * Feature Y has a "selects X" tag and will forcibly enable X if selected.

CONFIG_CAN_*CALC*_BITTIMING is on the left side of an arrow starting
from CONFIG_CAN_NETLINK so it "depends" on CONFIG_CAN_NETLINK. On the
other hand, CONFIG_CAN_*CALC*_BITTIMING does not have any arrow
starting from it so indeed, it can be enabled by the user
independently of the other features as long as CONFIG_CAN_NETLINK is
selected.
CONFIG_CAN_RX_OFFLOAD is also on the left side of an arrow starting
from CONFIG_CAN_NETLINK. Furthermore, there is an arrow starting from
CONFIG_CAN_RX_OFFLOAD going to the "rx offload drivers". So those
drivers need CONFIG_CAN_RX_OFFLOAD (which is implemented using the
"selects CONFIG_CAN_RX_OFFLOAD"). However, CONFIG_CAN_RX_OFFLOAD can
be selected independently of the "rx offload drivers" as long as its
CONFIG_CAN_NETLINK dependency is met.

So I think that the diagram is correct. Maybe rephrasing the cover
letter as below would address your concerns?

| Below diagrams illustrate the changes made. The arrow symbol "X --> Y"
| denotes that "Y needs X". Most of the time, it is implemented using "if X"
| and "endif" predicates in X’s Kbuild to encapsulate Y so that Y
| does not show up unless X is selected. The exception is rx
| offload which is implemented by adding a "selects
| CONFIG_CAN_RX_OFFLOAD" tag in Y’s Kbuild.

> >                |
> >                +-> CAN devices relying on rx offload
> >                    (at time of writing: flexcan, m_can, mcp251xfd and ti_hecc)

Yours sincerely,
Vincent Mailhol
Marc Kleine-Budde June 7, 2022, 7:13 a.m. UTC | #7
On 07.06.2022 11:49:30, Vincent MAILHOL wrote:
[...]
> So I think that the diagram is correct. Maybe rephrasing the cover
> letter as below would address your concerns?

BTW: I got the OK from Jakub to send PR with merges.

If you think the cover letter needs rephrasing, send a new series and
I'm going to force push that over can-next/master. After that let's
consider can-next/master as fast-forward only.

regards,
Marc
Vincent Mailhol June 7, 2022, 8:49 a.m. UTC | #8
On Tue. 7 Jun. 2022 at 16:13, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 07.06.2022 11:49:30, Vincent MAILHOL wrote:
> [...]
> > So I think that the diagram is correct. Maybe rephrasing the cover
> > letter as below would address your concerns?
>
> BTW: I got the OK from Jakub to send PR with merges.
>
> If you think the cover letter needs rephrasing, send a new series and
> I'm going to force push that over can-next/master. After that let's
> consider can-next/master as fast-forward only.

I will first wait for Oliver’s feedback. Once we are aligned, I can do
the v6 and I really hope that would be the last one.
Oliver Hartkopp June 7, 2022, 8:12 p.m. UTC | #9
Hi Vincent,

On 07.06.22 04:49, Vincent MAILHOL wrote:
> On Tue. 7 Jun. 2022 at 04:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>

>>>             |
>>>             +-> All other CAN devices not relying on RX offload
>>>             |
>>>             +-> CAN rx offload
>>>                 symbol: CONFIG_CAN_RX_OFFLOAD
>>
>> Is this still true in patch series 5?
>>
>> If I understood it correctly CONFIG_CAN_BITTIMING and
>> CONFIG_CAN_RX_OFFLOAD can be enabled by the user and
>> (alternatively/additionally) the selection of "flexcan, m_can, mcp251xfd
>> and ti_hecc" enables CONFIG_CAN_RX_OFFLOAD too.
>>
>> Right?
> 
> Yes, this is correct. Maybe what troubles you is the meaning of the
> "x --> y" arrow in the graph. I said it denotes that "y depends on x".
> Here "depends on" has a loose meaning. It translates to either:
>    * Feature Y is encapsulated in Kbuild by some "if X/endif" and won't
> show up unless X is selected.
>    * Feature Y has a "selects X" tag and will forcibly enable X if selected.
> 
> CONFIG_CAN_*CALC*_BITTIMING is on the left side of an arrow starting
> from CONFIG_CAN_NETLINK so it "depends" on CONFIG_CAN_NETLINK. On the
> other hand, CONFIG_CAN_*CALC*_BITTIMING does not have any arrow
> starting from it so indeed, it can be enabled by the user
> independently of the other features as long as CONFIG_CAN_NETLINK is
> selected.

Ok.

> CONFIG_CAN_RX_OFFLOAD is also on the left side of an arrow starting
> from CONFIG_CAN_NETLINK. Furthermore, there is an arrow starting from
> CONFIG_CAN_RX_OFFLOAD going to the "rx offload drivers". So those
> drivers need CONFIG_CAN_RX_OFFLOAD (which is implemented using the
> "selects CONFIG_CAN_RX_OFFLOAD"). However, CONFIG_CAN_RX_OFFLOAD can
> be selected independently of the "rx offload drivers" as long as its
> CONFIG_CAN_NETLINK dependency is met.
> 
> So I think that the diagram is correct. Maybe rephrasing the cover
> letter as below would address your concerns?
I applied your series and played with the options and it works like 
charm - and as expected.

But the point remains that from your figure I would still assume that 
the M_CAN driver would only show up when CONFIG_CAN_RX_OFFLOAD was 
selected by the user.

But the current (good) implementation shows *all* drivers and selects 
CONFIG_CAN_RX_OFFLOAD when e.g. M_CAN is selected.

So what about:

   symbol: CONFIG_NETDEVICES
   |
   +-> CAN Device Drivers
       symbol: CONFIG_CAN_DEV
       |
       +-> software/virtual CAN device drivers
       |   (at time of writing: slcan, vcan, vxcan)
       |
       +-> hardware 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
           |
           +-> CAN rx offload (optional but selected by some drivers)
           |   symbol: CONFIG_CAN_RX_OFFLOAD
           |
           +-> CAN devices drivers
               (some may select CONFIG_CAN_RX_OFFLOAD)

(I also added 'hardware' to CAN device drivers with Netlink support) to 
have a distinction to 'software/virtual' CAN device drivers)

At least this would help me to understand the new configuration setup.

Best regards,
Oliver
Marc Kleine-Budde June 7, 2022, 8:27 p.m. UTC | #10
On 07.06.2022 22:12:46, Oliver Hartkopp wrote:
> So what about:
> 
>   symbol: CONFIG_NETDEVICES
>   |
>   +-> CAN Device Drivers
>       symbol: CONFIG_CAN_DEV
>       |
>       +-> software/virtual CAN device drivers
>       |   (at time of writing: slcan, vcan, vxcan)
>       |
>       +-> hardware 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
>           |
>           +-> CAN rx offload (optional but selected by some drivers)
>           |   symbol: CONFIG_CAN_RX_OFFLOAD
>           |
>           +-> CAN devices drivers
>               (some may select CONFIG_CAN_RX_OFFLOAD)
> 
> (I also added 'hardware' to CAN device drivers with Netlink support) to have
> a distinction to 'software/virtual' CAN device drivers)

The line between hardware and software/virtual devices ist blurry, the
new can327 driver uses netlink and the slcan is currently being
converted....

> At least this would help me to understand the new configuration setup.

Marc
Oliver Hartkopp June 7, 2022, 8:51 p.m. UTC | #11
On 07.06.22 22:27, Marc Kleine-Budde wrote:
> On 07.06.2022 22:12:46, Oliver Hartkopp wrote:
>> So what about:
>>
>>    symbol: CONFIG_NETDEVICES
>>    |
>>    +-> CAN Device Drivers
>>        symbol: CONFIG_CAN_DEV
>>        |
>>        +-> software/virtual CAN device drivers
>>        |   (at time of writing: slcan, vcan, vxcan)
>>        |
>>        +-> hardware 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
>>            |
>>            +-> CAN rx offload (optional but selected by some drivers)
>>            |   symbol: CONFIG_CAN_RX_OFFLOAD
>>            |
>>            +-> CAN devices drivers
>>                (some may select CONFIG_CAN_RX_OFFLOAD)
>>
>> (I also added 'hardware' to CAN device drivers with Netlink support) to have
>> a distinction to 'software/virtual' CAN device drivers)
> 
> The line between hardware and software/virtual devices ist blurry, the
> new can327 driver uses netlink and the slcan is currently being
> converted....

Right, which could mean that slcan and can327 should be located in the 
'usual' CAN device driver section and not in the sw/virtual device section.

The slcan and can327 need some kind of hardware - while vcan and vxcan 
don't.

Best regards,
Oliver
Vincent Mailhol June 7, 2022, 11:59 p.m. UTC | #12
On Wed. 8 Jun 2022 at 05:51, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 07.06.22 22:27, Marc Kleine-Budde wrote:
> > On 07.06.2022 22:12:46, Oliver Hartkopp wrote:
> >> So what about:
> >>
> >>    symbol: CONFIG_NETDEVICES
> >>    |
> >>    +-> CAN Device Drivers
> >>        symbol: CONFIG_CAN_DEV
> >>        |
> >>        +-> software/virtual CAN device drivers
> >>        |   (at time of writing: slcan, vcan, vxcan)
> >>        |
> >>        +-> hardware 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
> >>            |
> >>            +-> CAN rx offload (optional but selected by some drivers)
> >>            |   symbol: CONFIG_CAN_RX_OFFLOAD
> >>            |
> >>            +-> CAN devices drivers
> >>                (some may select CONFIG_CAN_RX_OFFLOAD)

OK, this does not follow the definition I set for the "x --> y" arrow,
but it is easy to read. I am OK with your suggestion. I will also
remove the definition of the "x --> y" arrow because your diagram is
self explanatory.

> >> (I also added 'hardware' to CAN device drivers with Netlink support) to have
> >> a distinction to 'software/virtual' CAN device drivers)

This line you modified is the verbatim copy of the title in
menuconfig. So you are suggesting adding "hardware" to the menuconfig
as well? It did not have this word in the title before this series.
I was hesitating on this. If we name the symbol CAN_NETLINK, then I do
not see the need to also add "hardware" in the title. If you look at
the help menu, you will see: "This is required by all platform and
hardware CAN drivers." Mentioning it in the help menu is enough for
me.

And because of the blur line between slcan (c.f. Marc's comment
below), I am not convinced to add this.

> > The line between hardware and software/virtual devices ist blurry, the
> > new can327 driver uses netlink and the slcan is currently being
> > converted....
>
> Right, which could mean that slcan and can327 should be located in the
> 'usual' CAN device driver section and not in the sw/virtual device section.

ACK, but as discussed with Marc, I will just focus on the series
itself and ignore (for the moment) that slcan will probably be moved
within CAN_NETLINK scope in the future.
https://lore.kernel.org/linux-can/20220607103923.5m6j4rykvitofsv4@pengutronix.de/

> The slcan and can327 need some kind of hardware - while vcan and vxcan
> don't.


Yours sincerely,
Vincent Mailhol
Oliver Hartkopp June 8, 2022, 8:10 p.m. UTC | #13
On 08.06.22 01:59, Vincent MAILHOL wrote:
> On Wed. 8 Jun 2022 at 05:51, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

>>>> (I also added 'hardware' to CAN device drivers with Netlink support) to have
>>>> a distinction to 'software/virtual' CAN device drivers)
> 
> This line you modified is the verbatim copy of the title in
> menuconfig. So you are suggesting adding "hardware" to the menuconfig
> as well? It did not have this word in the title before this series.
> I was hesitating on this. If we name the symbol CAN_NETLINK, then I do
> not see the need to also add "hardware" in the title. If you look at
> the help menu, you will see: "This is required by all platform and
> hardware CAN drivers." Mentioning it in the help menu is enough for
> me.
> 
> And because of the blur line between slcan (c.f. Marc's comment
> below), I am not convinced to add this.

Yes, discussing about this change I'm not convinced either ;-)

>>> The line between hardware and software/virtual devices ist blurry, the
>>> new can327 driver uses netlink and the slcan is currently being
>>> converted....
>>
>> Right, which could mean that slcan and can327 should be located in the
>> 'usual' CAN device driver section and not in the sw/virtual device section.
> 
> ACK, but as discussed with Marc, I will just focus on the series
> itself and ignore (for the moment) that slcan will probably be moved
> within CAN_NETLINK scope in the future.
> https://lore.kernel.org/linux-can/20220607103923.5m6j4rykvitofsv4@pengutronix.de/

Ok.

Sorry for the noise!

Best regards,
Oliver