struct net_device shouldn't be embedded into any structure, instead,
the owner should use the private space to embed their state into
net_device.
But, in some cases the net_device is embedded inside the private
structure, which blocks the usage of zero-length arrays inside
net_device.
Create a helper to allocate a dummy device at dynamically runtime, and
move the Ethernet devices to use it, instead of embedding the dummy
device inside the private structure.
This fixes all the network cases except for wireless drivers.
PS: Due to lack of hardware, unfortunately all these patches are
compiled tested only.
---
Changelog:
v1:
* https://lore.kernel.org/all/[email protected]/
v2:
* Patch 1: Use a pre-defined name ("dummy#") for the dummy
net_devices.
* Patch 2-5: Added users for the new helper.
Breno Leitao (5):
net: create a dummy net_device allocator
net: marvell: prestera: allocate dummy net_device dynamically
net: mediatek: mtk_eth_sock: allocate dummy net_device dynamically
net: ipa: allocate dummy net_device dynamically
net: ibm/emac: allocate dummy net_device dynamically
drivers/net/ethernet/ibm/emac/mal.c | 13 +++--
drivers/net/ethernet/ibm/emac/mal.h | 2 +-
.../ethernet/marvell/prestera/prestera_rxtx.c | 15 ++++--
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 17 ++++--
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
drivers/net/ipa/gsi.c | 12 +++--
drivers/net/ipa/gsi.h | 2 +-
include/linux/netdevice.h | 3 ++
net/core/dev.c | 54 ++++++++++++-------
9 files changed, 85 insertions(+), 35 deletions(-)
--
2.43.0
It is impossible to use init_dummy_netdev together with alloc_netdev()
as the 'setup' argument.
This is because alloc_netdev() initializes some fields in the net_device
structure, and later init_dummy_netdev() memzero them all. This causes
some problems as reported here:
https://lore.kernel.org/all/[email protected]/
Split the init_dummy_netdev() function in two. Create a new function called
init_dummy_netdev_core() that does not memzero the net_device structure.
Then have init_dummy_netdev() memzero-ing and calling
init_dummy_netdev_core(), keeping the old behaviour.
init_dummy_netdev_core() is the new function that could be called as an
argument for alloc_netdev().
Also, create a helper to allocate and initialize dummy net devices,
leveraging init_dummy_netdev_core() as the setup argument. This function
basically simplify the allocation of dummy devices, by allocating and
initializing it. Freeing the device continue to be done through
free_netdev()
Suggested-by: Jakub Kicinski <[email protected]>
Signed-off-by: Breno Leitao <[email protected]>
---
include/linux/netdevice.h | 3 +++
net/core/dev.c | 54 ++++++++++++++++++++++++++-------------
2 files changed, 39 insertions(+), 18 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e41d30ebaca6..8aa4a2645ad5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4546,6 +4546,9 @@ static inline void netif_addr_unlock_bh(struct net_device *dev)
void ether_setup(struct net_device *dev);
+/* Allocate dummy net_device */
+struct net_device *alloc_netdev_dummy(int sizeof_priv);
+
/* Support for loadable net-drivers */
struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
unsigned char name_assign_type,
diff --git a/net/core/dev.c b/net/core/dev.c
index 5d36a634f468..300e097b97d8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10391,25 +10391,12 @@ int register_netdevice(struct net_device *dev)
}
EXPORT_SYMBOL(register_netdevice);
-/**
- * init_dummy_netdev - init a dummy network device for NAPI
- * @dev: device to init
- *
- * This takes a network device structure and initialize the minimum
- * amount of fields so it can be used to schedule NAPI polls without
- * registering a full blown interface. This is to be used by drivers
- * that need to tie several hardware interfaces to a single NAPI
- * poll scheduler due to HW limitations.
+/* Initialize the core of a dummy net device.
+ * This is useful if you are calling this function after alloc_netdev(),
+ * since it does not memset the net_device fields.
*/
-void init_dummy_netdev(struct net_device *dev)
+static void init_dummy_netdev_core(struct net_device *dev)
{
- /* Clear everything. Note we don't initialize spinlocks
- * are they aren't supposed to be taken by any of the
- * NAPI code and this dummy netdev is supposed to be
- * only ever used for NAPI polls
- */
- memset(dev, 0, sizeof(struct net_device));
-
/* make sure we BUG if trying to hit standard
* register/unregister code path
*/
@@ -10430,8 +10417,28 @@ void init_dummy_netdev(struct net_device *dev)
* its refcount.
*/
}
-EXPORT_SYMBOL_GPL(init_dummy_netdev);
+/**
+ * init_dummy_netdev - init a dummy network device for NAPI
+ * @dev: device to init
+ *
+ * This takes a network device structure and initialize the minimum
+ * amount of fields so it can be used to schedule NAPI polls without
+ * registering a full blown interface. This is to be used by drivers
+ * that need to tie several hardware interfaces to a single NAPI
+ * poll scheduler due to HW limitations.
+ */
+void init_dummy_netdev(struct net_device *dev)
+{
+ /* Clear everything. Note we don't initialize spinlocks
+ * are they aren't supposed to be taken by any of the
+ * NAPI code and this dummy netdev is supposed to be
+ * only ever used for NAPI polls
+ */
+ memset(dev, 0, sizeof(struct net_device));
+ init_dummy_netdev_core(dev);
+}
+EXPORT_SYMBOL_GPL(init_dummy_netdev);
/**
* register_netdev - register a network device
@@ -11042,6 +11049,17 @@ void free_netdev(struct net_device *dev)
}
EXPORT_SYMBOL(free_netdev);
+/**
+ * alloc_netdev_dummy - Allocate and initialize a dummy net device.
+ * @sizeof_priv: size of private data to allocate space for
+ */
+struct net_device *alloc_netdev_dummy(int sizeof_priv)
+{
+ return alloc_netdev(sizeof_priv, "dummy#", NET_NAME_UNKNOWN,
+ init_dummy_netdev_core);
+}
+EXPORT_SYMBOL_GPL(alloc_netdev_dummy);
+
/**
* synchronize_net - Synchronize with packet receive processing
*
--
2.43.0
Embedding net_device into structures prohibits the usage of flexible
arrays in the net_device structure. For more details, see the discussion
at [1].
Un-embed the net_device from the private struct by converting it
into a pointer. Then use the leverage the new alloc_netdev_dummy()
helper to allocate and initialize dummy devices.
[1] https://lore.kernel.org/all/[email protected]/
Signed-off-by: Breno Leitao <[email protected]>
---
.../net/ethernet/marvell/prestera/prestera_rxtx.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_rxtx.c b/drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
index cc2a9ae794be..ed33a201a0f5 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
@@ -96,7 +96,7 @@ struct prestera_sdma {
struct dma_pool *desc_pool;
struct work_struct tx_work;
struct napi_struct rx_napi;
- struct net_device napi_dev;
+ struct net_device *napi_dev;
u32 map_addr;
u64 dma_mask;
/* protect SDMA with concurrent access from multiple CPUs */
@@ -654,13 +654,21 @@ static int prestera_sdma_switch_init(struct prestera_switch *sw)
if (err)
goto err_evt_register;
- init_dummy_netdev(&sdma->napi_dev);
+ sdma->napi_dev = alloc_netdev_dummy(0);
+ if (!sdma->napi_dev) {
+ dev_err(dev, "not able to initialize dummy device\n");
+ goto err_alloc_dummy;
+ }
+
- netif_napi_add(&sdma->napi_dev, &sdma->rx_napi, prestera_sdma_rx_poll);
+ netif_napi_add(sdma->napi_dev, &sdma->rx_napi, prestera_sdma_rx_poll);
napi_enable(&sdma->rx_napi);
return 0;
+err_alloc_dummy:
+ prestera_hw_event_handler_unregister(sw, PRESTERA_EVENT_TYPE_RXTX,
+ prestera_rxtx_handle_event);
err_evt_register:
err_tx_init:
prestera_sdma_tx_fini(sdma);
@@ -682,6 +690,7 @@ static void prestera_sdma_switch_fini(struct prestera_switch *sw)
prestera_sdma_tx_fini(sdma);
prestera_sdma_rx_fini(sdma);
dma_pool_destroy(sdma->desc_pool);
+ kfree(sdma->napi_dev);
}
static bool prestera_sdma_is_ready(struct prestera_sdma *sdma)
--
2.43.0
Embedding net_device into structures prohibits the usage of flexible
arrays in the net_device structure. For more details, see the discussion
at [1].
Un-embed the net_device from the private struct by converting it
into a pointer. Then use the leverage the new alloc_netdev_dummy()
helper to allocate and initialize dummy devices.
[1] https://lore.kernel.org/all/[email protected]/
Signed-off-by: Breno Leitao <[email protected]>
---
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 17 +++++++++++++----
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index caa13b9cedff..cef96c048fdf 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1710,7 +1710,7 @@ static struct page_pool *mtk_create_page_pool(struct mtk_eth *eth,
if (IS_ERR(pp))
return pp;
- err = __xdp_rxq_info_reg(xdp_q, ð->dummy_dev, id,
+ err = __xdp_rxq_info_reg(xdp_q, eth->dummy_dev, id,
eth->rx_napi.napi_id, PAGE_SIZE);
if (err < 0)
goto err_free_pp;
@@ -4188,6 +4188,8 @@ static int mtk_free_dev(struct mtk_eth *eth)
metadata_dst_free(eth->dsa_meta[i]);
}
+ kfree(eth->dummy_dev);
+
return 0;
}
@@ -4983,9 +4985,14 @@ static int mtk_probe(struct platform_device *pdev)
/* we run 2 devices on the same DMA ring so we need a dummy device
* for NAPI to work
*/
- init_dummy_netdev(ð->dummy_dev);
- netif_napi_add(ð->dummy_dev, ð->tx_napi, mtk_napi_tx);
- netif_napi_add(ð->dummy_dev, ð->rx_napi, mtk_napi_rx);
+ eth->dummy_dev = alloc_netdev_dummy(0);
+ if (!eth->dummy_dev) {
+ err = -ENOMEM;
+ dev_err(eth->dev, "failed to allocated dummy device\n");
+ goto err_unreg_netdev;
+ }
+ netif_napi_add(eth->dummy_dev, ð->tx_napi, mtk_napi_tx);
+ netif_napi_add(eth->dummy_dev, ð->rx_napi, mtk_napi_rx);
platform_set_drvdata(pdev, eth);
schedule_delayed_work(ð->reset.monitor_work,
@@ -4993,6 +5000,8 @@ static int mtk_probe(struct platform_device *pdev)
return 0;
+err_unreg_netdev:
+ mtk_unreg_dev(eth);
err_deinit_ppe:
mtk_ppe_deinit(eth);
mtk_mdio_cleanup(eth);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 9ae3b8a71d0e..723fc637027c 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -1242,7 +1242,7 @@ struct mtk_eth {
spinlock_t page_lock;
spinlock_t tx_irq_lock;
spinlock_t rx_irq_lock;
- struct net_device dummy_dev;
+ struct net_device *dummy_dev;
struct net_device *netdev[MTK_MAX_DEVS];
struct mtk_mac *mac[MTK_MAX_DEVS];
int irq[3];
--
2.43.0
Embedding net_device into structures prohibits the usage of flexible
arrays in the net_device structure. For more details, see the discussion
at [1].
Un-embed the net_device from the private struct by converting it
into a pointer. Then use the leverage the new alloc_netdev_dummy()
helper to allocate and initialize dummy devices.
[1] https://lore.kernel.org/all/[email protected]/
Signed-off-by: Breno Leitao <[email protected]>
---
drivers/net/ethernet/ibm/emac/mal.c | 13 ++++++++++---
drivers/net/ethernet/ibm/emac/mal.h | 2 +-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/ibm/emac/mal.c b/drivers/net/ethernet/ibm/emac/mal.c
index 2439f7e96e05..ae2b5a2993d1 100644
--- a/drivers/net/ethernet/ibm/emac/mal.c
+++ b/drivers/net/ethernet/ibm/emac/mal.c
@@ -605,9 +605,13 @@ static int mal_probe(struct platform_device *ofdev)
INIT_LIST_HEAD(&mal->list);
spin_lock_init(&mal->lock);
- init_dummy_netdev(&mal->dummy_dev);
+ mal->dummy_dev = alloc_netdev_dummy(0);
+ if (!mal->dummy_dev) {
+ err = -ENOMEM;
+ goto fail_unmap;
+ }
- netif_napi_add_weight(&mal->dummy_dev, &mal->napi, mal_poll,
+ netif_napi_add_weight(mal->dummy_dev, &mal->napi, mal_poll,
CONFIG_IBM_EMAC_POLL_WEIGHT);
/* Load power-on reset defaults */
@@ -637,7 +641,7 @@ static int mal_probe(struct platform_device *ofdev)
GFP_KERNEL);
if (mal->bd_virt == NULL) {
err = -ENOMEM;
- goto fail_unmap;
+ goto fail_dummy;
}
for (i = 0; i < mal->num_tx_chans; ++i)
@@ -703,6 +707,8 @@ static int mal_probe(struct platform_device *ofdev)
free_irq(mal->serr_irq, mal);
fail2:
dma_free_coherent(&ofdev->dev, bd_size, mal->bd_virt, mal->bd_dma);
+ fail_dummy:
+ kfree(mal->dummy_dev);
fail_unmap:
dcr_unmap(mal->dcr_host, 0x100);
fail:
@@ -739,6 +745,7 @@ static void mal_remove(struct platform_device *ofdev)
(NUM_TX_BUFF * mal->num_tx_chans +
NUM_RX_BUFF * mal->num_rx_chans), mal->bd_virt,
mal->bd_dma);
+ kfree(mal->dummy_dev);
kfree(mal);
}
diff --git a/drivers/net/ethernet/ibm/emac/mal.h b/drivers/net/ethernet/ibm/emac/mal.h
index d212373a72e7..e0ddc41186a2 100644
--- a/drivers/net/ethernet/ibm/emac/mal.h
+++ b/drivers/net/ethernet/ibm/emac/mal.h
@@ -205,7 +205,7 @@ struct mal_instance {
int index;
spinlock_t lock;
- struct net_device dummy_dev;
+ struct net_device *dummy_dev;
unsigned int features;
};
--
2.43.0
Embedding net_device into structures prohibits the usage of flexible
arrays in the net_device structure. For more details, see the discussion
at [1].
Un-embed the net_device from the private struct by converting it
into a pointer. Then use the leverage the new alloc_netdev_dummy()
helper to allocate and initialize dummy devices.
[1] https://lore.kernel.org/all/[email protected]/
Signed-off-by: Breno Leitao <[email protected]>
---
drivers/net/ipa/gsi.c | 12 ++++++++----
drivers/net/ipa/gsi.h | 2 +-
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 9a0b1fe4a93a..d2db54cbd46d 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -1730,10 +1730,10 @@ static int gsi_channel_setup_one(struct gsi *gsi, u32 channel_id)
gsi_channel_program(channel, true);
if (channel->toward_ipa)
- netif_napi_add_tx(&gsi->dummy_dev, &channel->napi,
+ netif_napi_add_tx(gsi->dummy_dev, &channel->napi,
gsi_channel_poll);
else
- netif_napi_add(&gsi->dummy_dev, &channel->napi,
+ netif_napi_add(gsi->dummy_dev, &channel->napi,
gsi_channel_poll);
return 0;
@@ -2369,12 +2369,14 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev,
/* GSI uses NAPI on all channels. Create a dummy network device
* for the channel NAPI contexts to be associated with.
*/
- init_dummy_netdev(&gsi->dummy_dev);
+ gsi->dummy_dev = alloc_netdev_dummy(0);
+ if (!gsi->dummy_dev)
+ return -ENOMEM;
init_completion(&gsi->completion);
ret = gsi_reg_init(gsi, pdev);
if (ret)
- return ret;
+ goto err_reg_exit;
ret = gsi_irq_init(gsi, pdev); /* No matching exit required */
if (ret)
@@ -2389,6 +2391,7 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev,
return 0;
err_reg_exit:
+ kfree(gsi->dummy_dev);
gsi_reg_exit(gsi);
return ret;
@@ -2400,6 +2403,7 @@ void gsi_exit(struct gsi *gsi)
mutex_destroy(&gsi->mutex);
gsi_channel_exit(gsi);
gsi_reg_exit(gsi);
+ kfree(gsi->dummy_dev);
}
/* The maximum number of outstanding TREs on a channel. This limits
diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h
index 42063b227c18..6b7ec2a39676 100644
--- a/drivers/net/ipa/gsi.h
+++ b/drivers/net/ipa/gsi.h
@@ -155,7 +155,7 @@ struct gsi {
struct mutex mutex; /* protects commands, programming */
struct gsi_channel channel[GSI_CHANNEL_COUNT_MAX];
struct gsi_evt_ring evt_ring[GSI_EVT_RING_COUNT_MAX];
- struct net_device dummy_dev; /* needed for NAPI */
+ struct net_device *dummy_dev; /* needed for NAPI */
};
/**
--
2.43.0
On Thu, 28 Mar 2024 16:52:02 -0700 Breno Leitao wrote:
> - init_dummy_netdev(&sdma->napi_dev);
> + sdma->napi_dev = alloc_netdev_dummy(0);
> + if (!sdma->napi_dev) {
> + dev_err(dev, "not able to initialize dummy device\n");
> + goto err_alloc_dummy;
> + }
> +
>
duplicated empty line here
> - netif_napi_add(&sdma->napi_dev, &sdma->rx_napi, prestera_sdma_rx_poll);
> + netif_napi_add(sdma->napi_dev, &sdma->rx_napi, prestera_sdma_rx_poll);
> napi_enable(&sdma->rx_napi);
>
> return 0;
>
> +err_alloc_dummy:
> + prestera_hw_event_handler_unregister(sw, PRESTERA_EVENT_TYPE_RXTX,
> + prestera_rxtx_handle_event);
> err_evt_register:
> err_tx_init:
> prestera_sdma_tx_fini(sdma);
> @@ -682,6 +690,7 @@ static void prestera_sdma_switch_fini(struct prestera_switch *sw)
> prestera_sdma_tx_fini(sdma);
> prestera_sdma_rx_fini(sdma);
> dma_pool_destroy(sdma->desc_pool);
> + kfree(sdma->napi_dev);
Why kfree()? Let's use free_netdev() consistently, in case one day
we have to undo something alloc_netdev_dummy() has done.
On Fri, Mar 29, 2024 at 08:56:33AM -0700, Jakub Kicinski wrote:
> > @@ -682,6 +690,7 @@ static void prestera_sdma_switch_fini(struct prestera_switch *sw)
> > prestera_sdma_tx_fini(sdma);
> > prestera_sdma_rx_fini(sdma);
> > dma_pool_destroy(sdma->desc_pool);
> > + kfree(sdma->napi_dev);
>
> Why kfree()? Let's use free_netdev() consistently, in case one day
> we have to undo something alloc_netdev_dummy() has done.
I should have used free_netdev() in fact. I will update.
On Thu, Mar 28, 2024 at 04:52:02PM -0700, Breno Leitao wrote:
> Embedding net_device into structures prohibits the usage of flexible
> arrays in the net_device structure. For more details, see the discussion
> at [1].
>
> Un-embed the net_device from the private struct by converting it
> into a pointer. Then use the leverage the new alloc_netdev_dummy()
> helper to allocate and initialize dummy devices.
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Breno Leitao <[email protected]>
> ---
> .../net/ethernet/marvell/prestera/prestera_rxtx.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_rxtx.c b/drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
..
> @@ -654,13 +654,21 @@ static int prestera_sdma_switch_init(struct prestera_switch *sw)
> if (err)
> goto err_evt_register;
>
> - init_dummy_netdev(&sdma->napi_dev);
> + sdma->napi_dev = alloc_netdev_dummy(0);
> + if (!sdma->napi_dev) {
> + dev_err(dev, "not able to initialize dummy device\n");
> + goto err_alloc_dummy;
Hi Breno,
This goto will result in the function returning err.
But err is 0 here. Perhaps it should be set to a negative error value
instead?
Flagged by Smatch.
> + }
> +
>
> - netif_napi_add(&sdma->napi_dev, &sdma->rx_napi, prestera_sdma_rx_poll);
> + netif_napi_add(sdma->napi_dev, &sdma->rx_napi, prestera_sdma_rx_poll);
> napi_enable(&sdma->rx_napi);
>
> return 0;
>
> +err_alloc_dummy:
> + prestera_hw_event_handler_unregister(sw, PRESTERA_EVENT_TYPE_RXTX,
> + prestera_rxtx_handle_event);
> err_evt_register:
> err_tx_init:
> prestera_sdma_tx_fini(sdma);
..
On 3/28/24 6:52 PM, Breno Leitao wrote:
> Embedding net_device into structures prohibits the usage of flexible
> arrays in the net_device structure. For more details, see the discussion
> at [1].
>
> Un-embed the net_device from the private struct by converting it
> into a pointer. Then use the leverage the new alloc_netdev_dummy()
> helper to allocate and initialize dummy devices.
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Breno Leitao <[email protected]>
Thanks for pointing this out, I didn't notice the earlier
discussion. Embedding the dummy netdev in this case was
probably done to eliminate the chance of an unlikely
allocation error at init time. It is not at all necessary.
I had to go find the rest of your series. If at least one patch
is addressed to me in a series, please copy me on all of them.
I see the dummy netdev now gets "fully initialized" but that's
a one-time thing, and seems harmless. But given that, shouldn't
the result of alloc_dummy_netdev() also have a free_dummy_netdev()
(rather than simply calling kfree(dummy_netdev))? (And I now
see that Jakub made a similar remark.)
More below.
> ---
> drivers/net/ipa/gsi.c | 12 ++++++++----
> drivers/net/ipa/gsi.h | 2 +-
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
> index 9a0b1fe4a93a..d2db54cbd46d 100644
> --- a/drivers/net/ipa/gsi.c
> +++ b/drivers/net/ipa/gsi.c
> @@ -1730,10 +1730,10 @@ static int gsi_channel_setup_one(struct gsi *gsi, u32 channel_id)
> gsi_channel_program(channel, true);
>
> if (channel->toward_ipa)
> - netif_napi_add_tx(&gsi->dummy_dev, &channel->napi,
> + netif_napi_add_tx(gsi->dummy_dev, &channel->napi,
> gsi_channel_poll);
> else
> - netif_napi_add(&gsi->dummy_dev, &channel->napi,
> + netif_napi_add(gsi->dummy_dev, &channel->napi,
> gsi_channel_poll);
>
> return 0;
> @@ -2369,12 +2369,14 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev,
> /* GSI uses NAPI on all channels. Create a dummy network device
> * for the channel NAPI contexts to be associated with.
> */
> - init_dummy_netdev(&gsi->dummy_dev);
> + gsi->dummy_dev = alloc_netdev_dummy(0);
> + if (!gsi->dummy_dev)
> + return -ENOMEM;
> init_completion(&gsi->completion);
>
> ret = gsi_reg_init(gsi, pdev);
> if (ret)
> - return ret;
> + goto err_reg_exit;
Assuming you change it to not just use kfree() to free the
dummy netdev, the above call won't work. You'll want to do
something like:
if (ret)
goto err_netdev_free;
. .
err_netdev_free:
free_dummy_netdev(gsi->dummy_dev);
err_reg_exit:
>
> ret = gsi_irq_init(gsi, pdev); /* No matching exit required */
> if (ret)
> @@ -2389,6 +2391,7 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev,
> return 0;
>
> err_reg_exit:
> + kfree(gsi->dummy_dev);
> gsi_reg_exit(gsi);
>
> return ret;
> @@ -2400,6 +2403,7 @@ void gsi_exit(struct gsi *gsi)
> mutex_destroy(&gsi->mutex);
> gsi_channel_exit(gsi);
Please call the free here, so the cleanup is done in
exactly the reverse order of the initialization.
-Alex
> gsi_reg_exit(gsi);
> + kfree(gsi->dummy_dev);
> }
>
> /* The maximum number of outstanding TREs on a channel. This limits
> diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h
> index 42063b227c18..6b7ec2a39676 100644
> --- a/drivers/net/ipa/gsi.h
> +++ b/drivers/net/ipa/gsi.h
> @@ -155,7 +155,7 @@ struct gsi {
> struct mutex mutex; /* protects commands, programming */
> struct gsi_channel channel[GSI_CHANNEL_COUNT_MAX];
> struct gsi_evt_ring evt_ring[GSI_EVT_RING_COUNT_MAX];
> - struct net_device dummy_dev; /* needed for NAPI */
> + struct net_device *dummy_dev; /* needed for NAPI */
> };
>
> /**
Hello Alex,
On Mon, Apr 01, 2024 at 08:56:46AM -0500, Alex Elder wrote:
> Thanks for pointing this out, I didn't notice the earlier
> discussion. Embedding the dummy netdev in this case was
> probably done to eliminate the chance of an unlikely
> allocation error at init time. It is not at all necessary.
>
> I had to go find the rest of your series. If at least one patch
> is addressed to me in a series, please copy me on all of them.
Sure, do you know if there ia way to do it using git send-email
identity?
I basically sent the patch series using git setnd-email with an
identity, and, for each patch, git send-email parses the patch and run
scripts/get_maintainer.pl for each patch, appeneding the "important"
people in that patch.
To do what you are suggesting, I would need to have a cumulative to: and
cc: list. Any tip here would be appreciate.
> I see the dummy netdev now gets "fully initialized" but that's
> a one-time thing, and seems harmless. But given that, shouldn't
> the result of alloc_dummy_netdev() also have a free_dummy_netdev()
> (rather than simply calling kfree(dummy_netdev))?
Right. I am moving to use free_netdev() now. I can us create a
free_dummy_netdev() macro that points to free_netdev(), but, I think
that might not be necessary.
> > @@ -2369,12 +2369,14 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev,
> > /* GSI uses NAPI on all channels. Create a dummy network device
> > * for the channel NAPI contexts to be associated with.
> > */
> > - init_dummy_netdev(&gsi->dummy_dev);
> > + gsi->dummy_dev = alloc_netdev_dummy(0);
> > + if (!gsi->dummy_dev)
> > + return -ENOMEM;
> > init_completion(&gsi->completion);
> > ret = gsi_reg_init(gsi, pdev);
> > if (ret)
> > - return ret;
> > + goto err_reg_exit;
>
> Assuming you change it to not just use kfree() to free the
> dummy netdev, the above call won't work. You'll want to do
> something like:
>
> if (ret)
> goto err_netdev_free;
>
> . . .
>
> err_netdev_free:
> free_dummy_netdev(gsi->dummy_dev);
> err_reg_exit:
I am not sure I followed this one. All the exit paths should free the
device, if I have err_netdev_free: label, then it will replace
err_reg_exit: label completely.
If I apply your suggestion, it will look like the following (with some
concerns I have).
gsi->dummy_dev = alloc_netdev_dummy(0);
if (!gsi->dummy_dev)
return -ENOMEM;
ret = gsi_reg_init(gsi, pdev);
if (ret)
goto err_netdev_free;
ret = gsi_irq_init(gsi, pdev);
if (ret)
goto err_reg_exit; <-- This needs to point to err_netdev_free also
ret = gsi_channel_init(gsi, count, data);
if (ret)
goto err_reg_exit; <-- This needs to point to err_netdev_free also
mutex_init(&gsi->mutex);
return 0;
err_netdev_free:
free_netdev(gsi->dummy_dev);
err_reg_exit: <-- This label will be unused
gsi_reg_exit(gsi);
That said, basically fixing the concerns above will result in the same code I
originally proposed.
> @@ -2400,6 +2403,7 @@ void gsi_exit(struct gsi *gsi)
> > mutex_destroy(&gsi->mutex);
> > gsi_channel_exit(gsi);
>
> Please call the free here, so the cleanup is done in
> exactly the reverse order of the initialization.
Ack!
Thanks for the feedback.
On Sun, Mar 31, 2024 at 09:54:38AM +0100, Simon Horman wrote:
> On Thu, Mar 28, 2024 at 04:52:02PM -0700, Breno Leitao wrote:
> > Embedding net_device into structures prohibits the usage of flexible
> > arrays in the net_device structure. For more details, see the discussion
> > at [1].
> >
> > Un-embed the net_device from the private struct by converting it
> > into a pointer. Then use the leverage the new alloc_netdev_dummy()
> > helper to allocate and initialize dummy devices.
> >
> > [1] https://lore.kernel.org/all/[email protected]/
> >
> > Signed-off-by: Breno Leitao <[email protected]>
> > ---
> > .../net/ethernet/marvell/prestera/prestera_rxtx.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_rxtx.c b/drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
>
> ...
>
> > @@ -654,13 +654,21 @@ static int prestera_sdma_switch_init(struct prestera_switch *sw)
> > if (err)
> > goto err_evt_register;
> >
> > - init_dummy_netdev(&sdma->napi_dev);
> > + sdma->napi_dev = alloc_netdev_dummy(0);
> > + if (!sdma->napi_dev) {
> > + dev_err(dev, "not able to initialize dummy device\n");
> > + goto err_alloc_dummy;
>
> Hi Breno,
>
> This goto will result in the function returning err.
> But err is 0 here. Perhaps it should be set to a negative error value
> instead?
Definitely, that was a good catch.
> Flagged by Smatch.
I am curious how you are running Smatch. I tried to run it here
according to[1] , and I found different and valid errors also, that I
will fix soon. For instance:
drivers/net/ethernet/marvell/prestera/prestera_main.c:433 prestera_port_sfp_bind() error: uninitialized symbol 'err'.
drivers/net/ethernet/marvell/prestera/prestera_main.c:861 prestera_switch_set_base_mac_addr() error: uninitialized symbol 'ret'.
[1] https://rajanvaja.wordpress.com/2021/02/06/how-to-run-smatch-on-linux-kernel/
Thanks