Re: [PATCH v5 5/6] extcon: qcom-spmi-misc: match PM8xxx USB ID platform device

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

Pre-existing issues:
- [High] Unbalanced IRQ wake state and lost error codes in suspend/resume due to overwritten return value.
- [High] Failure to read ID IRQ line state aborts VBUS detection entirely.
--

--- Patch [5]: [PATCH v5 5/6] extcon: qcom-spmi-misc: match PM8xxx USB ID platform device ---
commit b4c56e1cb1867cf0c94f9ba05c18446ffacc8bd5
Author: Alexandre MINETTE <[email protected]>

extcon: qcom-spmi-misc: match PM8xxx USB ID platform device

The PM8xxx MFD registers the PM8921 USB ID detector as a platform device
named "qcom-pm8xxx-usb-id". Its reused firmware node remains compatible
with "qcom,pm8921", so it does not match the extcon driver's OF table.

Add a platform device ID so the existing extcon driver binds to this
device and consumes its named USB ID interrupt.

> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> index afaba5685c3dc..e16c109b553e2 100644
> --- a/drivers/extcon/extcon-qcom-spmi-misc.c
> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> @@ -204,8 +204,15 @@ static const struct of_device_id qcom_usb_extcon_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, qcom_usb_extcon_dt_match);
>  
> +static const struct platform_device_id qcom_usb_extcon_id[] = {
> +	{ "qcom-pm8xxx-usb-id" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, qcom_usb_extcon_id);
> +
>  static struct platform_driver qcom_usb_extcon_driver = {
>  	.probe		= qcom_usb_extcon_probe,
> +	.id_table	= qcom_usb_extcon_id,
>  	.driver		= {
>  		.name	= "extcon-pm8941-misc",
>  		.pm	= &qcom_usb_extcon_pm_ops,

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in the 
suspend/resume path that might cause unbalanced IRQ wake disables or leaked 
wake_depth refcounts:

drivers/extcon/extcon-qcom-spmi-misc.c:qcom_usb_extcon_suspend() {
    ...
	if (device_may_wakeup(dev)) {
		if (info->id_irq > 0)
			ret = enable_irq_wake(info->id_irq);
		if (info->vbus_irq > 0)
			ret = enable_irq_wake(info->vbus_irq);
	}

	return ret;
}

If enable_irq_wake(info->id_irq) fails, the error code is stored in ret.
However, if enable_irq_wake(info->vbus_irq) succeeds, it returns 0 and
overwrites the previous error code in ret.

If this happens, the system will suspend and eventually call
qcom_usb_extcon_resume(). In the resume path, disable_irq_wake(info->id_irq)
will be called on an IRQ that was never successfully enabled for wake, which
triggers a kernel WARN for an unbalanced IRQ wake disable. 

Conversely, if id_irq succeeds but vbus_irq fails, ret returns an error to
the PM core. The suspend is aborted, meaning qcom_usb_extcon_resume() is
skipped for this device. This prevents disable_irq_wake(info->id_irq) from
being called, permanently leaking the IRQ's wake_depth refcount.

Should the return codes for these two wake IRQs be handled independently?


[Severity: High]
This isn't a bug introduced by this patch, but I also noticed a potential
logic issue in how cable detection is handled:

drivers/extcon/extcon-qcom-spmi-misc.c:qcom_usb_extcon_detect_cable() {
    ...
	if (info->id_irq > 0) {
		/* check ID and update cable state */
		ret = irq_get_irqchip_state(info->id_irq,
				IRQCHIP_STATE_LINE_LEVEL, &state);
		if (ret)
			return;
    ...
}

If reading the ID IRQ line state fails (for instance, due to an SPMI bus
transaction error), the function returns immediately. 

Since the VBUS state detection happens after this in the same function,
returning early completely aborts the independent VBUS detection logic. This
means USB insertions and removals could be completely missed by the extcon
driver just because of a read failure on the ID pin line.

Would it be better to let the VBUS detection proceed even if the ID line
state read fails?

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