| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <61c9f4d5-0572-46fb-86c6-483025dd3ca2@molgen.mpg.de>
Date: Wed, 7 Aug 2024 07:05:14 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Christopher S M Hall <christopher.s.hall@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, david.zage@...el.com,
vinschen@...hat.com, vinicius.gomes@...el.com, netdev@...r.kernel.org,
Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
rodrigo.cadore@...coustics.com
Subject: Re: [Intel-wired-lan] [PATCH iwl-net v1 1/5] igc: Ensure the PTM
cycle is reliably triggered
Dear Christopher,
Thank you for the patch.
Am 07.08.24 um 02:30 schrieb christopher.s.hall@...el.com:
> From: Christopher S M Hall <christopher.s.hall@...el.com>
>
> Writing to clear the PTM status 'valid' bit while the PTM cycle is
> triggered results in unreliable PTM operation. To fix this, clear the
> PTM 'trigger' and status after each PTM transaction.
I do not understand, why the *valid* bit is not needed anymore, and why
it’s the trigger bit seems to do the same task. It’d be great if you
elaborated.
Is that also documented in the dataheet?
> The issue can be reproduced with the following:
>
> $ sudo phc2sys -R 1000 -O 0 -i tsn0 -m
>
> Note: 1000 Hz (-R 1000) is unrealistically large, but provides a way to
> quickly reproduce the issue.
>
> PHC2SYS exits with:
>
> "ioctl PTP_OFFSET_PRECISE: Connection timed out" when the PTM transaction
> fails
>
> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
> Signed-off-by: Christopher S M Hall <christopher.s.hall@...el.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> ---
> drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
> drivers/net/ethernet/intel/igc/igc_ptp.c | 70 ++++++++++++--------
> 2 files changed, 42 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 511384f3ec5c..ec191d26c650 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -583,6 +583,7 @@
> #define IGC_PTM_STAT_T4M1_OVFL BIT(3) /* T4 minus T1 overflow */
> #define IGC_PTM_STAT_ADJUST_1ST BIT(4) /* 1588 timer adjusted during 1st PTM cycle */
> #define IGC_PTM_STAT_ADJUST_CYC BIT(5) /* 1588 timer adjusted during non-1st PTM cycle */
> +#define IGC_PTM_STAT_ALL GENMASK(5, 0) /* Used to clear all status */
>
> /* PCIe PTM Cycle Control */
> #define IGC_PTM_CYCLE_CTRL_CYC_TIME(msec) ((msec) & 0x3ff) /* PTM Cycle Time (msec) */
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index 946edbad4302..00cc80d8d164 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -974,11 +974,38 @@ static void igc_ptm_log_error(struct igc_adapter *adapter, u32 ptm_stat)
> }
> }
>
> +static void igc_ptm_trigger(struct igc_hw *hw)
> +{
> + u32 ctrl;
> +
> + /* To "manually" start the PTM cycle we need to set the
> + * trigger (TRIG) bit
> + */
> + ctrl = rd32(IGC_PTM_CTRL);
> + ctrl |= IGC_PTM_CTRL_TRIG;
> + wr32(IGC_PTM_CTRL, ctrl);
> + /* Perform flush after write to CTRL register otherwise
> + * transaction may not start
> + */
> + wrfl();
> +}
> +
> +static void igc_ptm_reset(struct igc_hw *hw)
> +{
> + u32 ctrl;
> +
> + ctrl = rd32(IGC_PTM_CTRL);
> + ctrl &= ~IGC_PTM_CTRL_TRIG;
> + wr32(IGC_PTM_CTRL, ctrl);
> + /* Write to clear all status */
> + wr32(IGC_PTM_STAT, IGC_PTM_STAT_ALL);
> +}
> +
> static int igc_phc_get_syncdevicetime(ktime_t *device,
> struct system_counterval_t *system,
> void *ctx)
> {
> - u32 stat, t2_curr_h, t2_curr_l, ctrl;
> + u32 stat, t2_curr_h, t2_curr_l;
> struct igc_adapter *adapter = ctx;
> struct igc_hw *hw = &adapter->hw;
> int err, count = 100;
> @@ -994,25 +1021,13 @@ static int igc_phc_get_syncdevicetime(ktime_t *device,
> * are transitory. Repeating the process returns valid
> * data eventually.
> */
> -
> - /* To "manually" start the PTM cycle we need to clear and
> - * then set again the TRIG bit.
> - */
> - ctrl = rd32(IGC_PTM_CTRL);
> - ctrl &= ~IGC_PTM_CTRL_TRIG;
> - wr32(IGC_PTM_CTRL, ctrl);
> - ctrl |= IGC_PTM_CTRL_TRIG;
> - wr32(IGC_PTM_CTRL, ctrl);
> -
> - /* The cycle only starts "for real" when software notifies
> - * that it has read the registers, this is done by setting
> - * VALID bit.
> - */
> - wr32(IGC_PTM_STAT, IGC_PTM_STAT_VALID);
> + igc_ptm_trigger(hw);
>
> err = readx_poll_timeout(rd32, IGC_PTM_STAT, stat,
> stat, IGC_PTM_STAT_SLEEP,
> IGC_PTM_STAT_TIMEOUT);
> + igc_ptm_reset(hw);
> +
> if (err < 0) {
> netdev_err(adapter->netdev, "Timeout reading IGC_PTM_STAT register\n");
> return err;
> @@ -1021,15 +1036,7 @@ static int igc_phc_get_syncdevicetime(ktime_t *device,
> if ((stat & IGC_PTM_STAT_VALID) == IGC_PTM_STAT_VALID)
> break;
>
> - if (stat & ~IGC_PTM_STAT_VALID) {
> - /* An error occurred, log it. */
> - igc_ptm_log_error(adapter, stat);
> - /* The STAT register is write-1-to-clear (W1C),
> - * so write the previous error status to clear it.
> - */
> - wr32(IGC_PTM_STAT, stat);
> - continue;
> - }
> + igc_ptm_log_error(adapter, stat);
Could this refactoring be a separate commit?
> } while (--count);
>
> if (!count) {
> @@ -1255,7 +1262,7 @@ void igc_ptp_stop(struct igc_adapter *adapter)
> void igc_ptp_reset(struct igc_adapter *adapter)
> {
> struct igc_hw *hw = &adapter->hw;
> - u32 cycle_ctrl, ctrl;
> + u32 cycle_ctrl, ctrl, stat;
> unsigned long flags;
> u32 timadj;
>
> @@ -1290,14 +1297,19 @@ void igc_ptp_reset(struct igc_adapter *adapter)
> ctrl = IGC_PTM_CTRL_EN |
> IGC_PTM_CTRL_START_NOW |
> IGC_PTM_CTRL_SHRT_CYC(IGC_PTM_SHORT_CYC_DEFAULT) |
> - IGC_PTM_CTRL_PTM_TO(IGC_PTM_TIMEOUT_DEFAULT) |
> - IGC_PTM_CTRL_TRIG;
> + IGC_PTM_CTRL_PTM_TO(IGC_PTM_TIMEOUT_DEFAULT);
>
> wr32(IGC_PTM_CTRL, ctrl);
>
> /* Force the first cycle to run. */
> - wr32(IGC_PTM_STAT, IGC_PTM_STAT_VALID);
> + igc_ptm_trigger(hw);
> +
> + if (readx_poll_timeout_atomic(rd32, IGC_PTM_STAT, stat,
> + stat, IGC_PTM_STAT_SLEEP,
> + IGC_PTM_STAT_TIMEOUT))
> + netdev_err(adapter->netdev, "Timeout reading IGC_PTM_STAT register\n");
I’d add the timeout value to the message.
>
> + igc_ptm_reset(hw);
> break;
> default:
> /* No work to do. */
Kind regards,
Paul
Powered by blists - more mailing lists