From: Breno Leitao <leitao@debian•org>
To: Alex Elder <elder@ieee•org>
Cc: aleksander.lobakin@intel•com, kuba@kernel•org,
davem@davemloft•net, pabeni@redhat•com, edumazet@google•com,
Alex Elder <elder@kernel•org>,
quic_jjohnson@quicinc•com, kvalo@kernel•org, leon@kernel•org,
dennis.dalessandro@cornelisnetworks•com,
linux-kernel@vger•kernel.org, netdev@vger•kernel.org
Subject: Re: [PATCH net-next v2 4/5] net: ipa: allocate dummy net_device dynamically
Date: Wed, 3 Apr 2024 06:38:58 -0700 [thread overview]
Message-ID: <Zg1b8vuTs5Z+1Obv@gmail.com> (raw)
In-Reply-To: <c03b8113-e1be-4cf3-a85c-43de15163ab1@ieee.org>
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.
next prev parent reply other threads:[~2024-04-03 13:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-28 23:52 [PATCH net-next v2 0/5] allocate dummy device dynamically Breno Leitao
2024-03-28 23:52 ` [PATCH net-next v2 1/5] net: create a dummy net_device allocator Breno Leitao
2024-03-28 23:52 ` [PATCH net-next v2 2/5] net: marvell: prestera: allocate dummy net_device dynamically Breno Leitao
2024-03-29 15:56 ` Jakub Kicinski
2024-03-29 17:21 ` Breno Leitao
2024-03-31 8:54 ` Simon Horman
2024-04-03 14:27 ` Breno Leitao
2024-03-28 23:52 ` [PATCH net-next v2 3/5] net: mediatek: mtk_eth_sock: " Breno Leitao
2024-03-28 23:52 ` [PATCH net-next v2 4/5] net: ipa: " Breno Leitao
2024-04-01 13:56 ` Alex Elder
2024-04-03 13:38 ` Breno Leitao [this message]
2024-03-28 23:52 ` [PATCH net-next v2 5/5] net: ibm/emac: " Breno Leitao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zg1b8vuTs5Z+1Obv@gmail.com \
--to=leitao@debian$(echo .)org \
--cc=aleksander.lobakin@intel$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dennis.dalessandro@cornelisnetworks$(echo .)com \
--cc=edumazet@google$(echo .)com \
--cc=elder@ieee$(echo .)org \
--cc=elder@kernel$(echo .)org \
--cc=kuba@kernel$(echo .)org \
--cc=kvalo@kernel$(echo .)org \
--cc=leon@kernel$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=quic_jjohnson@quicinc$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox