Re: [PATCH 2/4] usb: cdns3: sky1: Add cdnsp-sky1 glue driver

On 26-05-11 23:17:34, [email protected] wrote:
> EXTERNAL EMAIL
> 
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Probe returns success (0) when memory resource retrieval fails, leading to a NULL pointer dereference.
> - [High] Probe forcefully overwrites the mode strap configuration for all USB controllers on the SoC.
> - [Medium] Resource leak of clocks on early probe error paths.
> - [Medium] Unconditional assignment of dev_pm_ops violates Power Management Subsystem rules.
> - [Medium] Commit message and implementation mismatch regarding hardware resets.
> - [Low] Using of_match_ptr() with a statically allocated array causes compiler warnings.
> --
> 
> commit a626bc93f86adbca6a4dce0b669265133a32a8cb
> Author: Peter Chen <[email protected]>
> 
> usb: cdns3: sky1: Add cdnsp-sky1 glue driver
> 
> Add a CIX sky1 platform glue driver with Kconfig and Makefile entry.
> It calls APIs exported from cdns3-plat.c for probe/remote/suspend/resume
> routines.
> 
> >         Glue driver for the Cadence USB dual-role controllers on CIX Sky1
> >         (device tree compatible cix,sky1-usb3). It enables clocks and resets
> >         from the SoC, then uses the shared cdns3 platform core (cdns.ko).
> 
> The commit message mentions it enables clocks and resets from the SoC, but is
> there missing code to handle the resets? The implementation includes linux/reset.h
> but doesn't appear to request or interact with any reset lines.

Sorry, I have a local version with PHY driver, it needs to have reset operation, I
forget to delete it at commit message.

> 
> > diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> > new file mode 100644
> > index 0000000000000..049044e3d09bd
> > --- /dev/null
> > +++ b/drivers/usb/cdns3/cdnsp-sky1.c
> 
> [ ... ]
> 
> > +static int cdnsp_sky1_set_all_controllers_otg(struct regmap *syscon)
> > +{
> > +     int id, ret;
> > +
> > +     for (id = 0; id < SKY1_USB_S5_NUM; id++) {
> > +             ret = cdnsp_sky1_set_mode_by_id(syscon, id, MODE_STRAP_OTG);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> 
> Does this code unintentionally alter the state of other independent USB
> controllers? It appears to iterate through all controllers on the SoC and
> updates them to MODE_STRAP_OTG, which might affect concurrent controllers
> that are explicitly configured as host-only (like U2_HOST0_ID).

All controllers for CIX have OTG hardware, it depends on OTG register to set
proper mode, so set all controllers DR mode as OTG by default.

> 
> [ ... ]
> 
> > +static int cdnsp_sky1_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct cdnsp_sky1 *priv;
> > +     struct cdns *cdns;
> > +     struct cdns3_probe_data probe_data;
> > +     struct resource *res;
> > +     int ret;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     priv->dev = dev;
> > +     priv->num_clks = ARRAY_SIZE(cdnsp_sky1_cdns_core_clks);
> > +     priv->clks = devm_kmemdup(dev, cdnsp_sky1_cdns_core_clks,
> > +                                sizeof(cdnsp_sky1_cdns_core_clks), GFP_KERNEL);
> > +     if (!priv->clks)
> > +             return -ENOMEM;
> > +
> > +     ret = devm_clk_bulk_get(dev, priv->num_clks, priv->clks);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to get clocks\n");
> > +
> > +     ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to enable clocks\n");
> > +
> > +     priv->usb_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
> > +                                                        "cix,syscon-usb");
> > +     if (IS_ERR(priv->usb_syscon))
> > +             return dev_err_probe(dev, PTR_ERR(priv->usb_syscon),
> > +                                  "failed to get cix,syscon-usb regmap\n");
> > +
> > +     ret = cdnsp_sky1_set_all_controllers_otg(priv->usb_syscon);
> > +     if (ret)
> > +             return dev_err_probe(dev, ret,
> > +                                  "failed to set USB controllers to OTG strap\n");
> 
> Can these error paths cause a resource leak? The clocks were previously enabled
> via clk_bulk_prepare_enable(), but these direct returns bypass the err_clk
> label where clk_bulk_disable_unprepare() is called.

Will fix at v2.

> 
> > +
> > +     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "glue");
> > +     if (!res)
> > +             goto err_clk;
> 
> If this resource retrieval fails, does the probe function return success?
> 

No, the "glue" region is mandatory.

> The ret variable was previously set to 0 by the successful call to
> cdnsp_sky1_set_all_controllers_otg(), so jumping to err_clk without updating
> ret to an error code like -ENODEV would result in returning 0.
> 
> Could this lead to a NULL pointer dereference during subsequent power
> management operations? Since the device would be considered bound without
> calling cdns3_core_probe(), dev_get_drvdata() might return NULL in
> callbacks like cdnsp_sky1_runtime_suspend().

NAK. Other values except for NULL is correct for platform_get_resource_byname.

> 
> [ ... ]
> 
> > +static const struct of_device_id cdnsp_sky1_of_match[] = {
> > +     { .compatible = "cix,sky1-usb3" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, cdnsp_sky1_of_match);
> > +
> > +static struct platform_driver cdnsp_sky1_driver = {
> > +     .probe          = cdnsp_sky1_probe,
> > +     .remove         = cdnsp_sky1_remove,
> > +     .driver         = {
> > +             .name           = "cdnsp-sky1",
> > +             .of_match_table = of_match_ptr(cdnsp_sky1_of_match),
> 
> If the kernel is built with CONFIG_OF disabled, of_match_ptr() evaluates to
> NULL. Since the cdnsp_sky1_of_match array is declared statically and without
> __maybe_unused annotations, will this result in an unused variable compiler
> warning?

Will fix it by adding CONFIG_OF for cdnsp_sky1_of_match.
> 
> > +             .pm             = &cdnsp_sky1_pm_ops,
> 
> Does unconditionally assigning the pm callback structure produce dead code when
> CONFIG_PM is disabled? According to the power management subsystem guidelines,
> this should likely be wrapped with the pm_ptr() macro so it evaluates to NULL
> without PM support.

Will use pm_ptr for it.

-- 

Best regards,
Peter