Re: [PATCH net-next v9 13/15] dpll: sit9531x: add support to get fractional frequency offset

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 3

- [High] sit9531x_divn_runtime() feeds an unvalidated 48-bit sampled
  numerator and a raw sampled denominator (only checked for != 0) into…
- [Medium] sit9531x_dbg_sample() writes SIT9531X_PLL_DEBUG_UNLOCK (0xC3)
  to the per-PLL debug key register SIT9531X_PLL_REG_DEBUG (0xBD) but…
- [Low] The documentation block for sit9531x_dbg_sample() lists @sitdev,
  @pll_idx, @read_code, @buf and @len, but the implemented signature…
- [Low] SIT9531X_DIVN_RT_INT_HI_BIT is defined in regs.h under a comment
  that describes it as belonging to the integer part of the DIVN tap…
- [Low] The kernel-doc of sit9531x_pll_ffo_ppt() states 'Return: 0 on
  success, -ENODATA when DIVN is not programmed, <0 on error', but the…

--

dpll: sit9531x: add support to get fractional frequency offset

This adds a fractional frequency offset readback for the sit9531x DPLL
inputs, derived from the difference between the DIVN the digital loop is
running and the DIVN the configuration registers hold.  The running value
is sampled through the same latched debug window that the phase offset
readback uses.

A few questions on the new code below.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> index 356d1eac4ec54..88bfa63b29755 100644
> --- a/drivers/dpll/sit9531x/core.c
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -1224,6 +1224,76 @@ static int sit9531x_is_xo_doubler_enabled(struct sit9531x_dev *sitdev)
>  	return (~val >> SIT9531X_XO_DOUBLER_ENB_BIT) & 1u;
>  }
>  
> +/*
> + * sit9531x_dbg_sample - latch and read a signal pathway debug sample
> + * @sitdev:	device pointer
> + * @pll_idx:	PLL index (0-3)
> + * @read_code:	which tap of the pathway to sample
> + * @buf:	result, least significant byte first
> + * @len:	bytes to read, at most SIT9531X_DBG_DATA_BYTES
> + *
> + * Return: 0 on success, <0 on error
> + */
> +static int sit9531x_dbg_sample(struct sit9531x_dev *sitdev, u8 pll_idx,
> +			       u8 read_code, bool low_freq_clk,
> +			       u8 *buf, unsigned int len)

[Severity: Low]
This isn't a bug, but the comment block for sit9531x_dbg_sample() documents
five parameters (sitdev, pll_idx, read_code, buf, len) while the function
takes six, with bool low_freq_clk sitting between read_code and buf.  Since
the clock-select argument is the one the block goes on to explain in prose,
should it get a @low_freq_clk entry too?

> +{
> +	unsigned int i;
> +	int rc;
> +	u8 v;
> +
> +	if (len > SIT9531X_DBG_DATA_BYTES)
> +		return -EINVAL;
> +
> +	rc = sit9531x_write_pll_u8(sitdev, pll_idx, SIT9531X_PLL_REG_DEBUG,
> +				   SIT9531X_PLL_DEBUG_UNLOCK);
> +	if (rc)
> +		return rc;

[Severity: Medium]
Should this helper close the debug window before returning?

SIT9531X_PLL_REG_DEBUG is written with SIT9531X_PLL_DEBUG_UNLOCK here, and
none of the four error returns below, nor the final return 0, writes
SIT9531X_PLL_DEBUG_LOCK back.

The existing user of the same window, sit9531x_phase_offset_read(), funnels
every exit through a relock label and states what the open window means:

	relock:
		/*
		 * Close the debug window again.  The key register opens every debug
		 * register on this PLL while it holds the unlock value, and this read
		 * runs on every pin-get of a connected input, so leaving it open
		 * would mean normal monitoring permanently unlocks the block.
		 */
		lock_rc = sit9531x_write_pll_u8(sitdev, pll_idx,
						SIT9531X_PLL_REG_DEBUG,
						SIT9531X_PLL_DEBUG_LOCK);

The new path sit9531x_dpll_input_pin_ffo_get() ->
sit9531x_pll_ffo_ppt() -> sit9531x_divn_runtime() -> sit9531x_dbg_sample()
runs on every pin-get of a connected input and calls the helper twice, and
sit9531x_divn_runtime() returns straight out on the first error, so nothing
above it re-locks either.  Does that leave the PLL's debug block unlocked
after every frequency offset read?

> +
> +	/*
> +	 * Select the debug clock this tap needs.  The device never clears
> +	 * the bit, so a read that left it to whatever the previous one set
> +	 * would depend on the order the taps happened to be read in.  Taps
> +	 * below 200 kHz need the slow clock; the divider taps do not.
> +	 */

[ ... ]

> @@ -1291,6 +1361,130 @@ static int sit9531x_divn_static(struct sit9531x_dev *sitdev, u8 pll_idx,
>  	return 0;
>  }
>  

[ ... ]

> +static int sit9531x_divn_runtime(struct sit9531x_dev *sitdev, u8 pll_idx,
> +				 s64 *divn)
> +{
> +	u8 buf[SIT9531X_DBG_DATA_BYTES];
> +	u64 fracn_raw = 0, fracd = 0;
> +	u32 int_part;
> +	int rc, i;

[ ... ]

> +	for (i = 5; i >= 0; i--)
> +		fracd = (fracd << 8) | buf[i];
> +
> +	fracd |= (u64)(buf[6] & SIT9531X_DIVN_RT_INT_HI_BIT) << 48;
> +
> +	/*
> +	 * sit9531x_divn_fixed() drops the fractional part when the
> +	 * denominator is zero.  That is right for a configuration register
> +	 * that was never programmed, but a running loop reading zero means
> +	 * the sample is unusable, and returning the whole part alone would
> +	 * put a wrong DIVN into the frequency offset without saying so.
> +	 */
> +	if (!fracd)
> +		return -ENODATA;
> +
> +	*divn = sit9531x_divn_fixed(int_part,
> +				    sign_extend64(fracn_raw,
> +						  SIT9531X_DIVN_RT_NUM_BITS - 1),
> +				    fracd);

[Severity: High]
Can a small but nonzero denominator sample trap here?

The only check on the sampled pair is "if (!fracd) return -ENODATA;", so any
fracd in 1..2^49-1 is passed on together with a numerator of up to 2^47 in
magnitude after sign_extend64().  sit9531x_divn_fixed() then does:

	frac = mul_u64_u64_div_u64(abs(fracn), SIT9531X_DIVN_SCALE, fracd);

With SIT9531X_DIVN_SCALE at 1e12, abs(fracn) near 2^47 gives a product of
about 1.4e26, so the quotient exceeds U64_MAX for any fracd below roughly
7.6e6.  On x86-64 that is not a wrong value but a divide-error exception, per
arch/x86/include/asm/div64.h:

	/*
	 * Will generate an #DE when the result doesn't fit u64, could fix with an
	 * __ex_table[] entry when it becomes an issue.
	 */
	...
		asm ("divq %[div]" : "+a" (rax), "+d" (rdx) : [div] "rm" (div));

On architectures using the generic helper the result comes back as ~0 and is
reported to userspace as a frequency offset instead.

Both operands come from the I2C part through the latched debug window, and
the code already accepts that the tap may return something that is not a
running loop when it special-cases fracd == 0.  Would rejecting samples where
abs(fracn) >= fracd (or otherwise bounding the ratio) before calling
sit9531x_divn_fixed() be appropriate?  sit9531x_pll_ffo_ppt() adds exactly
this class of guard for the configured divider:

	if (configured < SIT9531X_DIVN_SCALE)
		return -ENODATA;

Note the pre-existing sit9531x_divn_static() path only supplies a 32-bit
numerator, so the runtime tap widens the range of inputs that can overflow
that division considerably.

> +
> +	return 0;
> +}
> +
> +/**
> + * sit9531x_pll_ffo_ppt - fractional frequency offset of a PLL's reference
> + * @sitdev:	device pointer
> + * @pll_idx:	PLL index (0-3)
> + * @ffo:	result in parts per trillion

[ ... ]

> + * Caller must hold sitdev->multiop_lock.
> + *
> + * Return: 0 on success, -ENODATA when DIVN is not programmed, <0 on
> + * error.
> + */

[Severity: Low]
This isn't a bug, but the documented return contract of
sit9531x_pll_ffo_ppt() lists -ENODATA only for an unprogrammed DIVN, while
two other paths return it as well:

sit9531x_divn_runtime() returns -ENODATA when the sampled runtime
denominator reads zero, which its own comment describes as "the sample is
unusable" rather than an unprogrammed divider.

sit9531x_pll_ffo_ppt() itself returns -ENODATA when the configured divider
is below one whole unit.

Could the Return: line cover those cases too?

[ ... ]

> diff --git a/drivers/dpll/sit9531x/regs.h b/drivers/dpll/sit9531x/regs.h
> index 08eae8a2c279a..78e8a47e96ab0 100644
> --- a/drivers/dpll/sit9531x/regs.h
> +++ b/drivers/dpll/sit9531x/regs.h
> @@ -309,6 +309,18 @@
>  #define SIT9531X_TDC_SIGN_BIT		3
>  #define SIT9531X_TDC_MAG_HI_MASK	GENMASK(2, 0)
>  
> +/*
> + * Read codes of the running DIVN taps.  Unlike the configuration
> + * registers these report what the digital loop currently commands, so
> + * they carry the correction the loop applies to track its reference.
> + * The integer part and the numerator share one tap, the denominator
> + * has its own.
> + */
> +#define SIT9531X_DBG_READ_CODE_DIVN		0x57
> +#define SIT9531X_DBG_READ_CODE_DIVN_DEN	0x56
> +#define SIT9531X_DIVN_RT_NUM_BITS		48
> +#define SIT9531X_DIVN_RT_INT_HI_BIT		BIT(0)

[Severity: Low]
This isn't a bug, but SIT9531X_DIVN_RT_INT_HI_BIT is named and commented as
the high bit of the integer part of the DIVN tap, and it is used that way in
sit9531x_divn_runtime():

	int_part = buf[6] | ((u32)(buf[7] & SIT9531X_DIVN_RT_INT_HI_BIT) << 8);

The same macro is then reused as the bit-48 mask of the denominator, which
is a different byte of a different tap:

	fracd |= (u64)(buf[6] & SIT9531X_DIVN_RT_INT_HI_BIT) << 48;

Would a separate SIT9531X_DIVN_RT_DEN_HI_BIT make the two fields
distinguishable, so a later change to the integer field width does not also
change the denominator read?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915000015.80480-1-arouhi%40sitime.com