On Wed, Nov 22, 2023 at 12:57:37PM +0100, Johan Hovold wrote: > On Mon, Oct 16, 2023 at 08:11:12PM -0700, Bjorn Andersson wrote: > > The DWC3 IP block is handled by three distinct device drivers: XHCI, > > DWC3 core and a platform specific (optional) DWC3 glue driver. > > > > This has resulted in, at least in the case of the Qualcomm glue, the > > presence of a number of layering violations, where the glue code either > > can't handle, or has to work around, the fact that core might not probe > > deterministically. > > > > An example of this is that the suspend path should operate slightly > > different depending on the device operating in host or peripheral mode, > > and the only way to determine the operating state is to peek into the > > core's drvdata. > > > > The Qualcomm glue driver is expected to make updates in the qscratch > > register region (the "glue" region) during role switch events, but with > > the glue and core split using the driver model, there is no reasonable > > way to introduce listeners for mode changes. > > > > Split the dwc3 core platfrom_driver callbacks and their implementation > > and export the implementation, to make it possible to deterministically > > instantiate the dwc3 core as part of the dwc3 glue drivers and to > > allow flattening of the DeviceTree representation. > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx> > > --- > > drivers/usb/dwc3/core.c | 134 ++++++++++++++++++++++++++++++++---------------- > > drivers/usb/dwc3/core.h | 10 ++++ > > 2 files changed, 100 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index d25490965b27..71e376bebb16 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > > return 0; > > } > > > > -static int dwc3_probe(struct platform_device *pdev) > > +struct dwc3 *dwc3_probe(struct platform_device *pdev) > > Perhaps you should move allocation of struct dwc3 to the caller as well > as you are going to need some way to pass in callback to core which need > to be set before you register the xhci platform device. > > There could be other ways, like passing in a struct of callbacks, which > can be added incrementally but it may be good think this through from > the start. > My intention was to have callbacks and quirks passed through additional arguments in an incremental patch. IMHO passing such information through a pre-allocated and partially initialized struct is more obscure than passing that information as explicit parameters to the function... > > { > > struct device *dev = &pdev->dev; > > struct resource *res, dwc_res; > > @@ -1886,14 +1886,14 @@ static int dwc3_probe(struct platform_device *pdev) > > > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > > if (!dwc) > > - return -ENOMEM; > > + return ERR_PTR(-ENOMEM); > > > > dwc->dev = dev; > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > if (!res) { > > dev_err(dev, "missing memory resource\n"); > > - return -ENODEV; > > + return ERR_PTR(-ENODEV); > > } > > > > dwc->xhci_resources[0].start = res->start; > > @@ -1922,7 +1922,7 @@ static int dwc3_probe(struct platform_device *pdev) > > > > regs = devm_ioremap_resource(dev, &dwc_res); > > if (IS_ERR(regs)) > > - return PTR_ERR(regs); > > + return ERR_CAST(regs); > > > > dwc->regs = regs; > > dwc->regs_size = resource_size(&dwc_res); > > @@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev) > > goto err_disable_clks; > > } > > > > - platform_set_drvdata(pdev, dwc); > > This is broken however as the pm ops access the data driver data and can > be called as soon as you call pm_runtime_put() below. > You're right, thanks for spotting that. Regards, Bjorn > > dwc3_cache_hwparams(dwc); > > > > if (!dwc->sysdev_is_parent && > > @@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev) > > > > pm_runtime_put(dev); > > That is here. > > > - return 0; > > + return dwc; > > > -static void dwc3_remove(struct platform_device *pdev) > > +static int dwc3_plat_probe(struct platform_device *pdev) > > { > > - struct dwc3 *dwc = platform_get_drvdata(pdev); > > + struct dwc3 *dwc; > > + > > + dwc = dwc3_probe(pdev); > > + if (IS_ERR(dwc)) > > + return PTR_ERR(dwc); > > And that leaves a window, for example, here where you can hit a NULL > pointer dereference. > > > + platform_set_drvdata(pdev, dwc); > > + > > + return 0; > > +} > > Johan >