On 25.07.2022 23:41:17, Vincent MAILHOL wrote:
> On Mon. 25 juil. 2022 at 23:31, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 25.07.2022 22:32:02, Vincent Mailhol wrote:
> > > Tools based on libpcap (such as tcpdump) expect the SIOCSHWTSTAMP
> > > ioctl call to be supported. This is also specified in the kernel doc
> > > [1]. The purpose of this ioctl is to toggle the hardware timestamps.
> > >
> > > Currently, CAN devices which support hardware timestamping have those
> > > always activated. can_eth_ioctl_hwts() is a dumb function that will
> > > always succeed when requested to set tx_type to HWTSTAMP_TX_ON or
> > > rx_filter to HWTSTAMP_FILTER_ALL.
> > >
> > > [1] Kernel doc: Timestamping, section 3.1 "Hardware Timestamping
> > > Implementation: Device Drivers"
> > > Link: https://docs.kernel.org/networking/timestamping.html#hardware-timestamping-implementation-device-drivers
> > >
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > ---
> > >  drivers/net/can/dev/dev.c | 29 +++++++++++++++++++++++++++++
> > >  include/linux/can/dev.h   |  1 +
> > >  2 files changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
> > > index 7ad56d31cec9..750dc7cae9d4 100644
> > > --- a/drivers/net/can/dev/dev.c
> > > +++ b/drivers/net/can/dev/dev.c
> > > @@ -322,6 +322,35 @@ int can_change_mtu(struct net_device *dev, int new_mtu)
> > >  }
> > >  EXPORT_SYMBOL_GPL(can_change_mtu);
> > >
> > > +/* generic implementation of netdev_ops::ndo_eth_ioctl for CAN devices
> > > + * supporting hardware RX timestamps
> > > + */
> > > +int can_eth_ioctl_hwts(struct net_device *netdev, struct ifreq *ifr, int cmd)
> > > +{
> > > +     struct hwtstamp_config hwts_cfg = { 0 };
> > > +
> > > +     switch (cmd) {
> > > +     case SIOCSHWTSTAMP: /* set */
> > > +             if (copy_from_user(&hwts_cfg, ifr->ifr_data, sizeof(hwts_cfg)))
> > > +                     return -EFAULT;
> > > +             if (hwts_cfg.tx_type == HWTSTAMP_TX_ON &&
> > > +                 hwts_cfg.rx_filter == HWTSTAMP_FILTER_ALL)
> > > +                     return 0;
> >
> > I have a WIP hwts patch series for the mcp251xfd. IIRC the driver is
> > allowed to add RX timestamps to more packages than requested without
> > failing, so the relevant code my WIP patches looks like this:
> >
> > +       switch (config.tx_type) {
> > +       case HWTSTAMP_TX_OFF:
> > +               break;
> > +       default:
> > +               return -ERANGE;
> > +       }
> > +
> > +       switch (config.rx_filter) {
> > +       case HWTSTAMP_FILTER_NONE:
> > +               clear_bit(MCP251XFD_FLAGS_TIMESTAMP_RX, priv->flags);
> > +               break;
> > +       default:
> > +               set_bit(MCP251XFD_FLAGS_TIMESTAMP_RX, priv->flags);
> > +               config.rx_filter = HWTSTAMP_FILTER_ALL;
> > +       }
> 
> What is the default value for rx_filter?

Currently switched off.

> Currently, candump -H
> implicitly expects rx_filter to be HWTSTAMP_FILTER_ALL. Defaulting to
> HWTSTAMP_FILTER_NONE would break the current versions of candump.

My series is still WIP, haven't thought that far. I was falling down the
rabbit hole due 2 mcp251xfd "specialties" with regards to reading the
time stamp counter register.

> so I
> was wondering if it would be better for CAN to start with hardware
> timestamps as active (in my series, I assume that HWTSTAMP_FILTER_NONE
> is not supported and thus avoid this problem).

There's hwstamp_ctl to configure time stamping, but that's part of
linuxptp.

> Moving forward, should I keep mcp251xfd in this series or should I
> remove it and let you take care of it?

Keep the patch, I'll adopt mine.

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 |