Re: [net-next,5/6] dpll: zl3073x: add reference priority to zl3073x_chan
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-03-14 19:54:30
Also in:
lkml
This is an AI-generated review of your patch. The human sending this email says "Is this legit? Quite hard to follow the logic of this change :/" For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html --- dpll: zl3073x: add reference priority to zl3073x_chan This commit caches the ZL_REG_DPLL_REF_PRIO registers in the zl3073x_chan cfg group, adding a ref_prio[] array and inline helpers for accessing priority values. It extends state_fetch and state_set with DPLL mailbox operations to read and write these priority registers.
quoted hunk ↗ jump to hunk
diff --git a/drivers/dpll/zl3073x/chan.c b/drivers/dpll/zl3073x/chan.c index 10189c8a5ded..74d00cee624e 100644 --- a/drivers/dpll/zl3073x/chan.c +++ b/drivers/dpll/zl3073x/chan.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -85,7 +102,9 @@ const struct zl3073x_chan *zl3073x_chan_state_get(struct zl3073x_dev *zldev, * @chan: desired channel state * * Skips the HW write if the configuration is unchanged, and otherwise - * writes the mode_refsel register to hardware. + * writes only the changed registers to hardware. The mode_refsel register + * is written directly, while the reference priority registers are written + * via the DPLL mailbox interface. * * Return: 0 on success, <0 on HW error */@@ -93,14 +112,48 @@ int zl3073x_chan_state_set(struct zl3073x_dev *zldev, u8 index, const struct zl3073x_chan *chan) { struct zl3073x_chan *dchan = &zldev->chan[index]; - int rc; + int rc, i; /* Skip HW write if configuration hasn't changed */ if (!memcmp(&dchan->cfg, &chan->cfg, sizeof(chan->cfg))) return 0; - rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MODE_REFSEL(index), - chan->mode_refsel); + /* Direct register write for mode_refsel */ + if (dchan->mode_refsel != chan->mode_refsel) { + rc = zl3073x_write_u8(zldev, ZL_REG_DPLL_MODE_REFSEL(index), + chan->mode_refsel); + if (rc) + return rc; + } + + /* Mailbox write for ref_prio if changed */ + if (!memcmp(dchan->ref_prio, chan->ref_prio, sizeof(chan->ref_prio))) { + dchan->cfg = chan->cfg; + return 0; + } + + guard(mutex)(&zldev->multiop_lock); + + /* Read DPLL configuration into mailbox */ + rc = zl3073x_mb_op(zldev, ZL_REG_DPLL_MB_SEM, ZL_DPLL_MB_SEM_RD, + ZL_REG_DPLL_MB_MASK, BIT(index)); + if (rc) + return rc; + + /* Update changed ref_prio registers */ + for (i = 0; i < ARRAY_SIZE(chan->ref_prio); i++) { + if (dchan->ref_prio[i] != chan->ref_prio[i]) { + rc = zl3073x_write_u8(zldev, + ZL_REG_DPLL_REF_PRIO(i), + chan->ref_prio[i]); + if (rc) + return rc; + } + } + + /* Commit DPLL configuration */ + rc = zl3073x_mb_op(zldev, ZL_REG_DPLL_MB_SEM, ZL_DPLL_MB_SEM_WR, + ZL_REG_DPLL_MB_MASK, BIT(index)); if (rc) return rc;
Could this function leave the cache and hardware out of sync?
When both mode_refsel and ref_prio have changed, the function writes
mode_refsel to hardware first. If the subsequent mailbox operation fails
(during the mailbox read, a ref_prio write, or the mailbox commit), the
function returns an error without executing 'dchan->cfg = chan->cfg'.
This leaves dchan->mode_refsel cached at the old value while the hardware
has the new value:
zl3073x_chan_state_set()
dchan->mode_refsel != chan->mode_refsel is true
zl3073x_write_u8(... mode_refsel) succeeds
hardware now has new mode_refsel value
zl3073x_mb_op(...READ...) fails
return rc without updating dchan->cfg
dchan->mode_refsel still has old value
Later calls that get state via zl3073x_chan_state_get() will see the stale
cached mode_refsel. If they try to set mode_refsel back to the old value,
the comparison 'dchan->mode_refsel != chan->mode_refsel' evaluates to false
(both match the old value), skipping the write, while hardware retains the
new value.
Before this patch, mode_refsel was written in a single atomic step with no
possibility of partial failure, so this desynchronization scenario couldn't
occur.