Re: [PATCH net-next 02/10] ice: dpll: serialize changes of the SyncE CGU registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ice: ptp: unconditional R10 write and lock scope in
  ice_ptp_link_change()
- [Low] ice: dpll: missing Fixes trailer for commit ad1df4f2d591
--

commit 6fdc60cccd47bebe7d61ff60807ee77303bf58bd
Author: Ivan Vecera <ivecera@redhat.com>

> ice: dpll: serialize changes of the SyncE CGU registers
>
> On dual complex E825 only complex 0 has a functional CGU and the second
> complex reaches it through cgu_peer, so PFs of both complexes share the
> CGU registers R10 and R11. They read, modify and write them when the
> state of an RCLK pin changes, and ice_ptp_link_change() rewrites R10 when
> a link changes, under the pf->dplls.lock of their own PF only.
>
> ice_ptp_link_change() does not hold dpll_lock, so it can already overwrite
> an update of another PF. RCLK callbacks are serialized by dpll_lock, but
> the DPLL core is going to serialize callbacks per (module, clock_id), and
> each complex has its own DSN and so its own clock_id. With that change, a
> test toggling RCLK pins of both complexes lost an update.
>
> Add a mutex to struct ice_adapter, which all E825C PFs share, and hold it
> around these sequences.
>
> Signed-off-by: Ivan Vecera <ivecera@redhat.com>

[Severity: Low]
Since this fixes concurrent CGU register updates in E825-C SyncE support,
should it include:

  Fixes: ad1df4f2d591 ("ice: dpll: Support E825-C SyncE and dynamic pin discovery")

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
> index 3df08714510da..4d4b6aaaf30c2 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
> @@ -1395,7 +1395,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",

[Severity: Medium]
Should cgu_synce_lock also cover ice_tspll_bypass_mux_active_e825c(), and
should ice_tspll_cfg_synce_ethdiv_e825c() be skipped when active is false?

ice_ptp_link_change() checks active via ice_tspll_bypass_mux_active_e825c(),
but then calls ice_tspll_cfg_synce_ethdiv_e825c() unconditionally:

drivers/net/ethernet/intel/ice/ice_ptp.c:ice_ptp_link_change() {
    ...
			err = ice_tspll_bypass_mux_active_e825c(hw,
								port_num,
								&active,
								clk_pin);
			if (err) {
    ...
				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) {
    ...
}

Because ICE_CGU_R10 has a single divider per SyncE output pin shared across
all ports, a link-up on any inactive port with a non-zero link_speed
overwrites the divider of whichever port is actively driving clk_pin.

Even if gated on active, reading the mux state outside cgu_synce_lock leaves
a window where another PF running ice_dpll_synce_update_e825c() can switch
the mux to a new port and program its divider before ice_ptp_link_change()
takes cgu_synce_lock and overwrites ICE_CGU_R10 with the old port's divider.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106121005.1327603-1-ivecera@redhat.com?part=2