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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dpll: sit9531x: race condition in regmap cache invalidation on I2C failure
--

commit 1fc3c6add951f10b8266bc8b2d155767ac703b33
Author: Oleg Zadorozhnyi <Oleg.Zadorozhnyi@devoxsoftware.com>

dpll: add basic SiTime SiT9531x support

This commit adds basic I2C support for the SiTime SiT9531x clock generator
family. It introduces paged regmap configuration, register access helpers,
and device probe functionality.

> diff --git a/drivers/dpll/sit9531x/core.c b/drivers/dpll/sit9531x/core.c
> new file mode 100644
> index 0000000000000..c7c14442df922
> --- /dev/null
> +++ b/drivers/dpll/sit9531x/core.c
[ ... ]
> +int sit9531x_read_u8(struct sit9531x_dev *sitdev, unsigned int reg,
> +		     u8 *val)
> +{
> +	unsigned int vreg, tmp;
> +	int rc;
> +
> +	vreg = (SIT9531X_REG_PAGE(reg) * SIT9531X_PAGE_SIZE) +
> +	       SIT9531X_REG_OFFSET(reg) + SIT9531X_RANGE_OFFSET;
> +
> +	rc = regmap_read(sitdev->regmap, vreg, &tmp);
> +	if (rc) {
> +		sit9531x_page_cache_drop(sitdev);

[Severity: Medium]
Could this create a race window during the cache invalidation?

The internal regmap lock is dropped after regmap_read returns. If an I2C
transfer fails, there is an unprotected race window between regmap dropping
its lock and regcache_drop_region re-acquiring it inside
sit9531x_page_cache_drop.

A concurrent thread could initiate another regmap access in this window,
observe the stale cached page selector, incorrectly assume the hardware is
on the requested page, and perform an I2C transfer. Since the failed
transfer may have left the hardware on a different page, the concurrent
access could hit the wrong registers.

Should the driver serialize the regmap access and the error-path cache
invalidation within a single lock (such as sitdev->multiop_lock) to ensure
the cache is invalidated atomically?

> +		dev_err(sitdev->dev,
> +			"Failed to read page 0x%02x reg 0x%02x: %d\n",
> +			SIT9531X_REG_PAGE(reg), SIT9531X_REG_OFFSET(reg), rc);
> +	} else {
> +		*val = (u8)tmp;
> +	}
> +
> +	return rc;
> +}
> +
> +/*
> + * sit9531x_write_u8 - write an 8-bit register
> + * @reg:	register in SIT9531X_REG(page, offset) form
> + * @val:	value to write
> + */
> +int sit9531x_write_u8(struct sit9531x_dev *sitdev, unsigned int reg,
> +		      u8 val)
> +{
> +	unsigned int vreg;
> +	int rc;
> +
> +	vreg = (SIT9531X_REG_PAGE(reg) * SIT9531X_PAGE_SIZE) +
> +	       SIT9531X_REG_OFFSET(reg) + SIT9531X_RANGE_OFFSET;
> +
> +	rc = regmap_write(sitdev->regmap, vreg, val);
> +	if (rc) {
> +		sit9531x_page_cache_drop(sitdev);

[Severity: Medium]
Does this have the same race window where the internal regmap lock is
dropped after regmap_write returns, before the cache is invalidated?

> +		dev_err(sitdev->dev,
> +			"Failed to write page 0x%02x reg 0x%02x: %d\n",
> +			SIT9531X_REG_PAGE(reg), SIT9531X_REG_OFFSET(reg), rc);
> +	}
> +
> +	return rc;
> +}
[ ... ]
> +int sit9531x_update_pll_u8(struct sit9531x_dev *sitdev, u8 pll_idx,
> +			   u8 offset, u8 mask, u8 val)
> +{
> +	u8 page = sit9531x_pll_page(pll_idx);
> +	unsigned int vreg;
> +	int rc;
> +
> +	vreg = (page * SIT9531X_PAGE_SIZE) + offset + SIT9531X_RANGE_OFFSET;
> +
> +	/*
> +	 * A read-modify-write is a read and a write, and either can leave
> +	 * the cached page selector wrong, so fail the way the single
> +	 * accessors do.
> +	 */
> +	rc = regmap_update_bits(sitdev->regmap, vreg, mask, val);
> +	if (rc) {
> +		sit9531x_page_cache_drop(sitdev);

[Severity: Medium]
Does this also expose the same race window when regmap_update_bits releases
its lock before the cache invalidation can happen?

> +		dev_err(sitdev->dev,
> +			"Failed to update page 0x%02x reg 0x%02x: %d\n",
> +			page, offset, rc);
> +	}
> +
> +	return rc;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009183151.78497-1-arouhi@sitime.com?part=3