Re: [PATCH ethtool,v2] ethtool: add support show/set-time-stamping

[Eric Dumazet <edumazet@google.com>]

On Mon, Sep 7, 2020 at 2:53 PM Michal Kubecek <mkubecek@suse.cz> wrote:
>
> On Thu, Sep 03, 2020 at 10:07:14AM -0400, Kevin(Yudong) Yang wrote:
> > Before this patch, ethtool has -T/--show-time-stamping that only
> > shows the device's time stamping capabilities but not the time
> > stamping policy that is used by the device.
> >
> > This patch adds support to set/get device time stamping policy at
> > the driver level by calling ioctl(SIOCSHWTSTAMP).
> >
> > Tested: ran following cmds on a Mellanox NIC with mlx4_en driver:
> > ./ethtool -T eth1
> > ...
> > Hardware Transmit Timestamp Modes:
> >         off                   (HWTSTAMP_TX_OFF)
> >         on                    (HWTSTAMP_TX_ON)
> > Hardware Receive Filter Modes:
> >         none                  (HWTSTAMP_FILTER_NONE)
> >         all                   (HWTSTAMP_FILTER_ALL)
> > Hardware Timestamping Policy:
> >         Rx filter 0, none                  (HWTSTAMP_FILTER_NONE)
> >         Tx type 0, off                   (HWTSTAMP_TX_OFF)
> >
> > ./ethtool --set-time-stamping eth1 rx 1; ./ethtool -T eth1;
> > ...
> > Hardware Timestamping Policy:
> >       Rx filter 1, all                   (HWTSTAMP_FILTER_ALL)
> >       Tx type 0, off                   (HWTSTAMP_TX_OFF)
> >
> > ./ethtool --set-time-stamping eth1 rx 1 tx 1; ./ethtool -T eth1;
> > rx unmodified, ignoring
> > ...
> > Hardware Timestamping Policy:
> >       Rx filter 1, all                   (HWTSTAMP_FILTER_ALL)
> >       Tx type 1, on                    (HWTSTAMP_TX_ON)
> >
> > ./ethtool --set-time-stamping eth1 rx 0; ./ethtool -T eth1;
> > ...
> > Hardware Timestamping Policy:
> >       Rx filter 0, none                  (HWTSTAMP_FILTER_NONE)
> >       Tx type 1, on                    (HWTSTAMP_TX_ON)
> >
> > ./ethtool --set-time-stamping eth1 tx 0; ./ethtool -T eth1
> > ...
> > Hardware Timestamping Policy:
> >       Rx filter 0, none                  (HWTSTAMP_FILTER_NONE)
> >       Tx type 0, off                   (HWTSTAMP_TX_OFF)
> >
> > ./ethtool --set-time-stamping eth1 rx 123 tx 456
> > rx should be in [0..15], tx should be in [0..2]
> >
> > Signed-off-by: Kevin Yang <yyd@google.com>
> > Reviewed-by: Neal Cardwell <ncardwell@google.com>
> > Reviewed-by: Eric Dumazet <edumazet@google.com>
> > ---
>
> As I said in response to v1 patch, I don't like the idea of adding a new
> ioctl interface to ethool when we are working on replacing and
> deprecating the existing ones. Is there a strong reason why this feature
> shouldn't be implemented using netlink?

I do not think this is a fair request.

All known kernels support the ioctl(), none of them support netlink so far.

Are you working on the netlink interface, or are you requesting us to
implement it ?

The ioctl has been added years ago, and Kevin patch is reasonable enough.

Thank you.