Re: [PATCH 3/6] usb: typec: add Qualcomm PMI8998 role-switch driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Race condition and Use-After-Free during driver removal due to mixed devm and manual cleanup.
- [Medium] VBUS regulator enable count leak on probe error path.
- [Medium] Silently ignored return value of hardware initialization in probe().
--

commit 2be57d2a22cd876c2f33d5adcda09af34cd9c497
Author: taygoth <taygoth@gmail.com>

usb: typec: add Qualcomm PMI8998 role-switch driver

This commit introduces a new USB Type-C role-switch driver for the Qualcomm
PMI8998 PMIC. It translates the hardware-detected role into a consumer
switch state, and enables an optional VBUS supply regulator when acting as
a USB host.

> diff --git a/drivers/usb/typec/qcom_pmi8998_typec.c b/drivers/usb/typec/qcom_pmi8998_typec.c
> new file mode 100644
> index 0000000000000..77662b4f1b0c6
> --- /dev/null
> +++ b/drivers/usb/typec/qcom_pmi8998_typec.c

[ ... ]

> +static int pmi8998_typec_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	platform_set_drvdata(pdev, typec);
> +
> +	pmi8998_typec_apply(typec);

Should the return value of pmi8998_typec_apply() be checked here? 

If operations like regmap_read() or regulator_enable() fail inside apply(),
the probe will silently succeed with an unsynchronized hardware state.

> +
> +	ret = devm_request_threaded_irq(dev, typec->irq, NULL,
> +					pmi8998_typec_isr, IRQF_ONESHOT,
> +					dev_name(dev), typec);
> +	if (ret)
> +		goto err_role_put;
> +
> +	return 0;
> +
> +err_role_put:
> +	usb_role_switch_put(typec->role_sw);

If pmi8998_typec_apply() above evaluates to USB_ROLE_HOST, it will enable
the vbus regulator and set typec->vbus_enabled to true. 

If the interrupt request then fails, does this error path leak the regulator
enable count by missing a regulator_disable() call?

> +	return ret;
> +}
> +
> +static void pmi8998_typec_remove(struct platform_device *pdev)
> +{
> +	struct pmi8998_typec *typec = platform_get_drvdata(pdev);
> +
> +	if (typec->vbus_enabled)
> +		regulator_disable(typec->vbus);
> +	usb_role_switch_put(typec->role_sw);

Because the interrupt was requested using devm_request_threaded_irq(), it
will not be freed until after pmi8998_typec_remove() returns.

If a hardware interrupt fires right after we disable the regulator and
put the role switch:

pmi8998_typec_remove()
    regulator_disable(typec->vbus);
    usb_role_switch_put(typec->role_sw);

-> interrupt fires
pmi8998_typec_isr()
    pmi8998_typec_apply()

Could the ISR attempt to use the freed typec->role_sw or perform an
unbalanced regulator_disable() since typec->vbus_enabled is never set
to false in remove()?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779127507.git.taygoth@gmail.com?part=3