Messages in this thread | | | Date | Wed, 10 Apr 2024 21:36:25 -0400 | From | Willem de Bruijn <> | Subject | Re: [PATCH net-next] gve: Correctly report software timestamping capabilities |
| |
Rahul Rameshbabu wrote: > > On Wed, 10 Apr, 2024 15:31:56 -0400 Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > 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 | > > If there is a desire to enforce all drivers need to implement > .get_is_info, would the following make sense?
The only reason to enforce this is if we want to enforce them to also implement tx software timestamping. Generally, these features are opt in.
> My biggest objection to > this idea was mainly my concern that the drivers would miss setting > info->so_timestamping with SOF_TIMESTAMPING_RX_SOFTWARE | > SOF_TIMESTAMPING_SOFTWARE, which I do not think should be a > responsibility of the driver author since this is happening in the core > stack. > > So maybe something like this (taking Willem's proposal for > __ethtool_get_ts_info and modifying it a bit)? > > int err = 0; > > ... > > info->phc_index = -1; > > if (phy_has_tsinfo(phydev)) > err = phy_ts_info(phydev, info); > else > err = ops->get_ts_info(dev, info); > > info->so_timestamping |= SOF_TIMESTAMPING_RX_SOFTWARE | > SOF_TIMESTAMPING_SOFTWARE; > > return err;
Yes, this is what I meant as well. (the code I showed was just copied verbatim from net-next as context, not a suggested change.)
> > > > 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. > > I think this view aligns with my opinion above (though good point about > timestamping reporting bits in general should be deduced based on the > timestamp generation bits set rather than needing to be set as well).
| |