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

[Ivan Vecera <ivecera@redhat.com>]

On 11/7/26 13:11, sashiko-bot@kernel.org wrote:
> 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")
> 

Yes, the race with ice_ptp_link_change() is there since that commit. The
patch now goes to net with this tag:

https://lore.kernel.org/netdev/20261110094350.184185-1-ivecera@redhat.com/

v2 of this series will drop it.

> [ ... ]
> 
>> 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?

Yes to both, also there since ad1df4f2d591. Patch 2 of the net posting
skips the outputs the port does not drive and checks the mux under
cgu_synce_lock.

Thanks,
Ivan