Re: [PATCH v2] usb: cdnsp: Add support for device-only configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The platform probe function silently swallows `-EPROBE_DEFER` for the optional OTG IRQ, permanently breaking DRD mode.
- [High] The error path for platform device registration in `cdnsp_pci_probe()` incorrectly frees a shared driver wrapper, causing a Use-After-Free.
- [High] Concurrent execution of `cdnsp_pci_probe` across both PCI functions leads to a NULL pointer dereference.
- [Medium] Concurrent execution of `cdnsp_pci_remove` across both PCI functions causes the shared driver data to be leaked.
- [Medium] PHY mode is left unconfigured when operating in peripheral-only mode.
- [Medium] Forcing IORESOURCE_MEM on an unpopulated Host BAR creates a conflicting, zero-length memory resource.
- [Low] `cdns_get_id()` returns the Host ID instead of the Peripheral ID for Device-only mode.
--

Note: The format of this report has been altered to provide a free-form summary
of the findings due to recitation restrictions. Direct patch quoting has been
omitted.

commit a05f3bd009fe3424669de67ea28711725bd2a6c9
Author: Pawel Laszczak <pawell@cadence.com>
Subject: usb: cdnsp: Add support for device-only configuration

This commit introduces support for operating the Cadence USBSSP controller
in a peripheral-only mode, bypassing the Dual-Role Device logic. It also
updates the BAR indexing for the peripheral-only configuration.

In cdns3_plat_probe(), the code fetches the optional OTG interrupt via
platform_get_irq_byname_optional(). If it returns -EPROBE_DEFER, the check
for negative values sets cdns->no_drd = 1 and continues in Device-only mode.

Does this break the probe deferral mechanism? On platforms where the IRQ
controller probes later, the controller would be permanently degraded to
peripheral-only mode instead of deferring and retrying.

Looking at cdnsp_pci_probe(), when the second PCI function probes, it fetches
the shared state wrapper via pci_get_drvdata(). If the platform device
registration fails, the error path unconditionally calls kfree() on the wrapper.

Can this lead to a use-after-free? The wrapper was allocated by the first
function and is actively stored in its driver data. Freeing it would corrupt
the first function's state, and when removed later, cdnsp_pci_remove() would
dereference the freed wrapper.

Also in cdnsp_pci_probe(), the driver relies on pci_is_enabled() to check if
the sibling function has already been probed. Since pcim_enable_device()
increments the enable counter early in the probe, concurrent probes could
both see pci_is_enabled() as true.

Could this cause a null pointer dereference? One thread might call
pci_get_drvdata() before the other has called pci_set_drvdata(),
receiving a null pointer that is immediately dereferenced when accessing
the wrapper's resources.

Similarly, in cdnsp_pci_remove(), the shared wrapper is conditionally freed
if the sibling function is not enabled. Since pcim_enable_device() uses
managed resources, the actual PCI disablement occurs after the remove
callback completes.

If both functions are unbound concurrently, would both threads see the
sibling as still enabled, causing them both to skip the kfree() and leak
the wrapper memory?

The updated cdns_drd_gadget_on() function returns early if no_drd is true.

Does this leave the PHY mode unconfigured? Skipping the rest of the function
also skips the phy_set_mode() calls for the USB2 and USB3 PHYs, leaving
them in an undefined state which might prevent the link from establishing.

In cdnsp_pci_probe(), the driver sets up the host resource using BAR 0 and
assigns the memory resource flag.

In a device-only configuration where the host controller is absent and
BAR 0 is unpopulated, pci_resource_start() and pci_resource_end() typically
return 0. Could passing a base-0 memory resource cause the platform core to
attempt claiming physical address 0 and abort the probe?

When the no_drd flag is true, cdns_get_id() is modified to return 0.

Since 0 corresponds to the host ID, does this incorrectly identify the
device-only controller as a host? Even if the role state machine handles
it later, returning the host ID for a peripheral-only setup seems
structurally flawed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505-device_only_branch_v2-v2-1-98cc19fbd02a@cadence.com?part=1