Messages in this thread Patch in this message | | | Date | Wed, 3 Apr 2024 06:38:58 -0700 | From | Breno Leitao <> | Subject | Re: [PATCH net-next v2 4/5] net: ipa: allocate dummy net_device dynamically |
| |
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.
| |