Received-SPF: pass (google.com: domain of linux-kernel+bounces-126695-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <c03b8113-e1be-4cf3-a85c-43de15163ab1@ieee.org>
Date: Mon, 1 Apr 2024 08:56:46 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v2 4/5] net: ipa: allocate dummy net_device
 dynamically
To: Breno Leitao <leitao@debian.org>, aleksander.lobakin@intel.com,
 kuba@kernel.org, davem@davemloft.net, pabeni@redhat.com,
 edumazet@google.com, Alex Elder <elder@kernel.org>
Cc: quic_jjohnson@quicinc.com, kvalo@kernel.org, leon@kernel.org,
 dennis.dalessandro@cornelisnetworks.com, linux-kernel@vger.kernel.org,
 netdev@vger.kernel.org
References: <20240328235214.4079063-1-leitao@debian.org>
 <20240328235214.4079063-5-leitao@debian.org>
Content-Language: en-US
From: Alex Elder <elder@ieee.org>
In-Reply-To: <20240328235214.4079063-5-leitao@debian.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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 */
>   };
>   
>   /**