Re: [PATCH 2/2] usb: typec: hd3ss3220: Add support for supply regulators

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

Pre-existing issues:
- [High] The device interrupt `client->irq` is managed via `devm_request_threaded_irq()` and can fire while manual cleanup is performed in `hd3ss3220_remove()` and the `probe()` error paths, leading to a use-after-free.
--

--- Patch [2]: [PATCH 2/2] usb: typec: hd3ss3220: Add support for supply regulators ---
commit 1452abf55fe5c07be0afe4c56142e011df996897
Author: Alexey Charkov <[email protected]>

usb: typec: hd3ss3220: Add support for supply regulators

This commit adds support for supply regulators for the HD3SS3220. It ensures
VDD5 is enabled 2ms before VCC33 is applied to prevent the device from being
backpowered and wedging the I2C bus.

> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index d0de5a2488f95..b53455abc0480 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c

[ ... ]

> @@ -379,6 +407,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
>  	if (IS_ERR(hd3ss3220->regmap))
>  		return PTR_ERR(hd3ss3220->regmap);
>  
> +	ret = hd3ss3220_power_up(hd3ss3220->dev);
> +	if (ret)
> +		return ret;
> +
>  	/* For backward compatibility check the connector child node first */
>  	connector = device_get_named_child_node(hd3ss3220->dev, "connector");
>  	if (connector) {

[Severity: High]
This is a pre-existing issue, but could the manual resource cleanup in the
error paths and remove function cause a use-after-free?

The device interrupt client->irq is managed via devm_request_threaded_irq().
Devres resources are destroyed by the core after the remove callback returns,
or after probe returns an error.

However, typec_unregister_port() and usb_role_switch_put() are called
manually inside hd3ss3220_remove() and the probe error path before the
interrupt is disabled by devres:

drivers/usb/typec/hd3ss3220.c:hd3ss3220_probe() {
    ...
err_unreg_port:
    typec_unregister_port(hd3ss3220->port);
err_put_role:
    usb_role_switch_put(hd3ss3220->role_sw);
    ...
}

If an interrupt fires concurrently during this window,
hd3ss3220_irq_handler() will call hd3ss3220_set_role(), which dereferences
hd3ss3220->port and hd3ss3220->role_sw after they have been freed.

Does the interrupt need to be explicitly disabled, or should the port and role
switch cleanup be handled via devm_add_action_or_reset() to ensure correct
ordering?

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