| Message ID | 20241003123933.2589036-2-vadfed@meta.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | eth: fbnic: add timestamping support | expand |
On 10/3/2024 5:39 AM, Vadim Fedorenko wrote: > Add software TX timestamping support. RX software timestamping is > implemented in the core and there is no need to provide special flag > in the driver anymore. > > Signed-off-by: Vadim Fedorenko <vadfed@meta.com> > --- > drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c | 11 +++++++++++ > drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 3 +++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c > index 5d980e178941..ffc773014e0f 100644 > --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c > @@ -6,6 +6,16 @@ > #include "fbnic_netdev.h" > #include "fbnic_tlv.h" > > +static int > +fbnic_get_ts_info(struct net_device *netdev, > + struct kernel_ethtool_ts_info *tsinfo) > +{ > + tsinfo->so_timestamping = > + SOF_TIMESTAMPING_TX_SOFTWARE; > + > + return 0; > +} > + You could use ethtool_op_get_ts_info(), but I imagine future patches will update this for hardware timestamping, so I don't think thats a big deal. I think you *do* still want to report SOF_TIMESTAMPING_RX_SOFTWARE and SOF_TIMESTAMPING_SOFTWARE to get the API correct... Perhaps that could be improved in the core stack though.... Or did that already get changed recently? You should also set phc_index to -1 until you have a PTP clock device. > static void > fbnic_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo) > { > @@ -66,6 +76,7 @@ fbnic_get_eth_mac_stats(struct net_device *netdev, > > static const struct ethtool_ops fbnic_ethtool_ops = { > .get_drvinfo = fbnic_get_drvinfo, > + .get_ts_info = fbnic_get_ts_info, > .get_eth_mac_stats = fbnic_get_eth_mac_stats, > }; > > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c > index 6a6d7e22f1a7..8337d49bad0b 100644 > --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c > @@ -205,6 +205,9 @@ fbnic_tx_map(struct fbnic_ring *ring, struct sk_buff *skb, __le64 *meta) > > ring->tail = tail; > > + /* Record SW timestamp */ > + skb_tx_timestamp(skb); > + > /* Verify there is room for another packet */ > fbnic_maybe_stop_tx(skb->dev, ring, FBNIC_MAX_SKB_DESC); >
On 10/4/2024 3:55 PM, Jacob Keller wrote: > I think you *do* still want to report SOF_TIMESTAMPING_RX_SOFTWARE and > SOF_TIMESTAMPING_SOFTWARE to get the API correct... Perhaps that could > be improved in the core stack though.... Or did that already get changed > recently? > Ah, this did change recently, but the helper ethtool_op_get_ts_info() was not updated to do the same. :D For other reviewers who weren't aware, this is true since 12d337339d9f ("ethtool: RX software timestamp for all")
On 04/10/2024 23:55, Jacob Keller wrote: > > > On 10/3/2024 5:39 AM, Vadim Fedorenko wrote: >> Add software TX timestamping support. RX software timestamping is >> implemented in the core and there is no need to provide special flag >> in the driver anymore. >> >> Signed-off-by: Vadim Fedorenko <vadfed@meta.com> >> --- >> drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c | 11 +++++++++++ >> drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 3 +++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c >> index 5d980e178941..ffc773014e0f 100644 >> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c >> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c >> @@ -6,6 +6,16 @@ >> #include "fbnic_netdev.h" >> #include "fbnic_tlv.h" >> >> +static int >> +fbnic_get_ts_info(struct net_device *netdev, >> + struct kernel_ethtool_ts_info *tsinfo) >> +{ >> + tsinfo->so_timestamping = >> + SOF_TIMESTAMPING_TX_SOFTWARE; >> + >> + return 0; >> +} >> + > > You could use ethtool_op_get_ts_info(), but I imagine future patches > will update this for hardware timestamping, so I don't think thats a big > deal. > > I think you *do* still want to report SOF_TIMESTAMPING_RX_SOFTWARE and > SOF_TIMESTAMPING_SOFTWARE to get the API correct... Perhaps that could > be improved in the core stack though.... Or did that already get changed > recently? Yeah, as you found in the next mail, software RX timestamping was moved to the core recently. > You should also set phc_index to -1 until you have a PTP clock device. That's definitely missing, thanks! I'll add it to the next version. > >> static void >> fbnic_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo) >> { >> @@ -66,6 +76,7 @@ fbnic_get_eth_mac_stats(struct net_device *netdev, >> >> static const struct ethtool_ops fbnic_ethtool_ops = { >> .get_drvinfo = fbnic_get_drvinfo, >> + .get_ts_info = fbnic_get_ts_info, >> .get_eth_mac_stats = fbnic_get_eth_mac_stats, >> }; >> >> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c >> index 6a6d7e22f1a7..8337d49bad0b 100644 >> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c >> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c >> @@ -205,6 +205,9 @@ fbnic_tx_map(struct fbnic_ring *ring, struct sk_buff *skb, __le64 *meta) >> >> ring->tail = tail; >> >> + /* Record SW timestamp */ >> + skb_tx_timestamp(skb); >> + >> /* Verify there is room for another packet */ >> fbnic_maybe_stop_tx(skb->dev, ring, FBNIC_MAX_SKB_DESC); >> >
On 10/7/2024 2:56 AM, Vadim Fedorenko wrote: > On 04/10/2024 23:55, Jacob Keller wrote: >> >> >> On 10/3/2024 5:39 AM, Vadim Fedorenko wrote: >>> Add software TX timestamping support. RX software timestamping is >>> implemented in the core and there is no need to provide special flag >>> in the driver anymore. >>> >>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com> >>> --- >>> drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c | 11 +++++++++++ >>> drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 3 +++ >>> 2 files changed, 14 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c >>> index 5d980e178941..ffc773014e0f 100644 >>> --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c >>> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c >>> @@ -6,6 +6,16 @@ >>> #include "fbnic_netdev.h" >>> #include "fbnic_tlv.h" >>> >>> +static int >>> +fbnic_get_ts_info(struct net_device *netdev, >>> + struct kernel_ethtool_ts_info *tsinfo) >>> +{ >>> + tsinfo->so_timestamping = >>> + SOF_TIMESTAMPING_TX_SOFTWARE; >>> + >>> + return 0; >>> +} >>> + >> >> You could use ethtool_op_get_ts_info(), but I imagine future patches >> will update this for hardware timestamping, so I don't think thats a big >> deal. >> >> I think you *do* still want to report SOF_TIMESTAMPING_RX_SOFTWARE and >> SOF_TIMESTAMPING_SOFTWARE to get the API correct... Perhaps that could >> be improved in the core stack though.... Or did that already get changed >> recently? > > Yeah, as you found in the next mail, software RX timestamping was moved > to the core recently. > >> You should also set phc_index to -1 until you have a PTP clock device. > > That's definitely missing, thanks! I'll add it to the next version. > Since I had missed this, I reviewed the patch that changed Rx timestamp to the core. It also initialized PHC index to -1 automatically so it shouldn't be necessary. Apologies for the misleading comment.
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c index 5d980e178941..ffc773014e0f 100644 --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c @@ -6,6 +6,16 @@ #include "fbnic_netdev.h" #include "fbnic_tlv.h" +static int +fbnic_get_ts_info(struct net_device *netdev, + struct kernel_ethtool_ts_info *tsinfo) +{ + tsinfo->so_timestamping = + SOF_TIMESTAMPING_TX_SOFTWARE; + + return 0; +} + static void fbnic_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo) { @@ -66,6 +76,7 @@ fbnic_get_eth_mac_stats(struct net_device *netdev, static const struct ethtool_ops fbnic_ethtool_ops = { .get_drvinfo = fbnic_get_drvinfo, + .get_ts_info = fbnic_get_ts_info, .get_eth_mac_stats = fbnic_get_eth_mac_stats, }; diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c index 6a6d7e22f1a7..8337d49bad0b 100644 --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c @@ -205,6 +205,9 @@ fbnic_tx_map(struct fbnic_ring *ring, struct sk_buff *skb, __le64 *meta) ring->tail = tail; + /* Record SW timestamp */ + skb_tx_timestamp(skb); + /* Verify there is room for another packet */ fbnic_maybe_stop_tx(skb->dev, ring, FBNIC_MAX_SKB_DESC);
Add software TX timestamping support. RX software timestamping is implemented in the core and there is no need to provide special flag in the driver anymore. Signed-off-by: Vadim Fedorenko <vadfed@meta.com> --- drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c | 11 +++++++++++ drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 3 +++ 2 files changed, 14 insertions(+)