Re: [PATCH] dt-bindings: usb: xlnx,usb2: Make clocks required

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Making `clocks` and `clock-names` unconditionally required breaks backward compatibility for older device trees.

Pre-existing issues:
- [High] devm_request_irq is called before read_fn, write_fn, and spinlock are initialized, risking a NULL pointer dereference.
- [High] Device clock is disabled before the devm-managed IRQ is freed, causing a potential bus fault if an interrupt fires.
--

--- Patch [1]: [PATCH] dt-bindings: usb: xlnx,usb2: Make clocks required ---
commit e81394ee7d6f7bd713fd2fe198723a128037fcee
Author: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>

dt-bindings: usb: xlnx,usb2: Make clocks required

Add s_axi_aclk AXI4 clock support as a required property. Traditionally this IP
was used on MicroBlaze platforms which had fixed clocks enabled all the time.
But since it is a PL IP, it can also be used on SoC platforms like Zynq
UltraScale+ MPSoC where it is mandatory to explicitly enable IP clocks.

> diff --git a/Documentation/devicetree/bindings/usb/xlnx,usb2.yaml b/Documentation/devicetree/bindings/usb/xlnx,usb2.yaml
> index f295aa9d9ee79..156d3676f9f99 100644
> --- a/Documentation/devicetree/bindings/usb/xlnx,usb2.yaml
> +++ b/Documentation/devicetree/bindings/usb/xlnx,usb2.yaml
> @@ -34,6 +34,8 @@ required:
>    - compatible
>    - reg
>    - interrupts
> +  - clocks
> +  - clock-names

[Severity: Medium]
Will this break backward compatibility for older device trees? The commit
message mentions this IP was traditionally used on MicroBlaze platforms which
had fixed clocks enabled and likely omitted these properties. Making them
unconditionally required will cause existing, valid legacy device trees to fail
schema validation.

>  
>  additionalProperties: false

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a risk of a null pointer dereference
during probe?

In drivers/usb/gadget/udc/udc-xilinx.c:xudc_probe(), devm_request_irq() is
called before read_fn, write_fn, and the spinlock are initialized:

drivers/usb/gadget/udc/udc-xilinx.c:xudc_probe() {
    ...
    ret = devm_request_irq(&pdev->dev, irq, xudc_irq, 0,
                           dev_name(&pdev->dev), udc);
    ...
    /* initialized later */
    udc->read_fn = ...
    udc->write_fn = ...
}

If an interrupt fires immediately after devm_request_irq() returns, the
xudc_irq() handler will execute and attempt to use the uninitialized spinlock
and udc->read_fn(), potentially crashing the kernel.


[Severity: High]
This is a pre-existing issue, but does disabling the clock before the interrupt
is freed introduce a potential bus fault?

In drivers/usb/gadget/udc/udc-xilinx.c:xudc_remove() and the probe error path,
clk_disable_unprepare() is called on the device clock:

drivers/usb/gadget/udc/udc-xilinx.c:xudc_remove() {
    ...
    clk_disable_unprepare(udc->clk);
    ...
}

Because the interrupt is managed by devm_request_irq(), it remains active until
the devres cleanup runs after the function returns. If a late interrupt triggers
in this window, xudc_irq() will attempt to access hardware registers with the
clock disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810154548.2133565-1-radhey.shyam.pandey@amd.com?part=1