[PATCH net-next 0/2] sfc: fix mtd memleak and simplify list handling

[PATCH net-next 0/2] sfc: fix mtd memleak and simplify list handling

[Íñigo Huguet]

These patches fix a memleak described in the first one and simplify
the mtd list handling to make more clear how it works and avoid similar
problems in the future.

Íñigo Huguet (2):
  sfc: fix memory leak on mtd_probe
  sfc: simplify mtd partitions list handling

 drivers/net/ethernet/sfc/ef10.c        | 17 +++++++++--
 drivers/net/ethernet/sfc/efx.h         |  4 +--
 drivers/net/ethernet/sfc/efx_common.c  |  3 --
 drivers/net/ethernet/sfc/mtd.c         | 42 ++++++++++----------------
 drivers/net/ethernet/sfc/net_driver.h  |  9 ++++--
 drivers/net/ethernet/sfc/siena/siena.c |  5 +++
 6 files changed, 43 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH net-next 1/2] sfc: fix memory leak on mtd_probe

[Íñigo Huguet]

In some cases there is no mtd partitions that can be probed, so the mtd
partitions list stays empty. This happens, for example, in SFC9220
devices on the second port of the NIC.

The memory for the mtd partitions is deallocated in efx_mtd_remove,
recovering the address of the first element of efx->mtd_list and then
deallocating it. But if the list is empty, the address passed to kfree
doesn't point to the memory allocated for the mtd partitions, but to the
list head itself. Despite this hasn't caused other problems other than
the memory leak, this is obviously incorrect.

This patch deallocates the memory during mtd_probe in the case that
there are no probed partitions, avoiding the leak.

This was detected with kmemleak, output example:
unreferenced object 0xffff88819cfa0000 (size 46560):
  comm "kworker/0:2", pid 48435, jiffies 4364987018 (age 45.924s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<000000000f8e92d9>] kmalloc_order_trace+0x19/0x130
    [<0000000042a03844>] efx_ef10_mtd_probe+0x12d/0x320 [sfc]
    [<000000004555654f>] efx_pci_probe.cold+0x4e1/0x6db [sfc]
    [<00000000b03d5126>] local_pci_probe+0xde/0x170
    [<00000000376cc8d9>] work_for_cpu_fn+0x51/0xa0
    [<00000000141f8de9>] process_one_work+0x8cb/0x1590
    [<00000000cb2d8065>] worker_thread+0x707/0x1010
    [<000000001ef4b9f6>] kthread+0x364/0x420
    [<0000000014767137>] ret_from_fork+0x22/0x30

Fixes: 8127d661e77f ("sfc: Add support for Solarflare SFC9100 family")
Reported-by: Liang Li <liali@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ef10.c        | 5 +++++
 drivers/net/ethernet/sfc/siena/siena.c | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index c9ee5011803f..15a229731296 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -3579,6 +3579,11 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
 		n_parts++;
 	}
 
+	if (n_parts == 0) {
+		kfree(parts);
+		return 0;
+	}
+
 	rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
 fail:
 	if (rc)
diff --git a/drivers/net/ethernet/sfc/siena/siena.c b/drivers/net/ethernet/sfc/siena/siena.c
index 741313aff1d1..32467782e8ef 100644
--- a/drivers/net/ethernet/sfc/siena/siena.c
+++ b/drivers/net/ethernet/sfc/siena/siena.c
@@ -943,6 +943,11 @@ static int siena_mtd_probe(struct efx_nic *efx)
 		nvram_types >>= 1;
 	}
 
+	if (n_parts == 0) {
+		kfree(parts);
+		return 0;
+	}
+
 	rc = siena_mtd_get_fw_subtypes(efx, parts, n_parts);
 	if (rc)
 		goto fail;
-- 
2.34.1

[PATCH net-next 2/2] sfc: simplify mtd partitions list handling

[Íñigo Huguet]

efx_mtd_partitions are embedded inside efx_mcdi_mtd_partition structs.
They contain a list entry that is appended to efx->mtd_list, which is
traversed to perform add/remove/rename/etc operations over all
efx_mtd_partitions.

However, almost all operations done on a efx_mtd_partition asume that it
is actually embedded inside an efx_mcdi_mtd_partition, and the
deallocation asume that the first member of the list is located at the
beginning of the allocated memory.

Given all that asumptions, the possibility of having an
efx_mtd_partition not embedded in an efx_mcdi_efx_partition doesn't
exist. Neither it does the possibility of being in a memory position
other the one allocated for the efx_mcdi_mtd_partition array. Also, they
never need to be reordered.

Given all that, it is better to get rid of the list and use directly the
efx_mcdi_mtd_partition array. This shows more clearly how they lay
in memory, list traversal is more obvious and it save a small amount
of memory on the list nodes.

Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ef10.c       | 12 ++++++--
 drivers/net/ethernet/sfc/efx.h        |  4 +--
 drivers/net/ethernet/sfc/efx_common.c |  3 --
 drivers/net/ethernet/sfc/mtd.c        | 42 ++++++++++-----------------
 drivers/net/ethernet/sfc/net_driver.h |  9 ++++--
 5 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 15a229731296..b5284fa529b7 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -3584,10 +3584,16 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
 		return 0;
 	}
 
-	rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
-fail:
+	rc = efx_mtd_add(efx, parts, n_parts);
 	if (rc)
-		kfree(parts);
+		goto fail;
+	efx->mcdi_mtd_parts = parts;
+	efx->n_mcdi_mtd_parts = n_parts;
+
+	return 0;
+
+fail:
+	kfree(parts);
 	return rc;
 }
 
diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
index c05a83da9e44..2ab9ba691b0d 100644
--- a/drivers/net/ethernet/sfc/efx.h
+++ b/drivers/net/ethernet/sfc/efx.h
@@ -181,8 +181,8 @@ void efx_update_sw_stats(struct efx_nic *efx, u64 *stats);
 
 /* MTD */
 #ifdef CONFIG_SFC_MTD
-int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
-		size_t n_parts, size_t sizeof_part);
+int efx_mtd_add(struct efx_nic *efx, struct efx_mcdi_mtd_partition *parts,
+		size_t n_parts);
 static inline int efx_mtd_probe(struct efx_nic *efx)
 {
 	return efx->type->mtd_probe(efx);
diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
index f6577e74d6e6..8802790403e9 100644
--- a/drivers/net/ethernet/sfc/efx_common.c
+++ b/drivers/net/ethernet/sfc/efx_common.c
@@ -987,9 +987,6 @@ int efx_init_struct(struct efx_nic *efx,
 	INIT_LIST_HEAD(&efx->node);
 	INIT_LIST_HEAD(&efx->secondary_list);
 	spin_lock_init(&efx->biu_lock);
-#ifdef CONFIG_SFC_MTD
-	INIT_LIST_HEAD(&efx->mtd_list);
-#endif
 	INIT_WORK(&efx->reset_work, efx_reset_work);
 	INIT_DELAYED_WORK(&efx->monitor_work, efx_monitor);
 	efx_selftest_async_init(efx);
diff --git a/drivers/net/ethernet/sfc/mtd.c b/drivers/net/ethernet/sfc/mtd.c
index 273c08e5455f..4d06e8a9a729 100644
--- a/drivers/net/ethernet/sfc/mtd.c
+++ b/drivers/net/ethernet/sfc/mtd.c
@@ -12,6 +12,7 @@
 
 #include "net_driver.h"
 #include "efx.h"
+#include "mcdi.h"
 
 #define to_efx_mtd_partition(mtd)				\
 	container_of(mtd, struct efx_mtd_partition, mtd)
@@ -48,18 +49,16 @@ static void efx_mtd_remove_partition(struct efx_mtd_partition *part)
 		ssleep(1);
 	}
 	WARN_ON(rc);
-	list_del(&part->node);
 }
 
-int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
-		size_t n_parts, size_t sizeof_part)
+int efx_mtd_add(struct efx_nic *efx, struct efx_mcdi_mtd_partition *parts,
+		size_t n_parts)
 {
 	struct efx_mtd_partition *part;
 	size_t i;
 
 	for (i = 0; i < n_parts; i++) {
-		part = (struct efx_mtd_partition *)((char *)parts +
-						    i * sizeof_part);
+		part = &parts[i].common;
 
 		part->mtd.writesize = 1;
 
@@ -78,47 +77,38 @@ int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
 
 		if (mtd_device_register(&part->mtd, NULL, 0))
 			goto fail;
-
-		/* Add to list in order - efx_mtd_remove() depends on this */
-		list_add_tail(&part->node, &efx->mtd_list);
 	}
 
 	return 0;
 
 fail:
-	while (i--) {
-		part = (struct efx_mtd_partition *)((char *)parts +
-						    i * sizeof_part);
-		efx_mtd_remove_partition(part);
-	}
+	while (i--)
+		efx_mtd_remove_partition(&parts[i].common);
+
 	/* Failure is unlikely here, but probably means we're out of memory */
 	return -ENOMEM;
 }
 
 void efx_mtd_remove(struct efx_nic *efx)
 {
-	struct efx_mtd_partition *parts, *part, *next;
+	int i;
 
 	WARN_ON(efx_dev_registered(efx));
 
-	if (list_empty(&efx->mtd_list))
-		return;
-
-	parts = list_first_entry(&efx->mtd_list, struct efx_mtd_partition,
-				 node);
+	for (i = 0; i < efx->n_mcdi_mtd_parts; i++)
+		efx_mtd_remove_partition(&efx->mcdi_mtd_parts[i].common);
 
-	list_for_each_entry_safe(part, next, &efx->mtd_list, node)
-		efx_mtd_remove_partition(part);
-
-	kfree(parts);
+	kfree(efx->mcdi_mtd_parts);
+	efx->mcdi_mtd_parts = NULL;
+	efx->n_mcdi_mtd_parts = 0;
 }
 
 void efx_mtd_rename(struct efx_nic *efx)
 {
-	struct efx_mtd_partition *part;
+	int i;
 
 	ASSERT_RTNL();
 
-	list_for_each_entry(part, &efx->mtd_list, node)
-		efx->type->mtd_rename(part);
+	for (i = 0; i < efx->n_mcdi_mtd_parts; i++)
+		efx->type->mtd_rename(&efx->mcdi_mtd_parts[i].common);
 }
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 318db906a154..5d20b25b0e82 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -107,6 +107,8 @@ struct hwtstamp_config;
 
 struct efx_self_tests;
 
+struct efx_mcdi_mtd_partition;
+
 /**
  * struct efx_buffer - A general-purpose DMA buffer
  * @addr: host base address of the buffer
@@ -865,7 +867,8 @@ enum efx_xdp_tx_queues_mode {
  * @irq_zero_count: Number of legacy IRQs seen with queue flags == 0
  * @irq_level: IRQ level/index for IRQs not triggered by an event queue
  * @selftest_work: Work item for asynchronous self-test
- * @mtd_list: List of MTDs attached to the NIC
+ * @mcdi_mtd_parts: Array of MTDs attached to the NIC
+ * @n_mcdi_mtd_parts: Number of MTDs attached to the NIC
  * @nic_data: Hardware dependent state
  * @mcdi: Management-Controller-to-Driver Interface state
  * @mac_lock: MAC access lock. Protects @port_enabled, @phy_mode,
@@ -1033,7 +1036,8 @@ struct efx_nic {
 	struct delayed_work selftest_work;
 
 #ifdef CONFIG_SFC_MTD
-	struct list_head mtd_list;
+	struct efx_mcdi_mtd_partition *mcdi_mtd_parts;
+	unsigned int n_mcdi_mtd_parts;
 #endif
 
 	void *nic_data;
@@ -1134,7 +1138,6 @@ static inline unsigned int efx_port_num(struct efx_nic *efx)
 }
 
 struct efx_mtd_partition {
-	struct list_head node;
 	struct mtd_info mtd;
 	const char *dev_type_name;
 	const char *type_name;
-- 
2.34.1

Re: [PATCH net-next 1/2] sfc: fix memory leak on mtd_probe

[Martin Habets]

The same patch was submitted earlier, see
https://lore.kernel.org/netdev/20220510153619.32464-1-ap420073@gmail.com/

Martin

On Wed, May 11, 2022 at 12:36:03PM +0200, Íñigo Huguet wrote:
> In some cases there is no mtd partitions that can be probed, so the mtd
> partitions list stays empty. This happens, for example, in SFC9220
> devices on the second port of the NIC.
> 
> The memory for the mtd partitions is deallocated in efx_mtd_remove,
> recovering the address of the first element of efx->mtd_list and then
> deallocating it. But if the list is empty, the address passed to kfree
> doesn't point to the memory allocated for the mtd partitions, but to the
> list head itself. Despite this hasn't caused other problems other than
> the memory leak, this is obviously incorrect.
> 
> This patch deallocates the memory during mtd_probe in the case that
> there are no probed partitions, avoiding the leak.
> 
> This was detected with kmemleak, output example:
> unreferenced object 0xffff88819cfa0000 (size 46560):
>   comm "kworker/0:2", pid 48435, jiffies 4364987018 (age 45.924s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<000000000f8e92d9>] kmalloc_order_trace+0x19/0x130
>     [<0000000042a03844>] efx_ef10_mtd_probe+0x12d/0x320 [sfc]
>     [<000000004555654f>] efx_pci_probe.cold+0x4e1/0x6db [sfc]
>     [<00000000b03d5126>] local_pci_probe+0xde/0x170
>     [<00000000376cc8d9>] work_for_cpu_fn+0x51/0xa0
>     [<00000000141f8de9>] process_one_work+0x8cb/0x1590
>     [<00000000cb2d8065>] worker_thread+0x707/0x1010
>     [<000000001ef4b9f6>] kthread+0x364/0x420
>     [<0000000014767137>] ret_from_fork+0x22/0x30
> 
> Fixes: 8127d661e77f ("sfc: Add support for Solarflare SFC9100 family")
> Reported-by: Liang Li <liali@redhat.com>
> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> ---
>  drivers/net/ethernet/sfc/ef10.c        | 5 +++++
>  drivers/net/ethernet/sfc/siena/siena.c | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index c9ee5011803f..15a229731296 100644
> --- a/drivers/net/ethernet/sfc/ef10.c
> +++ b/drivers/net/ethernet/sfc/ef10.c
> @@ -3579,6 +3579,11 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
>  		n_parts++;
>  	}
>  
> +	if (n_parts == 0) {
> +		kfree(parts);
> +		return 0;
> +	}
> +
>  	rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
>  fail:
>  	if (rc)
> diff --git a/drivers/net/ethernet/sfc/siena/siena.c b/drivers/net/ethernet/sfc/siena/siena.c
> index 741313aff1d1..32467782e8ef 100644
> --- a/drivers/net/ethernet/sfc/siena/siena.c
> +++ b/drivers/net/ethernet/sfc/siena/siena.c
> @@ -943,6 +943,11 @@ static int siena_mtd_probe(struct efx_nic *efx)
>  		nvram_types >>= 1;
>  	}
>  
> +	if (n_parts == 0) {
> +		kfree(parts);
> +		return 0;
> +	}
> +
>  	rc = siena_mtd_get_fw_subtypes(efx, parts, n_parts);
>  	if (rc)
>  		goto fail;
> -- 
> 2.34.1

Re: [PATCH net-next 1/2] sfc: fix memory leak on mtd_probe

[Íñigo Huguet]

On Wed, May 11, 2022 at 2:58 PM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> The same patch was submitted earlier, see
> https://lore.kernel.org/netdev/20220510153619.32464-1-ap420073@gmail.com/

Seriously? It has been there for 9 years and 2 persons find it and
send the fix with hours of difference? Is someone spying me?

Please review the other one and if it's OK, I will send it alone.

>
> Martin
>
> On Wed, May 11, 2022 at 12:36:03PM +0200, Íñigo Huguet wrote:
> > In some cases there is no mtd partitions that can be probed, so the mtd
> > partitions list stays empty. This happens, for example, in SFC9220
> > devices on the second port of the NIC.
> >
> > The memory for the mtd partitions is deallocated in efx_mtd_remove,
> > recovering the address of the first element of efx->mtd_list and then
> > deallocating it. But if the list is empty, the address passed to kfree
> > doesn't point to the memory allocated for the mtd partitions, but to the
> > list head itself. Despite this hasn't caused other problems other than
> > the memory leak, this is obviously incorrect.
> >
> > This patch deallocates the memory during mtd_probe in the case that
> > there are no probed partitions, avoiding the leak.
> >
> > This was detected with kmemleak, output example:
> > unreferenced object 0xffff88819cfa0000 (size 46560):
> >   comm "kworker/0:2", pid 48435, jiffies 4364987018 (age 45.924s)
> >   hex dump (first 32 bytes):
> >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >   backtrace:
> >     [<000000000f8e92d9>] kmalloc_order_trace+0x19/0x130
> >     [<0000000042a03844>] efx_ef10_mtd_probe+0x12d/0x320 [sfc]
> >     [<000000004555654f>] efx_pci_probe.cold+0x4e1/0x6db [sfc]
> >     [<00000000b03d5126>] local_pci_probe+0xde/0x170
> >     [<00000000376cc8d9>] work_for_cpu_fn+0x51/0xa0
> >     [<00000000141f8de9>] process_one_work+0x8cb/0x1590
> >     [<00000000cb2d8065>] worker_thread+0x707/0x1010
> >     [<000000001ef4b9f6>] kthread+0x364/0x420
> >     [<0000000014767137>] ret_from_fork+0x22/0x30
> >
> > Fixes: 8127d661e77f ("sfc: Add support for Solarflare SFC9100 family")
> > Reported-by: Liang Li <liali@redhat.com>
> > Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> > ---
> >  drivers/net/ethernet/sfc/ef10.c        | 5 +++++
> >  drivers/net/ethernet/sfc/siena/siena.c | 5 +++++
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> > index c9ee5011803f..15a229731296 100644
> > --- a/drivers/net/ethernet/sfc/ef10.c
> > +++ b/drivers/net/ethernet/sfc/ef10.c
> > @@ -3579,6 +3579,11 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
> >               n_parts++;
> >       }
> >
> > +     if (n_parts == 0) {
> > +             kfree(parts);
> > +             return 0;
> > +     }
> > +
> >       rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
> >  fail:
> >       if (rc)
> > diff --git a/drivers/net/ethernet/sfc/siena/siena.c b/drivers/net/ethernet/sfc/siena/siena.c
> > index 741313aff1d1..32467782e8ef 100644
> > --- a/drivers/net/ethernet/sfc/siena/siena.c
> > +++ b/drivers/net/ethernet/sfc/siena/siena.c
> > @@ -943,6 +943,11 @@ static int siena_mtd_probe(struct efx_nic *efx)
> >               nvram_types >>= 1;
> >       }
> >
> > +     if (n_parts == 0) {
> > +             kfree(parts);
> > +             return 0;
> > +     }
> > +
> >       rc = siena_mtd_get_fw_subtypes(efx, parts, n_parts);
> >       if (rc)
> >               goto fail;
> > --
> > 2.34.1
>


-- 
Íñigo Huguet

Re: [PATCH net-next 2/2] sfc: simplify mtd partitions list handling

[Martin Habets]

On Wed, May 11, 2022 at 12:36:04PM +0200, Íñigo Huguet wrote:
> efx_mtd_partitions are embedded inside efx_mcdi_mtd_partition structs.
> They contain a list entry that is appended to efx->mtd_list, which is
> traversed to perform add/remove/rename/etc operations over all
> efx_mtd_partitions.
> 
> However, almost all operations done on a efx_mtd_partition asume that it
> is actually embedded inside an efx_mcdi_mtd_partition, and the
> deallocation asume that the first member of the list is located at the
> beginning of the allocated memory.
> 
> Given all that asumptions, the possibility of having an
> efx_mtd_partition not embedded in an efx_mcdi_efx_partition doesn't
> exist. Neither it does the possibility of being in a memory position
> other the one allocated for the efx_mcdi_mtd_partition array. Also, they
> never need to be reordered.
> 
> Given all that, it is better to get rid of the list and use directly the
> efx_mcdi_mtd_partition array. This shows more clearly how they lay
> in memory, list traversal is more obvious and it save a small amount
> of memory on the list nodes.

I like this. Just 1 comment below.

> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> ---
>  drivers/net/ethernet/sfc/ef10.c       | 12 ++++++--
>  drivers/net/ethernet/sfc/efx.h        |  4 +--
>  drivers/net/ethernet/sfc/efx_common.c |  3 --
>  drivers/net/ethernet/sfc/mtd.c        | 42 ++++++++++-----------------
>  drivers/net/ethernet/sfc/net_driver.h |  9 ++++--
>  5 files changed, 33 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> index 15a229731296..b5284fa529b7 100644
> --- a/drivers/net/ethernet/sfc/ef10.c
> +++ b/drivers/net/ethernet/sfc/ef10.c
> @@ -3584,10 +3584,16 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
>  		return 0;
>  	}
>  
> -	rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
> -fail:
> +	rc = efx_mtd_add(efx, parts, n_parts);
>  	if (rc)
> -		kfree(parts);
> +		goto fail;
> +	efx->mcdi_mtd_parts = parts;
> +	efx->n_mcdi_mtd_parts = n_parts;
> +
> +	return 0;
> +
> +fail:
> +	kfree(parts);
>  	return rc;
>  }
>  
> diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
> index c05a83da9e44..2ab9ba691b0d 100644
> --- a/drivers/net/ethernet/sfc/efx.h
> +++ b/drivers/net/ethernet/sfc/efx.h
> @@ -181,8 +181,8 @@ void efx_update_sw_stats(struct efx_nic *efx, u64 *stats);
>  
>  /* MTD */
>  #ifdef CONFIG_SFC_MTD
> -int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
> -		size_t n_parts, size_t sizeof_part);
> +int efx_mtd_add(struct efx_nic *efx, struct efx_mcdi_mtd_partition *parts,
> +		size_t n_parts);
>  static inline int efx_mtd_probe(struct efx_nic *efx)
>  {
>  	return efx->type->mtd_probe(efx);
> diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
> index f6577e74d6e6..8802790403e9 100644
> --- a/drivers/net/ethernet/sfc/efx_common.c
> +++ b/drivers/net/ethernet/sfc/efx_common.c
> @@ -987,9 +987,6 @@ int efx_init_struct(struct efx_nic *efx,
>  	INIT_LIST_HEAD(&efx->node);
>  	INIT_LIST_HEAD(&efx->secondary_list);
>  	spin_lock_init(&efx->biu_lock);
> -#ifdef CONFIG_SFC_MTD
> -	INIT_LIST_HEAD(&efx->mtd_list);
> -#endif
>  	INIT_WORK(&efx->reset_work, efx_reset_work);
>  	INIT_DELAYED_WORK(&efx->monitor_work, efx_monitor);
>  	efx_selftest_async_init(efx);
> diff --git a/drivers/net/ethernet/sfc/mtd.c b/drivers/net/ethernet/sfc/mtd.c
> index 273c08e5455f..4d06e8a9a729 100644
> --- a/drivers/net/ethernet/sfc/mtd.c
> +++ b/drivers/net/ethernet/sfc/mtd.c
> @@ -12,6 +12,7 @@
>  
>  #include "net_driver.h"
>  #include "efx.h"
> +#include "mcdi.h"
>  
>  #define to_efx_mtd_partition(mtd)				\
>  	container_of(mtd, struct efx_mtd_partition, mtd)
> @@ -48,18 +49,16 @@ static void efx_mtd_remove_partition(struct efx_mtd_partition *part)
>  		ssleep(1);
>  	}
>  	WARN_ON(rc);
> -	list_del(&part->node);
>  }
>  
> -int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
> -		size_t n_parts, size_t sizeof_part)
> +int efx_mtd_add(struct efx_nic *efx, struct efx_mcdi_mtd_partition *parts,
> +		size_t n_parts)
>  {
>  	struct efx_mtd_partition *part;
>  	size_t i;
>  
>  	for (i = 0; i < n_parts; i++) {
> -		part = (struct efx_mtd_partition *)((char *)parts +
> -						    i * sizeof_part);
> +		part = &parts[i].common;
>  
>  		part->mtd.writesize = 1;
>  
> @@ -78,47 +77,38 @@ int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
>  
>  		if (mtd_device_register(&part->mtd, NULL, 0))
>  			goto fail;
> -
> -		/* Add to list in order - efx_mtd_remove() depends on this */
> -		list_add_tail(&part->node, &efx->mtd_list);
>  	}
>  
>  	return 0;
>  
>  fail:
> -	while (i--) {
> -		part = (struct efx_mtd_partition *)((char *)parts +
> -						    i * sizeof_part);
> -		efx_mtd_remove_partition(part);
> -	}
> +	while (i--)
> +		efx_mtd_remove_partition(&parts[i].common);
> +
>  	/* Failure is unlikely here, but probably means we're out of memory */
>  	return -ENOMEM;
>  }
>  
>  void efx_mtd_remove(struct efx_nic *efx)
>  {
> -	struct efx_mtd_partition *parts, *part, *next;
> +	int i;
>  
>  	WARN_ON(efx_dev_registered(efx));
>  
> -	if (list_empty(&efx->mtd_list))
> -		return;
> -
> -	parts = list_first_entry(&efx->mtd_list, struct efx_mtd_partition,
> -				 node);
> +	for (i = 0; i < efx->n_mcdi_mtd_parts; i++)
> +		efx_mtd_remove_partition(&efx->mcdi_mtd_parts[i].common);
>  
> -	list_for_each_entry_safe(part, next, &efx->mtd_list, node)
> -		efx_mtd_remove_partition(part);
> -
> -	kfree(parts);
> +	kfree(efx->mcdi_mtd_parts);
> +	efx->mcdi_mtd_parts = NULL;
> +	efx->n_mcdi_mtd_parts = 0;

It is safer to first set to NULL/0 before freeing the memory.
With this sequence bad things can happen if the thread gets descheduled
right after the kfree.

Martin

>  }
>  
>  void efx_mtd_rename(struct efx_nic *efx)
>  {
> -	struct efx_mtd_partition *part;
> +	int i;
>  
>  	ASSERT_RTNL();
>  
> -	list_for_each_entry(part, &efx->mtd_list, node)
> -		efx->type->mtd_rename(part);
> +	for (i = 0; i < efx->n_mcdi_mtd_parts; i++)
> +		efx->type->mtd_rename(&efx->mcdi_mtd_parts[i].common);
>  }
> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> index 318db906a154..5d20b25b0e82 100644
> --- a/drivers/net/ethernet/sfc/net_driver.h
> +++ b/drivers/net/ethernet/sfc/net_driver.h
> @@ -107,6 +107,8 @@ struct hwtstamp_config;
>  
>  struct efx_self_tests;
>  
> +struct efx_mcdi_mtd_partition;
> +
>  /**
>   * struct efx_buffer - A general-purpose DMA buffer
>   * @addr: host base address of the buffer
> @@ -865,7 +867,8 @@ enum efx_xdp_tx_queues_mode {
>   * @irq_zero_count: Number of legacy IRQs seen with queue flags == 0
>   * @irq_level: IRQ level/index for IRQs not triggered by an event queue
>   * @selftest_work: Work item for asynchronous self-test
> - * @mtd_list: List of MTDs attached to the NIC
> + * @mcdi_mtd_parts: Array of MTDs attached to the NIC
> + * @n_mcdi_mtd_parts: Number of MTDs attached to the NIC
>   * @nic_data: Hardware dependent state
>   * @mcdi: Management-Controller-to-Driver Interface state
>   * @mac_lock: MAC access lock. Protects @port_enabled, @phy_mode,
> @@ -1033,7 +1036,8 @@ struct efx_nic {
>  	struct delayed_work selftest_work;
>  
>  #ifdef CONFIG_SFC_MTD
> -	struct list_head mtd_list;
> +	struct efx_mcdi_mtd_partition *mcdi_mtd_parts;
> +	unsigned int n_mcdi_mtd_parts;
>  #endif
>  
>  	void *nic_data;
> @@ -1134,7 +1138,6 @@ static inline unsigned int efx_port_num(struct efx_nic *efx)
>  }
>  
>  struct efx_mtd_partition {
> -	struct list_head node;
>  	struct mtd_info mtd;
>  	const char *dev_type_name;
>  	const char *type_name;
> -- 
> 2.34.1

Re: [PATCH net-next 2/2] sfc: simplify mtd partitions list handling

[Íñigo Huguet]

On Fri, May 13, 2022 at 10:02 AM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> On Wed, May 11, 2022 at 12:36:04PM +0200, Íñigo Huguet wrote:
> > efx_mtd_partitions are embedded inside efx_mcdi_mtd_partition structs.
> > They contain a list entry that is appended to efx->mtd_list, which is
> > traversed to perform add/remove/rename/etc operations over all
> > efx_mtd_partitions.
> >
> > However, almost all operations done on a efx_mtd_partition asume that it
> > is actually embedded inside an efx_mcdi_mtd_partition, and the
> > deallocation asume that the first member of the list is located at the
> > beginning of the allocated memory.
> >
> > Given all that asumptions, the possibility of having an
> > efx_mtd_partition not embedded in an efx_mcdi_efx_partition doesn't
> > exist. Neither it does the possibility of being in a memory position
> > other the one allocated for the efx_mcdi_mtd_partition array. Also, they
> > never need to be reordered.
> >
> > Given all that, it is better to get rid of the list and use directly the
> > efx_mcdi_mtd_partition array. This shows more clearly how they lay
> > in memory, list traversal is more obvious and it save a small amount
> > of memory on the list nodes.
>
> I like this. Just 1 comment below.

I will prepare a new version, dropping the previous patch

>
> > Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> > ---
> >  drivers/net/ethernet/sfc/ef10.c       | 12 ++++++--
> >  drivers/net/ethernet/sfc/efx.h        |  4 +--
> >  drivers/net/ethernet/sfc/efx_common.c |  3 --
> >  drivers/net/ethernet/sfc/mtd.c        | 42 ++++++++++-----------------
> >  drivers/net/ethernet/sfc/net_driver.h |  9 ++++--
> >  5 files changed, 33 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
> > index 15a229731296..b5284fa529b7 100644
> > --- a/drivers/net/ethernet/sfc/ef10.c
> > +++ b/drivers/net/ethernet/sfc/ef10.c
> > @@ -3584,10 +3584,16 @@ static int efx_ef10_mtd_probe(struct efx_nic *efx)
> >               return 0;
> >       }
> >
> > -     rc = efx_mtd_add(efx, &parts[0].common, n_parts, sizeof(*parts));
> > -fail:
> > +     rc = efx_mtd_add(efx, parts, n_parts);
> >       if (rc)
> > -             kfree(parts);
> > +             goto fail;
> > +     efx->mcdi_mtd_parts = parts;
> > +     efx->n_mcdi_mtd_parts = n_parts;
> > +
> > +     return 0;
> > +
> > +fail:
> > +     kfree(parts);
> >       return rc;
> >  }
> >
> > diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
> > index c05a83da9e44..2ab9ba691b0d 100644
> > --- a/drivers/net/ethernet/sfc/efx.h
> > +++ b/drivers/net/ethernet/sfc/efx.h
> > @@ -181,8 +181,8 @@ void efx_update_sw_stats(struct efx_nic *efx, u64 *stats);
> >
> >  /* MTD */
> >  #ifdef CONFIG_SFC_MTD
> > -int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
> > -             size_t n_parts, size_t sizeof_part);
> > +int efx_mtd_add(struct efx_nic *efx, struct efx_mcdi_mtd_partition *parts,
> > +             size_t n_parts);
> >  static inline int efx_mtd_probe(struct efx_nic *efx)
> >  {
> >       return efx->type->mtd_probe(efx);
> > diff --git a/drivers/net/ethernet/sfc/efx_common.c b/drivers/net/ethernet/sfc/efx_common.c
> > index f6577e74d6e6..8802790403e9 100644
> > --- a/drivers/net/ethernet/sfc/efx_common.c
> > +++ b/drivers/net/ethernet/sfc/efx_common.c
> > @@ -987,9 +987,6 @@ int efx_init_struct(struct efx_nic *efx,
> >       INIT_LIST_HEAD(&efx->node);
> >       INIT_LIST_HEAD(&efx->secondary_list);
> >       spin_lock_init(&efx->biu_lock);
> > -#ifdef CONFIG_SFC_MTD
> > -     INIT_LIST_HEAD(&efx->mtd_list);
> > -#endif
> >       INIT_WORK(&efx->reset_work, efx_reset_work);
> >       INIT_DELAYED_WORK(&efx->monitor_work, efx_monitor);
> >       efx_selftest_async_init(efx);
> > diff --git a/drivers/net/ethernet/sfc/mtd.c b/drivers/net/ethernet/sfc/mtd.c
> > index 273c08e5455f..4d06e8a9a729 100644
> > --- a/drivers/net/ethernet/sfc/mtd.c
> > +++ b/drivers/net/ethernet/sfc/mtd.c
> > @@ -12,6 +12,7 @@
> >
> >  #include "net_driver.h"
> >  #include "efx.h"
> > +#include "mcdi.h"
> >
> >  #define to_efx_mtd_partition(mtd)                            \
> >       container_of(mtd, struct efx_mtd_partition, mtd)
> > @@ -48,18 +49,16 @@ static void efx_mtd_remove_partition(struct efx_mtd_partition *part)
> >               ssleep(1);
> >       }
> >       WARN_ON(rc);
> > -     list_del(&part->node);
> >  }
> >
> > -int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
> > -             size_t n_parts, size_t sizeof_part)
> > +int efx_mtd_add(struct efx_nic *efx, struct efx_mcdi_mtd_partition *parts,
> > +             size_t n_parts)
> >  {
> >       struct efx_mtd_partition *part;
> >       size_t i;
> >
> >       for (i = 0; i < n_parts; i++) {
> > -             part = (struct efx_mtd_partition *)((char *)parts +
> > -                                                 i * sizeof_part);
> > +             part = &parts[i].common;
> >
> >               part->mtd.writesize = 1;
> >
> > @@ -78,47 +77,38 @@ int efx_mtd_add(struct efx_nic *efx, struct efx_mtd_partition *parts,
> >
> >               if (mtd_device_register(&part->mtd, NULL, 0))
> >                       goto fail;
> > -
> > -             /* Add to list in order - efx_mtd_remove() depends on this */
> > -             list_add_tail(&part->node, &efx->mtd_list);
> >       }
> >
> >       return 0;
> >
> >  fail:
> > -     while (i--) {
> > -             part = (struct efx_mtd_partition *)((char *)parts +
> > -                                                 i * sizeof_part);
> > -             efx_mtd_remove_partition(part);
> > -     }
> > +     while (i--)
> > +             efx_mtd_remove_partition(&parts[i].common);
> > +
> >       /* Failure is unlikely here, but probably means we're out of memory */
> >       return -ENOMEM;
> >  }
> >
> >  void efx_mtd_remove(struct efx_nic *efx)
> >  {
> > -     struct efx_mtd_partition *parts, *part, *next;
> > +     int i;
> >
> >       WARN_ON(efx_dev_registered(efx));
> >
> > -     if (list_empty(&efx->mtd_list))
> > -             return;
> > -
> > -     parts = list_first_entry(&efx->mtd_list, struct efx_mtd_partition,
> > -                              node);
> > +     for (i = 0; i < efx->n_mcdi_mtd_parts; i++)
> > +             efx_mtd_remove_partition(&efx->mcdi_mtd_parts[i].common);
> >
> > -     list_for_each_entry_safe(part, next, &efx->mtd_list, node)
> > -             efx_mtd_remove_partition(part);
> > -
> > -     kfree(parts);
> > +     kfree(efx->mcdi_mtd_parts);
> > +     efx->mcdi_mtd_parts = NULL;
> > +     efx->n_mcdi_mtd_parts = 0;
>
> It is safer to first set to NULL/0 before freeing the memory.
> With this sequence bad things can happen if the thread gets descheduled
> right after the kfree.

To prevent this we'd also need to use memory barriers. I don't think
we need them because all usages of these fields are done under the
rtnl lock. I won't apply this change in the new version because I find
it useless, but please explain me the reason if you disagree.

>
> Martin
>
> >  }
> >
> >  void efx_mtd_rename(struct efx_nic *efx)
> >  {
> > -     struct efx_mtd_partition *part;
> > +     int i;
> >
> >       ASSERT_RTNL();
> >
> > -     list_for_each_entry(part, &efx->mtd_list, node)
> > -             efx->type->mtd_rename(part);
> > +     for (i = 0; i < efx->n_mcdi_mtd_parts; i++)
> > +             efx->type->mtd_rename(&efx->mcdi_mtd_parts[i].common);
> >  }
> > diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> > index 318db906a154..5d20b25b0e82 100644
> > --- a/drivers/net/ethernet/sfc/net_driver.h
> > +++ b/drivers/net/ethernet/sfc/net_driver.h
> > @@ -107,6 +107,8 @@ struct hwtstamp_config;
> >
> >  struct efx_self_tests;
> >
> > +struct efx_mcdi_mtd_partition;
> > +
> >  /**
> >   * struct efx_buffer - A general-purpose DMA buffer
> >   * @addr: host base address of the buffer
> > @@ -865,7 +867,8 @@ enum efx_xdp_tx_queues_mode {
> >   * @irq_zero_count: Number of legacy IRQs seen with queue flags == 0
> >   * @irq_level: IRQ level/index for IRQs not triggered by an event queue
> >   * @selftest_work: Work item for asynchronous self-test
> > - * @mtd_list: List of MTDs attached to the NIC
> > + * @mcdi_mtd_parts: Array of MTDs attached to the NIC
> > + * @n_mcdi_mtd_parts: Number of MTDs attached to the NIC
> >   * @nic_data: Hardware dependent state
> >   * @mcdi: Management-Controller-to-Driver Interface state
> >   * @mac_lock: MAC access lock. Protects @port_enabled, @phy_mode,
> > @@ -1033,7 +1036,8 @@ struct efx_nic {
> >       struct delayed_work selftest_work;
> >
> >  #ifdef CONFIG_SFC_MTD
> > -     struct list_head mtd_list;
> > +     struct efx_mcdi_mtd_partition *mcdi_mtd_parts;
> > +     unsigned int n_mcdi_mtd_parts;
> >  #endif
> >
> >       void *nic_data;
> > @@ -1134,7 +1138,6 @@ static inline unsigned int efx_port_num(struct efx_nic *efx)
> >  }
> >
> >  struct efx_mtd_partition {
> > -     struct list_head node;
> >       struct mtd_info mtd;
> >       const char *dev_type_name;
> >       const char *type_name;
> > --
> > 2.34.1
>


-- 
Íñigo Huguet