On Mon. 25 juil. 2022 at 23:31, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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, candump -H implicitly expects rx_filter to be HWTSTAMP_FILTER_ALL. Defaulting to HWTSTAMP_FILTER_NONE would break the current versions of candump. 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). Moving forward, should I keep mcp251xfd in this series or should I remove it and let you take care of it? Yours sincerely, Vincent Mailhol
