> Subject: Re: [PATCH 1/2] of: platform: introduce platform data length for
> auxdata
>
> On 20-12-10 09:38:49, Rob Herring wrote:
> > On Thu, Dec 10, 2020 at 7:42 AM Peter Chen <peter.chen@xxxxxxxxxx>
> wrote:
> > >
> > > From: Peter Chen <peter.chen@xxxxxxx>
> > >
> > > When a platform device is released, it frees the device
> > > platform_data memory region using kfree, if the memory is not
> > > allocated by kmalloc, it may run into trouble. See the below comments from
> kfree API.
> > >
> > > * Don't free memory not originally allocated by kmalloc()
> > > * or you will run into trouble.
> > >
> > > For the device which is created dynamically using
> > > of_platform_populate, if the platform_data is existed at
> > > of_dev_auxdata structure, the OF code simply assigns the
> > > platform_data pointer to newly created device, but not using
> > > platform_device_add_data to allocate one. For most of platform data
> > > region at device driver, which may not be allocated by kmalloc, they are at
> global data region or at stack region at some situations.
> >
> > auxdata is a "temporary" thing for transitioning to DT which I want to
> > remove. So I don't really want to see it expanded nor new users. We've
> > got about a dozen arm32 platforms and 5 cases under drivers/.
> >
>
> How to handle the below user case:
> Parent device creates child device through device tree node (eg, usb/dwc3,
> usb/cdns3), there are some platform quirks at parent device(vendor glue
> layer) need child device (core IP device) driver to handle. The quirks are not
> limited to the hardware quirk, may include the callbacks, software flag (eg:
> XHCI_DEFAULT_PM_RUNTIME_ALLOW/XHCI_SKIP_PHY_INIT, at
> drivers/usb/host/xhci.h)
>
> > > + int platform_data_length = 0;
> > > int rc = 0;
> > >
> > > /* Make sure it has a compatible property */ @@ -378,6
> > > +387,9 @@ static int of_platform_bus_create(struct device_node *bus,
> > > if (auxdata) {
> > > bus_id = auxdata->name;
> > > platform_data = auxdata->platform_data;
> > > + platform_data_length =
> auxdata->platform_data_length;
> > > + if (platform_data && !platform_data_length)
> > > + pr_warn("Make sure platform_data is
> > > + allocated by kmalloc\n");
> >
> > Isn't this going to warn on the majority of users as static data is the norm.
>
> This warning only triggers at the cases which driver defines auxdata and
> platform_data pointer is in it. Besides, directly assign the address of static data
> to device platfrom_data pointer is wrong thing, this region will be freed using
> kfree at platform_device_release. Using platform_device_add_data API is the
> correct thing to do that.
>
Hi Rob,
Would you please give me some suggestions how we could fix/workaround this issue?
Peter
