gve has supported software timestamp generation since its inception,
but has not advertised that support via ethtool. This patch correctly
advertises that support.
Reviewed-by: Praveen Kaligineedi <[email protected]>
Reviewed-by: Harshitha Ramamurthy <[email protected]>
Signed-off-by: John Fraker <[email protected]>
---
drivers/net/ethernet/google/gve/gve_ethtool.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index 815dead..99f5aeb 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -4,6 +4,8 @@
* Copyright (C) 2015-2021 Google, Inc.
*/
+#include <linux/ethtool.h>
+#include <linux/net_tstamp.h>
#include <linux/rtnetlink.h>
#include "gve.h"
#include "gve_adminq.h"
@@ -763,6 +765,15 @@ static int gve_set_coalesce(struct net_device *netdev,
return 0;
}
+static int gve_get_ts_info(struct net_device *netdev, struct ethtool_ts_info *info)
+{
+ info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
+ SOF_TIMESTAMPING_TX_SOFTWARE |
+ SOF_TIMESTAMPING_SOFTWARE;
+
+ return 0;
+}
+
const struct ethtool_ops gve_ethtool_ops = {
.supported_coalesce_params = ETHTOOL_COALESCE_USECS,
.supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT,
@@ -784,5 +795,6 @@ const struct ethtool_ops gve_ethtool_ops = {
.set_tunable = gve_set_tunable,
.get_priv_flags = gve_get_priv_flags,
.set_priv_flags = gve_set_priv_flags,
- .get_link_ksettings = gve_get_link_ksettings
+ .get_link_ksettings = gve_get_link_ksettings,
+ .get_ts_info = gve_get_ts_info
};
--
2.44.0.478.gd926399ef9-goog
John Fraker wrote:
> gve has supported software timestamp generation since its inception,
> but has not advertised that support via ethtool. This patch correctly
> advertises that support.
>
> Reviewed-by: Praveen Kaligineedi <[email protected]>
> Reviewed-by: Harshitha Ramamurthy <[email protected]>
> Signed-off-by: John Fraker <[email protected]>
Reviewed-by: Willem de Bruijn <[email protected]>
>
> ---
> drivers/net/ethernet/google/gve/gve_ethtool.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> index 815dead..99f5aeb 100644
> --- a/drivers/net/ethernet/google/gve/gve_ethtool.c
> +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
> @@ -4,6 +4,8 @@
> * Copyright (C) 2015-2021 Google, Inc.
> */
>
> +#include <linux/ethtool.h>
> +#include <linux/net_tstamp.h>
> #include <linux/rtnetlink.h>
> #include "gve.h"
> #include "gve_adminq.h"
> @@ -763,6 +765,15 @@ static int gve_set_coalesce(struct net_device *netdev,
> return 0;
> }
>
> +static int gve_get_ts_info(struct net_device *netdev, struct ethtool_ts_info *info)
> +{
> + info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
> + SOF_TIMESTAMPING_TX_SOFTWARE |
> + SOF_TIMESTAMPING_SOFTWARE;
> +
> + return 0;
This device calls skb_tx_timestamp in its ndo_start_xmit: the
prerequisite for SOF_TIMESTAMPING_TX_SOFTWARE.
All devices support SOF_TIMESTAMPING_RX_SOFTWARE by virtue of
net_timestamp_check being called in the device independent code.
To ethtool timestamping maintainers: It's quite unnecessary to have
each device advertise SOF_TIMESTAMPING_RX_SOFTWARE |
SOF_TIMESTAMPING_SOFTWARE. In __ethtool_get_ts_info we could just
always add those flags to the result from the callees.
if (phy_has_tsinfo(phydev))
return phy_ts_info(phydev, info);
if (ops->get_ts_info)
return ops->get_ts_info(dev, info);
info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
SOF_TIMESTAMPING_SOFTWARE;
> +}
> +
> const struct ethtool_ops gve_ethtool_ops = {
> .supported_coalesce_params = ETHTOOL_COALESCE_USECS,
> .supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT,
> @@ -784,5 +795,6 @@ const struct ethtool_ops gve_ethtool_ops = {
> .set_tunable = gve_set_tunable,
> .get_priv_flags = gve_get_priv_flags,
> .set_priv_flags = gve_set_priv_flags,
> - .get_link_ksettings = gve_get_link_ksettings
> + .get_link_ksettings = gve_get_link_ksettings,
> + .get_ts_info = gve_get_ts_info
> };
> --
> 2.44.0.478.gd926399ef9-goog
>
On Mon, 8 Apr 2024 11:09:01 -0700 John Fraker wrote:
> gve has supported software timestamp generation since its inception,
> but has not advertised that support via ethtool. This patch correctly
> advertises that support.
>
> Reviewed-by: Praveen Kaligineedi <[email protected]>
> Reviewed-by: Harshitha Ramamurthy <[email protected]>
> Signed-off-by: John Fraker <[email protected]>
I think it should be a single line diff:
+ .get_ts_info = ethtool_op_get_ts_info,
right?
On Tue, 09 Apr 2024 10:29:55 -0400 Willem de Bruijn wrote:
> This device calls skb_tx_timestamp in its ndo_start_xmit: the
> prerequisite for SOF_TIMESTAMPING_TX_SOFTWARE.
>
> All devices support SOF_TIMESTAMPING_RX_SOFTWARE by virtue of
> net_timestamp_check being called in the device independent code.
>
> To ethtool timestamping maintainers: It's quite unnecessary to have
> each device advertise SOF_TIMESTAMPING_RX_SOFTWARE |
> SOF_TIMESTAMPING_SOFTWARE. In __ethtool_get_ts_info we could just
> always add those flags to the result from the callees.
>
> if (phy_has_tsinfo(phydev))
> return phy_ts_info(phydev, info);
> if (ops->get_ts_info)
> return ops->get_ts_info(dev, info);
>
> info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
> SOF_TIMESTAMPING_SOFTWARE;
My gut tells me we force drivers to set the ethtool op because
while at it they will probably also implement tx stamping.
Even more unhelpful point I'll risk making is that we could
add a test and make people who submit new drivers run it :)
On Tue, 09 Apr, 2024 17:28:38 -0700 Jakub Kicinski <[email protected]> wrote:
> On Tue, 09 Apr 2024 10:29:55 -0400 Willem de Bruijn wrote:
>> This device calls skb_tx_timestamp in its ndo_start_xmit: the
>> prerequisite for SOF_TIMESTAMPING_TX_SOFTWARE.
>>
>> All devices support SOF_TIMESTAMPING_RX_SOFTWARE by virtue of
>> net_timestamp_check being called in the device independent code.
>>
>> To ethtool timestamping maintainers: It's quite unnecessary to have
>> each device advertise SOF_TIMESTAMPING_RX_SOFTWARE |
>> SOF_TIMESTAMPING_SOFTWARE. In __ethtool_get_ts_info we could just
>> always add those flags to the result from the callees.
>>
>> if (phy_has_tsinfo(phydev))
>> return phy_ts_info(phydev, info);
>> if (ops->get_ts_info)
>> return ops->get_ts_info(dev, info);
>>
>> info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE |
>> SOF_TIMESTAMPING_SOFTWARE;
>
> 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?
>
> Even more unhelpful point I'll risk making is that we could
> add a test and make people who submit new drivers run it :)
--
Thanks,
Rahul Rameshbabu
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?
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.
Rahul Rameshbabu wrote:
>
> On Wed, 10 Apr, 2024 15:31:56 -0400 Willem de Bruijn <[email protected]> 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).
Jakub Kicinski wrote:
> On Mon, 8 Apr 2024 11:09:01 -0700 John Fraker wrote:
> > gve has supported software timestamp generation since its inception,
> > but has not advertised that support via ethtool. This patch correctly
> > advertises that support.
> >
> > Reviewed-by: Praveen Kaligineedi <[email protected]>
> > Reviewed-by: Harshitha Ramamurthy <[email protected]>
> > Signed-off-by: John Fraker <[email protected]>
>
> I think it should be a single line diff:
>
> + .get_ts_info = ethtool_op_get_ts_info,
>
> right?
If inserted above .get_link_ksettings that works. The current
ordering is not based on actual struct layout anyway.
Probably all statements should just end in a comma, including a
trailing comma. To avoid these two line changes on each subsequent
change.
The rest of the discussion in this thread is actually quite
unrelated to this patch. Didn't meant to sidetrack that.
On Wed, Apr 10, 2024 at 8:22 PM Willem de Bruijn
<[email protected]> wrote:
>
> Jakub Kicinski wrote:
> > On Mon, 8 Apr 2024 11:09:01 -0700 John Fraker wrote:
> > > gve has supported software timestamp generation since its inception,
> > > but has not advertised that support via ethtool. This patch correctly
> > > advertises that support.
> > >
> > > Reviewed-by: Praveen Kaligineedi <[email protected]>
> > > Reviewed-by: Harshitha Ramamurthy <[email protected]>
> > > Signed-off-by: John Fraker <[email protected]>
> >
> > I think it should be a single line diff:
> >
> > + .get_ts_info = ethtool_op_get_ts_info,
> >
> > right?
>
> If inserted above .get_link_ksettings that works. The current
> ordering is not based on actual struct layout anyway.
>
> Probably all statements should just end in a comma, including a
> trailing comma. To avoid these two line changes on each subsequent
> change.
Thanks all!
I'll send the one-line v2.
> The rest of the discussion in this thread is actually quite
> unrelated to this patch. Didn't meant to sidetrack that.