Re: [PATCH net-next v14 03/12] dpll: add basic SiTime SiT9531x support

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The binding
  (Documentation/devicetree/bindings/dpll/sitime,sit95316.yaml)…
- [Medium] The page-selector cache recovery does not cover every case.
- [Low] In sit9531x_dev_probe(), the XIN/XO reference is enabled with
  devm_clk_get_optional_enabled() (line 227) before the reset GPIO is…

--

dpll: add basic SiTime SiT9531x support

This adds an I2C driver for the SiTime SiT9531x clock generator. It has a
paged regmap configuration and page:offset register accessors. Probe reads
the XO rate, releases the optional reset line and identifies the variant
from page 0 offset 0x02.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> new file mode 100644
> index 0000000000000..20ccf024077bc
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c
> @@ -0,0 +1,320 @@

[ ... ]

> +/*
> + * Everything the device holds can change without the driver writing it,
> + * so nothing here is cacheable except the page selector.  Caching that
> + * one spares a read of it before every access: the range code selects
> + * the page through a read-modify-write, and with no cache that read goes
> + * to the bus each time.
> + *
> + * The cached value is only as good as the last transfer, though.  A
> + * transfer that fails can leave the part on another page than the cache
> + * records, so every accessor drops the entry on an error and the next
> + * access reads the selector again.
> + */
> +static bool sit9531x_volatile_reg(struct device *dev __maybe_unused,
> +				  unsigned int reg)
> +{
> +	return reg != SIT9531X_PAGE_SEL;
> +}

[ ... ]

> +static void sit9531x_page_cache_drop(struct sit9531x_dev *sitdev)
> +{
> +	regcache_drop_region(sitdev->regmap, SIT9531X_PAGE_SEL,
> +			     SIT9531X_PAGE_SEL);
> +}

[ ... ]

> +int sit9531x_read_u8(struct sit9531x_dev *sitdev, unsigned int reg,
> +		     u8 *val)
> +{

[ ... ]

> +	rc = regmap_read(sitdev->regmap, vreg, &tmp);
> +	if (rc) {
> +		sit9531x_page_cache_drop(sitdev);

[Severity: Medium]
Does this recovery still work when something other than these accessors
uses the map?

The selector is the only cached register, so _regmap_select_page() skips
the selector write when the cached page already matches. _regmap_write()
also updates the cache before the bus write and does not roll it back on
failure:

drivers/base/regmap/regmap.c:_regmap_write() {
    ...
	if (!map->cache_bypass && !map->defer_caching) {
		ret = regcache_write(map, reg, val);
    ...
	ret = map->reg_write(context, reg, val);
    ...
}

So a failed selector write leaves the cache at page P while the chip
stays on page Q.

sit9531x_page_cache_drop() only runs after regmap_read() has released
map->lock, and regcache_drop_region() then takes the lock again. Could
another map user take map->lock in that window, trust the stale page P,
and access page Q?

Map users that bypass sit9531x_read_u8(), sit9531x_write_u8() and
sit9531x_update_pll_u8() never drop the entry. For example,
regmap_read_debugfs() calls regmap_read() directly and only prints 'X'
on error:

drivers/base/regmap/regmap-debugfs.c:regmap_read_debugfs() {
    ...
			ret = regmap_read(map, i, &val);
			if (ret == 0)
				snprintf(buf + buf_pos, count - buf_pos,
					 "%.*x", map->debugfs_val_len, val);
			else
				memset(buf + buf_pos, 'X',
				       map->debugfs_val_len);
    ...
}

Suppose a selector write fails while the debugfs registers file is being
dumped, and the dump stops before the next page switch. Would the
driver's next access to page P then skip the selector write and silently
read or write the same offset on page Q?

The comment above sit9531x_volatile_reg() says "every accessor drops the
entry on an error". That seems to hold only while the driver's accessors
are the only users of the map.

[ ... ]

> +static int sit9531x_dev_probe(struct sit9531x_dev *sitdev)
> +{

[ ... ]

> +	xtal_clk = devm_clk_get_optional_enabled(sitdev->dev, "xtal");
> +	if (IS_ERR(xtal_clk))
> +		return dev_err_probe(sitdev->dev, PTR_ERR(xtal_clk),
> +				     "Failed to get xtal clock\n");

[Severity: Low]
The XO is enabled here, before the steps that can defer or fail. These
include devm_gpiod_get_optional() for the reset line, which returns
-EPROBE_DEFER if RESETB sits on a GPIO expander that has not probed yet,
and the variant ID read and match.

On those paths devres calls clk_disable_unprepare() on the xtal clock.
If the XO is a gateable clock that the bootloader left running, with a
CCF enable_count of 0 before probe, does this gate the reference of a
chip that is already running from efuse or a pre-probe blob?

The PLLs would then lose lock, and the outputs would glitch or stop on
every deferral cycle until probe succeeds.

Could the resources that can defer be looked up before the clock is
enabled and reset is deasserted? For example, the reset GPIO could be
requested with GPIOD_ASIS.

[ ... ]

> +	sitdev->reset_gpio = devm_gpiod_get_optional(sitdev->dev, "reset",
> +						     GPIOD_OUT_LOW);
> +	if (IS_ERR(sitdev->reset_gpio))
> +		return dev_err_probe(sitdev->dev, PTR_ERR(sitdev->reset_gpio),
> +				     "Failed to request reset gpio\n");
> +	if (sitdev->reset_gpio)
> +		fsleep(10000);	/* internal boot after release */
> +
> +	rc = sit9531x_read_variant_id(sitdev, &variant_id);
> +	if (rc)
> +		return rc;

[Severity: Medium]
The binding in sitime,sit95316.yaml documents vdd-supply, vddin-supply,
vdds-supply and the vddo*-supply rails, and its example wires several of
them. The driver never gets or enables any regulator, and that is still
true at the end of the series.

If one of these rails is a switchable regulator that is off at boot,
sit9531x_dev_probe() releases reset and reads the variant ID over I2C
from an unpowered part. Wouldn't probe then fail?

A rail might instead be left on by the bootloader or regulator-boot-on,
without being always-on and with no other consumer. Nothing holds a
reference to it, so regulator_late_cleanup() would switch it off:

drivers/regulator/core.c:regulator_late_cleanup() {
    ...
	if (rdev->use_count)
		goto unlock;
    ...
		ret = _regulator_do_disable(rdev);
    ...
}

Would that cut power to the running clock generator? It would lose the
efuse or pre-probe configuration that the driver avoids resetting for,
and downstream consumers would lose their reference clocks.

Should probe get and enable the documented supplies before releasing
reset?

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