linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] net: freescale/fman: Split the main resource region reservation
@ 2020-12-02 16:15 Patrick Havelange
  2020-12-02 16:15 ` [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region Patrick Havelange
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Patrick Havelange @ 2020-12-02 16:15 UTC (permalink / raw)
  To: Madalin Bucur, David S. Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: Patrick Havelange

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(-)

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;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region
  2020-12-02 16:15 [PATCH 1/4] net: freescale/fman: Split the main resource region reservation Patrick Havelange
@ 2020-12-02 16:15 ` Patrick Havelange
  2020-12-03  8:44   ` Madalin Bucur
  2020-12-02 16:15 ` [PATCH 3/4] net: freescale/fman-mac: " Patrick Havelange
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Patrick Havelange @ 2020-12-02 16:15 UTC (permalink / raw)
  To: Madalin Bucur, David S. Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: Patrick Havelange

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/4] net: freescale/fman-mac: remove direct use of __devm_request_region
  2020-12-02 16:15 [PATCH 1/4] net: freescale/fman: Split the main resource region reservation Patrick Havelange
  2020-12-02 16:15 ` [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region Patrick Havelange
@ 2020-12-02 16:15 ` Patrick Havelange
  2020-12-02 16:16 ` [PATCH 4/4] net: freescale/fman: remove fman_get_mem_region Patrick Havelange
  2020-12-02 16:49 ` [PATCH 1/4] net: freescale/fman: Split the main resource region reservation Andrew Lunn
  3 siblings, 0 replies; 7+ messages in thread
From: Patrick Havelange @ 2020-12-02 16:15 UTC (permalink / raw)
  To: Madalin Bucur, David S. Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: Patrick Havelange

Since the main fman driver is no longer reserving the complete fman
memory region, it is no longer needed to use a custom call to
__devm_request_region, so replace it with devm_request_mem_region

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
---
 drivers/net/ethernet/freescale/fman/mac.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/mac.c b/drivers/net/ethernet/freescale/fman/mac.c
index 901749a7a318..35ca33335aed 100644
--- a/drivers/net/ethernet/freescale/fman/mac.c
+++ b/drivers/net/ethernet/freescale/fman/mac.c
@@ -690,12 +690,10 @@ static int mac_probe(struct platform_device *_of_dev)
 		goto _return_of_get_parent;
 	}
 
-	mac_dev->res = __devm_request_region(dev,
-					     fman_get_mem_region(priv->fman),
-					     res.start, resource_size(&res),
-					     "mac");
+	mac_dev->res = devm_request_mem_region(dev, res.start,
+					       resource_size(&res), "mac");
 	if (!mac_dev->res) {
-		dev_err(dev, "__devm_request_mem_region(mac) failed\n");
+		dev_err(dev, "devm_request_mem_region(mac) failed\n");
 		err = -EBUSY;
 		goto _return_of_get_parent;
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 4/4] net: freescale/fman: remove fman_get_mem_region
  2020-12-02 16:15 [PATCH 1/4] net: freescale/fman: Split the main resource region reservation Patrick Havelange
  2020-12-02 16:15 ` [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region Patrick Havelange
  2020-12-02 16:15 ` [PATCH 3/4] net: freescale/fman-mac: " Patrick Havelange
@ 2020-12-02 16:16 ` Patrick Havelange
  2020-12-02 16:49 ` [PATCH 1/4] net: freescale/fman: Split the main resource region reservation Andrew Lunn
  3 siblings, 0 replies; 7+ messages in thread
From: Patrick Havelange @ 2020-12-02 16:16 UTC (permalink / raw)
  To: Madalin Bucur, David S. Miller, Jakub Kicinski, netdev, linux-kernel
  Cc: Patrick Havelange

This function is no longer used, so we can remove it.
The pointer to the resource that was kept inside
struct fman_state_struct can also be removed for the same reason.

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
---
 drivers/net/ethernet/freescale/fman/fman.c | 17 -----------------
 drivers/net/ethernet/freescale/fman/fman.h |  2 --
 2 files changed, 19 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 2e85209d560d..bf78e12a1683 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -531,8 +531,6 @@ struct fman_state_struct {
 
 	u32 qman_channel_base;
 	u32 num_of_qman_channels;
-
-	struct resource *res;
 };
 
 /* Structure that holds FMan initial configuration */
@@ -1737,7 +1735,6 @@ static int fman_config(struct fman *fman)
 	fman->state->qman_channel_base = fman->dts_params.qman_channel_base;
 	fman->state->num_of_qman_channels =
 		fman->dts_params.num_of_qman_channels;
-	fman->state->res = fman->dts_params.res;
 	fman->exception_cb = fman_exceptions;
 	fman->bus_error_cb = fman_bus_error;
 	fman->fpm_regs = fman->dts_params.base_addr_pol + FPM_OFFSET_FROM_POL;
@@ -2405,20 +2402,6 @@ u32 fman_get_qman_channel_id(struct fman *fman, u32 port_id)
 }
 EXPORT_SYMBOL(fman_get_qman_channel_id);
 
-/**
- * fman_get_mem_region
- * @fman:	A Pointer to FMan device
- *
- * Get FMan memory region
- *
- * Return: A structure with FMan memory region information
- */
-struct resource *fman_get_mem_region(struct fman *fman)
-{
-	return fman->state->res;
-}
-EXPORT_SYMBOL(fman_get_mem_region);
-
 /* Bootargs defines */
 /* Extra headroom for RX buffers - Default, min and max */
 #define FSL_FM_RX_EXTRA_HEADROOM	64
diff --git a/drivers/net/ethernet/freescale/fman/fman.h b/drivers/net/ethernet/freescale/fman/fman.h
index e6b339c57230..e326aa37b8b2 100644
--- a/drivers/net/ethernet/freescale/fman/fman.h
+++ b/drivers/net/ethernet/freescale/fman/fman.h
@@ -398,8 +398,6 @@ int fman_set_mac_max_frame(struct fman *fman, u8 mac_id, u16 mfl);
 
 u32 fman_get_qman_channel_id(struct fman *fman, u32 port_id);
 
-struct resource *fman_get_mem_region(struct fman *fman);
-
 u16 fman_get_max_frm(void);
 
 int fman_get_rx_extra_headroom(void);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/4] net: freescale/fman: Split the main resource region reservation
  2020-12-02 16:15 [PATCH 1/4] net: freescale/fman: Split the main resource region reservation Patrick Havelange
                   ` (2 preceding siblings ...)
  2020-12-02 16:16 ` [PATCH 4/4] net: freescale/fman: remove fman_get_mem_region Patrick Havelange
@ 2020-12-02 16:49 ` Andrew Lunn
  3 siblings, 0 replies; 7+ messages in thread
From: Andrew Lunn @ 2020-12-02 16:49 UTC (permalink / raw)
  To: Patrick Havelange
  Cc: Madalin Bucur, David S. Miller, Jakub Kicinski, netdev, linux-kernel

Hi Patrick

Please always include a patch [0/x] which explains the big picture,
what the patchset as a whole is trying to achieve.

     Andrew

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region
  2020-12-02 16:15 ` [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region Patrick Havelange
@ 2020-12-03  8:44   ` Madalin Bucur
  2020-12-03 13:54     ` Patrick Havelange
  0 siblings, 1 reply; 7+ messages in thread
From: Madalin Bucur @ 2020-12-03  8:44 UTC (permalink / raw)
  To: Patrick Havelange, David S. Miller, Jakub Kicinski, netdev, linux-kernel

> -----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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region
  2020-12-03  8:44   ` Madalin Bucur
@ 2020-12-03 13:54     ` Patrick Havelange
  0 siblings, 0 replies; 7+ messages in thread
From: Patrick Havelange @ 2020-12-03 13:54 UTC (permalink / raw)
  To: Madalin Bucur, David S. Miller, Jakub Kicinski, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-12-03 13:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-02 16:15 [PATCH 1/4] net: freescale/fman: Split the main resource region reservation Patrick Havelange
2020-12-02 16:15 ` [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region Patrick Havelange
2020-12-03  8:44   ` Madalin Bucur
2020-12-03 13:54     ` Patrick Havelange
2020-12-02 16:15 ` [PATCH 3/4] net: freescale/fman-mac: " Patrick Havelange
2020-12-02 16:16 ` [PATCH 4/4] net: freescale/fman: remove fman_get_mem_region Patrick Havelange
2020-12-02 16:49 ` [PATCH 1/4] net: freescale/fman: Split the main resource region reservation Andrew Lunn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).