Received-SPF: pass (google.com: domain of linux-kernel+bounces-129855-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 3 Apr 2024 06:38:58 -0700
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
Message-ID: <Zg1b8vuTs5Z+1Obv@gmail.com>
References: <20240328235214.4079063-1-leitao@debian.org>
 <20240328235214.4079063-5-leitao@debian.org>
 <c03b8113-e1be-4cf3-a85c-43de15163ab1@ieee.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
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.