Re: [PATCH v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3930 bytes --]

On 05.04.2021 11:29:31, Vincent MAILHOL wrote:
> Hi Marc,
> 
> On Wed. 17 Mar 2021 at 00:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 17.03.2021 00:16:01, Vincent MAILHOL wrote:
> > > > I just had a look at the ethtool-netlink interface:
> > > >
> > > > | Documentation/networking/ethtool-netlink.rst
> > > >
> > > > this is much better designed than the CAN netlink interface. It was done
> > > > by the pros and much later than CAN. :D So I'd like to have a similar
> > > > structure for new CAN netlink stuff.
> > > >
> > > > So I think I'll remove this patch for now from can-next-testing. The
> > > > kernel internal interface to tdc is still OK, we can leave it as is and
> > > > change it if needed. But netlink is user space and I'd like to have it
> > > > properly designed.
> > >
> > > Understood. However, I will need more time to read and understand
> > > the ethtool-netlink interface. The new patch will come later, I
> > > do not know when.
> >
> > No Problem
> 
> I started to look at Ethtool netlink, but as far as my understanding
> goes, this seems purely restricted to the ethtool application (i.e.
> not to iproute2). I double checked the latest versions of iproute2
> but there isn’t a single #include <linux/ethtool_netlink.h> nor
> anything else related to that new API.
> 
> Please let me know if I missed the point.

For example have a look at the RINGS_GET of ethtool-netlink.rst:

| RINGS_GET
| =========
| 
| Gets ring sizes like ``ETHTOOL_GRINGPARAM`` ioctl request.
| 
| Request contents:
| 
|   ====================================  ======  ==========================
|   ``ETHTOOL_A_RINGS_HEADER``            nested  request header
|   ====================================  ======  ==========================
| 
| Kernel response contents:
| 
|   ====================================  ======  ==========================
|   ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
|   ``ETHTOOL_A_RINGS_RX_MAX``            u32     max size of RX ring
|   ``ETHTOOL_A_RINGS_RX_MINI_MAX``       u32     max size of RX mini ring
|   ``ETHTOOL_A_RINGS_RX_JUMBO_MAX``      u32     max size of RX jumbo ring
|   ``ETHTOOL_A_RINGS_TX_MAX``            u32     max size of TX ring
|   ``ETHTOOL_A_RINGS_RX``                u32     size of RX ring
|   ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
|   ``ETHTOOL_A_RINGS_RX_JUMBO``          u32     size of RX jumbo ring
|   ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
|   ====================================  ======  ==========================

The response consists of a header and several nested values. This looks
to me to be the netlink way to serialize a C-struct. If I understand
your code correctly it was just the values without the nested header.

| RINGS_SET
| =========
| 
| Sets ring sizes like ``ETHTOOL_SRINGPARAM`` ioctl request.
| 
| Request contents:
| 
|   ====================================  ======  ==========================
|   ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
|   ``ETHTOOL_A_RINGS_RX``                u32     size of RX ring
|   ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
|   ``ETHTOOL_A_RINGS_RX_JUMBO``          u32     size of RX jumbo ring
|   ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
|   ====================================  ======  ==========================
| 
| Kernel checks that requested ring sizes do not exceed limits reported by
| driver. Driver may impose additional constraints and may not suspport all
| attributes.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]