[PATCH v2] IB/hfi1: allocate dummy net_device dynamically

[PATCH v2] IB/hfi1: allocate dummy net_device dynamically

[Breno Leitao]

struct net_device shouldn't be embedded into any structure, instead,
the owner should use the priv space to embed their state into net_device.

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 struct hfi1_netdev_rx by converting it
into a pointer. Then use the leverage alloc_netdev() to allocate the
net_device object at hfi1_alloc_rx().

[1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/

Signed-off-by: Breno Leitao <leitao@debian.org>

----
PS: this diff needs d160c66cda0ac8614 ("net: Do not return value from
init_dummy_netdev()") in order to apply and build cleanly.
---
Changelog:

v2:
	* Free struct hfi1_netdev_rx allocation if alloc_netdev() fails
	* Pass zero as the private size for alloc_netdev().
	* Remove wrong reference for iwl in the comments
---
 drivers/infiniband/hw/hfi1/netdev.h    |  2 +-
 drivers/infiniband/hw/hfi1/netdev_rx.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/netdev.h b/drivers/infiniband/hw/hfi1/netdev.h
index 8aa074670a9c..07c8f77c9181 100644
--- a/drivers/infiniband/hw/hfi1/netdev.h
+++ b/drivers/infiniband/hw/hfi1/netdev.h
@@ -49,7 +49,7 @@ struct hfi1_netdev_rxq {
  *		When 0 receive queues will be freed.
  */
 struct hfi1_netdev_rx {
-	struct net_device rx_napi;
+	struct net_device *rx_napi;
 	struct hfi1_devdata *dd;
 	struct hfi1_netdev_rxq *rxq;
 	int num_rx_q;
diff --git a/drivers/infiniband/hw/hfi1/netdev_rx.c b/drivers/infiniband/hw/hfi1/netdev_rx.c
index 720d4c85c9c9..cd6e78e257ef 100644
--- a/drivers/infiniband/hw/hfi1/netdev_rx.c
+++ b/drivers/infiniband/hw/hfi1/netdev_rx.c
@@ -188,7 +188,7 @@ static int hfi1_netdev_rxq_init(struct hfi1_netdev_rx *rx)
 	int i;
 	int rc;
 	struct hfi1_devdata *dd = rx->dd;
-	struct net_device *dev = &rx->rx_napi;
+	struct net_device *dev = rx->rx_napi;
 
 	rx->num_rx_q = dd->num_netdev_contexts;
 	rx->rxq = kcalloc_node(rx->num_rx_q, sizeof(*rx->rxq),
@@ -360,7 +360,12 @@ int hfi1_alloc_rx(struct hfi1_devdata *dd)
 	if (!rx)
 		return -ENOMEM;
 	rx->dd = dd;
-	init_dummy_netdev(&rx->rx_napi);
+	rx->rx_napi = alloc_netdev(0, "dummy", NET_NAME_UNKNOWN,
+				   init_dummy_netdev);
+	if (!rx->rx_napi) {
+		kfree(rx);
+		return -ENOMEM;
+	}
 
 	xa_init(&rx->dev_tbl);
 	atomic_set(&rx->enabled, 0);
@@ -374,6 +379,7 @@ void hfi1_free_rx(struct hfi1_devdata *dd)
 {
 	if (dd->netdev_rx) {
 		dd_dev_info(dd, "hfi1 rx freed\n");
+		free_netdev(dd->netdev_rx->rx_napi);
 		kfree(dd->netdev_rx);
 		dd->netdev_rx = NULL;
 	}
-- 
2.43.0

Re: [PATCH v2] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Wed, Mar 13, 2024 at 03:33:10AM -0700, Breno Leitao wrote:
> struct net_device shouldn't be embedded into any structure, instead,
> the owner should use the priv space to embed their state into net_device.

"shouldn't" is a strong word, given the fact that original init_dummy_netdev()
was designed to be used in such way.

> 
> 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 struct hfi1_netdev_rx by converting it
> into a pointer. Then use the leverage alloc_netdev() to allocate the
> net_device object at hfi1_alloc_rx().
> 
> [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> ----
> PS: this diff needs d160c66cda0ac8614 ("net: Do not return value from
> init_dummy_netdev()") in order to apply and build cleanly.

We are in merge window now, so if Dennis approves, I will apply it after the merge window

Thanks

Re: [PATCH v2] IB/hfi1: allocate dummy net_device dynamically

[Jakub Kicinski]

On Wed, 13 Mar 2024 12:42:52 +0200 Leon Romanovsky wrote:
> > PS: this diff needs d160c66cda0ac8614 ("net: Do not return value from
> > init_dummy_netdev()") in order to apply and build cleanly.  
> 
> We are in merge window now, so if Dennis approves, I will apply it
> after the merge window

Can we do a shared branch? We don't want to wait full release cycle
for a single driver outside of netdev.

Re: [PATCH v2] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Wed, Mar 13, 2024 at 06:55:26AM -0700, Jakub Kicinski wrote:
> On Wed, 13 Mar 2024 12:42:52 +0200 Leon Romanovsky wrote:
> > > PS: this diff needs d160c66cda0ac8614 ("net: Do not return value from
> > > init_dummy_netdev()") in order to apply and build cleanly.  
> > 
> > We are in merge window now, so if Dennis approves, I will apply it
> > after the merge window
> 
> Can we do a shared branch? We don't want to wait full release cycle
> for a single driver outside of netdev.

No problem, I will create a branch based on -rc1 for it.

Thanks

Re: [PATCH v2] IB/hfi1: allocate dummy net_device dynamically

[Dennis Dalessandro]

On 3/13/24 6:33 AM, Breno Leitao wrote:
> struct net_device shouldn't be embedded into any structure, instead,
> the owner should use the priv space to embed their state into net_device.
> 
> 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 struct hfi1_netdev_rx by converting it
> into a pointer. Then use the leverage alloc_netdev() to allocate the
> net_device object at hfi1_alloc_rx().
> 
> [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> ----
> PS: this diff needs d160c66cda0ac8614 ("net: Do not return value from
> init_dummy_netdev()") in order to apply and build cleanly.
> ---
> Changelog:
> 
> v2:
> 	* Free struct hfi1_netdev_rx allocation if alloc_netdev() fails
> 	* Pass zero as the private size for alloc_netdev().
> 	* Remove wrong reference for iwl in the comments
> ---

Very lightly tested, but interface came up and I could send traffic. Code seems
OK too.

I'd prefer to at least remove the first sentence of the commit message. It makes
it sound like hfi1 was doing something incorrectly when it was using the
interface as it was designed. Instead it should read more like this is an
enhancement.


Regardless, we can call this one acked.


Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>

Re: [PATCH v2] IB/hfi1: allocate dummy net_device dynamically

[Breno Leitao]

On Fri, Mar 15, 2024 at 12:12:15PM -0400, Dennis Dalessandro wrote:
> On 3/13/24 6:33 AM, Breno Leitao wrote:
> > struct net_device shouldn't be embedded into any structure, instead,
> > the owner should use the priv space to embed their state into net_device.
> > 
> > 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 struct hfi1_netdev_rx by converting it
> > into a pointer. Then use the leverage alloc_netdev() to allocate the
> > net_device object at hfi1_alloc_rx().
> > 
> > [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > 
> > ----
> > PS: this diff needs d160c66cda0ac8614 ("net: Do not return value from
> > init_dummy_netdev()") in order to apply and build cleanly.
> > ---
> > Changelog:
> > 
> > v2:
> > 	* Free struct hfi1_netdev_rx allocation if alloc_netdev() fails
> > 	* Pass zero as the private size for alloc_netdev().
> > 	* Remove wrong reference for iwl in the comments
> > ---
> 
> Very lightly tested, but interface came up and I could send traffic. Code seems
> OK too.
> 
> I'd prefer to at least remove the first sentence of the commit message.

That is OK for me. Would you like to remove it when merging it, or,
would you prefer me to resend it?

Thanks

Re: [PATCH v2] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Fri, Mar 15, 2024 at 10:51:46AM -0700, Breno Leitao wrote:
> On Fri, Mar 15, 2024 at 12:12:15PM -0400, Dennis Dalessandro wrote:
> > On 3/13/24 6:33 AM, Breno Leitao wrote:
> > > struct net_device shouldn't be embedded into any structure, instead,
> > > the owner should use the priv space to embed their state into net_device.
> > > 
> > > 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 struct hfi1_netdev_rx by converting it
> > > into a pointer. Then use the leverage alloc_netdev() to allocate the
> > > net_device object at hfi1_alloc_rx().
> > > 
> > > [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> > > 
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > 
> > > ----
> > > PS: this diff needs d160c66cda0ac8614 ("net: Do not return value from
> > > init_dummy_netdev()") in order to apply and build cleanly.
> > > ---
> > > Changelog:
> > > 
> > > v2:
> > > 	* Free struct hfi1_netdev_rx allocation if alloc_netdev() fails
> > > 	* Pass zero as the private size for alloc_netdev().
> > > 	* Remove wrong reference for iwl in the comments
> > > ---
> > 
> > Very lightly tested, but interface came up and I could send traffic. Code seems
> > OK too.
> > 
> > I'd prefer to at least remove the first sentence of the commit message.
> 
> That is OK for me. Would you like to remove it when merging it, or,
> would you prefer me to resend it?

Please resend together with Dennis's Acked-by.

Thanks

> 
> Thanks