Messages in this thread | | | Date | Wed, 10 Apr 2024 15:31:56 -0400 | From | Willem de Bruijn <> | Subject | Re: [PATCH net-next] gve: Correctly report software timestamping capabilities |
| |
Jakub Kicinski wrote: > On Tue, 09 Apr 2024 21:40:46 -0700 Rahul Rameshbabu wrote: > > > My gut tells me we force drivers to set the ethtool op because > > > while at it they will probably also implement tx stamping. > > > > I think the logic should be the other way (in terms of the > > relationship). A call to skb_tx_timestamp should throw a warning if the > > driver does not advertise its timestamping capabilities. This way, a > > naive netdev driver for some lightweight device does not need to worry > > about this. I agree that anyone implementing tx timestamping should have > > this operation defined. An skb does not contain any mechanism to > > reference the driver's ethtool callback. Maybe the right choice is to > > have a ts capability function registered for each netdev that can be > > used by the core stack and that powers the ethtool operation as well > > instead of the existing callback for ethtool? > > Adding a check which only need to runs once in the lifetime of > the driver to the fastpath may be a little awkward. Another option > would be a sufficiently intelligent grep, which would understand > which files constitute a driver. At which point grepping for > the ethtool op and skb_tx_timestamp would be trivial?
Many may not define the flags themselves, but defer this to ethtool_op_get_ts_info.
A not so much intelligent, but sufficiently ugly, grep indicates not a a massive amount of many missing entries among ethernet drivers. But this first attempt is definitely lossy.
$ for symbol in skb_tx_timestamp get_ts_info SOF_TIMESTAMPING_TX_SOFTWARE ethtool_op_get_ts_info "(SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info)"; do echo -n "$symbol: "; for i in `grep -nrIE "$symbol" drivers/net/ethernet/ | awk '{print $1}' | xargs dirname | uniq`; do echo $i; done | wc -l; done
skb_tx_timestamp: 69 get_ts_info: 66 SOF_TIMESTAMPING_TX_SOFTWARE: 33 ethtool_op_get_ts_info: 40 (SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info): 59
This does not add up, but that's because some drivers share prefixes, and some drivers have different paths where one open codes and the other calls ethtool_op_get_ts_info. Marvell is a good example of both:
$ grep -nrIE '(SOF_TIMESTAMPING_TX_SOFTWARE|ethtool_op_get_ts_info)' drivers/net/ethernet /marvell drivers/net/ethernet/marvell/pxa168_eth.c:1367: .get_ts_info = ethtool_op_get_ts_info, drivers/net/ethernet/marvell/mv643xx_eth.c:1756: .get_ts_info = ethtool_op_get_ts_info, drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:5266: info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE | drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:962: return ethtool_op_get_ts_info(netdev, info); drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c:964: info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
One more aside, no driver should have to advertise SOF_TIMESTAMPING_SOFTWARE or SOF_TIMESTAMPING_RAW_HARDWARE. Per Documentation/networking/timestamping.rst these are reporting flags, not recording flags. Devices only optionall record a timestamp.
| |