Re: [PATCH net-next v2 1/2] dpll: zl3073x: update all DPLL channels on ref_sync_set

Ivan Vecera <[email protected]>
Newsgroups org.kernel.vger.netdev,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
Replies to Sashiko findings:

 > Is this only a preparation change? ... Should this carry a Fixes: tag
 > so it gets picked up for stable?

It is preparation only. The old code is correct — the DPLL core's
xa_for_each loop calls ref_sync_set once per registered DPLL, and
since zl3073x registers input pins with every channel, each channel
gets its own callback invocation with the correct pin_priv. All
channels receive the priority update. The bug would only appear
after patch 2 removes the xa_for_each, which is why this patch moves
the iteration into the driver first.

 > Is the invariant in this comment accurate? ... 
zl3073x_dpll_pin_register()
 > reads them with no lock at all ... can a ref-sync DPLL_CMD_PIN_SET on
 > channel 0's pin enter the new loop and write zldev->chan[1].ref_prio
 > while channel 1's zl3073x_dpll_pin_register() is reading 
chan[1].ref_prio?

No race. dpll_lock is held across the entire dpll_pin_register()
call (acquired in dpll_core.c) and across the entire netlink
DPLL_CMD_PIN_SET handler (acquired in dpll_netlink.c). The device
startup loop calls zl3073x_dpll_register() -> dpll_pin_register()
for each channel under dpll_lock, and ref_sync_set also runs under
dpll_lock, so they cannot overlap.

 > Should the sync pin get a change notification for the other channels?

Good catch. Will add a sync_notify flag — set it when a channel's
priority is actually changed from non-NONE to NONE, and call
__dpll_pin_change_ntf(sync_pin->dpll_pin) after the loop if set.

 > Is the downgrade of this failure to a warning intentional? ...
 > The new comment calls the priority step advisory and "not fatal",
 > which reads as the opposite of the changelog statement that "the
 > priority update must cover all channels".

The ref sync pair configuration (zl3073x_ref_state_set) is the
essential operation whose failure is propagated. The per-channel
priority exclusion is a datasheet recommendation — the ref sync pair
functions regardless. Will align the changelog to say "recommended"
instead of "must". A mailbox failure here would indicate a serious
HW/bus problem that would surface through other error paths.

Thanks,
Ivan
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.