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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes a race that already exists in the tree, but it
  has no Fixes: tag and no stable Cc.

--

ice: dpll: serialize changes of the SyncE CGU registers

This patch adds a cgu_synce_lock mutex to struct ice_adapter, which all
E825C PFs share. It holds that lock around the CGU R10/R11
read-modify-write sequences in ice_dpll_synce_update_e825c(), and around
the ice_tspll_cfg_synce_ethdiv_e825c() call in ice_ptp_link_change().

> 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.

[ ... ]

> Signed-off-by: Ivan Vecera <ivecera@redhat.com>

[Severity: Low]
Should this patch have a Fixes: tag?

The commit message says this race already exists in the tree. Before
this patch, ice_ptp_link_change() on one PF calls
ice_tspll_cfg_synce_ethdiv_e825c(). That function reads ICE_CGU_R10 once
and writes the whole register back twice.

At the same time, on another PF, the path
ice_dpll_rclk_state_on_pin_set()->ice_dpll_synce_update_e825c() does a
read-modify-write of R10 and R11. Each of these paths holds only its own
PF's pf->dplls.lock.

Both paths reach the same CGU. All E825C PFs share one ice_adapter, and
the secondary complex reaches the primary CGU through
ice_sbq_dev_cgu_peer. So a link flap on one port can write back a stale
R10. That undoes a mux or divider change that another PF just made
(S_REF_CLK, ETHCLKO_SEL, CLKO_SEL, *_DIV_M1).

Both racing sequences seem to come from commit ad1df4f2d591 ("ice: dpll:
Support E825-C SyncE and dynamic pin discovery"). A related fix,
bf6dbadb72b9 ("ice: fix PTP timestamping broken by SyncE code on
E825C"), already has a Fixes: tag for that commit.

Without the tag, stable kernels that carry ad1df4f2d591 would probably
not get the fix for this race. Could you add the following tag?

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

Another option is to split this patch out of the series and send it to
net.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106121005.1327603-1-ivecera%40redhat.com