1. Patchew
  2. linux
  3. [PATCH net-next v2 0/5] allocate dummy device dynamically
  4. View patch

[PATCH net-next v2 4/5] net: ipa: allocate dummy net_device dynamically

Breno Leitao posted 5 patches 5 months, 3 weeks ago
There is a newer version of this series
[PATCH net-next v2 4/5] net: ipa: allocate dummy net_device dynamically
Posted by Breno Leitao 5 months, 3 weeks ago 
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>
---
 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;
 
 	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);
 	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 */
 };
 
 /**
-- 
2.43.0
Re: [PATCH net-next v2 4/5] net: ipa: allocate dummy net_device dynamically
Posted by Alex Elder 5 months, 3 weeks ago 
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 */
>   };
>   
>   /**
Re: [PATCH net-next v2 4/5] net: ipa: allocate dummy net_device dynamically
Posted by Breno Leitao 5 months, 2 weeks ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.