Re: [PATCH v7 2/2] usb: cdns3: Add CIX Sky1 glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [High] usb: cdns3: incomplete teardown before clock disable in
  cdnsp_sky1_shutdown()
- [High] usb: cdns3: unrecovered OTG state after reset in
  cdnsp_sky1_system_resume()
- [High] usb: cdns3: invalid kfree() of devres cdns_sky1_pdata on child
  release
- [Medium] usb: cdns3: broken D0 poll and error return in
  cdns_sky1_platform_suspend()
- [Medium] usb: cdns3: inconsistent wakeup checks across suspend and
  shutdown in cdnsp-sky1
- [Medium] usb: cdns3: unconditional data->wakeup overwrite in
  cdns_sky1_platform_suspend()
--

commit a740c36e5f7b737668f9904cdc187603aff7c8f4
Author: Hongliang Yang <hongliang.yang@cixtech.com>

usb: cdns3: Add CIX Sky1 glue driver

This commit adds a platform glue driver for the Cadence USBSSP dual-role
controller on the CIX Sky1 SoC, managing clocks, resets, RCSU AXI settings,
mode straps, and power management.

> diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> new file mode 100644
> index 0000000000000..65c2c692d2a35
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.c

[ ... ]

> +static int cdns_sky1_platform_suspend(struct device *dev,
> +				      bool suspend, bool wakeup)
> +{
> +	struct cdns *cdns = dev_get_drvdata(dev);
> +	struct platform_device *xhci_dev = cdns->host_dev;
> +	struct usb_hcd  *hcd;
> +	struct device *parent = cdns->dev->parent;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(parent);
> +	u32 value;
> +	int poll;
> +	int ret = 0;
> +	int count = 3;
> +
> +	data->wakeup = wakeup;

[Severity: Medium]
Can overwriting data->wakeup on every call cause cdnsp_sky1_system_suspend()
to reset an active controller?

cdns3_controller_resume() calls platform_suspend(dev, false, false), which
clears data->wakeup.  During system suspend with wakeup enabled on the child
device, runtime PM remains enabled until the suspend_late phase:

CPU0                              CPU1
cdns3_plat_suspend()
  cdns3_controller_suspend()
    cdns_sky1_platform_suspend(..., true, true)
      data->wakeup = true;
    cdns->in_lpm = true;
                                  cdns_wakeup_irq()
                                    pm_request_resume(...)
                                    cdns3_controller_resume()
                                      cdns_sky1_platform_suspend(..., false, false)
                                        data->wakeup = false;
                                      cdns->in_lpm = false;
cdnsp_sky1_system_suspend()
  if (!data->wakeup)
    reset_control_assert(...)     <-- resets active controller

On the subsequent system resume, cdns3_controller_resume() sees
!cdns->in_lpm and returns early without reinitializing the controller.

> +
> +	if (cdns->role != USB_ROLE_HOST)
> +		return 0;
> +
> +	if (!xhci_dev) {
> +		dev_dbg(dev, "host controller has not been registered\n");
> +		return 0;
> +	}
> +
> +	hcd = dev_get_drvdata(&xhci_dev->dev);
> +	if (!hcd) {
> +		dev_dbg(dev, "host controller have not registered\n");
> +		return 0;
> +	}
> +
> +	if (suspend) {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D3 | PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);
> +			/* After controller enters D3, disable AXI and SOF
> +			 * until AXI valid flag changes to 0.
> +			 */
> +			if (sky1_rcsu_handshake(data, AXI_CLOCK_VALID, 0,
> +						100ULL * 1000)) {
> +				u32 ctst;
> +
> +				ctst = sky1_rcsu_read_status(data);
> +				dev_dbg(dev, "enter D3 failed, register value:%x\n",
> +					ctst);
> +			} else {
> +				break;
> +			}
> +		}
> +		if (count < 0) {
> +			u32 ctst;
> +
> +			ctst = sky1_rcsu_read_status(data);
> +			dev_err(dev, "enter D3 failed after retries, register value:%x\n",
> +				ctst);
> +		}
> +	} else {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D0;
> +			value &= ~PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);
> +			/* Wait power state back to D0 */
> +			poll = readl_poll_timeout(hcd->regs + XECP_PM_PMCSR,
> +						  value,
> +						  (value & PS_MASK) == 0 ||
> +						  value == U32_MAX,
> +						  10, 100ULL * 1000);
> +			if (poll) {
> +				dev_dbg(dev, "exit D3 timeout, power state=0x%lx\n",
> +					value & PS_MASK);
> +			} else {
> +				break;
> +			}
> +		}
> +		if (count < 0) {
> +			dev_err(dev, "exit D3 timeout after retries, power state=0x%lx\n",
> +				value & PS_MASK);
> +		}
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
Does the D0 polling loop treat value == U32_MAX as a successful transition,
and does cdns_sky1_platform_suspend() always return 0 even when all retries
fail?

In the resume path, readl_poll_timeout() returns 0 whenever its condition
holds, so reading U32_MAX breaks out of the retry loop without checking for
U32_MAX (unlike sky1_rcsu_handshake()).  Also, ret is never updated when
count < 0 in either the suspend or resume branch, and the suspend branch
sets PS_PME_En unconditionally even when wakeup is false.

[ ... ]

> +	/*
> +	 * The platform data is devm-allocated, so it stays alive as long as
> +	 * this device; the child devices are depopulated before it is freed.
> +	 */
> +	cdns_sky1_pdata = devm_kzalloc(dev, sizeof(*cdns_sky1_pdata), GFP_KERNEL);
> +	if (!cdns_sky1_pdata) {
> +		ret = -ENOMEM;
> +		goto err_quiesce;
> +	}
> +	cdns_sky1_pdata->platform_suspend = cdns_sky1_platform_suspend;
> +	cdns_sky1_pdata->quirks = CDNS3_DEFAULT_PM_RUNTIME_ALLOW;
> +	cdns_sky1_auxdata[0].platform_data = cdns_sky1_pdata;
> +	ret = of_platform_populate(node, NULL, cdns_sky1_auxdata, dev);

[Severity: High]
Does passing devm-allocated cdns_sky1_pdata via of_dev_auxdata cause an
invalid kfree() and double free when the child platform device is released?

of_platform_populate() passes auxdata->platform_data to
of_platform_device_create_pdata(), which assigns pdev->dev.platform_data
directly on a platform_device allocated with platform_device_alloc().  When
of_platform_depopulate() unregisters that child in cdnsp_sky1_remove(),
platform_device_release() frees platform_data with kfree():

drivers/base/platform.c:platform_device_release() {
	struct platform_object *pa = container_of(dev, struct platform_object,
						  pdev.dev);

	of_node_put(pa->pdev.dev.of_node);
	kfree(pa->pdev.dev.platform_data);
	kfree(pa->pdev.mfd_cell);
	kfree(pa->pdev.resource);
	kfree(pa->pdev.driver_override);
	kfree(pa);
}

Because devm_kzalloc() returns an interior pointer (dr->data) inside struct
devres, kfree(pa->pdev.dev.platform_data) corrupts the slab allocator and
devres later frees the enclosing allocation a second time.  Keeping struct
cdns3_platform_data as a field inside struct cdnsp_sky1 breaks only if
platform_device_release() still calls kfree() on it, so platform_data passed
through of_dev_auxdata cannot be devres-managed or embedded unless
pdev->dev.platform_data is cleared before put_device() or allocated with
kzalloc().

[ ... ]

> +static int cdnsp_sky1_system_suspend(struct device *dev)
> +{
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	if (!data->wakeup) {
> +		reset_control_assert(data->reset);
> +		reset_control_assert(data->preset);
> +	}
> +	cdnsp_sky1_clk_disable_suspend(dev);
> +	return 0;
> +}
> +
> +static int cdnsp_sky1_system_resume(struct device *dev)
> +{
> +	int ret = 0;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	ret = cdnsp_sky1_clk_enable_resume(dev);
> +	if (ret)
> +		return ret;
> +	if (!data->wakeup) {
> +		writel(CIX_USB_AXI_WR_CACHE_VALUE, data->rcsu_base);
> +		cdnsp_sky1_set_mode(data->dev, MODE_STRAP_OTG);
> +		reset_control_deassert(data->preset);
> +		cdnsp_sky1_configure_controller(data);
> +		reset_control_deassert(data->reset);
> +	}
> +	return 0;
> +}

[Severity: High]
When wakeup is disabled, does asserting data->reset and data->preset across
system suspend leave the child cdns3 OTG registers uninitialized on resume?

Resetting the controller clears cdns->otg_cdnsp_regs->override
(OVERRIDE_IDPULLUP) and cdns->otg_irq_regs->ien, which are only programmed
during probe in cdns_drd_init() -> cdns_drd_update_mode().  When the child
device resumes afterwards in cdns3_controller_resume(), it first calls
cdns_sky1_platform_suspend(dev, false, false) to poll XECP_PM_PMCSR in
hcd->regs before cdns_resume() has re-enabled the host bus in the OTG block,
and cdns_resume() never re-runs cdns_drd_update_mode(), leaving ID detection
and OTG interrupts disabled after resume.

[ ... ]

> +static void cdnsp_sky1_shutdown(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct device *cdns_dev = NULL;
> +	struct device *gadget_dev = NULL;
> +	struct cdns *cdns = NULL;
> +
> +	if (!device_may_wakeup(dev)) {

[Severity: Medium]
Why does cdnsp_sky1_shutdown() check device_may_wakeup(dev) on the parent
glue device while cdnsp_sky1_system_suspend() checks data->wakeup from the
child cdns-usb3 device?

Both cdnsp_sky1_probe() and cdns3_plat_probe() call
device_set_wakeup_capable(dev, true) on their respective devices.  If
userspace enables wakeup on the child cdns-usb3 device (which owns the
wakeup IRQ), device_may_wakeup(dev) on the parent glue device remains false,
so cdnsp_sky1_shutdown() still asserts resets and disables all clocks.

> +		/*
> +		 * Find the cdns3 child device, then find its gadget device
> +		 * and release the function driver before disabling clocks.
> +		 * This ensures all register accesses in gadget_unbind_driver
> +		 * complete before clocks are turned off.
> +		 */
> +		device_for_each_child(dev, &cdns_dev, cdnsp_sky1_find_cdns_dev);
> +		if (cdns_dev)
> +			cdns = dev_get_drvdata(cdns_dev);
> +		if (cdns)
> +			device_for_each_child(cdns_dev, &gadget_dev,
> +					      cdnsp_sky1_find_gadget_match);
> +		if (gadget_dev)
> +			device_release_driver(gadget_dev);
> +		if (cdns && cdns->host_dev) {
> +			struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);
> +
> +			if (hcd && hcd->irq > 0) {
> +				/*
> +				 * Clear HCD_FLAG_HW_ACCESSIBLE before disable_irq.
> +				 * This prevents usb_hcd_irq from calling xhci_irq
> +				 * (which reads USBSTS) after clocks are disabled.
> +				 */
> +				clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +				disable_irq(hcd->irq);
> +				synchronize_irq(hcd->irq);
> +			}
> +		}
> +		reset_control_assert(data->reset);
> +		reset_control_assert(data->preset);
> +		cdnsp_sky1_clk_disable_all(dev);
> +		put_device(gadget_dev);
> +		put_device(cdns_dev);
> +	}
> +}

[Severity: High]
Can asserting resets and disabling all clocks in cdnsp_sky1_shutdown()
while child interrupts and role switches are still active cause an unclocked
MMIO access or use-after-free?

Unlike cdnsp_sky1_remove(), which calls pm_runtime_get_sync(dev) and
of_platform_depopulate(dev) before asserting resets and disabling clocks,
cdnsp_sky1_shutdown() leaves the child cdns3 device bound:

- cdns_dev may be runtime-suspended (CDNS3_DEFAULT_PM_RUNTIME_ALLOW is set)
  when device_release_driver(gadget_dev) accesses controller registers in
  cdnsp_gadget_pullup() and cdnsp_gadget_udc_stop().
- device_release_driver(gadget_dev) only unbinds the gadget function driver;
  cdns->dev_irq, cdns->otg_irq, and cdns->wakeup_irq remain enabled and
  registered, so any in-flight or subsequent interrupt after
  reset_control_assert() and cdnsp_sky1_clk_disable_all() reads unclocked,
  reset-asserted MMIO registers.
- cdns->host_dev is read without holding cdns->mutex while
  cdns_drd_thread_irq() can concurrently run cdns_hw_role_switch() ->
  cdns_host_exit() and unregister cdns->host_dev, and only the primary
  hcd->flags has HCD_FLAG_HW_ACCESSIBLE cleared while xhci->shared_hcd
  remains marked accessible.

Could cdnsp_sky1_shutdown() instead wake the device with
pm_runtime_get_sync(dev) and call of_platform_depopulate(dev) like
cdnsp_sky1_remove() does?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106101210.1059256-1-hongliang.yang@cixtech.com?part=2