Re: [PATCH v3 5/8] extcon: qcom-spmi-misc: Add PM8921 compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver incorrectly handles negative error codes from platform_get_irq_byname_optional(), breaking -EPROBE_DEFER for the IRQs.
- [High] An early return during ID IRQ state reading causes VBUS state changes to be permanently lost.
--

commit 6e0b86ed0580b2ce5f0ed399603c0e9bc321f55d
Author: Alexandre MINETTE <contact@alex-min.fr>

extcon: qcom-spmi-misc: Add PM8921 compatible

This commit adds the PM8921 compatible to the qcom-spmi-misc extcon driver.
It allows APQ8064 boards to describe the PM8921 USB ID interrupt as an
extcon provider for USB role detection.

> diff --git a/drivers/extcon/extcon-qcom-spmi-misc.c b/drivers/extcon/extcon-qcom-spmi-misc.c
> index afaba5685c3dc..6811d3383254f 100644
> --- a/drivers/extcon/extcon-qcom-spmi-misc.c
> +++ b/drivers/extcon/extcon-qcom-spmi-misc.c
> @@ -199,6 +199,7 @@ static SIMPLE_DEV_PM_OPS(qcom_usb_extcon_pm_ops,
>  			 qcom_usb_extcon_suspend, qcom_usb_extcon_resume);
>  
>  static const struct of_device_id qcom_usb_extcon_dt_match[] = {
> +	{ .compatible = "qcom,pm8921-misc", },

While this isn't a bug introduced by this patch, there are some pre-existing
issues in the driver that might affect platforms using this compatible.

Does the driver correctly handle -EPROBE_DEFER for the IRQs?

drivers/extcon/extcon-qcom-spmi-misc.c:qcom_usb_extcon_probe() {
    ...
	info->id_irq = platform_get_irq_byname_optional(pdev, "usb_id");
	if (info->id_irq > 0) {
    ...
	info->vbus_irq = platform_get_irq_byname_optional(pdev, "usb_vbus");
	if (info->vbus_irq > 0) {
    ...
	if (info->id_irq < 0 && info->vbus_irq < 0) {
		dev_err(dev, "ID and VBUS IRQ not found\n");
		return -EINVAL;
	}
    ...
}

When platform_get_irq_byname_optional() returns -EPROBE_DEFER, the checks
if (info->id_irq > 0) and if (info->vbus_irq > 0) fail, bypassing the IRQ
requests.

If both IRQs return -EPROBE_DEFER, or if one is missing and the other defers,
if (info->id_irq < 0 && info->vbus_irq < 0) evaluates to true. The driver
logs an error and returns -EINVAL. This converts a deferred probe into a
permanent failure, preventing the driver from ever successfully probing.

If one IRQ is successfully found (> 0) and the other returns -EPROBE_DEFER,
the probe succeeds and returns 0. The deferred IRQ is silently dropped and
the driver loads with permanently broken hardware capability.


Can an early return during ID IRQ state reading cause VBUS state changes to be
permanently lost?

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 (info->vbus_irq > 0) {
    ...
}

In the qcom_usb_extcon_detect_cable() workqueue handler, the driver checks
both ID and VBUS states sequentially.

It first reads the ID IRQ line level via irq_get_irqchip_state(). If this
call fails, the handler executes if (ret) return; and exits immediately.

This completely skips the subsequent block that checks info->vbus_irq.

If the handler was scheduled to process a VBUS interrupt, and the ID IRQ read
fails, the VBUS state change will be dropped entirely. Since edge-triggered
interrupts are used, the extcon state will remain permanently out of sync
until the next physical cable plug/unplug event.

>  	{ .compatible = "qcom,pm8941-misc", },
>  	{ }
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-mainline-send-v1-sending-v3-0-3dd7aa125353@alex-min.fr?part=5