| Message ID | 20201210134215.20424-1-peter.chen@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] of: platform: introduce platform data length for auxdata | expand |
On 20-12-10 09:38:49, Rob Herring wrote: > On Thu, Dec 10, 2020 at 7:42 AM Peter Chen <peter.chen@kernel.org> wrote: > > > > From: Peter Chen <peter.chen@nxp.com> > > > > 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. -- Thanks, Peter Chen
> 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@kernel.org> > wrote: > > > > > > From: Peter Chen <peter.chen@nxp.com> > > > > > > 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
On Thu, Dec 10, 2020 at 7:02 PM Peter Chen <peter.chen@nxp.com> wrote: > > On 20-12-10 09:38:49, Rob Herring wrote: > > On Thu, Dec 10, 2020 at 7:42 AM Peter Chen <peter.chen@kernel.org> wrote: > > > > > > From: Peter Chen <peter.chen@nxp.com> > > > > > > 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) The split of these between a platform specific driver and the core IP driver was just wrong to begin with. There should only be 1 driver with common 'library' functions like we do for every other case of common, licensed IP. Perhaps the core driver should stop pretending it is generic and figure out the quirks for itself by looking at the parent node. Rob
On 21-01-04 08:00:07, Rob Herring wrote: > On Thu, Dec 10, 2020 at 7:02 PM Peter Chen <peter.chen@nxp.com> wrote: > > > > On 20-12-10 09:38:49, Rob Herring wrote: > > > On Thu, Dec 10, 2020 at 7:42 AM Peter Chen <peter.chen@kernel.org> wrote: > > > > > > > > From: Peter Chen <peter.chen@nxp.com> > > > > > > > > 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) > > The split of these between a platform specific driver and the core IP > driver was just wrong to begin with. There should only be 1 driver > with common 'library' functions like we do for every other case of > common, licensed IP. Perhaps the core driver should stop pretending it > is generic and figure out the quirks for itself by looking at the > parent node. > Not only hardware quirks, but software quirks and callbacks which are implemented at platform specific driver and are called at core driver. -- Thanks, Peter Chen
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index b557a0fcd4ba..4afb775779f3 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -157,7 +157,8 @@ EXPORT_SYMBOL(of_device_alloc); * of_platform_device_create_pdata - Alloc, initialize and register an of_device * @np: pointer to node to create device for * @bus_id: name to assign device - * @platform_data: pointer to populate platform_data pointer with + * @platform_data: platform_data pointer from device driver + * @platform_data_length: the length of platform_data * @parent: Linux device model parent device. * * Returns pointer to created platform device, or NULL if a device was not @@ -167,6 +168,7 @@ static struct platform_device *of_platform_device_create_pdata( struct device_node *np, const char *bus_id, void *platform_data, + int platform_data_length, struct device *parent) { struct platform_device *dev; @@ -183,16 +185,22 @@ static struct platform_device *of_platform_device_create_pdata( if (!dev->dev.dma_mask) dev->dev.dma_mask = &dev->dev.coherent_dma_mask; dev->dev.bus = &platform_bus_type; - dev->dev.platform_data = platform_data; + if (platform_data_length) { + if (platform_device_add_data(dev, platform_data, platform_data_length)) + goto err_put_device; + } else { + dev->dev.platform_data = platform_data; + } + of_msi_configure(&dev->dev, dev->dev.of_node); - if (of_device_add(dev) != 0) { - platform_device_put(dev); - goto err_clear_flag; - } + if (of_device_add(dev) != 0) + goto err_put_device; return dev; +err_put_device: + platform_device_put(dev); err_clear_flag: of_node_clear_flag(np, OF_POPULATED); return NULL; @@ -211,7 +219,7 @@ struct platform_device *of_platform_device_create(struct device_node *np, const char *bus_id, struct device *parent) { - return of_platform_device_create_pdata(np, bus_id, NULL, parent); + return of_platform_device_create_pdata(np, bus_id, NULL, 0, parent); } EXPORT_SYMBOL(of_platform_device_create); @@ -353,6 +361,7 @@ static int of_platform_bus_create(struct device_node *bus, struct platform_device *dev; const char *bus_id = NULL; void *platform_data = NULL; + 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"); } if (of_device_is_compatible(bus, "arm,primecell")) { @@ -389,7 +401,8 @@ static int of_platform_bus_create(struct device_node *bus, return 0; } - dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); + dev = of_platform_device_create_pdata(bus, bus_id, platform_data, + platform_data_length, parent); if (!dev || !of_match_node(matches, bus)) return 0; diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h index 84a966623e78..c3b02236b110 100644 --- a/include/linux/of_platform.h +++ b/include/linux/of_platform.h @@ -18,11 +18,14 @@ * @phys_addr: Start address of registers to match against node * @name: Name to assign for matching nodes * @platform_data: platform_data to assign for matching nodes + * @platform_data_length: the length of platform data * * This lookup table allows the caller of of_platform_populate() to override * the names of devices when creating devices from the device tree. The table - * should be terminated with an empty entry. It also allows the platform_data - * pointer to be set. + * should be terminated with an empty entry. The platform_data_length should be + * given if the platform_data is existed and is not allocated by kmalloc, it + * could avoid potential kfree issue when the platform_data is freed by + * platform_device_release. * * The reason for this functionality is that some Linux infrastructure uses * the device name to look up a specific device, but the Linux-specific names @@ -39,6 +42,7 @@ struct of_dev_auxdata { resource_size_t phys_addr; char *name; void *platform_data; + int platform_data_length; }; /* Macro to simplify populating a lookup table */