Re: [PATCH 2/3] reset: Add Apple SoC CIO reset driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Race condition in `t6000_cio_deassert()` polling logic allows premature success before hardware busy assertion.
- [Medium] Unchecked return value of `of_device_get_match_data()` leads to NULL pointer dereference on manual sysfs bind.
- [Medium] Shared register overwrite in `t6000_cio_deassert()` via `regmap_write()`.
--

--- Patch [2]: [PATCH 2/3] reset: Add Apple SoC CIO reset driver ---
commit b460799e8ebb76e6526aa8a4370ed4331dbda4b0
Author: Sven Peter <[email protected]>

reset: Add Apple SoC CIO reset driver

Adds a driver for the reset of the CIO (USB4/Thunderbolt) blocks on
Apple Silicon SoCs which has to be deasserted before their co-processor
can be booted.

> --- /dev/null
> +++ b/drivers/reset/reset-apple-cio.c

[ ... ]

> +static int t6000_cio_deassert(struct apple_cio_reset *priv, unsigned long id)
> +{
> +	u32 val;
> +	int ret;
> +
> +	guard(mutex)(&priv->lock);
> +
> +	ret = regmap_write(priv->regmap, priv->offset, T6000_CIO_CTRL_INIT_REQ(id));

[Severity: Medium]
Will this regmap_write() overwrite the state bits of all sibling ports?

Because the t600x variant uses a single shared register for all ports, 
writing the entire 32-bit register might write 0 to the INIT_REQ bits
of previously initialized ports. Should this use regmap_update_bits() 
instead to preserve active state?

> +	if (ret)
> +		return ret;
> +
> +	return regmap_read_poll_timeout(priv->regmap, priv->offset, val,
> +					!(val & T6000_CIO_CTRL_INIT_BUSY(id)),
> +					APPLE_CIO_RESET_POLL_US,
> +					APPLE_CIO_RESET_TIMEOUT_US);
> +}

[Severity: High]
Could this polling loop exit prematurely before the hardware begins 
initialization?

Since regmap_read_poll_timeout() (without sleep_before_read) performs 
its first read instantly, any hardware synchronization delay might cause 
the first read to see the INIT_BUSY bit as 0 (its pre-assertion state). 

This would cause the driver to report success before the hardware actually 
starts the reset process. Is it possible to ensure the BUSY bit is 
asserted first, or add a preliminary read delay?

[ ... ]

> +static int apple_cio_reset_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct apple_cio_reset *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->variant = of_device_get_match_data(dev);
> +
> +	ret = devm_mutex_init(dev, &priv->lock);
> +	if (ret)
> +		return ret;
> +
> +	if (priv->variant->pmgr_child) {

[Severity: Medium]
Can priv->variant be NULL here if the driver is bound manually via sysfs?

If a user writes "apple-cio-reset" to the driver's bind file without an
associated Device Tree node, of_device_get_match_data() will return NULL. 
The probe function then immediately dereferences it for pmgr_child, causing 
a kernel oops. Should we add a check for !priv->variant before continuing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2