[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zg1b8vuTs5Z+1Obv@gmail.com>
Date: Wed, 3 Apr 2024 06:38:58 -0700
From: Breno Leitao <leitao@...ian.org>
To: Alex Elder <elder@...e.org>
Cc: aleksander.lobakin@...el.com, kuba@...nel.org, davem@...emloft.net,
pabeni@...hat.com, edumazet@...gle.com,
Alex Elder <elder@...nel.org>, quic_jjohnson@...cinc.com,
kvalo@...nel.org, leon@...nel.org,
dennis.dalessandro@...nelisnetworks.com,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org
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.
Powered by blists - more mailing lists