On 25.07.2022 23:41:17, Vincent MAILHOL wrote: > 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 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 |
Attachment:
signature.asc
Description: PGP signature
