Re: [PATCH v9 2/2] usb: cdnsp: Add support for device-only configuration

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error path in `cdnsp_pci_probe` incorrectly frees the shared `wrap` structure, leading to a Use-After-Free and Double-Free when the sibling PCI function is subsequently removed.
- [High] Concurrent probing of the two PCI functions can cause a race condition leading to a NULL pointer dereference of the `wrap` object.
- [High] Missing cleanup of the registered USB role switch and hardware state in the `cdns3_plat_probe` error path.
- [High] Failure to call `phy_exit()` for `usb2_phy` when `usb3_phy` acquisition fails in `cdns3_plat_probe()`.
- [High] TOCTOU race in `cdns_role_start()` and `cdns_role_stop()` allows multiple concurrent starts or stops of a USB role.
- [High] Unbinding the first PCI function disables its MMIO resources while the registered platform device remains active, causing system crashes on subsequent accesses.
--

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

This patch adds support for the Cadence USBSSP (cdnsp) controller in hardware
configurations where the Dual-Role Device (DRD) register block is missing.
It allows the driver to operate exclusively in a fixed peripheral or host
mode without relying on DRD logic. It also adjusts BAR indexing for 32-bit
addressing used in this layout.

Link: Closes: https://lore.kernel.org/oe-kbuild-all/202605141023.18vWXyw3-lkp@intel.com/

> diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> index 3fe3109a3688a..0d29c3cf58f11 100644
> --- a/drivers/usb/cdns3/cdns3-plat.c
> +++ b/drivers/usb/cdns3/cdns3-plat.c

[ ... ]

> @@ -113,21 +119,22 @@ static int cdns3_plat_probe(struct platform_device *pdev)
>  
>  	cdns->dev_regs	= regs;
>  
> -	cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
> -	if (cdns->otg_irq < 0)
> -		return dev_err_probe(dev, cdns->otg_irq,
> -				     "Failed to get otg IRQ\n");
> -
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
> -	if (!res) {
> -		dev_err(dev, "couldn't get otg resource\n");
> -		return -ENXIO;
> +	if (!cdns->no_drd) {
> +		cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
> +		if (cdns->otg_irq < 0)
> +			return dev_err_probe(dev, cdns->otg_irq,
> +					     "Failed to get otg IRQ\n");
> +
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
> +		if (!res) {
> +			dev_err(dev, "couldn't get otg resource\n");
> +			return -ENXIO;
> +		}
> +		cdns->otg_res = *res;
>  	}
>  
>  	cdns->phyrst_a_enable = device_property_read_bool(dev, "cdns,phyrst-a-enable");
>  
> -	cdns->otg_res = *res;
> -
>  	cdns->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
>  	if (cdns->wakeup_irq == -EPROBE_DEFER)
>  		return cdns->wakeup_irq;

[Severity: High]
This is a pre-existing issue, but further down in cdns3_plat_probe, when
acquiring usb3_phy, if devm_phy_optional_get() fails, it returns directly
via dev_err_probe(). 

Does this leak the initialized state of the usb2_phy since it bypasses the
err_phy3_init label where phy_exit() is called?

[Severity: High]
This is a pre-existing issue, but also further down in cdns3_plat_probe, if
cdns_core_init_role(cdns) fails, the error path jumps to err_cdns_init.

Does this error path miss unregistering the USB role switch? If cdns_init()
allocates and registers a non-devres managed role switch, bypassing the
cleanup could leave dangling sysfs nodes and cause issues on reprobe.

> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> index 432007cfe6954..c38a3dc7111a7 100644
> --- a/drivers/usb/cdns3/cdnsp-pci.c
> +++ b/drivers/usb/cdns3/cdnsp-pci.c

[ ... ]

> @@ -93,7 +104,7 @@ static int cdnsp_pci_probe(struct pci_dev *pdev,
>  
>  	pci_set_master(pdev);
>  
> -	if (pci_is_enabled(func)) {
> +	if (func && pci_is_enabled(func)) {
>  		wrap = pci_get_drvdata(func);
>  	} else {
>  		wrap = kzalloc_obj(*wrap);

[Severity: High]
This is a pre-existing issue, but could this race if both PCI functions are
probed concurrently? 

If both call pcim_enable_device(pdev), both would increment their enable
counts. Both would then see pci_is_enabled(func) as true, attempt to get
the wrap structure via pci_get_drvdata(func), and receive a NULL pointer
since pci_set_drvdata() hasn't been called yet. Could this lead to a NULL
pointer dereference when wrap->dev_res is accessed?

[ ... ]

> @@ -145,9 +158,21 @@ static int cdnsp_pci_probe(struct pci_dev *pdev,
>  		wrap->dev_res[RES_IRQ_OTG_ID].flags = IORESOURCE_IRQ;
>  	}
>  
> -	if (pci_is_enabled(func)) {
> +	if (no_drd || pci_is_enabled(func)) {
> +		u8 idx = 0;
> +
>  		/* set up platform device info */
>  		pdata.override_apb_timeout = CHICKEN_APB_TIMEOUT_VALUE;
> +
> +		if (no_drd) {
> +			wrap->prop[idx++] = PROPERTY_ENTRY_STRING("compatible",
> +								  "cdns,cdnsp");
> +			wrap->prop[idx++] = PROPERTY_ENTRY_STRING("dr_mode", "peripheral");
> +		} else {
> +			wrap->prop[idx++] = PROPERTY_ENTRY_STRING("dr_mode", "otg");
> +			wrap->prop[idx++] = PROPERTY_ENTRY_BOOL("usb-role-switch");
> +		}
> +
>  		memset(&plat_info, 0, sizeof(plat_info));
>  		plat_info.parent = &pdev->dev;
>  		plat_info.fwnode = pdev->dev.fwnode;
> @@ -158,6 +183,7 @@ static int cdnsp_pci_probe(struct pci_dev *pdev,
>  		plat_info.dma_mask = pdev->dma_mask;
>  		plat_info.data = &pdata;
>  		plat_info.size_data = sizeof(pdata);
> +		plat_info.properties = wrap->prop;
>  		wrap->devfn = pdev->devfn;
>  		/* register platform device */
>  		wrap->plat_dev = platform_device_register_full(&plat_info);

[Severity: High]
This is a pre-existing issue, but is it safe to unconditionally call
kfree(wrap) in the error path of platform_device_register_full()?

If the sibling function has already stored wrap in its drvdata, this might
leave a dangling pointer attached to the sibling function. When the sibling
is later unbound, could its remove routine retrieve the dangling pointer
and double-free it?

> @@ -185,13 +211,17 @@ static void cdnsp_pci_remove(struct pci_dev *pdev)
>  	if (wrap->devfn == pdev->devfn)
>  		platform_device_unregister(wrap->plat_dev);
>  
> -	if (!pci_is_enabled(func))
> +	if (!func || !pci_is_enabled(func))
>  		kfree(wrap);
>  
>  	pci_dev_put(func);
>  }

[Severity: High]
This is a pre-existing issue, but could this lead to a crash if the user
unbinds the first PCI function?

If wrap->devfn points to the second function, unbinding the first function
would skip unregistering the platform device. The PCI core would then
disable the first function and unmap its BARs. Since the platform device
remains active, would subsequent MMIO accesses to the unmapped regions
trigger a PCI master abort or page fault?

> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 6a8d1fefbc0d0..504bdf13ea804 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -70,7 +70,8 @@ static void cdns_role_stop(struct cdns *cdns)
>  static void cdns_exit_roles(struct cdns *cdns)
>  {
>  	cdns_role_stop(cdns);
> -	cdns_drd_exit(cdns);
> +	if (!cdns->no_drd)
> +		cdns_drd_exit(cdns);
>  }

[Severity: High]
This is a pre-existing issue, but does cdns_role_stop() have a race condition
if multiple threads attempt to start or stop a role concurrently? 

Since cdns_role_stop() checks cdns->roles[role]->state outside of the
cdns->mutex, two threads could observe the state as active, acquire the
lock sequentially, and invoke the stop() callback twice. A similar risk
exists in cdns_role_start().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-no_drd_config_v9-v9-0-2512cef10104@cadence.com?part=2