diff mbox series

[iwl-net,v2,4/4] igc: Add lock preventing multiple simultaneous PTM transactions

Message ID 20241023023040.111429-5-christopher.s.hall@intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series igc: Fix PTM timeout | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: anthony.l.nguyen@intel.com; 7 maintainers not CCed: anthony.l.nguyen@intel.com edumazet@google.com andrew+netdev@lunn.ch przemyslaw.kitszel@intel.com pabeni@redhat.com richardcochran@gmail.com kuba@kernel.org
netdev/build_clang success Errors and warnings before: 5 this patch: 5
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 4 this patch: 4
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 32 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 6 this patch: 6
netdev/source_inline success Was 0 now: 0

Commit Message

Hall, Christopher S Oct. 23, 2024, 2:30 a.m. UTC 
From: Christopher S M Hall <christopher.s.hall@intel.com>

Add a mutex around the PTM transaction to prevent multiple transactors

Multiple processes try to initiate a PTM transaction, one or all may
fail. This can be reproduced by running two instances of the
following:

$ sudo phc2sys -O 0 -i tsn0 -m

PHC2SYS exits with:

"ioctl PTP_OFFSET_PRECISE: Connection timed out" when the PTM transaction
 fails

Note: Normally two instance of PHC2SYS will not run, but one process
 should not break another.

Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
Signed-off-by: Christopher S M Hall <christopher.s.hall@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h     |  1 +
 drivers/net/ethernet/intel/igc/igc_ptp.c | 12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

Comments

Tony Nguyen Oct. 29, 2024, 9:20 p.m. UTC | #1 
On 10/22/2024 7:30 PM, Chris H wrote:
> From: Christopher S M Hall <christopher.s.hall@intel.com>
> 
> Add a mutex around the PTM transaction to prevent multiple transactors
> 
> Multiple processes try to initiate a PTM transaction, one or all may
> fail. This can be reproduced by running two instances of the
> following:
> 
> $ sudo phc2sys -O 0 -i tsn0 -m
> 
> PHC2SYS exits with:
> 
> "ioctl PTP_OFFSET_PRECISE: Connection timed out" when the PTM transaction
>   fails
> 
> Note: Normally two instance of PHC2SYS will not run, but one process
>   should not break another.
> 
> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
> Signed-off-by: Christopher S M Hall <christopher.s.hall@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc.h     |  1 +
>   drivers/net/ethernet/intel/igc/igc_ptp.c | 12 ++++++++++--
>   2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index eac0f966e0e4..323db1e2be38 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -319,6 +319,7 @@ struct igc_adapter {
>   	struct timespec64 prev_ptp_time; /* Pre-reset PTP clock */
>   	ktime_t ptp_reset_start; /* Reset time in clock mono */
>   	struct system_time_snapshot snapshot;
> +	struct mutex ptm_lock; /* Only allow one PTM transaction at a time */
>   
>   	char fw_version[32];
>   
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index fb885fcaa97c..4e1379b7d2ee 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -1068,9 +1068,16 @@ static int igc_ptp_getcrosststamp(struct ptp_clock_info *ptp,
>   {
>   	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
>   						   ptp_caps);
> +	int ret;
>   
> -	return get_device_system_crosststamp(igc_phc_get_syncdevicetime,
> -					     adapter, &adapter->snapshot, cts);
> +	/* This blocks until any in progress PTM transactions complete */
> +	mutex_lock(&adapter->ptm_lock);
> +
> +	ret = get_device_system_crosststamp(igc_phc_get_syncdevicetime,
> +					    adapter, &adapter->snapshot, cts);
> +	mutex_unlock(&adapter->ptm_lock);
> +
> +	return ret;
>   }
>   
>   static int igc_ptp_getcyclesx64(struct ptp_clock_info *ptp,
> @@ -1169,6 +1176,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
>   	spin_lock_init(&adapter->ptp_tx_lock);
>   	spin_lock_init(&adapter->free_timer_lock);
>   	spin_lock_init(&adapter->tmreg_lock);
> +	mutex_init(&adapter->ptm_lock);

I believe there should be a corresponding mutex_destroy()?

Thanks,
Tony

>   	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
>   	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
Corinna Vinschen Oct. 30, 2024, 10:20 a.m. UTC | #2 
Hi Chris,

Same question as in the v1 thread:

On Oct 23 02:30, Chris H wrote:
> From: Christopher S M Hall <christopher.s.hall@intel.com>
> 
> Add a mutex around the PTM transaction to prevent multiple transactors
> 
> Multiple processes try to initiate a PTM transaction, one or all may
> fail. This can be reproduced by running two instances of the
> following:
> 
> $ sudo phc2sys -O 0 -i tsn0 -m
> 
> PHC2SYS exits with:
> 
> "ioctl PTP_OFFSET_PRECISE: Connection timed out" when the PTM transaction
>  fails
> 
> Note: Normally two instance of PHC2SYS will not run, but one process
>  should not break another.
> 
> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
> Signed-off-by: Christopher S M Hall <christopher.s.hall@intel.com>

I saw a former version of the patch which additionally added a mutex
lock/unlock in igc_ptp_reset() just before calling igc_ptm_trigger().
Is it safe to skip that?  igc_ptp_reset() is called from igc_reset()
which in turn is called from quite a few places.


Thanks,
Corinna





> ---
>  drivers/net/ethernet/intel/igc/igc.h     |  1 +
>  drivers/net/ethernet/intel/igc/igc_ptp.c | 12 ++++++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index eac0f966e0e4..323db1e2be38 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -319,6 +319,7 @@ struct igc_adapter {
>  	struct timespec64 prev_ptp_time; /* Pre-reset PTP clock */
>  	ktime_t ptp_reset_start; /* Reset time in clock mono */
>  	struct system_time_snapshot snapshot;
> +	struct mutex ptm_lock; /* Only allow one PTM transaction at a time */
>  
>  	char fw_version[32];
>  
> diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
> index fb885fcaa97c..4e1379b7d2ee 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ptp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
> @@ -1068,9 +1068,16 @@ static int igc_ptp_getcrosststamp(struct ptp_clock_info *ptp,
>  {
>  	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
>  						   ptp_caps);
> +	int ret;
>  
> -	return get_device_system_crosststamp(igc_phc_get_syncdevicetime,
> -					     adapter, &adapter->snapshot, cts);
> +	/* This blocks until any in progress PTM transactions complete */
> +	mutex_lock(&adapter->ptm_lock);
> +
> +	ret = get_device_system_crosststamp(igc_phc_get_syncdevicetime,
> +					    adapter, &adapter->snapshot, cts);
> +	mutex_unlock(&adapter->ptm_lock);
> +
> +	return ret;
>  }
>  
>  static int igc_ptp_getcyclesx64(struct ptp_clock_info *ptp,
> @@ -1169,6 +1176,7 @@ void igc_ptp_init(struct igc_adapter *adapter)
>  	spin_lock_init(&adapter->ptp_tx_lock);
>  	spin_lock_init(&adapter->free_timer_lock);
>  	spin_lock_init(&adapter->tmreg_lock);
> +	mutex_init(&adapter->ptm_lock);
>  
>  	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
>  	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;
> -- 
> 2.34.1
Mor Bar-Gabay Oct. 31, 2024, 9:36 a.m. UTC | #3 
On 23/10/2024 5:30, Chris H wrote:
> From: Christopher S M Hall <christopher.s.hall@intel.com>
> 
> Add a mutex around the PTM transaction to prevent multiple transactors
> 
> Multiple processes try to initiate a PTM transaction, one or all may
> fail. This can be reproduced by running two instances of the
> following:
> 
> $ sudo phc2sys -O 0 -i tsn0 -m
> 
> PHC2SYS exits with:
> 
> "ioctl PTP_OFFSET_PRECISE: Connection timed out" when the PTM transaction
>   fails
> 
> Note: Normally two instance of PHC2SYS will not run, but one process
>   should not break another.
> 
> Fixes: a90ec8483732 ("igc: Add support for PTP getcrosststamp()")
> Signed-off-by: Christopher S M Hall <christopher.s.hall@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc.h     |  1 +
>   drivers/net/ethernet/intel/igc/igc_ptp.c | 12 ++++++++++--
>   2 files changed, 11 insertions(+), 2 deletions(-)
> 
Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index eac0f966e0e4..323db1e2be38 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -319,6 +319,7 @@  struct igc_adapter {
 	struct timespec64 prev_ptp_time; /* Pre-reset PTP clock */
 	ktime_t ptp_reset_start; /* Reset time in clock mono */
 	struct system_time_snapshot snapshot;
+	struct mutex ptm_lock; /* Only allow one PTM transaction at a time */
 
 	char fw_version[32];
 
diff --git a/drivers/net/ethernet/intel/igc/igc_ptp.c b/drivers/net/ethernet/intel/igc/igc_ptp.c
index fb885fcaa97c..4e1379b7d2ee 100644
--- a/drivers/net/ethernet/intel/igc/igc_ptp.c
+++ b/drivers/net/ethernet/intel/igc/igc_ptp.c
@@ -1068,9 +1068,16 @@  static int igc_ptp_getcrosststamp(struct ptp_clock_info *ptp,
 {
 	struct igc_adapter *adapter = container_of(ptp, struct igc_adapter,
 						   ptp_caps);
+	int ret;
 
-	return get_device_system_crosststamp(igc_phc_get_syncdevicetime,
-					     adapter, &adapter->snapshot, cts);
+	/* This blocks until any in progress PTM transactions complete */
+	mutex_lock(&adapter->ptm_lock);
+
+	ret = get_device_system_crosststamp(igc_phc_get_syncdevicetime,
+					    adapter, &adapter->snapshot, cts);
+	mutex_unlock(&adapter->ptm_lock);
+
+	return ret;
 }
 
 static int igc_ptp_getcyclesx64(struct ptp_clock_info *ptp,
@@ -1169,6 +1176,7 @@  void igc_ptp_init(struct igc_adapter *adapter)
 	spin_lock_init(&adapter->ptp_tx_lock);
 	spin_lock_init(&adapter->free_timer_lock);
 	spin_lock_init(&adapter->tmreg_lock);
+	mutex_init(&adapter->ptm_lock);
 
 	adapter->tstamp_config.rx_filter = HWTSTAMP_FILTER_NONE;
 	adapter->tstamp_config.tx_type = HWTSTAMP_TX_OFF;