| Message ID | 20221101225240.421525-6-jacob.e.keller@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ice: improve Tx timestamp corner cases | expand |
2> -----Original Message----- > From: Keller, Jacob E <jacob.e.keller@intel.com> > Sent: Tuesday, November 1, 2022 3:53 PM > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org> > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Ertman, David M > <david.m.ertman@intel.com> > Subject: [PATCH net-next 05/15] ice: always call ice_ptp_link_change and > make it void > > The ice_ptp_link_change function is currently only called for E822 based > hardware. Future changes are going to extend this function to perform > additional tasks on link change. > > Always call this function, moving the e810 check from the callers down to > just before we call the E822-specific function required to restart the PHY. > > This function also returns an error value, but none of the callers actually > check it. In general, the errors it produces are more likely systemic > problems such as invalid or corrupt port numbers. No caller checks these, > and so no warning is logged. > > Re-order the flag checks so that ICE_FLAG_PTP is checked first. Drop the > unnecessary check for ICE_FLAG_PTP_SUPPORTED, as ICE_FLAG_PTP will > not be > set except when ICE_FLAG_PTP_SUPPORTED is set. > > Convert the port checks to WARN_ON_ONCE, in order to generate a kernel > stack trace when they are hit. > > Convert the function to void since no caller actually checks these return > values. > > Co-authored-by: Dave Ertman <david.m.ertman@intel.com> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_main.c | 9 +++------ > drivers/net/ethernet/intel/ice/ice_ptp.c | 24 +++++++++++------------ > drivers/net/ethernet/intel/ice/ice_ptp.h | 7 ++++--- > 3 files changed, 19 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c > b/drivers/net/ethernet/intel/ice/ice_main.c > index 84fe01fbd83b..881dc9c29d63 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -1125,8 +1125,7 @@ ice_link_event(struct ice_pf *pf, struct > ice_port_info *pi, bool link_up, > if (link_up == old_link && link_speed == old_link_speed) > return 0; > > - if (!ice_is_e810(&pf->hw)) > - ice_ptp_link_change(pf, pf->hw.pf_id, link_up); > + ice_ptp_link_change(pf, pf->hw.pf_id, link_up); > > if (ice_is_dcb_active(pf)) { > if (test_bit(ICE_FLAG_DCB_ENA, pf->flags)) > @@ -6434,8 +6433,7 @@ static int ice_up_complete(struct ice_vsi *vsi) > ice_print_link_msg(vsi, true); > netif_tx_start_all_queues(vsi->netdev); > netif_carrier_on(vsi->netdev); > - if (!ice_is_e810(&pf->hw)) > - ice_ptp_link_change(pf, pf->hw.pf_id, true); > + ice_ptp_link_change(pf, pf->hw.pf_id, true); > } > > /* Perform an initial read of the statistics registers now to > @@ -6867,8 +6865,7 @@ int ice_down(struct ice_vsi *vsi) > > if (vsi->netdev && vsi->type == ICE_VSI_PF) { > vlan_err = ice_vsi_del_vlan_zero(vsi); > - if (!ice_is_e810(&vsi->back->hw)) > - ice_ptp_link_change(vsi->back, vsi->back->hw.pf_id, > false); > + ice_ptp_link_change(vsi->back, vsi->back->hw.pf_id, false); > netif_carrier_off(vsi->netdev); > netif_tx_disable(vsi->netdev); > } else if (vsi->type == ICE_VSI_SWITCHDEV_CTRL) { > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c > b/drivers/net/ethernet/intel/ice/ice_ptp.c > index 6a36256748fe..3b154cdb2a32 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c > @@ -1305,33 +1305,33 @@ ice_ptp_port_phy_restart(struct ice_ptp_port > *ptp_port) > } > > /** > - * ice_ptp_link_change - Set or clear port registers for timestamping > + * ice_ptp_link_change - Reconfigure PTP after link status change > * @pf: Board private structure > * @port: Port for which the PHY start is set > * @linkup: Link is up or down > */ > -int ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) > +void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) > { > struct ice_ptp_port *ptp_port; > > - if (!test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags)) > - return 0; > + if (!test_bit(ICE_FLAG_PTP, pf->flags)) > + return; > > - if (port >= ICE_NUM_EXTERNAL_PORTS) > - return -EINVAL; > + if (WARN_ON_ONCE(port >= ICE_NUM_EXTERNAL_PORTS)) > + return; > > ptp_port = &pf->ptp.port; > - if (ptp_port->port_num != port) > - return -EINVAL; > + if (WARN_ON_ONCE(ptp_port->port_num != port)) > + return; > > /* Update cached link status for this port immediately */ > ptp_port->link_up = linkup; > > - if (!test_bit(ICE_FLAG_PTP, pf->flags)) > - /* PTP is not setup */ > - return -EAGAIN; > + /* E810 devices do not need to reconfigure the PHY */ > + if (ice_is_e810(&pf->hw)) > + return; > > - return ice_ptp_port_phy_restart(ptp_port); > + ice_ptp_port_phy_restart(ptp_port); > } > > /** > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h > b/drivers/net/ethernet/intel/ice/ice_ptp.h > index 5250ff29a574..28b1dc901819 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ptp.h > +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h > @@ -258,7 +258,7 @@ void ice_ptp_reset(struct ice_pf *pf); > void ice_ptp_prepare_for_reset(struct ice_pf *pf); > void ice_ptp_init(struct ice_pf *pf); > void ice_ptp_release(struct ice_pf *pf); > -int ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup); > +void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup); > #else /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */ > static inline int ice_ptp_set_ts_config(struct ice_pf *pf, struct ifreq *ifr) > { > @@ -293,7 +293,8 @@ static inline void ice_ptp_reset(struct ice_pf *pf) { } > static inline void ice_ptp_prepare_for_reset(struct ice_pf *pf) { } > static inline void ice_ptp_init(struct ice_pf *pf) { } > static inline void ice_ptp_release(struct ice_pf *pf) { } > -static inline int ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) > -{ return 0; } > +static inline void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) > +{ > +} > #endif /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */ > #endif /* _ICE_PTP_H_ */ > -- > 2.38.0.83.gd420dda05763 Ack
On 11/1/2022 3:52 PM, Jacob Keller wrote: > The ice_ptp_link_change function is currently only called for E822 based > hardware. Future changes are going to extend this function to perform > additional tasks on link change. > > Always call this function, moving the e810 check from the callers down to > just before we call the E822-specific function required to restart the PHY. > > This function also returns an error value, but none of the callers actually > check it. In general, the errors it produces are more likely systemic > problems such as invalid or corrupt port numbers. No caller checks these, > and so no warning is logged. > > Re-order the flag checks so that ICE_FLAG_PTP is checked first. Drop the > unnecessary check for ICE_FLAG_PTP_SUPPORTED, as ICE_FLAG_PTP will not be > set except when ICE_FLAG_PTP_SUPPORTED is set. > > Convert the port checks to WARN_ON_ONCE, in order to generate a kernel > stack trace when they are hit. > > Convert the function to void since no caller actually checks these return > values. > > Co-authored-by: Dave Ertman <david.m.ertman@intel.com> Checkpatch is reporting this: WARNING: Non-standard signature: Co-authored-by: I see this used in the git log, however, seems like a Co-developed-by & Signed-off-by would communicate the same and make checkpatch happy. > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
On 11/3/2022 1:44 PM, Tony Nguyen wrote: > On 11/1/2022 3:52 PM, Jacob Keller wrote: >> The ice_ptp_link_change function is currently only called for E822 based >> hardware. Future changes are going to extend this function to perform >> additional tasks on link change. >> >> Always call this function, moving the e810 check from the callers down to >> just before we call the E822-specific function required to restart the >> PHY. >> >> This function also returns an error value, but none of the callers >> actually >> check it. In general, the errors it produces are more likely systemic >> problems such as invalid or corrupt port numbers. No caller checks these, >> and so no warning is logged. >> >> Re-order the flag checks so that ICE_FLAG_PTP is checked first. Drop the >> unnecessary check for ICE_FLAG_PTP_SUPPORTED, as ICE_FLAG_PTP will not be >> set except when ICE_FLAG_PTP_SUPPORTED is set. >> >> Convert the port checks to WARN_ON_ONCE, in order to generate a kernel >> stack trace when they are hit. >> >> Convert the function to void since no caller actually checks these return >> values. >> >> Co-authored-by: Dave Ertman <david.m.ertman@intel.com> > > Checkpatch is reporting this: > WARNING: Non-standard signature: Co-authored-by: > > I see this used in the git log, however, seems like a Co-developed-by & > Signed-off-by would communicate the same and make checkpatch happy. Will fix to use those tags, thanks. > >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 84fe01fbd83b..881dc9c29d63 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -1125,8 +1125,7 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up, if (link_up == old_link && link_speed == old_link_speed) return 0; - if (!ice_is_e810(&pf->hw)) - ice_ptp_link_change(pf, pf->hw.pf_id, link_up); + ice_ptp_link_change(pf, pf->hw.pf_id, link_up); if (ice_is_dcb_active(pf)) { if (test_bit(ICE_FLAG_DCB_ENA, pf->flags)) @@ -6434,8 +6433,7 @@ static int ice_up_complete(struct ice_vsi *vsi) ice_print_link_msg(vsi, true); netif_tx_start_all_queues(vsi->netdev); netif_carrier_on(vsi->netdev); - if (!ice_is_e810(&pf->hw)) - ice_ptp_link_change(pf, pf->hw.pf_id, true); + ice_ptp_link_change(pf, pf->hw.pf_id, true); } /* Perform an initial read of the statistics registers now to @@ -6867,8 +6865,7 @@ int ice_down(struct ice_vsi *vsi) if (vsi->netdev && vsi->type == ICE_VSI_PF) { vlan_err = ice_vsi_del_vlan_zero(vsi); - if (!ice_is_e810(&vsi->back->hw)) - ice_ptp_link_change(vsi->back, vsi->back->hw.pf_id, false); + ice_ptp_link_change(vsi->back, vsi->back->hw.pf_id, false); netif_carrier_off(vsi->netdev); netif_tx_disable(vsi->netdev); } else if (vsi->type == ICE_VSI_SWITCHDEV_CTRL) { diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 6a36256748fe..3b154cdb2a32 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -1305,33 +1305,33 @@ ice_ptp_port_phy_restart(struct ice_ptp_port *ptp_port) } /** - * ice_ptp_link_change - Set or clear port registers for timestamping + * ice_ptp_link_change - Reconfigure PTP after link status change * @pf: Board private structure * @port: Port for which the PHY start is set * @linkup: Link is up or down */ -int ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) +void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) { struct ice_ptp_port *ptp_port; - if (!test_bit(ICE_FLAG_PTP_SUPPORTED, pf->flags)) - return 0; + if (!test_bit(ICE_FLAG_PTP, pf->flags)) + return; - if (port >= ICE_NUM_EXTERNAL_PORTS) - return -EINVAL; + if (WARN_ON_ONCE(port >= ICE_NUM_EXTERNAL_PORTS)) + return; ptp_port = &pf->ptp.port; - if (ptp_port->port_num != port) - return -EINVAL; + if (WARN_ON_ONCE(ptp_port->port_num != port)) + return; /* Update cached link status for this port immediately */ ptp_port->link_up = linkup; - if (!test_bit(ICE_FLAG_PTP, pf->flags)) - /* PTP is not setup */ - return -EAGAIN; + /* E810 devices do not need to reconfigure the PHY */ + if (ice_is_e810(&pf->hw)) + return; - return ice_ptp_port_phy_restart(ptp_port); + ice_ptp_port_phy_restart(ptp_port); } /** diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h index 5250ff29a574..28b1dc901819 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h @@ -258,7 +258,7 @@ void ice_ptp_reset(struct ice_pf *pf); void ice_ptp_prepare_for_reset(struct ice_pf *pf); void ice_ptp_init(struct ice_pf *pf); void ice_ptp_release(struct ice_pf *pf); -int ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup); +void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup); #else /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */ static inline int ice_ptp_set_ts_config(struct ice_pf *pf, struct ifreq *ifr) { @@ -293,7 +293,8 @@ static inline void ice_ptp_reset(struct ice_pf *pf) { } static inline void ice_ptp_prepare_for_reset(struct ice_pf *pf) { } static inline void ice_ptp_init(struct ice_pf *pf) { } static inline void ice_ptp_release(struct ice_pf *pf) { } -static inline int ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) -{ return 0; } +static inline void ice_ptp_link_change(struct ice_pf *pf, u8 port, bool linkup) +{ +} #endif /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */ #endif /* _ICE_PTP_H_ */
The ice_ptp_link_change function is currently only called for E822 based hardware. Future changes are going to extend this function to perform additional tasks on link change. Always call this function, moving the e810 check from the callers down to just before we call the E822-specific function required to restart the PHY. This function also returns an error value, but none of the callers actually check it. In general, the errors it produces are more likely systemic problems such as invalid or corrupt port numbers. No caller checks these, and so no warning is logged. Re-order the flag checks so that ICE_FLAG_PTP is checked first. Drop the unnecessary check for ICE_FLAG_PTP_SUPPORTED, as ICE_FLAG_PTP will not be set except when ICE_FLAG_PTP_SUPPORTED is set. Convert the port checks to WARN_ON_ONCE, in order to generate a kernel stack trace when they are hit. Convert the function to void since no caller actually checks these return values. Co-authored-by: Dave Ertman <david.m.ertman@intel.com> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> --- drivers/net/ethernet/intel/ice/ice_main.c | 9 +++------ drivers/net/ethernet/intel/ice/ice_ptp.c | 24 +++++++++++------------ drivers/net/ethernet/intel/ice/ice_ptp.h | 7 ++++--- 3 files changed, 19 insertions(+), 21 deletions(-)