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