| Message ID | 20201202161600.23738-1-patrick.havelange@essensium.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] net: freescale/fman: Split the main resource region reservation | expand |
> -----Original Message----- > From: Patrick Havelange <patrick.havelange@essensium.com> > Sent: 02 December 2020 18:16 > To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Patrick Havelange <patrick.havelange@essensium.com> > Subject: [PATCH 2/4] net: freescale/fman-port: remove direct use of > __devm_request_region > > This driver was directly calling __devm_request_region with a specific > resource on the stack as parameter. This is invalid as > __devm_request_region expects the given resource passed as parameter > to live longer than the call itself, as the pointer to the resource > will be stored inside the internal struct used by the devres > management. > > In addition to this issue, a related bug has been found by kmemleak > with this trace : > unreferenced object 0xc0000001efc01880 (size 64): > comm "swapper/0", pid 1, jiffies 4294669078 (age 3620.536s) > hex dump (first 32 bytes): > 00 00 00 0f fe 4a c0 00 00 00 00 0f fe 4a cf ff .....J.......J.. > c0 00 00 00 00 ee 9d 98 00 00 00 00 80 00 02 00 ................ > backtrace: > [<c000000000078874>] .alloc_resource+0xb8/0xe0 > [<c000000000079b50>] .__request_region+0x70/0x1c4 > [<c00000000007a010>] .__devm_request_region+0x8c/0x138 > [<c0000000006e0dc8>] .fman_port_probe+0x170/0x420 > [<c0000000005cecb8>] .platform_drv_probe+0x84/0x108 > [<c0000000005cc620>] .driver_probe_device+0x2c4/0x394 > [<c0000000005cc814>] .__driver_attach+0x124/0x128 > [<c0000000005c9ad4>] .bus_for_each_dev+0xb4/0x110 > [<c0000000005cca1c>] .driver_attach+0x34/0x4c > [<c0000000005ca9b0>] .bus_add_driver+0x264/0x2a4 > [<c0000000005cd9e0>] .driver_register+0x94/0x160 > [<c0000000005cfea4>] .__platform_driver_register+0x60/0x7c > [<c000000000f86a00>] .fman_port_load+0x28/0x64 > [<c000000000f4106c>] .do_one_initcall+0xd4/0x1a8 > [<c000000000f412fc>] .kernel_init_freeable+0x1bc/0x2a4 > [<c00000000000180c>] .kernel_init+0x24/0x138 > > Indeed, the new resource (created in __request_region) will be linked > to the given resource living on the stack, which will end its lifetime > after the function calling __devm_request_region has finished. > Meaning the new resource allocated is no longer reachable. > > Now that the main fman driver is no longer reserving the region > used by fman-port, this previous hack is no longer needed > and we can use the regular call to devm_request_mem_region instead, > solving those bugs at the same time. > > Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com> > --- > drivers/net/ethernet/freescale/fman/fman_port.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c > b/drivers/net/ethernet/freescale/fman/fman_port.c > index d9baac0dbc7d..354974939d9d 100644 > --- a/drivers/net/ethernet/freescale/fman/fman_port.c > +++ b/drivers/net/ethernet/freescale/fman/fman_port.c > @@ -1878,10 +1878,10 @@ static int fman_port_probe(struct platform_device > *of_dev) > > of_node_put(port_node); > > - dev_res = __devm_request_region(port->dev, &res, res.start, > - resource_size(&res), "fman-port"); > + dev_res = devm_request_mem_region(port->dev, res.start, > + resource_size(&res), "fman-port"); > if (!dev_res) { > - dev_err(port->dev, "%s: __devm_request_region() failed\n", > + dev_err(port->dev, "%s: devm_request_mem_region() failed\n", > __func__); > err = -EINVAL; > goto free_port; > -- > 2.17.1 Hi Patrick, please send a fix for the issue, targeting the net tree, separate from the other change you are trying to introduce. We need a better explanation for the latter and it should go through the net-next tree, if accepted. Madalin
On 2020-12-03 09:44, Madalin Bucur wrote: >> -----Original Message----- >> From: Patrick Havelange <patrick.havelange@essensium.com> >> Sent: 02 December 2020 18:16 >> To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller >> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; >> netdev@vger.kernel.org; linux-kernel@vger.kernel.org >> Cc: Patrick Havelange <patrick.havelange@essensium.com> >> Subject: [PATCH 2/4] net: freescale/fman-port: remove direct use of >> __devm_request_region >> >> This driver was directly calling __devm_request_region with a specific >> resource on the stack as parameter. This is invalid as >> __devm_request_region expects the given resource passed as parameter >> to live longer than the call itself, as the pointer to the resource >> will be stored inside the internal struct used by the devres >> management. >> >> In addition to this issue, a related bug has been found by kmemleak >> with this trace : >> unreferenced object 0xc0000001efc01880 (size 64): >> comm "swapper/0", pid 1, jiffies 4294669078 (age 3620.536s) >> hex dump (first 32 bytes): >> 00 00 00 0f fe 4a c0 00 00 00 00 0f fe 4a cf ff .....J.......J.. >> c0 00 00 00 00 ee 9d 98 00 00 00 00 80 00 02 00 ................ >> backtrace: >> [<c000000000078874>] .alloc_resource+0xb8/0xe0 >> [<c000000000079b50>] .__request_region+0x70/0x1c4 >> [<c00000000007a010>] .__devm_request_region+0x8c/0x138 >> [<c0000000006e0dc8>] .fman_port_probe+0x170/0x420 >> [<c0000000005cecb8>] .platform_drv_probe+0x84/0x108 >> [<c0000000005cc620>] .driver_probe_device+0x2c4/0x394 >> [<c0000000005cc814>] .__driver_attach+0x124/0x128 >> [<c0000000005c9ad4>] .bus_for_each_dev+0xb4/0x110 >> [<c0000000005cca1c>] .driver_attach+0x34/0x4c >> [<c0000000005ca9b0>] .bus_add_driver+0x264/0x2a4 >> [<c0000000005cd9e0>] .driver_register+0x94/0x160 >> [<c0000000005cfea4>] .__platform_driver_register+0x60/0x7c >> [<c000000000f86a00>] .fman_port_load+0x28/0x64 >> [<c000000000f4106c>] .do_one_initcall+0xd4/0x1a8 >> [<c000000000f412fc>] .kernel_init_freeable+0x1bc/0x2a4 >> [<c00000000000180c>] .kernel_init+0x24/0x138 >> >> Indeed, the new resource (created in __request_region) will be linked >> to the given resource living on the stack, which will end its lifetime >> after the function calling __devm_request_region has finished. >> Meaning the new resource allocated is no longer reachable. >> >> Now that the main fman driver is no longer reserving the region >> used by fman-port, this previous hack is no longer needed >> and we can use the regular call to devm_request_mem_region instead, >> solving those bugs at the same time. >> >> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com> >> --- >> drivers/net/ethernet/freescale/fman/fman_port.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c >> b/drivers/net/ethernet/freescale/fman/fman_port.c >> index d9baac0dbc7d..354974939d9d 100644 >> --- a/drivers/net/ethernet/freescale/fman/fman_port.c >> +++ b/drivers/net/ethernet/freescale/fman/fman_port.c >> @@ -1878,10 +1878,10 @@ static int fman_port_probe(struct platform_device >> *of_dev) >> >> of_node_put(port_node); >> >> - dev_res = __devm_request_region(port->dev, &res, res.start, >> - resource_size(&res), "fman-port"); >> + dev_res = devm_request_mem_region(port->dev, res.start, >> + resource_size(&res), "fman-port"); >> if (!dev_res) { >> - dev_err(port->dev, "%s: __devm_request_region() failed\n", >> + dev_err(port->dev, "%s: devm_request_mem_region() failed\n", >> __func__); >> err = -EINVAL; >> goto free_port; >> -- >> 2.17.1 > > Hi Patrick, > > please send a fix for the issue, targeting the net tree, separate from the > other change you are trying to introduce. We need a better explanation for > the latter and it should go through the net-next tree, if accepted. Hello, I've resent the series with a cover letter having a bit more explanations. I think all the patches should be applied on net, as they are linked to the same issue/resolution, and are not independent. BR, Patrick H.
diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c index ce0a121580f6..2e85209d560d 100644 --- a/drivers/net/ethernet/freescale/fman/fman.c +++ b/drivers/net/ethernet/freescale/fman/fman.c @@ -58,12 +58,15 @@ /* Modules registers offsets */ #define BMI_OFFSET 0x00080000 #define QMI_OFFSET 0x00080400 -#define KG_OFFSET 0x000C1000 -#define DMA_OFFSET 0x000C2000 -#define FPM_OFFSET 0x000C3000 -#define IMEM_OFFSET 0x000C4000 -#define HWP_OFFSET 0x000C7000 -#define CGP_OFFSET 0x000DB000 +#define SIZE_REGION_0 0x00081000 +#define POL_OFFSET 0x000C0000 +#define KG_OFFSET_FROM_POL 0x00001000 +#define DMA_OFFSET_FROM_POL 0x00002000 +#define FPM_OFFSET_FROM_POL 0x00003000 +#define IMEM_OFFSET_FROM_POL 0x00004000 +#define HWP_OFFSET_FROM_POL 0x00007000 +#define CGP_OFFSET_FROM_POL 0x0001B000 +#define SIZE_REGION_FROM_POL 0x00020000 /* Exceptions bit map */ #define EX_DMA_BUS_ERROR 0x80000000 @@ -1433,7 +1436,7 @@ static int clear_iram(struct fman *fman) struct fman_iram_regs __iomem *iram; int i, count; - iram = fman->base_addr + IMEM_OFFSET; + iram = fman->base_addr_pol + IMEM_OFFSET_FROM_POL; /* Enable the auto-increment */ iowrite32be(IRAM_IADD_AIE, &iram->iadd); @@ -1710,11 +1713,8 @@ static int set_num_of_open_dmas(struct fman *fman, u8 port_id, static int fman_config(struct fman *fman) { - void __iomem *base_addr; int err; - base_addr = fman->dts_params.base_addr; - fman->state = kzalloc(sizeof(*fman->state), GFP_KERNEL); if (!fman->state) goto err_fm_state; @@ -1740,13 +1740,14 @@ static int fman_config(struct fman *fman) fman->state->res = fman->dts_params.res; fman->exception_cb = fman_exceptions; fman->bus_error_cb = fman_bus_error; - fman->fpm_regs = base_addr + FPM_OFFSET; - fman->bmi_regs = base_addr + BMI_OFFSET; - fman->qmi_regs = base_addr + QMI_OFFSET; - fman->dma_regs = base_addr + DMA_OFFSET; - fman->hwp_regs = base_addr + HWP_OFFSET; - fman->kg_regs = base_addr + KG_OFFSET; - fman->base_addr = base_addr; + fman->fpm_regs = fman->dts_params.base_addr_pol + FPM_OFFSET_FROM_POL; + fman->bmi_regs = fman->dts_params.base_addr_0 + BMI_OFFSET; + fman->qmi_regs = fman->dts_params.base_addr_0 + QMI_OFFSET; + fman->dma_regs = fman->dts_params.base_addr_pol + DMA_OFFSET_FROM_POL; + fman->hwp_regs = fman->dts_params.base_addr_pol + HWP_OFFSET_FROM_POL; + fman->kg_regs = fman->dts_params.base_addr_pol + KG_OFFSET_FROM_POL; + fman->base_addr_0 = fman->dts_params.base_addr_0; + fman->base_addr_pol = fman->dts_params.base_addr_pol; spin_lock_init(&fman->spinlock); fman_defconfig(fman->cfg); @@ -1937,8 +1938,8 @@ static int fman_init(struct fman *fman) fman->state->exceptions &= ~FMAN_EX_QMI_SINGLE_ECC; /* clear CPG */ - memset_io((void __iomem *)(fman->base_addr + CGP_OFFSET), 0, - fman->state->fm_port_num_of_cg); + memset_io((void __iomem *)(fman->base_addr_pol + CGP_OFFSET_FROM_POL), + 0, fman->state->fm_port_num_of_cg); /* Save LIODN info before FMan reset * Skipping non-existent port 0 (i = 1) @@ -2717,13 +2718,11 @@ static struct fman *read_dts_node(struct platform_device *of_dev) { struct fman *fman; struct device_node *fm_node, *muram_node; - struct resource *res; + struct resource *tmp_res, *main_res; u32 val, range[2]; int err, irq; struct clk *clk; u32 clk_rate; - phys_addr_t phys_base_addr; - resource_size_t mem_size; fman = kzalloc(sizeof(*fman), GFP_KERNEL); if (!fman) @@ -2740,34 +2739,31 @@ static struct fman *read_dts_node(struct platform_device *of_dev) fman->dts_params.id = (u8)val; /* Get the FM interrupt */ - res = platform_get_resource(of_dev, IORESOURCE_IRQ, 0); - if (!res) { + tmp_res = platform_get_resource(of_dev, IORESOURCE_IRQ, 0); + if (!tmp_res) { dev_err(&of_dev->dev, "%s: Can't get FMan IRQ resource\n", __func__); goto fman_node_put; } - irq = res->start; + irq = tmp_res->start; /* Get the FM error interrupt */ - res = platform_get_resource(of_dev, IORESOURCE_IRQ, 1); - if (!res) { + tmp_res = platform_get_resource(of_dev, IORESOURCE_IRQ, 1); + if (!tmp_res) { dev_err(&of_dev->dev, "%s: Can't get FMan Error IRQ resource\n", __func__); goto fman_node_put; } - fman->dts_params.err_irq = res->start; + fman->dts_params.err_irq = tmp_res->start; /* Get the FM address */ - res = platform_get_resource(of_dev, IORESOURCE_MEM, 0); - if (!res) { + main_res = platform_get_resource(of_dev, IORESOURCE_MEM, 0); + if (!main_res) { dev_err(&of_dev->dev, "%s: Can't get FMan memory resource\n", __func__); goto fman_node_put; } - phys_base_addr = res->start; - mem_size = resource_size(res); - clk = of_clk_get(fm_node, 0); if (IS_ERR(clk)) { dev_err(&of_dev->dev, "%s: Failed to get FM%d clock structure\n", @@ -2832,22 +2828,47 @@ static struct fman *read_dts_node(struct platform_device *of_dev) } } - fman->dts_params.res = - devm_request_mem_region(&of_dev->dev, phys_base_addr, - mem_size, "fman"); - if (!fman->dts_params.res) { - dev_err(&of_dev->dev, "%s: request_mem_region() failed\n", + err = devm_request_resource(&of_dev->dev, &iomem_resource, main_res); + if (err) { + dev_err(&of_dev->dev, "%s: devm_request_resource() failed\n", + __func__); + goto fman_free; + } + + fman->dts_params.res = main_res; + + tmp_res = devm_request_mem_region(&of_dev->dev, main_res->start, + SIZE_REGION_0, "fman"); + if (!tmp_res) { + dev_err(&of_dev->dev, "%s: devm_request_mem_region() failed\n", __func__); goto fman_free; } - fman->dts_params.base_addr = - devm_ioremap(&of_dev->dev, phys_base_addr, mem_size); - if (!fman->dts_params.base_addr) { + fman->dts_params.base_addr_0 = + devm_ioremap(&of_dev->dev, tmp_res->start, + resource_size(tmp_res)); + if (!fman->dts_params.base_addr_0) { dev_err(&of_dev->dev, "%s: devm_ioremap() failed\n", __func__); goto fman_free; } + tmp_res = devm_request_mem_region(&of_dev->dev, + main_res->start + POL_OFFSET, + SIZE_REGION_FROM_POL, "fman"); + if (!tmp_res) { + dev_err(&of_dev->dev, "%s: devm_request_mem_region() failed\n", + __func__); + goto fman_free; + } + + fman->dts_params.base_addr_pol = + devm_ioremap(&of_dev->dev, tmp_res->start, + resource_size(tmp_res)); + if (!fman->dts_params.base_addr_pol) { + dev_err(&of_dev->dev, "%s: devm_ioremap() failed\n", __func__); + goto fman_free; + } fman->dev = &of_dev->dev; err = of_platform_populate(fm_node, NULL, NULL, &of_dev->dev); diff --git a/drivers/net/ethernet/freescale/fman/fman.h b/drivers/net/ethernet/freescale/fman/fman.h index f2ede1360f03..e6b339c57230 100644 --- a/drivers/net/ethernet/freescale/fman/fman.h +++ b/drivers/net/ethernet/freescale/fman/fman.h @@ -306,7 +306,11 @@ typedef irqreturn_t (fman_bus_error_cb)(struct fman *fman, u8 port_id, /* Structure that holds information received from device tree */ struct fman_dts_params { - void __iomem *base_addr; /* FMan virtual address */ + void __iomem *base_addr_0; /* FMan virtual address */ + void __iomem *base_addr_pol; /* FMan virtual address + * second region starting at + * policer offset + */ struct resource *res; /* FMan memory resource */ u8 id; /* FMan ID */ @@ -322,7 +326,8 @@ struct fman_dts_params { struct fman { struct device *dev; - void __iomem *base_addr; + void __iomem *base_addr_0; + void __iomem *base_addr_pol; struct fman_intr_src intr_mng[FMAN_EV_CNT]; struct fman_fpm_regs __iomem *fpm_regs;
The main fman driver is only using some parts of the fman memory region. Split the reservation of the main region in 2, so that the other regions that will be used by fman-port and fman-mac drivers can be reserved properly and not be in conflict with the main fman reservation. Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com> --- drivers/net/ethernet/freescale/fman/fman.c | 103 +++++++++++++-------- drivers/net/ethernet/freescale/fman/fman.h | 9 +- 2 files changed, 69 insertions(+), 43 deletions(-)