Re: [PATCH net 1/2] ice: dpll: serialize changes of the SyncE CGU registers

[Petr Oros <poros@redhat.com>]

On 11/10/26 10:43 AM, Ivan Vecera wrote:
> All E825C PFs share the CGU registers R10 and R11. On dual complex E825
> the second complex reaches the CGU of complex 0 through cgu_peer. RCLK pin
> callbacks read, modify and write these registers under dpll_lock and the
> pf->dplls.lock of their PF. ice_ptp_link_change() rewrites R10 under the
> pf->dplls.lock of its PF only, so a link change on one PF can write back
> a stale R10 and undo an RCLK pin change made by another PF.
> 
> Add a mutex to struct ice_adapter, which all E825C PFs share, and hold it
> around these sequences.
> 
> Tested on GNR-D with dual complex E825, with a 2 to 4 ms delay after the
> read of R10 in the link change path to widen the window. Toggling the
> RCLK pin of one port while another port of the same complex went down
> and up 143 times lost 120 of 86814 updates of the mux in R10 without
> this patch, and none of 76992 with it.
> 
> Fixes: ad1df4f2d591 ("ice: dpll: Support E825-C SyncE and dynamic pin discovery")
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_adapter.c |  2 ++
>   drivers/net/ethernet/intel/ice/ice_adapter.h |  4 ++++
>   drivers/net/ethernet/intel/ice/ice_dpll.c    | 11 ++++++-----
>   drivers/net/ethernet/intel/ice/ice_dpll.h    |  4 ++++
>   drivers/net/ethernet/intel/ice/ice_ptp.c     |  2 ++
>   drivers/net/ethernet/intel/ice/ice_tspll.c   |  4 ++--
>   6 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.c b/drivers/net/ethernet/intel/ice/ice_adapter.c
> index 536923b6ae979d..aa2d8addae5bef 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.c
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.c
> @@ -72,6 +72,7 @@ static struct ice_adapter *ice_adapter_new(struct pci_dev *pdev)
>   	spin_lock_init(&adapter->txq_ctx_lock);
>   	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
>   		mutex_init(&adapter->cpi_phy_lock[i]);
> +	mutex_init(&adapter->cgu_synce_lock);
>   	refcount_set(&adapter->refcount, 1);
>   
>   	spin_lock_init(&adapter->ports.lock);
> @@ -85,6 +86,7 @@ static void ice_adapter_free(struct ice_adapter *adapter)
>   	WARN_ON(!list_empty(&adapter->ports.list));
>   	for (int i = 0; i < ARRAY_SIZE(adapter->cpi_phy_lock); i++)
>   		mutex_destroy(&adapter->cpi_phy_lock[i]);
> +	mutex_destroy(&adapter->cgu_synce_lock);
>   
>   	cleanup_srcu_struct(&adapter->ports.srcu);
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_adapter.h b/drivers/net/ethernet/intel/ice/ice_adapter.h
> index c61bd645e460de..fd5585005d91ba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_adapter.h
> +++ b/drivers/net/ethernet/intel/ice/ice_adapter.h
> @@ -41,6 +41,8 @@ struct ice_port_list {
>    * @txq_ctx_lock: Spinlock protecting access to the GLCOMM_QTX_CNTX_CTL register
>    * @cpi_phy_lock: Per-PHY mutex serializing CPI REQ/ACK transactions.
>    *               Index 0 = PHY0, index 1 = PHY1. Used on E825C devices.
> + * @cgu_synce_lock: Mutex serializing changes of the CGU registers R10 and R11,
> + *                  which all PFs reach. Used on E825C devices.
>    * @ctrl_pf: Control PF of the adapter
>    * @ports: Ports list
>    * @index: 64-bit index cached for collision detection on 32bit systems
> @@ -53,6 +55,8 @@ struct ice_adapter {
>   	spinlock_t txq_ctx_lock;
>   	/* Serialize CPI REQ/ACK transactions per PHY (E825C only) */
>   	struct mutex cpi_phy_lock[ICE_E825_MAX_PHYS];
> +	/* Serialize changes of the SyncE CGU registers (E825C only) */
> +	struct mutex cgu_synce_lock;
>   
>   	struct ice_pf *ctrl_pf;
>   	struct ice_port_list ports;
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index ade1fb3dd06743..5148e419f49a07 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
> @@ -2027,7 +2027,7 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
>    *
>    * DPLL subsystem callback. Set proper signals to recover clock from port.
>    *
> - * Context: Called under pf->dplls.lock
> + * Context: Called under pf->dplls.lock, acquires adapter->cgu_synce_lock
>    * Return:
>    * * 0 - success
>    * * negative - error
> @@ -2035,14 +2035,15 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
>   static int ice_dpll_synce_update_e825c(struct ice_hw *hw, bool ena,
>   				       u32 port_num, enum ice_synce_clk output)
>   {
> +	struct ice_pf *pf = hw->back;
>   	int err;
>   
> +	mutex_lock(&pf->adapter->cgu_synce_lock);
>   	/* configure the mux to deliver proper signal to DPLL from the MUX */
>   	err = ice_tspll_cfg_bypass_mux_e825c(hw, ena, port_num, output);
> -	if (err)
> -		return err;
> -
> -	err = ice_tspll_cfg_synce_ethdiv_e825c(hw, output);
> +	if (!err)
> +		err = ice_tspll_cfg_synce_ethdiv_e825c(hw, output);
> +	mutex_unlock(&pf->adapter->cgu_synce_lock);
>   	if (err)
>   		return err;
>   
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
> index bce0bea9a664bc..3f9a993129acef 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.h
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
> @@ -158,6 +158,7 @@ struct ice_dpll {
>    *     txclk_notify_rwsem (read)
>    *       -> pf->dplls.lock
>    *         -> ctrl_pf->dplls.lock
> + *         -> adapter->cgu_synce_lock
>    *
>    *   - @lock serializes all DPLL state mutations on this PF. When the
>    *     controlling PF's lock must also be taken (e.g. updating the shared
> @@ -169,6 +170,9 @@ struct ice_dpll {
>    *     dpll_*_change_ntf() calls. ice_dpll_deinit() takes the write side
>    *     standalone (not nested under any other lock) to drain in-flight
>    *     readers before pins and the TXC DPLL device are freed.
> + *   - adapter->cgu_synce_lock serializes changes of the CGU registers R10 and
> + *     R11. All E825C PFs reach one CGU, so @lock, which is per PF, does not
> + *     cover them.
>    */
>   struct ice_dplls {
>   	struct kthread_worker *kworker;
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 7980eaf9ce2535..449819cf8c8791 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1368,7 +1368,9 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
>   				break;
>   			}
>   
> +			mutex_lock(&pf->adapter->cgu_synce_lock);
>   			err = ice_tspll_cfg_synce_ethdiv_e825c(hw, clk_pin);
> +			mutex_unlock(&pf->adapter->cgu_synce_lock);
>   			if (active && err) {
>   				dev_err_once(ice_pf_to_dev(pf),
>   					     "Failed to configure SyncE ETH divider for pin %d, err %d\n",
> diff --git a/drivers/net/ethernet/intel/ice/ice_tspll.c b/drivers/net/ethernet/intel/ice/ice_tspll.c
> index 85bacb7c3a7903..b99e02cfe559bd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tspll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tspll.c
> @@ -792,7 +792,7 @@ int ice_tspll_bypass_mux_active_e825c(struct ice_hw *hw, u8 port, bool *active,
>    *
>    * Set reference clock source and output clock selection.
>    *
> - * Context: Called under pf->dplls.lock
> + * Context: Called under pf->dplls.lock and adapter->cgu_synce_lock
>    * Return:
>    * * 0 - success
>    * * negative - error
> @@ -898,7 +898,7 @@ static int ice_tspll_get_div_e825c(u16 link_speed, unsigned int *divider)
>    *
>    * Set the correct CGU divider for RCLKA or RCLKB.
>    *
> - * Context: Called under pf->dplls.lock
> + * Context: Called under pf->dplls.lock and adapter->cgu_synce_lock
>    * Return:
>    * * 0 - success
>    * * negative - error
Reviewed-by: Petr Oros <poros@redhat.com>