| Message ID | 20231026105955.282546-1-karol.kolacinski@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-next] ice: remove unnecessary discarding of timestamps | expand |
On 10/26/2023 3:59 AM, Karol Kolacinski wrote: > From: Jacob Keller <jacob.e.keller@intel.com> > > The ice driver currently discards any outstanding timestamps that are > happening very near to a .adjtime or .settime callback. This was > originally added by commit b1a582e64bf2 ("ice: introduce > ice_ptp_reset_cached_phctime function") and later extended by commit > d40fd6009332 ("ice: handle flushing stale Tx timestamps in > ice_ptp_tx_tstamp"). > > The original motivation for discarding timestamps was that extending an > old timestamp using the new cached value of PHC was a problem, as it > could produce incorrect results. The change did not describe what such > "incorrect results" were. > > There are no such incorrect results. Extending the 32 bit timestamp with > the new time value just means that the timestamp is reported in terms of > the newly updated and adjusted system clock. This won't produce > incorrect results or problematic timestamps to applications. Either the > timestamp will be extended with the value of the PHC just prior to the > time adjustment (if the timestamp completes prior to the adjust > callback), or it will be extended using the new PHC value after the > adjustment. In either case, the resulting extended timestamp value makes > sense. > > The timestamp extension logic is very similar to the logic found in > timecounter_cyc2time, the primary difference being that the ice hardware > maintains the full 64 bits of nanoseconds in the MAC rather than being > maintained purely by software as in the timecounter case. > > Indeed, I couldn't find an example of a driver using > timecounter_cyc2time which does discard timestamps that occur nearby > a time adjustment. The ice driver behavior of discarding such timestamps > just results in failure to deliver a Tx timestamp to userspace, > resulting in applications such as ptp4l to timeout and enter a fault > state. Reporting the extended timestamp based on the updated PHC value > isn't producing "garbage" results, and doesn't lead to incorrect > behavior. > > Remove the unnecessary stale timestamp logic. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > --- This doesn't compile when applied on top of dev-queue: > ../drivers/net/ethernet/intel/ice/ice_ptp.c: In function ‘ice_ptp_rebuild_owner’: > ../drivers/net/ethernet/intel/ice/ice_ptp.c:2509:2: error: implicit declaration of function ‘ice_ptp_flush_all_tx_tracker’; did you mean ‘ice_ptp_flush_tx_tracker’? [-Werror=implicit-function-declaration] > ice_ptp_flush_all_tx_tracker(pf); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ice_ptp_flush_tx_tracker Thanks, Jake
On 10/26/2023 3:59 AM, Karol Kolacinski wrote: > -/** > - * ice_ptp_flush_all_tx_tracker - Flush all timestamp trackers on this clock > - * @pf: Board private structure > - * > - * Called by the clock owner to flush all the Tx timestamp trackers associated > - * with the clock. > - */ > -static void > -ice_ptp_flush_all_tx_tracker(struct ice_pf *pf) > -{ > - struct ice_ptp_port *port; > - > - list_for_each_entry(port, &pf->ptp.ports_owner.ports, list_member) > - ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx); > -} > - I think the removal of ice_ptp_flush_all_tx_tracker is likely an accident possibly due to a merge conflict?
On 10/26/2023 3:59 AM, Karol Kolacinski wrote: > From: Jacob Keller <jacob.e.keller@intel.com> > > The ice driver currently discards any outstanding timestamps that are > happening very near to a .adjtime or .settime callback. This was > originally added by commit b1a582e64bf2 ("ice: introduce > ice_ptp_reset_cached_phctime function") and later extended by commit > d40fd6009332 ("ice: handle flushing stale Tx timestamps in > ice_ptp_tx_tstamp"). > > The original motivation for discarding timestamps was that extending an > old timestamp using the new cached value of PHC was a problem, as it > could produce incorrect results. The change did not describe what such > "incorrect results" were. > > There are no such incorrect results. Extending the 32 bit timestamp with > the new time value just means that the timestamp is reported in terms of > the newly updated and adjusted system clock. This won't produce > incorrect results or problematic timestamps to applications. Either the > timestamp will be extended with the value of the PHC just prior to the > time adjustment (if the timestamp completes prior to the adjust > callback), or it will be extended using the new PHC value after the > adjustment. In either case, the resulting extended timestamp value makes > sense. > > The timestamp extension logic is very similar to the logic found in > timecounter_cyc2time, the primary difference being that the ice hardware > maintains the full 64 bits of nanoseconds in the MAC rather than being > maintained purely by software as in the timecounter case. > > Indeed, I couldn't find an example of a driver using > timecounter_cyc2time which does discard timestamps that occur nearby > a time adjustment. The ice driver behavior of discarding such timestamps > just results in failure to deliver a Tx timestamp to userspace, > resulting in applications such as ptp4l to timeout and enter a fault > state. Reporting the extended timestamp based on the updated PHC value > isn't producing "garbage" results, and doesn't lead to incorrect > behavior. > > Remove the unnecessary stale timestamp logic. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > --- Karol, I have some other work related to this that I'd like to send together, so I am planning to re-spin this so that it passes compilation and include it in a series I'm working on now. Thanks, Jake
Hi Karol,
kernel test robot noticed the following build errors:
[auto build test ERROR on b57a67bab47efc24e7ea7bd626aa517ac76c4fc9]
url: https://github.com/intel-lab-lkp/linux/commits/Karol-Kolacinski/ice-remove-unnecessary-discarding-of-timestamps/20231026-192456
base: b57a67bab47efc24e7ea7bd626aa517ac76c4fc9
patch link: https://lore.kernel.org/r/20231026105955.282546-1-karol.kolacinski%40intel.com
patch subject: [Intel-wired-lan] [PATCH iwl-next] ice: remove unnecessary discarding of timestamps
config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20231031/202310310036.7Cgll3Lm-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231031/202310310036.7Cgll3Lm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310310036.7Cgll3Lm-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/net/ethernet/intel/ice/ice_ptp.c: In function 'ice_ptp_rebuild_owner':
>> drivers/net/ethernet/intel/ice/ice_ptp.c:2509:9: error: implicit declaration of function 'ice_ptp_flush_all_tx_tracker'; did you mean 'ice_ptp_flush_tx_tracker'? [-Werror=implicit-function-declaration]
2509 | ice_ptp_flush_all_tx_tracker(pf);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
| ice_ptp_flush_tx_tracker
cc1: some warnings being treated as errors
vim +2509 drivers/net/ethernet/intel/ice/ice_ptp.c
0c095c9988dd713 Karol Kolacinski 2023-10-25 2454
4809671015a1bd2 Karol Kolacinski 2021-12-20 2455 /**
9ea8140e2f947cb Jacob Keller 2023-10-25 2456 * ice_ptp_rebuild_owner - Initialize PTP clock owner after reset
4809671015a1bd2 Karol Kolacinski 2021-12-20 2457 * @pf: Board private structure
9ea8140e2f947cb Jacob Keller 2023-10-25 2458 *
9ea8140e2f947cb Jacob Keller 2023-10-25 2459 * Companion function for ice_ptp_rebuild() which handles tasks that only the
9ea8140e2f947cb Jacob Keller 2023-10-25 2460 * PTP clock owner instance should perform.
4809671015a1bd2 Karol Kolacinski 2021-12-20 2461 */
9ea8140e2f947cb Jacob Keller 2023-10-25 2462 static int ice_ptp_rebuild_owner(struct ice_pf *pf)
4809671015a1bd2 Karol Kolacinski 2021-12-20 2463 {
4809671015a1bd2 Karol Kolacinski 2021-12-20 2464 struct ice_ptp *ptp = &pf->ptp;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2465 struct ice_hw *hw = &pf->hw;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2466 struct timespec64 ts;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2467 u64 time_diff;
d695241a0a0ea02 Jacob Keller 2023-10-25 2468 int err;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2469
b2ee72565cd0ee2 Jacob Keller 2021-10-13 2470 err = ice_ptp_init_phc(hw);
4809671015a1bd2 Karol Kolacinski 2021-12-20 2471 if (err)
9ea8140e2f947cb Jacob Keller 2023-10-25 2472 return err;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2473
4809671015a1bd2 Karol Kolacinski 2021-12-20 2474 /* Acquire the global hardware lock */
4809671015a1bd2 Karol Kolacinski 2021-12-20 2475 if (!ice_ptp_lock(hw)) {
4809671015a1bd2 Karol Kolacinski 2021-12-20 2476 err = -EBUSY;
9ea8140e2f947cb Jacob Keller 2023-10-25 2477 return err;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2478 }
4809671015a1bd2 Karol Kolacinski 2021-12-20 2479
4809671015a1bd2 Karol Kolacinski 2021-12-20 2480 /* Write the increment time value to PHY and LAN */
78267d0c9cabf09 Jacob Keller 2021-10-13 2481 err = ice_ptp_write_incval(hw, ice_base_incval(pf));
4809671015a1bd2 Karol Kolacinski 2021-12-20 2482 if (err) {
4809671015a1bd2 Karol Kolacinski 2021-12-20 2483 ice_ptp_unlock(hw);
9ea8140e2f947cb Jacob Keller 2023-10-25 2484 return err;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2485 }
4809671015a1bd2 Karol Kolacinski 2021-12-20 2486
4809671015a1bd2 Karol Kolacinski 2021-12-20 2487 /* Write the initial Time value to PHY and LAN using the cached PHC
4809671015a1bd2 Karol Kolacinski 2021-12-20 2488 * time before the reset and time difference between stopping and
4809671015a1bd2 Karol Kolacinski 2021-12-20 2489 * starting the clock.
4809671015a1bd2 Karol Kolacinski 2021-12-20 2490 */
4809671015a1bd2 Karol Kolacinski 2021-12-20 2491 if (ptp->cached_phc_time) {
4809671015a1bd2 Karol Kolacinski 2021-12-20 2492 time_diff = ktime_get_real_ns() - ptp->reset_time;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2493 ts = ns_to_timespec64(ptp->cached_phc_time + time_diff);
4809671015a1bd2 Karol Kolacinski 2021-12-20 2494 } else {
4809671015a1bd2 Karol Kolacinski 2021-12-20 2495 ts = ktime_to_timespec64(ktime_get_real());
4809671015a1bd2 Karol Kolacinski 2021-12-20 2496 }
4809671015a1bd2 Karol Kolacinski 2021-12-20 2497 err = ice_ptp_write_init(pf, &ts);
4809671015a1bd2 Karol Kolacinski 2021-12-20 2498 if (err) {
4809671015a1bd2 Karol Kolacinski 2021-12-20 2499 ice_ptp_unlock(hw);
9ea8140e2f947cb Jacob Keller 2023-10-25 2500 return err;
4809671015a1bd2 Karol Kolacinski 2021-12-20 2501 }
4809671015a1bd2 Karol Kolacinski 2021-12-20 2502
4809671015a1bd2 Karol Kolacinski 2021-12-20 2503 /* Release the global hardware lock */
4809671015a1bd2 Karol Kolacinski 2021-12-20 2504 ice_ptp_unlock(hw);
4809671015a1bd2 Karol Kolacinski 2021-12-20 2505
b57a67bab47efc2 Jacob Keller 2023-10-25 2506 /* Flush software tracking of any outstanding timestamps since we're
b57a67bab47efc2 Jacob Keller 2023-10-25 2507 * about to flush the PHY timestamp block.
b57a67bab47efc2 Jacob Keller 2023-10-25 2508 */
b57a67bab47efc2 Jacob Keller 2023-10-25 @2509 ice_ptp_flush_all_tx_tracker(pf);
b57a67bab47efc2 Jacob Keller 2023-10-25 2510
3a7496234d179a7 Jacob Keller 2021-10-13 2511 if (!ice_is_e810(hw)) {
3a7496234d179a7 Jacob Keller 2021-10-13 2512 /* Enable quad interrupts */
d695241a0a0ea02 Jacob Keller 2023-10-25 2513 err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
9ea8140e2f947cb Jacob Keller 2023-10-25 2514 if (err)
9ea8140e2f947cb Jacob Keller 2023-10-25 2515 return err;
9ea8140e2f947cb Jacob Keller 2023-10-25 2516
9ea8140e2f947cb Jacob Keller 2023-10-25 2517 ice_ptp_restart_all_phy(pf);
9ea8140e2f947cb Jacob Keller 2023-10-25 2518 }
9ea8140e2f947cb Jacob Keller 2023-10-25 2519
9ea8140e2f947cb Jacob Keller 2023-10-25 2520 return 0;
9ea8140e2f947cb Jacob Keller 2023-10-25 2521 }
9ea8140e2f947cb Jacob Keller 2023-10-25 2522
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c index 5fb9dbbdfc16..bcc3bf105b71 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.c +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c @@ -534,9 +534,8 @@ ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx) * 2) check that a timestamp is ready and available in the PHY memory bank * 3) read and copy the timestamp out of the PHY register * 4) unlock the index by clearing the associated in_use bit - * 5) check if the timestamp is stale, and discard if so - * 6) extend the 40 bit timestamp value to get a 64 bit timestamp value - * 7) send this 64 bit timestamp to the stack + * 5) extend the 40 bit timestamp value to get a 64 bit timestamp value + * 6) send this 64 bit timestamp to the stack * * Note that we do not hold the tracking lock while reading the Tx timestamp. * This is because reading the timestamp requires taking a mutex that might @@ -556,12 +555,6 @@ ice_ptp_is_tx_tracker_up(struct ice_ptp_tx *tx) * interrupt for that timestamp should re-trigger this function once * a timestamp is ready. * - * In cases where the PTP hardware clock was directly adjusted, some - * timestamps may not be able to safely use the timestamp extension math. In - * this case, software will set the stale bit for any outstanding Tx - * timestamps when the clock is adjusted. Then this function will discard - * those captured timestamps instead of sending them to the stack. - * * If a Tx packet has been waiting for more than 2 seconds, it is not possible * to correctly extend the timestamp using the cached PHC time. It is * extremely unlikely that a packet will ever take this long to timestamp. If @@ -652,8 +645,6 @@ static void ice_ptp_process_tx_tstamp(struct ice_ptp_tx *tx) clear_bit(idx, tx->in_use); skb = tx->tstamps[idx].skb; tx->tstamps[idx].skb = NULL; - if (test_and_clear_bit(idx, tx->stale)) - drop_ts = true; spin_unlock(&tx->lock); /* It is unlikely but possible that the SKB will have been @@ -752,24 +743,21 @@ static enum ice_tx_tstamp_work ice_ptp_tx_tstamp(struct ice_ptp_tx *tx) static int ice_ptp_alloc_tx_tracker(struct ice_ptp_tx *tx) { - unsigned long *in_use, *stale; struct ice_tx_tstamp *tstamps; + unsigned long *in_use; tstamps = kcalloc(tx->len, sizeof(*tstamps), GFP_KERNEL); in_use = bitmap_zalloc(tx->len, GFP_KERNEL); - stale = bitmap_zalloc(tx->len, GFP_KERNEL); - if (!tstamps || !in_use || !stale) { + if (!tstamps || !in_use) { kfree(tstamps); bitmap_free(in_use); - bitmap_free(stale); return -ENOMEM; } tx->tstamps = tstamps; tx->in_use = in_use; - tx->stale = stale; tx->init = 1; spin_lock_init(&tx->lock); @@ -815,7 +803,6 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx) skb = tx->tstamps[idx].skb; tx->tstamps[idx].skb = NULL; clear_bit(idx, tx->in_use); - clear_bit(idx, tx->stale); spin_unlock(&tx->lock); /* Count the number of Tx timestamps flushed */ @@ -826,41 +813,6 @@ ice_ptp_flush_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx) } } -/** - * ice_ptp_mark_tx_tracker_stale - Mark unfinished timestamps as stale - * @tx: the tracker to mark - * - * Mark currently outstanding Tx timestamps as stale. This prevents sending - * their timestamp value to the stack. This is required to prevent extending - * the 40bit hardware timestamp incorrectly. - * - * This should be called when the PTP clock is modified such as after a set - * time request. - */ -static void -ice_ptp_mark_tx_tracker_stale(struct ice_ptp_tx *tx) -{ - spin_lock(&tx->lock); - bitmap_or(tx->stale, tx->stale, tx->in_use, tx->len); - spin_unlock(&tx->lock); -} - -/** - * ice_ptp_flush_all_tx_tracker - Flush all timestamp trackers on this clock - * @pf: Board private structure - * - * Called by the clock owner to flush all the Tx timestamp trackers associated - * with the clock. - */ -static void -ice_ptp_flush_all_tx_tracker(struct ice_pf *pf) -{ - struct ice_ptp_port *port; - - list_for_each_entry(port, &pf->ptp.ports_owner.ports, list_member) - ice_ptp_flush_tx_tracker(ptp_port_to_pf(port), &port->tx); -} - /** * ice_ptp_release_tx_tracker - Release allocated memory for Tx tracker * @pf: Board private structure @@ -886,9 +838,6 @@ ice_ptp_release_tx_tracker(struct ice_pf *pf, struct ice_ptp_tx *tx) bitmap_free(tx->in_use); tx->in_use = NULL; - bitmap_free(tx->stale); - tx->stale = NULL; - tx->len = 0; } @@ -1006,14 +955,12 @@ static int ice_ptp_update_cached_phctime(struct ice_pf *pf) * ice_ptp_reset_cached_phctime - Reset cached PHC time after an update * @pf: Board specific private structure * - * This function must be called when the cached PHC time is no longer valid, - * such as after a time adjustment. It marks any currently outstanding Tx - * timestamps as stale and updates the cached PHC time for both the PF and Rx - * rings. + * This function is called to immediately update the cached PHC time after + * a .settime or .adjtime call. * * If updating the PHC time cannot be done immediately, a warning message is - * logged and the work item is scheduled immediately to minimize the window - * with a wrong cached timestamp. + * logged and the work item is scheduled without delay to minimize the window + * where a timestamp is extended using the old cached value. */ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf) { @@ -1036,13 +983,6 @@ static void ice_ptp_reset_cached_phctime(struct ice_pf *pf) kthread_queue_delayed_work(pf->ptp.kworker, &pf->ptp.work, msecs_to_jiffies(10)); } - - /* Mark any outstanding timestamps as stale, since they might have - * been captured in hardware before the time update. This could lead - * to us extending them with the wrong cached value resulting in - * incorrect timestamp values. - */ - ice_ptp_mark_tx_tracker_stale(&pf->ptp.port.tx); } /** @@ -2416,7 +2356,6 @@ s8 ice_ptp_request_ts(struct ice_ptp_tx *tx, struct sk_buff *skb) * requests. */ set_bit(idx, tx->in_use); - clear_bit(idx, tx->stale); tx->tstamps[idx].start = jiffies; tx->tstamps[idx].skb = skb_get(skb); skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h index c0c8ef4de70f..ed2d3517db04 100644 --- a/drivers/net/ethernet/intel/ice/ice_ptp.h +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h @@ -123,7 +123,6 @@ enum ice_tx_tstamp_work { * @lock: lock to prevent concurrent access to fields of this struct * @tstamps: array of len to store outstanding requests * @in_use: bitmap of len to indicate which slots are in use - * @stale: bitmap of len to indicate slots which have stale timestamps * @block: which memory block (quad or port) the timestamps are captured in * @offset: offset into timestamp block to get the real index * @len: length of the tstamps and in_use fields. @@ -138,7 +137,6 @@ struct ice_ptp_tx { spinlock_t lock; /* lock protecting in_use bitmap */ struct ice_tx_tstamp *tstamps; unsigned long *in_use; - unsigned long *stale; u8 block; u8 offset; u8 len;