| Message ID | 20201210134215.20424-1-peter.chen@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] of: platform: introduce platform data length for auxdata | expand |
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/. > It fixed below oops during module unload process at imx8qxp mek platform > for Cadence USB3 driver. > > log1: > [ 333.501593] Unable to handle kernel paging request at virtual address 000000000004ae48 > [ 333.509558] Mem abort info: > [ 333.512369] ESR = 0x96000006 > [ 333.515476] EC = 0x25: DABT (current EL), IL = 32 bits > [ 333.520847] SET = 0, FnV = 0 > [ 333.523986] EA = 0, S1PTW = 0 > [ 333.527193] Data abort info: > [ 333.530124] ISV = 0, ISS = 0x00000006 > [ 333.534004] CM = 0, WnR = 0 > [ 333.536988] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000885d73000 > [ 333.543497] [000000000004ae48] pgd=0000000000000000, p4d=0000000000000000 > [ 333.550354] Internal error: Oops: 96000006 [#1] PREEMPT SMP > [ 333.555936] Modules linked in: fsl_jr_uio caam_jr caamkeyblob_desc caamhash_desc caamalg_desc crypto_engine rng_core authenc libdes crct10dif_ > ce mxc_jpeg_encdec phy_mxs_usb phy_cadence_salvo imx8_media_dev(C) caam error [last unloaded: cdns3] > [ 333.577484] CPU: 3 PID: 168 Comm: kworker/3:38 Tainted: G C 5.10.0-rc4-04454-gd479340c061 #30 > [ 333.587313] Hardware name: Freescale i.MX8QXP MEK (DT) > [ 333.592471] Workqueue: rcu_gp srcu_invoke_callbacks > [ 333.597359] pstate: a0000005 (NzCv daif -PAN -UAO -TCO BTYPE=--) > [ 333.603373] pc : kfree+0x78/0x3a0 > [ 333.606690] lr : platform_device_release+0x28/0x58 > [ 333.611481] sp : ffff8000135bbc20 > [ 333.614800] x29: ffff8000135bbc20 x28: ffff800012146000 > [ 333.620125] x27: ffff00083fa06470 x26: ffff800010108cb0 > [ 333.625442] x25: 0080000000000000 x24: ffff80001240f000 > [ 333.630767] x23: ffff0008012ef000 x22: ffff800010875478 > [ 333.636093] x21: ffff8000092b90c8 x20: ffff000804bf1b00 > [ 333.641418] x19: 000000000004ae40 x18: 0000000000000000 > [ 333.646735] x17: 0000000000000000 x16: 0000000000000000 > [ 333.652052] x15: 0000000000000000 x14: 0000000000000000 > [ 333.657368] x13: 0000000000000002 x12: 0000000000000000 > [ 333.662685] x11: 0000000000000040 x10: 0000000000000a00 > [ 333.668000] x9 : ffff800010875478 x8 : fefefefefefefeff > [ 333.673319] x7 : 0000000000000018 x6 : ffff00080101006c > [ 333.678644] x5 : ffff00083fa5c300 x4 : 0000000000000000 > [ 333.683969] x3 : 0000000000000002 x2 : 0000000000000000 > [ 333.689286] x1 : 0000000000000030 x0 : fffffdffffe00000 > [ 333.694605] Call trace: > [ 333.697059] kfree+0x78/0x3a0 > [ 333.700027] platform_device_release+0x28/0x58 > [ 333.704476] device_release+0x38/0x90 > [ 333.708144] kobject_put+0x78/0x208 > [ 333.711633] __device_link_free_srcu+0x50/0x78 > [ 333.716082] srcu_invoke_callbacks+0xf4/0x168 > [ 333.720445] process_one_work+0x1c8/0x480 > [ 333.724460] worker_thread+0x50/0x420 > [ 333.728124] kthread+0x148/0x168 > [ 333.731355] ret_from_fork+0x10/0x18 > [ 333.734940] Code: b26babe0 d34cfe73 f2dfbfe0 8b131813 (f9400660) > [ 333.741049] ---[ end trace 9a41d9fcbc0885e6 ]--- > [ 333.745671] Kernel panic - not syncing: Oops: Fatal exception in interrupt > [ 333.752551] SMP: stopping secondary CPUs > [ 333.756485] Kernel Offset: disabled > [ 333.759979] CPU features: 0x0240002,20002008 > [ 333.764251] Memory Limit: none > > log2: > [ 160.400179] kfree+0x78/0x3a0 > [ 160.403147] platform_device_release+0x28/0x58 > [ 160.407595] device_release+0x38/0x90 > [ 160.411264] kobject_put+0x78/0x208 > [ 160.414753] klist_children_put+0x1c/0x28 > [ 160.418766] klist_next+0xac/0x128 > [ 160.422172] device_for_each_child+0x4c/0xa8 > [ 160.426453] cdns_imx_remove+0x2c/0x40 [cdns3_imx] > [ 160.431251] platform_drv_remove+0x30/0x50 > [ 160.435353] device_release_driver_internal+0x114/0x1e8 > [ 160.440580] driver_detach+0x54/0xe0 > [ 160.444163] bus_remove_driver+0x60/0xd8 > [ 160.448087] driver_unregister+0x34/0x60 > [ 160.452013] platform_driver_unregister+0x18/0x20 > [ 160.456726] cdns_imx_driver_exit+0x14/0x478 [cdns3_imx] > [ 160.462042] __arm64_sys_delete_module+0x180/0x258 > [ 160.466837] el0_svc_common.constprop.0+0x70/0x168 > [ 160.471631] do_el0_svc+0x28/0x88 > [ 160.474950] el0_sync_handler+0x158/0x160 > [ 160.478961] el0_sync+0x140/0x180 > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > --- > drivers/of/platform.c | 29 +++++++++++++++++++++-------- > include/linux/of_platform.h | 8 ++++++-- > 2 files changed, 27 insertions(+), 10 deletions(-) > > 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"); Isn't this going to warn on the majority of users as static data is the norm. > } > > 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 */ > -- > 2.17.1 >
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.
> 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.
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 */