Re: [PATCH net-next] gve: Correctly report software timestamping capabilities

[Rahul Rameshbabu]

On Wed, 10 Apr, 2024 15:31:56 -0400 Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx> 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? 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;

>
> 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).

--
Thanks,

Rahul Rameshbabu