Re: [PATCH 1/4] usb: cdns3: plat: Expose platform core driver as library

On 26-05-11 23:02:32, [email protected] wrote:
> EXTERNAL EMAIL
> 
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Leaked usb2_phy initialization on usb3_phy acquisition failure.
> - [High] Leaked role switch and DRD resources on core role initialization failure.
> - [High] Unbalanced pm_runtime_forbid() call permanently leaks the runtime PM usage counter across bind/unbind cycles.
> - [High] Hardware PHY is powered off without calling synchronize_irq() during runtime suspend.
> - [Medium] Exported PM helpers in glue.h lack static inline stubs for disabled PM configurations.
> --
> 
> commit a327903170c6c0082aaebdb421558ca806f906da
> Author: Peter Chen <[email protected]>
> 
> usb: cdns3: plat: Expose platform core driver as library
> 
> This commit splits the Cadence USB3 platform probe, remove, and PM paths into
> exported core functions. It also adds a new header glue.h to document the
> probe data structure and public entry points, allowing SoC glue code to
> instantiate the core deterministically.
> 
> > diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> > index 3fe3109a3688a..2219cbff1c592 100644
> > --- a/drivers/usb/cdns3/cdns3-plat.c
> > +++ b/drivers/usb/cdns3/cdns3-plat.c
> [ ... ]
> > @@ -59,29 +60,21 @@ static int cdns3_plat_host_init(struct cdns *cdns)
> >  }
> >
> >  /**
> > - * cdns3_plat_probe - probe for cdns3 core device
> > - * @pdev: Pointer to cdns3 core platform device
> > + * cdns3_core_probe - Initialize the Cadence USB3 platform core
> > + * @data: Controller context and platform device supplied by the glue layer
> >   *
> >   * Returns 0 on success otherwise negative errno
> >   */
> > -static int cdns3_plat_probe(struct platform_device *pdev)
> > +int cdns3_core_probe(const struct cdns3_probe_data *data)
> >  {
> > +     struct platform_device *pdev = data->pdev;
> >       struct device *dev = &pdev->dev;
> > -     struct resource *res;
> > -     struct cdns *cdns;
> > +     struct cdns *cdns = data->cdns;
> > +     struct resource *res;
> >       void __iomem *regs;
> >       int ret;
> >
> > -     cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> > -     if (!cdns)
> > -             return -ENOMEM;
> > -
> > -     cdns->dev = dev;
> > -     cdns->pdata = dev_get_platdata(dev);
> > -     if (cdns->pdata && cdns->pdata->override_apb_timeout)
> > -             cdns->override_apb_timeout = cdns->pdata->override_apb_timeout;
> > -
> > -     platform_set_drvdata(pdev, cdns);
> > +     dev_set_drvdata(dev, cdns);
> >
> >       ret = platform_get_irq_byname(pdev, "host");
> 
> Further down in this function, if devm_phy_optional_get() for cdns->usb3_phy
> fails (e.g. with -EPROBE_DEFER), it returns directly via dev_err_probe(),
> bypassing the error cleanup path in err_phy3_init.
> 
> Does this leak the usb2_phy initialization since phy_exit(cdns->usb2_phy) is
> never called?
> 
> If probe is deferred and retried, will this repeatedly increment the PHY's
> init_count, preventing it from ever powering down?
> 
> Also in this function, after cdns_init(cdns) successfully registers resources
> like a role switch, if cdns_core_init_role(cdns) fails, the error path
> jumps to err_cdns_init:
> 
> err_cdns_init:
>         set_phy_power_off(cdns);
> err_phy_power_on:
> ...
> 
> This error path only powers off the PHYs. Can this permanently leak the role
> switch and DRD resources by omitting a call to cdns_remove(cdns) or
> usb_role_switch_unregister()?

Ack.

It is the issue current code exists, I will fix it first.

> 
> Towards the end of this function, pm_runtime_forbid(dev) is conditionally
> called to increase the device's runtime PM usage count:
> 
>         if (!(cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW)))
>                 pm_runtime_forbid(dev);
> 
> Should there be a corresponding pm_runtime_allow(dev) in the remove path to
> balance this count?

Ack.

> 
> [ ... ]
> > @@ -195,14 +188,41 @@ static int cdns3_plat_probe(struct platform_device *pdev)
> [ ... ]
> > +/**
> > + * cdns3_core_remove - Tear down the Cadence USB3 platform core
> > + * @cdns: Controller context previously initialized by cdns3_core_probe()
> > + */
> > +void cdns3_core_remove(struct cdns *cdns)
> >  {
> > -     struct cdns *cdns = platform_get_drvdata(pdev);
> >       struct device *dev = cdns->dev;
> >
> >       pm_runtime_get_sync(dev);
> > @@ -213,24 +233,30 @@ static void cdns3_plat_remove(struct platform_device *pdev)
> >       phy_exit(cdns->usb2_phy);
> >       phy_exit(cdns->usb3_phy);
> >  }
> > +EXPORT_SYMBOL_GPL(cdns3_core_remove);
> 
> Since pm_runtime_forbid(dev) was called in the probe function, could the
> absence of pm_runtime_allow(dev) here permanently leak the runtime PM usage
> counter across bind/unbind cycles?
> 
> If the driver is repeatedly unbound and bound, will the usage counter
> continuously increment, eventually preventing the device from entering a
> runtime suspended state?

The same comments for above issue.

> 
> [ ... ]
> > @@ -242,7 +268,7 @@ static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
> >       else
> >               wakeup = device_may_wakeup(dev);
> >
> > -     cdns3_set_platform_suspend(cdns->dev, true, wakeup);
> > +     cdns3_set_platform_suspend(cdns, true, wakeup);
> >       set_phy_power_off(cdns);
> >       spin_lock_irqsave(&cdns->lock, flags);
> 
> The controller uses shared IRQs, such as IRQF_SHARED for cdns->wakeup_irq.
> The PM subsystem guidelines require drivers using shared IRQs to call
> synchronize_irq() before powering down hardware.
> 
> Might calling set_phy_power_off(cdns) here before synchronize_irq() risk
> hardware PHY being powered off while an interrupt handler is executing
> concurrently on another CPU?

Ack.

Will call synchronize_irq() at the beginning of function cdns3_controller_suspend.

> 
> [ ... ]
> > diff --git a/drivers/usb/cdns3/glue.h b/drivers/usb/cdns3/glue.h
> > new file mode 100644
> > index 0000000000000..67cd1073b5551
> > --- /dev/null
> > +++ b/drivers/usb/cdns3/glue.h
> > @@ -0,0 +1,51 @@
> [ ... ]
> > +/*
> > + * The following callbacks are for glue drivers to invoke from their own
> > + * &dev_pm_ops, so platform-specific work can wrap the shared controller logic.
> > + */
> > +int cdns3_runtime_suspend(struct cdns *cdns);
> > +int cdns3_runtime_resume(struct cdns *cdns);
> > +int cdns3_pm_suspend(struct cdns *cdns);
> > +int cdns3_pm_resume(struct cdns *cdns);
> > +
> > +#endif /* __DRIVERS_USB_CDNS3_GLUE_H */
> 
> These functions are conditionally compiled in cdns3-plat.c depending on
> CONFIG_PM and CONFIG_PM_SLEEP. However, they are unconditionally declared
> here without empty static inline fallback stubs.
> 
> If a glue driver uses __maybe_unused annotations instead of #ifdef CONFIG_PM
> for its own callbacks, will the compiler still attempt to resolve these calls
> and produce undefined reference linker errors when PM is disabled?
> 

Ack

The PM helper declarations in glue.h should indeed be made robust for glue
drivers using __maybe_unused PM callbacks. The current Sky1 glue driver uses
CONFIG_PM/CONFIG_PM_SLEEP guards, but the public glue header should not rely
on every user doing that. I will add static inline stubs for the disabled PM
configurations in v2.

-- 

Best regards,
Peter