Re: [PATCH 3/5] dt-bindings: phy: motorola,cpcap-usb: replace se1 interrupt with chrg_det

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Device Tree ABI breakage: replacing 'se1' interrupt with 'chrg_det' breaks probe on older DTBs.
- [Medium] Global SysRq state is permanently disabled on error paths.

Pre-existing issues:
- [High] Race condition in driver removal allows the threaded IRQ handler to schedule work on freed memory (Use-After-Free).
--

commit d7b2a51c3f82ebdb2f326313f13ddf2d3edde1a7
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

dt-bindings: phy: motorola,cpcap-usb: replace se1 interrupt with chrg_det

The CPCAP USB PHY driver uses the CPCAP charger detection interrupt for DCP
detection rather than the SE1 interrupt. This commit updates the binding and
example DTS to use the corresponding "chrg_det" interrupt name.

> diff --git a/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml b/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
> index dd345cbd0a0b4..d54cf7fbf11a7 100644
> --- a/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
> @@ -27,7 +27,7 @@ properties:
>        - description: vbusvld interrupt
>        - description: sessvld interrupt
>        - description: sessend interrupt
> -      - description: se1 interrupt
> +      - description: charger detection interrupt
>        - description: dm interrupt
>        - description: dp interrupt
>  
> @@ -40,7 +40,7 @@ properties:
>        - const: vbusvld
>        - const: sessvld
>        - const: sessend
> -      - const: se1
> +      - const: chrg_det

[Severity: Critical]
Does replacing 'se1' with 'chrg_det' break probe for older device trees?

Since cpcap_usb_init_irq() in drivers/phy/motorola/phy-cpcap-usb.c fails the
probe if an expected interrupt is not found, updating the binding to require
'chrg_det' instead of 'se1' means booting a new kernel with an older DTB
(which only provides 'se1') could fail to probe the USB PHY.

>        - const: dm
>        - const: dp
>  

[ ... ]

While reviewing this, I noticed a few other regressions in
drivers/phy/motorola/phy-cpcap-usb.c:

[Severity: Medium]
Can this error path in cpcap_usb_set_idle_mode() leave the global SysRq state
permanently disabled?

drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_set_idle_mode() {
    ...
    sysrq = sysrq_mask();
    if (sysrq & 1)
        sysrq_toggle_support(sysrq & ~1);
    ...
out_err:
    dev_err(ddata->dev, "%s failed with %i\n", __func__, error);

    return error;
}

If an error occurs (e.g., during pinctrl_select_state), the function jumps to
out_err and returns without calling sysrq_toggle_support(sysrq) to restore it.


[Severity: High]
This is a pre-existing issue, but could there be a race condition during
driver removal that allows the threaded IRQ handler to schedule work on freed
memory?

During driver removal, cpcap_usb_phy_remove() cancels the work without
disabling the IRQ first:

drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_phy_remove() {
    ...
    atomic_set(&ddata->active, 0);
    ...
    cancel_delayed_work_sync(&ddata->detect_work);
    regulator_disable(ddata->vusb);
}

Concurrently, if an IRQ fires and cpcap_phy_irq_thread() checks 'active' but
is preempted before scheduling the work:

drivers/phy/motorola/phy-cpcap-usb.c:cpcap_phy_irq_thread() {
    ...
    if (!atomic_read(&ddata->active))
        return IRQ_NONE;

    schedule_delayed_work(&ddata->detect_work, msecs_to_jiffies(1));
    ...
}

Could the IRQ thread resume after cancel_delayed_work_sync() has completed,
scheduling the delayed work on a freed 'ddata' object?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com?part=3