| Message ID | 20221207210937.1099650-11-anthony.l.nguyen@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Intel Wired LAN Driver Updates 2022-12-07 (ice) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 43 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 07 Dec 13:09, Tony Nguyen wrote: >From: Jacob Keller <jacob.e.keller@intel.com> > >Introduce a new link_down field for the Tx tracker which indicates whether >the link is down for a given port. > >Use this bit to prevent any Tx timestamp requests from starting while link Can tx timestamp requests be generated in a context other than the xmit ndo ? how ? why not just use the same sync mechanisms we have for tx queues, carrier_down etc ..? Anyway I'm just curious.. >is down. This ensures that we do not try to start new timestamp requests >until after link has been restored. > >Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel) >Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> >--- > drivers/net/ethernet/intel/ice/ice_ptp.c | 11 ++++++++++- > drivers/net/ethernet/intel/ice/ice_ptp.h | 2 ++ > 2 files changed, 12 insertions(+), 1 deletion(-) > >diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c >index 481492d84e0e..dffcd50bac3f 100644 >--- a/drivers/net/ethernet/intel/ice/ice_ptp.c >+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c >@@ -613,7 +613,7 @@ ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx) > { > lockdep_assert_held(&tx->lock); > >- return tx->init && !tx->calibrating; >+ return tx->init && !tx->calibrating && !tx->link_down; > } > > /** >@@ -806,6 +806,8 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx) > } > > tx->init = 1; >+ tx->link_down = 0; >+ tx->calibrating = 0; > > spin_lock_init(&tx->lock); > >@@ -1396,6 +1398,13 @@ void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) > /* Update cached link status for this port immediately */ > ptp_port->link_up = linkup; > >+ /* Set the link status of the Tx tracker. While link is down, all Tx >+ * timestamp requests will be ignored. >+ */ >+ spin_lock(&ptp_port->tx.lock); >+ ptp_port->tx.link_down = !linkup; >+ spin_unlock(&ptp_port->tx.lock); >+ > /* E810 devices do not need to reconfigure the PHY */ > if (ice_is_e810(&pf->hw)) > return; >diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h >index 0bfafaaab6c7..75dcab8e1124 100644 >--- a/drivers/net/ethernet/intel/ice/ice_ptp.h >+++ b/drivers/net/ethernet/intel/ice/ice_ptp.h >@@ -119,6 +119,7 @@ struct ice_tx_tstamp { > * @init: if true, the tracker is initialized; > * @calibrating: if true, the PHY is calibrating the Tx offset. During this > * window, timestamps are temporarily disabled. >+ * @link_down: if true, the link is down and timestamp requests are disabled > * @verify_cached: if true, verify new timestamp differs from last read value > */ > struct ice_ptp_tx { >@@ -130,6 +131,7 @@ struct ice_ptp_tx { > u8 len; > u8 init : 1; > u8 calibrating : 1; >+ u8 link_down : 1; > u8 verify_cached : 1; > }; > >-- >2.35.1 >
On 12/7/2022 3:46 PM, Saeed Mahameed wrote: > On 07 Dec 13:09, Tony Nguyen wrote: >> From: Jacob Keller <jacob.e.keller@intel.com> >> >> Introduce a new link_down field for the Tx tracker which indicates >> whether >> the link is down for a given port. >> >> Use this bit to prevent any Tx timestamp requests from starting while >> link > Can tx timestamp requests be generated in a context other than the xmit ndo > ? how ? > As far as I know, there is no other way. Only the xmit ndo routine. > why not just use the same sync mechanisms we have for tx queues, > carrier_down etc ..? > Anyway I'm just curious.. Hmm. That's a good point... I'm not sure what caused the case where we began a transmit request while the link was down... I'll have to check into that.
On 12/7/2022 3:46 PM, Saeed Mahameed wrote: > On 07 Dec 13:09, Tony Nguyen wrote: >> From: Jacob Keller <jacob.e.keller@intel.com> >> >> Introduce a new link_down field for the Tx tracker which indicates >> whether >> the link is down for a given port. >> >> Use this bit to prevent any Tx timestamp requests from starting while >> link > Can tx timestamp requests be generated in a context other than the xmit ndo > ? how ? > > why not just use the same sync mechanisms we have for tx queues, > carrier_down etc ..? I can't find a record of a specific issue this fixed, and I checked and can confirm that the netdev stack already checks netif_carrier_ok. Lets just drop this from the series. We'll have a new one with the bitmap_or change posted this afternoon and this change dropped. Thanks, Jake
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 481492d84e0e..dffcd50bac3f 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -613,7 +613,7 @@ ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx) { lockdep_assert_held(&tx->lock); - return tx->init && !tx->calibrating; + return tx->init && !tx->calibrating && !tx->link_down; } /** @@ -806,6 +806,8 @@ ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx) } tx->init = 1; + tx->link_down = 0; + tx->calibrating = 0; spin_lock_init(&tx->lock); @@ -1396,6 +1398,13 @@ void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) /* Update cached link status for this port immediately */ ptp_port->link_up = linkup; + /* Set the link status of the Tx tracker. While link is down, all Tx + * timestamp requests will be ignored. + */ + spin_lock(&ptp_port->tx.lock); + ptp_port->tx.link_down = !linkup; + spin_unlock(&ptp_port->tx.lock); + /* E810 devices do not need to reconfigure the PHY */ if (ice_is_e810(&pf->hw)) return; diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h index 0bfafaaab6c7..75dcab8e1124 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h @@ -119,6 +119,7 @@ struct ice_tx_tstamp { * @init: if true, the tracker is initialized; * @calibrating: if true, the PHY is calibrating the Tx offset. During this * window, timestamps are temporarily disabled. + * @link_down: if true, the link is down and timestamp requests are disabled * @verify_cached: if true, verify new timestamp differs from last read value */ struct ice_ptp_tx { @@ -130,6 +131,7 @@ struct ice_ptp_tx { u8 len; u8 init : 1; u8 calibrating : 1; + u8 link_down : 1; u8 verify_cached : 1; };