On Sun, 5 Jun 2022 20:06:41 +0200
Marc Kleine-Budde <[email protected]> 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 <[email protected]> wrote:
>
> On Sun, 5 Jun 2022 20:06:41 +0200
> Marc Kleine-Budde <[email protected]> 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