[PATCH net 0/4] freescale/fman: remove usage of __devm_request_region

[PATCH net 0/4] freescale/fman: remove usage of __devm_request_region

[Patrick Havelange]

Hello, 

This patch series is solving a bug inside 
ethernet/freescale/fman/fman_port.c caused by an incorrect usage of
__devm_request_region.
The bug came from the fact that the resource given as parameter to the
function __devm_request_region is on the stack, leading to an invalid
pointer being stored inside the devres structure, and the new resource
pointer being lost.
In order to solve this, it is first necessary to make the regular call
devm_request_mem_region work.
This call cannot work as is, because the main fman driver has already
reserved the memory region used by the fman_port driver.

Thus the first action is to split the memory region reservation done in
the main fman driver in two, so that the regions used by fman_port will
not be reserved. The main memory regions have also been reduced to not
request the memory regions used by fman_mac.

Once this first step is done, regular usage of devm_request_mem_region
can be done.
This also leads to a bit of code removal done in the last patch.

1. fman: split the memory region of the main fman driver, so the memory that
will be used by fman_port & fman_mac will not be already reserved.

2. fman_port: replace call of __devm_request_region to devm_request_mem_region

3. fman_mac: replace call of __devm_request_region to devm_request_mem_region

4. the code is now simplified a bit, there is no need to store the main region
anymore

The whole point of this series is to be able to apply the patch N°2.
Since N°3 is a similar change, let's do it at the same time.

I think they all should be part of the same series.

Patrick Havelange (4):
  net: freescale/fman: Split the main resource region reservation
  net: freescale/fman-port: remove direct use of __devm_request_region
  net: freescale/fman-mac: remove direct use of __devm_request_region
  net: freescale/fman: remove fman_get_mem_region

 drivers/net/ethernet/freescale/fman/fman.c    | 120 +++++++++---------
 drivers/net/ethernet/freescale/fman/fman.h    |  11 +-
 .../net/ethernet/freescale/fman/fman_port.c   |   6 +-
 drivers/net/ethernet/freescale/fman/mac.c     |   8 +-
 4 files changed, 75 insertions(+), 70 deletions(-)


base-commit: 832e09798c261cf58de3a68cfcc6556408c16a5a
-- 
2.17.1

[PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[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

[PATCH net 2/4] net: freescale/fman-port: remove direct use of __devm_request_region

[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

[PATCH net 3/4] net: freescale/fman-mac: remove direct use of __devm_request_region

[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

[PATCH net 4/4] net: freescale/fman: remove fman_get_mem_region

[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

RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Madalin Bucur]

> -----Original Message-----
> From: Patrick Havelange <patrick.havelange@essensium.com>
> Sent: 03 December 2020 15:51
> 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 net 1/4] net: freescale/fman: Split the main resource
> region reservation
> 
> 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>

I think the problem you are trying to work on here is that the device
tree entry that describes the FMan IP allots to the parent FMan device the
whole memory-mapped registers area, as described in the device datasheet.
The smaller FMan building blocks (ports, MDIO controllers, etc.) are
each using a nested subset of this memory-mapped registers area.
While this hierarchical depiction of the hardware has not posed a problem
to date, it is possible to cause issues if both the FMan driver and any
of the sub-blocks drivers are trying to exclusively reserve the registers
area. I'm assuming this is the problem you are trying to address here,
besides the stack corruption issue. While for the latter I think we can
put together a quick fix, for the former I'd like to take a bit of time
to select the best fix, if one is really needed. So, please, let's split
the two problems and first address the incorrect stack memory use.

Regards,
Madalin

Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Patrick Havelange]

On 2020-12-03 16:47, Madalin Bucur wrote:
>> -----Original Message-----
>> From: Patrick Havelange <patrick.havelange@essensium.com>
>> Sent: 03 December 2020 15:51
>> 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 net 1/4] net: freescale/fman: Split the main resource
>> region reservation
>>
>> 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>
> 
> I think the problem you are trying to work on here is that the device
> tree entry that describes the FMan IP allots to the parent FMan device the
> whole memory-mapped registers area, as described in the device datasheet.
> The smaller FMan building blocks (ports, MDIO controllers, etc.) are
> each using a nested subset of this memory-mapped registers area.
> While this hierarchical depiction of the hardware has not posed a problem
> to date, it is possible to cause issues if both the FMan driver and any
> of the sub-blocks drivers are trying to exclusively reserve the registers
> area. I'm assuming this is the problem you are trying to address here,
> besides the stack corruption issue.

Yes exactly.
I did not add this behaviour (having a main region and subdrivers using 
subregions), I'm just trying to correct what is already there.
For example: this is some content of /proc/iomem for one board I'm 
working with, with the current existing code:
ffe400000-ffe4fdfff : fman
   ffe4e0000-ffe4e0fff : mac
   ffe4e2000-ffe4e2fff : mac
   ffe4e4000-ffe4e4fff : mac
   ffe4e6000-ffe4e6fff : mac
   ffe4e8000-ffe4e8fff : mac

and now with my patches:
ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000
   ffe400000-ffe480fff : fman
   ffe488000-ffe488fff : fman-port
   ffe489000-ffe489fff : fman-port
   ffe48a000-ffe48afff : fman-port
   ffe48b000-ffe48bfff : fman-port
   ffe48c000-ffe48cfff : fman-port
   ffe4a8000-ffe4a8fff : fman-port
   ffe4a9000-ffe4a9fff : fman-port
   ffe4aa000-ffe4aafff : fman-port
   ffe4ab000-ffe4abfff : fman-port
   ffe4ac000-ffe4acfff : fman-port
   ffe4c0000-ffe4dffff : fman
   ffe4e0000-ffe4e0fff : mac
   ffe4e2000-ffe4e2fff : mac
   ffe4e4000-ffe4e4fff : mac
   ffe4e6000-ffe4e6fff : mac
   ffe4e8000-ffe4e8fff : mac

> While for the latter I think we can
> put together a quick fix, for the former I'd like to take a bit of time
> to select the best fix, if one is really needed. So, please, let's split
> the two problems and first address the incorrect stack memory use.

I have no idea how you can fix it without a (more correct this time) 
dummy region passed as parameter (and you don't want to use the first 
patch). But then it will be useless to do the call anyway, as it won't 
do any proper verification at all, so it could also be removed entirely, 
which begs the question, why do it at all in the first place (the 
devm_request_mem_region).

I'm not an expert in that part of the code so feel free to correct me if 
I missed something.

BR,

Patrick H.

RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Madalin Bucur]

> -----Original Message-----
> From: Patrick Havelange <patrick.havelange@essensium.com>
> Sent: 08 December 2020 16:56
> 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
> Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource
> region reservation
> 
> On 2020-12-03 16:47, Madalin Bucur wrote:
> >> -----Original Message-----
> >> From: Patrick Havelange <patrick.havelange@essensium.com>
> >> Sent: 03 December 2020 15:51
> >> 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 net 1/4] net: freescale/fman: Split the main resource
> >> region reservation
> >>
> >> 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>
> >
> > I think the problem you are trying to work on here is that the device
> > tree entry that describes the FMan IP allots to the parent FMan device
> the
> > whole memory-mapped registers area, as described in the device datasheet.
> > The smaller FMan building blocks (ports, MDIO controllers, etc.) are
> > each using a nested subset of this memory-mapped registers area.
> > While this hierarchical depiction of the hardware has not posed a
> problem
> > to date, it is possible to cause issues if both the FMan driver and any
> > of the sub-blocks drivers are trying to exclusively reserve the
> registers
> > area. I'm assuming this is the problem you are trying to address here,
> > besides the stack corruption issue.
> 
> Yes exactly.
> I did not add this behaviour (having a main region and subdrivers using
> subregions), I'm just trying to correct what is already there.
> For example: this is some content of /proc/iomem for one board I'm
> working with, with the current existing code:
> ffe400000-ffe4fdfff : fman
>    ffe4e0000-ffe4e0fff : mac
>    ffe4e2000-ffe4e2fff : mac
>    ffe4e4000-ffe4e4fff : mac
>    ffe4e6000-ffe4e6fff : mac
>    ffe4e8000-ffe4e8fff : mac
> 
> and now with my patches:
> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000
>    ffe400000-ffe480fff : fman
>    ffe488000-ffe488fff : fman-port
>    ffe489000-ffe489fff : fman-port
>    ffe48a000-ffe48afff : fman-port
>    ffe48b000-ffe48bfff : fman-port
>    ffe48c000-ffe48cfff : fman-port
>    ffe4a8000-ffe4a8fff : fman-port
>    ffe4a9000-ffe4a9fff : fman-port
>    ffe4aa000-ffe4aafff : fman-port
>    ffe4ab000-ffe4abfff : fman-port
>    ffe4ac000-ffe4acfff : fman-port
>    ffe4c0000-ffe4dffff : fman
>    ffe4e0000-ffe4e0fff : mac
>    ffe4e2000-ffe4e2fff : mac
>    ffe4e4000-ffe4e4fff : mac
>    ffe4e6000-ffe4e6fff : mac
>    ffe4e8000-ffe4e8fff : mac
> 
> > While for the latter I think we can
> > put together a quick fix, for the former I'd like to take a bit of time
> > to select the best fix, if one is really needed. So, please, let's split
> > the two problems and first address the incorrect stack memory use.
> 
> I have no idea how you can fix it without a (more correct this time)
> dummy region passed as parameter (and you don't want to use the first
> patch). But then it will be useless to do the call anyway, as it won't
> do any proper verification at all, so it could also be removed entirely,
> which begs the question, why do it at all in the first place (the
> devm_request_mem_region).
> 
> I'm not an expert in that part of the code so feel free to correct me if
> I missed something.
> 
> BR,
> 
> Patrick H.

Hi, Patrick,

the DPAA entities are described in the device tree. Adding some hardcoding in
the driver is not really the solution for this problem. And I'm not sure we have
a clear problem statement to start with. Can you help me on that part?

Madalin

Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Patrick Havelange]

>>> area. I'm assuming this is the problem you are trying to address here,
>>> besides the stack corruption issue.
>>
>> Yes exactly.
>> I did not add this behaviour (having a main region and subdrivers using
>> subregions), I'm just trying to correct what is already there.
>> For example: this is some content of /proc/iomem for one board I'm
>> working with, with the current existing code:
>> ffe400000-ffe4fdfff : fman
>>     ffe4e0000-ffe4e0fff : mac
>>     ffe4e2000-ffe4e2fff : mac
>>     ffe4e4000-ffe4e4fff : mac
>>     ffe4e6000-ffe4e6fff : mac
>>     ffe4e8000-ffe4e8fff : mac
>>
>> and now with my patches:
>> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000
>>     ffe400000-ffe480fff : fman
>>     ffe488000-ffe488fff : fman-port
>>     ffe489000-ffe489fff : fman-port
>>     ffe48a000-ffe48afff : fman-port
>>     ffe48b000-ffe48bfff : fman-port
>>     ffe48c000-ffe48cfff : fman-port
>>     ffe4a8000-ffe4a8fff : fman-port
>>     ffe4a9000-ffe4a9fff : fman-port
>>     ffe4aa000-ffe4aafff : fman-port
>>     ffe4ab000-ffe4abfff : fman-port
>>     ffe4ac000-ffe4acfff : fman-port
>>     ffe4c0000-ffe4dffff : fman
>>     ffe4e0000-ffe4e0fff : mac
>>     ffe4e2000-ffe4e2fff : mac
>>     ffe4e4000-ffe4e4fff : mac
>>     ffe4e6000-ffe4e6fff : mac
>>     ffe4e8000-ffe4e8fff : mac
>>
>>> While for the latter I think we can
>>> put together a quick fix, for the former I'd like to take a bit of time
>>> to select the best fix, if one is really needed. So, please, let's split
>>> the two problems and first address the incorrect stack memory use.
>>
>> I have no idea how you can fix it without a (more correct this time)
>> dummy region passed as parameter (and you don't want to use the first
>> patch). But then it will be useless to do the call anyway, as it won't
>> do any proper verification at all, so it could also be removed entirely,
>> which begs the question, why do it at all in the first place (the
>> devm_request_mem_region).
>>
>> I'm not an expert in that part of the code so feel free to correct me if
>> I missed something.
>>
>> BR,
>>
>> Patrick H.
> 
> Hi, Patrick,
> 
> the DPAA entities are described in the device tree. Adding some hardcoding in
> the driver is not really the solution for this problem. And I'm not sure we have

I'm not seeing any problem here, the offsets used by the fman driver 
were already there, I just reorganized them in 2 blocks.

> a clear problem statement to start with. Can you help me on that part?

- The current call to __devm_request_region in fman_port.c is not correct.

One way to fix this is to use devm_request_mem_region, however this 
requires that the main fman would not be reserving the whole region. 
This leads to the second problem:
- Make sure the main fman driver is not reserving the whole region.

Is that clearer like this ?

Patrick H.

> 
> Madalin
> 

RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Madalin Bucur]

> -----Original Message-----
> From: Patrick Havelange <patrick.havelange@essensium.com>
> Sent: 09 December 2020 16:17
> 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
> Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource
> region reservation
> 
> >>> area. I'm assuming this is the problem you are trying to address here,
> >>> besides the stack corruption issue.
> >>
> >> Yes exactly.
> >> I did not add this behaviour (having a main region and subdrivers using
> >> subregions), I'm just trying to correct what is already there.
> >> For example: this is some content of /proc/iomem for one board I'm
> >> working with, with the current existing code:
> >> ffe400000-ffe4fdfff : fman
> >>     ffe4e0000-ffe4e0fff : mac
> >>     ffe4e2000-ffe4e2fff : mac
> >>     ffe4e4000-ffe4e4fff : mac
> >>     ffe4e6000-ffe4e6fff : mac
> >>     ffe4e8000-ffe4e8fff : mac
> >>
> >> and now with my patches:
> >> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000
> >>     ffe400000-ffe480fff : fman
> >>     ffe488000-ffe488fff : fman-port
> >>     ffe489000-ffe489fff : fman-port
> >>     ffe48a000-ffe48afff : fman-port
> >>     ffe48b000-ffe48bfff : fman-port
> >>     ffe48c000-ffe48cfff : fman-port
> >>     ffe4a8000-ffe4a8fff : fman-port
> >>     ffe4a9000-ffe4a9fff : fman-port
> >>     ffe4aa000-ffe4aafff : fman-port
> >>     ffe4ab000-ffe4abfff : fman-port
> >>     ffe4ac000-ffe4acfff : fman-port
> >>     ffe4c0000-ffe4dffff : fman
> >>     ffe4e0000-ffe4e0fff : mac
> >>     ffe4e2000-ffe4e2fff : mac
> >>     ffe4e4000-ffe4e4fff : mac
> >>     ffe4e6000-ffe4e6fff : mac
> >>     ffe4e8000-ffe4e8fff : mac
> >>
> >>> While for the latter I think we can
> >>> put together a quick fix, for the former I'd like to take a bit of
> time
> >>> to select the best fix, if one is really needed. So, please, let's
> split
> >>> the two problems and first address the incorrect stack memory use.
> >>
> >> I have no idea how you can fix it without a (more correct this time)
> >> dummy region passed as parameter (and you don't want to use the first
> >> patch). But then it will be useless to do the call anyway, as it won't
> >> do any proper verification at all, so it could also be removed entirely,
> >> which begs the question, why do it at all in the first place (the
> >> devm_request_mem_region).
> >>
> >> I'm not an expert in that part of the code so feel free to correct me
> if
> >> I missed something.
> >>
> >> BR,
> >>
> >> Patrick H.
> >
> > Hi, Patrick,
> >
> > the DPAA entities are described in the device tree. Adding some
> hardcoding in
> > the driver is not really the solution for this problem. And I'm not sure
> we have
> 
> I'm not seeing any problem here, the offsets used by the fman driver
> were already there, I just reorganized them in 2 blocks.
> 
> > a clear problem statement to start with. Can you help me on that part?
> 
> - The current call to __devm_request_region in fman_port.c is not correct.
> 
> One way to fix this is to use devm_request_mem_region, however this
> requires that the main fman would not be reserving the whole region.
> This leads to the second problem:
> - Make sure the main fman driver is not reserving the whole region.
> 
> Is that clearer like this ?
> 
> Patrick H.

The overlapping IO areas result from the device tree description, that in turn
mimics the HW description in the manual. If we really want to remove the nesting,
we should change the device trees, not the drivers. If we want to hack it,
instead of splitting ioremaps, we can reserve 4 kB in the FMan driver,
and keep the ioremap as it is now, with the benefit of less code churn.
In the end, what the reservation is trying to achieve is to make sure there
is a single driver controlling a certain peripeheral, and this basic
requirement would be addressed by that change plus devm_of_iomap() for child
devices (ports, MACs).

Madalin

Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Patrick Havelange]

On 2020-12-09 19:55, Madalin Bucur wrote:
>> -----Original Message-----
>> From: Patrick Havelange <patrick.havelange@essensium.com>
>> Sent: 09 December 2020 16:17
>> 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
>> Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource
>> region reservation
>>
>>>>> area. I'm assuming this is the problem you are trying to address here,
>>>>> besides the stack corruption issue.
>>>>
>>>> Yes exactly.
>>>> I did not add this behaviour (having a main region and subdrivers using
>>>> subregions), I'm just trying to correct what is already there.
>>>> For example: this is some content of /proc/iomem for one board I'm
>>>> working with, with the current existing code:
>>>> ffe400000-ffe4fdfff : fman
>>>>      ffe4e0000-ffe4e0fff : mac
>>>>      ffe4e2000-ffe4e2fff : mac
>>>>      ffe4e4000-ffe4e4fff : mac
>>>>      ffe4e6000-ffe4e6fff : mac
>>>>      ffe4e8000-ffe4e8fff : mac
>>>>
>>>> and now with my patches:
>>>> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000
>>>>      ffe400000-ffe480fff : fman
>>>>      ffe488000-ffe488fff : fman-port
>>>>      ffe489000-ffe489fff : fman-port
>>>>      ffe48a000-ffe48afff : fman-port
>>>>      ffe48b000-ffe48bfff : fman-port
>>>>      ffe48c000-ffe48cfff : fman-port
>>>>      ffe4a8000-ffe4a8fff : fman-port
>>>>      ffe4a9000-ffe4a9fff : fman-port
>>>>      ffe4aa000-ffe4aafff : fman-port
>>>>      ffe4ab000-ffe4abfff : fman-port
>>>>      ffe4ac000-ffe4acfff : fman-port
>>>>      ffe4c0000-ffe4dffff : fman
>>>>      ffe4e0000-ffe4e0fff : mac
>>>>      ffe4e2000-ffe4e2fff : mac
>>>>      ffe4e4000-ffe4e4fff : mac
>>>>      ffe4e6000-ffe4e6fff : mac
>>>>      ffe4e8000-ffe4e8fff : mac
>>>>
>>>>> While for the latter I think we can
>>>>> put together a quick fix, for the former I'd like to take a bit of
>> time
>>>>> to select the best fix, if one is really needed. So, please, let's
>> split
>>>>> the two problems and first address the incorrect stack memory use.
>>>>
>>>> I have no idea how you can fix it without a (more correct this time)
>>>> dummy region passed as parameter (and you don't want to use the first
>>>> patch). But then it will be useless to do the call anyway, as it won't
>>>> do any proper verification at all, so it could also be removed entirely,
>>>> which begs the question, why do it at all in the first place (the
>>>> devm_request_mem_region).
>>>>
>>>> I'm not an expert in that part of the code so feel free to correct me
>> if
>>>> I missed something.
>>>>
>>>> BR,
>>>>
>>>> Patrick H.
>>>
>>> Hi, Patrick,
>>>
>>> the DPAA entities are described in the device tree. Adding some
>> hardcoding in
>>> the driver is not really the solution for this problem. And I'm not sure
>> we have
>>
>> I'm not seeing any problem here, the offsets used by the fman driver
>> were already there, I just reorganized them in 2 blocks.
>>
>>> a clear problem statement to start with. Can you help me on that part?
>>
>> - The current call to __devm_request_region in fman_port.c is not correct.
>>
>> One way to fix this is to use devm_request_mem_region, however this
>> requires that the main fman would not be reserving the whole region.
>> This leads to the second problem:
>> - Make sure the main fman driver is not reserving the whole region.
>>
>> Is that clearer like this ?
>>
>> Patrick H.

Hi,

> 
> The overlapping IO areas result from the device tree description, that in turn
> mimics the HW description in the manual. If we really want to remove the nesting,
> we should change the device trees, not the drivers.

But then that change would not be compatible with the existing device 
trees in already existing hardware. I'm not sure how to handle that case 
properly.

> If we want to hack it,
> instead of splitting ioremaps, we can reserve 4 kB in the FMan driver,
> and keep the ioremap as it is now, with the benefit of less code churn.

but then the ioremap and the memory reservation do not match. Why bother 
at all then with the mem reservation, just ioremap only and be done with 
it. What I'm saying is, I don't see the point of having a "fake" 
reservation call if it does not correspond that what is being used.

> In the end, what the reservation is trying to achieve is to make sure there
> is a single driver controlling a certain peripeheral, and this basic
> requirement would be addressed by that change plus devm_of_iomap() for child
> devices (ports, MACs).

Again, correct me if I'm wrong, but with the fake mem reservation, it 
would *not* make sure that a single driver is controlling a certain 
peripheral.

My point is, either have a *correct* mem reservation, or don't have one 
at all. There is no point in trying to cheat the system.

Patrick H.

> 
> Madalin
> 

RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Madalin Bucur]

> -----Original Message-----
> From: Patrick Havelange <patrick.havelange@essensium.com>
> Sent: 10 December 2020 10:49
> 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
> Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource
> region reservation
> 
> On 2020-12-09 19:55, Madalin Bucur wrote:
> >> -----Original Message-----
> >> From: Patrick Havelange <patrick.havelange@essensium.com>
> >> Sent: 09 December 2020 16:17
> >> 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
> >> Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main
> resource
> >> region reservation
> >>
> >>>>> area. I'm assuming this is the problem you are trying to address
> here,
> >>>>> besides the stack corruption issue.
> >>>>
> >>>> Yes exactly.
> >>>> I did not add this behaviour (having a main region and subdrivers
> using
> >>>> subregions), I'm just trying to correct what is already there.
> >>>> For example: this is some content of /proc/iomem for one board I'm
> >>>> working with, with the current existing code:
> >>>> ffe400000-ffe4fdfff : fman
> >>>>      ffe4e0000-ffe4e0fff : mac
> >>>>      ffe4e2000-ffe4e2fff : mac
> >>>>      ffe4e4000-ffe4e4fff : mac
> >>>>      ffe4e6000-ffe4e6fff : mac
> >>>>      ffe4e8000-ffe4e8fff : mac
> >>>>
> >>>> and now with my patches:
> >>>> ffe400000-ffe4fdfff : /soc@ffe000000/fman@400000
> >>>>      ffe400000-ffe480fff : fman
> >>>>      ffe488000-ffe488fff : fman-port
> >>>>      ffe489000-ffe489fff : fman-port
> >>>>      ffe48a000-ffe48afff : fman-port
> >>>>      ffe48b000-ffe48bfff : fman-port
> >>>>      ffe48c000-ffe48cfff : fman-port
> >>>>      ffe4a8000-ffe4a8fff : fman-port
> >>>>      ffe4a9000-ffe4a9fff : fman-port
> >>>>      ffe4aa000-ffe4aafff : fman-port
> >>>>      ffe4ab000-ffe4abfff : fman-port
> >>>>      ffe4ac000-ffe4acfff : fman-port
> >>>>      ffe4c0000-ffe4dffff : fman
> >>>>      ffe4e0000-ffe4e0fff : mac
> >>>>      ffe4e2000-ffe4e2fff : mac
> >>>>      ffe4e4000-ffe4e4fff : mac
> >>>>      ffe4e6000-ffe4e6fff : mac
> >>>>      ffe4e8000-ffe4e8fff : mac
> >>>>
> >>>>> While for the latter I think we can
> >>>>> put together a quick fix, for the former I'd like to take a bit of
> >> time
> >>>>> to select the best fix, if one is really needed. So, please, let's
> >> split
> >>>>> the two problems and first address the incorrect stack memory use.
> >>>>
> >>>> I have no idea how you can fix it without a (more correct this time)
> >>>> dummy region passed as parameter (and you don't want to use the first
> >>>> patch). But then it will be useless to do the call anyway, as it
> won't
> >>>> do any proper verification at all, so it could also be removed
> entirely,
> >>>> which begs the question, why do it at all in the first place (the
> >>>> devm_request_mem_region).
> >>>>
> >>>> I'm not an expert in that part of the code so feel free to correct me
> >> if
> >>>> I missed something.
> >>>>
> >>>> BR,
> >>>>
> >>>> Patrick H.
> >>>
> >>> Hi, Patrick,
> >>>
> >>> the DPAA entities are described in the device tree. Adding some
> >> hardcoding in
> >>> the driver is not really the solution for this problem. And I'm not
> sure
> >> we have
> >>
> >> I'm not seeing any problem here, the offsets used by the fman driver
> >> were already there, I just reorganized them in 2 blocks.
> >>
> >>> a clear problem statement to start with. Can you help me on that part?
> >>
> >> - The current call to __devm_request_region in fman_port.c is not
> correct.
> >>
> >> One way to fix this is to use devm_request_mem_region, however this
> >> requires that the main fman would not be reserving the whole region.
> >> This leads to the second problem:
> >> - Make sure the main fman driver is not reserving the whole region.
> >>
> >> Is that clearer like this ?
> >>
> >> Patrick H.
> 
> Hi,

Hi, Patrick,

> > The overlapping IO areas result from the device tree description, that
> in turn
> > mimics the HW description in the manual. If we really want to remove the
> nesting,
> > we should change the device trees, not the drivers.
> 
> But then that change would not be compatible with the existing device
> trees in already existing hardware. I'm not sure how to handle that case
> properly.

One needs to be backwards compatible with the old device trees, so we do not
really have a simple answer, I know.

> > If we want to hack it,
> > instead of splitting ioremaps, we can reserve 4 kB in the FMan driver,
> > and keep the ioremap as it is now, with the benefit of less code churn.
> 
> but then the ioremap and the memory reservation do not match. Why bother
> at all then with the mem reservation, just ioremap only and be done with
> it. What I'm saying is, I don't see the point of having a "fake"
> reservation call if it does not correspond that what is being used.

The reservation is not fake, it just covering the first portion of the ioremap.
Another hypothetical FMan driver would presumably reserve and ioremap starting
from the same point, thus the desired error would be met.

Regarding removing reservation altogether, yes, we can do that, in the child
device drivers. That will fix that use after free issue you've found and align
with the custom, hierarchical structure of the FMan devices/drivers. But would
leave them without the double use guard we have when using the reservation.

> > In the end, what the reservation is trying to achieve is to make sure
> there
> > is a single driver controlling a certain peripeheral, and this basic
> > requirement would be addressed by that change plus devm_of_iomap() for
> child
> > devices (ports, MACs).
> 
> Again, correct me if I'm wrong, but with the fake mem reservation, it
> would *not* make sure that a single driver is controlling a certain
> peripheral.

Actually, it would. If the current FMan driver reserves the first part of the FMan
memory, then another FMan driver (I do not expect a random driver trying to map the
FMan register area) would likely try to use that same part (with the same or
a different size, it does not matter, there will be an error anyway) and the
reservation attempt will fail. If we fix the child device drivers, then they
will have normal mappings and reservations.

> My point is, either have a *correct* mem reservation, or don't have one
> at all. There is no point in trying to cheat the system.

Now we do not have correct reservations for the child devices because the
parent takes it all for himself. Reduce the parent reservation and make room
for correct reservations for the child. The two-sections change you've made may
try to be correct but it's overkill for the purpose of detecting double use.
And I also find the patch to obfuscate the already not so readable code so I'd
opt for a simpler fix.

Madalin

Re: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Patrick Havelange]

On 2020-12-10 10:05, Madalin Bucur wrote:
>> -----Original Message-----
>> From: Patrick Havelange <patrick.havelange@essensium.com>

[snipped]

>>
>> But then that change would not be compatible with the existing device
>> trees in already existing hardware. I'm not sure how to handle that case
>> properly.
> 
> One needs to be backwards compatible with the old device trees, so we do not
> really have a simple answer, I know.
> 
>>> If we want to hack it,
>>> instead of splitting ioremaps, we can reserve 4 kB in the FMan driver,
>>> and keep the ioremap as it is now, with the benefit of less code churn.
>>
>> but then the ioremap and the memory reservation do not match. Why bother
>> at all then with the mem reservation, just ioremap only and be done with
>> it. What I'm saying is, I don't see the point of having a "fake"
>> reservation call if it does not correspond that what is being used.
> 
> The reservation is not fake, it just covering the first portion of the ioremap.
> Another hypothetical FMan driver would presumably reserve and ioremap starting
> from the same point, thus the desired error would be met.
> 
> Regarding removing reservation altogether, yes, we can do that, in the child
> device drivers. That will fix that use after free issue you've found and align
> with the custom, hierarchical structure of the FMan devices/drivers. But would
> leave them without the double use guard we have when using the reservation.
> 
>>> In the end, what the reservation is trying to achieve is to make sure
>> there
>>> is a single driver controlling a certain peripeheral, and this basic
>>> requirement would be addressed by that change plus devm_of_iomap() for
>> child
>>> devices (ports, MACs).
>>
>> Again, correct me if I'm wrong, but with the fake mem reservation, it
>> would *not* make sure that a single driver is controlling a certain
>> peripheral.
> 
> Actually, it would. If the current FMan driver reserves the first part of the FMan
> memory, then another FMan driver (I do not expect a random driver trying to map the
> FMan register area)

Ha!, now I understand your point. I still think it is not a clean 
solution, as the reservation do not match the ioremap usage.

> would likely try to use that same part (with the same or
> a different size, it does not matter, there will be an error anyway) and the
> reservation attempt will fail. If we fix the child device drivers, then they
> will have normal mappings and reservations.
> 
>> My point is, either have a *correct* mem reservation, or don't have one
>> at all. There is no point in trying to cheat the system.
> 
> Now we do not have correct reservations for the child devices because the
> parent takes it all for himself. Reduce the parent reservation and make room
> for correct reservations for the child. The two-sections change you've made may
> try to be correct but it's overkill for the purpose of detecting double use.

But it is not overkill if we want to detect potential subdrivers mapping 
sections that would not start at the main fman region (but still part of 
the main fman region).

> And I also find the patch to obfuscate the already not so readable code so I'd
> opt for a simpler fix.

As said already, I'm not in favor of having a reservation that do not 
match the real usage.

And in my opinion, having a mismatch with the mem reservation and the 
mem usage is also the kind of obfuscation that we want to avoid.

Yes now the code is slightly more complex, but it is also slightly more 
correct.

I'm not seeing currently another way on how to make it simpler *and* 
correct at the same time.

Patrick H.

> 
> Madalin
> 

RE: [PATCH net 1/4] net: freescale/fman: Split the main resource region reservation

[Madalin Bucur]

> -----Original Message-----
> From: Patrick Havelange <patrick.havelange@essensium.com>
> Sent: 10 December 2020 12:06
> 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
> Subject: Re: [PATCH net 1/4] net: freescale/fman: Split the main resource
> region reservation
> 
> On 2020-12-10 10:05, Madalin Bucur wrote:
> >> -----Original Message-----
> >> From: Patrick Havelange <patrick.havelange@essensium.com>
> 
> [snipped]
> 
> >>
> >> But then that change would not be compatible with the existing device
> >> trees in already existing hardware. I'm not sure how to handle that
> case
> >> properly.
> >
> > One needs to be backwards compatible with the old device trees, so we do
> not
> > really have a simple answer, I know.
> >
> >>> If we want to hack it,
> >>> instead of splitting ioremaps, we can reserve 4 kB in the FMan driver,
> >>> and keep the ioremap as it is now, with the benefit of less code churn.
> >>
> >> but then the ioremap and the memory reservation do not match. Why
> bother
> >> at all then with the mem reservation, just ioremap only and be done
> with
> >> it. What I'm saying is, I don't see the point of having a "fake"
> >> reservation call if it does not correspond that what is being used.
> >
> > The reservation is not fake, it just covering the first portion of the
> ioremap.
> > Another hypothetical FMan driver would presumably reserve and ioremap
> starting
> > from the same point, thus the desired error would be met.
> >
> > Regarding removing reservation altogether, yes, we can do that, in the
> child
> > device drivers. That will fix that use after free issue you've found and
> align
> > with the custom, hierarchical structure of the FMan devices/drivers. But
> would
> > leave them without the double use guard we have when using the
> reservation.
> >
> >>> In the end, what the reservation is trying to achieve is to make sure
> >> there
> >>> is a single driver controlling a certain peripeheral, and this basic
> >>> requirement would be addressed by that change plus devm_of_iomap() for
> >> child
> >>> devices (ports, MACs).
> >>
> >> Again, correct me if I'm wrong, but with the fake mem reservation, it
> >> would *not* make sure that a single driver is controlling a certain
> >> peripheral.
> >
> > Actually, it would. If the current FMan driver reserves the first part
> of the FMan
> > memory, then another FMan driver (I do not expect a random driver trying
> to map the
> > FMan register area)
> 
> Ha!, now I understand your point. I still think it is not a clean
> solution, as the reservation do not match the ioremap usage.
> 
> > would likely try to use that same part (with the same or
> > a different size, it does not matter, there will be an error anyway) and
> the
> > reservation attempt will fail. If we fix the child device drivers, then
> they
> > will have normal mappings and reservations.
> >
> >> My point is, either have a *correct* mem reservation, or don't have one
> >> at all. There is no point in trying to cheat the system.
> >
> > Now we do not have correct reservations for the child devices because
> the
> > parent takes it all for himself. Reduce the parent reservation and make
> room
> > for correct reservations for the child. The two-sections change you've
> made may
> > try to be correct but it's overkill for the purpose of detecting double
> use.
> 
> But it is not overkill if we want to detect potential subdrivers mapping
> sections that would not start at the main fman region (but still part of
> the main fman region).
> 
> > And I also find the patch to obfuscate the already not so readable code
> so I'd
> > opt for a simpler fix.
> 
> As said already, I'm not in favor of having a reservation that do not
> match the real usage.
> 
> And in my opinion, having a mismatch with the mem reservation and the
> mem usage is also the kind of obfuscation that we want to avoid.
> 
> Yes now the code is slightly more complex, but it is also slightly more
> correct.
> 
> I'm not seeing currently another way on how to make it simpler *and*
> correct at the same time.


Ok, we've taken note on your report and will put together a fix.

Regards,
Madalin