| Message ID | 20240328235214.4079063-5-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | allocate dummy device dynamically | expand |
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/20240229225910.79e224cf@kernel.org/ > > Signed-off-by: Breno Leitao <leitao@debian.org> 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.
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 */ }; /**
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/20240229225910.79e224cf@kernel.org/ Signed-off-by: Breno Leitao <leitao@debian.org> --- drivers/net/ipa/gsi.c | 12 ++++++++---- drivers/net/ipa/gsi.h | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-)