[PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Breno Leitao]

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/

Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
Changelog

v5:
	* Basically replaced the old alloc_netdev() by the new helper
	  alloc_netdev_dummy().
v4:
	* Fix the changelog format
v3:
	* Re-worded the comment, by removing the first paragraph.
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 | 9 +++++++--
 2 files changed, 8 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..8608044203bb 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,11 @@ 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_dummy(0);
+	if (!rx->rx_napi) {
+		kfree(rx);
+		return -ENOMEM;
+	}
 
 	xa_init(&rx->dev_tbl);
 	atomic_set(&rx->enabled, 0);
@@ -374,6 +378,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 net-next] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Fri, Apr 26, 2024 at 01:56:05AM -0700, 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 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/
> 
> Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> Changelog
> 
> v5:
> 	* Basically replaced the old alloc_netdev() by the new helper
> 	  alloc_netdev_dummy().
> v4:
> 	* Fix the changelog format
> v3:
> 	* Re-worded the comment, by removing the first paragraph.
> 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 | 9 +++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)

Dennis,

Do you plan to send anything to rdma-next which can potentially create
conflicts with netdev in this cycle?

If not, it will be safe to apply this patch directly to net-next.

Thanks

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Dennis Dalessandro]

On 4/30/24 8:50 AM, Leon Romanovsky wrote:
> On Fri, Apr 26, 2024 at 01:56:05AM -0700, 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 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/
>>
>> Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
>> Signed-off-by: Breno Leitao <leitao@debian.org>
>> ---
>> Changelog
>>
>> v5:
>> 	* Basically replaced the old alloc_netdev() by the new helper
>> 	  alloc_netdev_dummy().
>> v4:
>> 	* Fix the changelog format
>> v3:
>> 	* Re-worded the comment, by removing the first paragraph.
>> 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 | 9 +++++++--
>>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> Dennis,
> 
> Do you plan to send anything to rdma-next which can potentially create
> conflicts with netdev in this cycle?
> 
> If not, it will be safe to apply this patch directly to net-next.
> 
> Thanks

Nothing right now. Should be safe to sent to net-next.

FYI, since I talked about it publicly at the OFA Workshop recently. We will be
starting the upstream of support for our new HW, soon.

-Denny

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Tue, Apr 30, 2024 at 10:03:49AM -0400, Dennis Dalessandro wrote:
> On 4/30/24 8:50 AM, Leon Romanovsky wrote:
> > On Fri, Apr 26, 2024 at 01:56:05AM -0700, 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 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/
> >>
> >> Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> >> Signed-off-by: Breno Leitao <leitao@debian.org>
> >> ---
> >> Changelog
> >>
> >> v5:
> >> 	* Basically replaced the old alloc_netdev() by the new helper
> >> 	  alloc_netdev_dummy().
> >> v4:
> >> 	* Fix the changelog format
> >> v3:
> >> 	* Re-worded the comment, by removing the first paragraph.
> >> 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 | 9 +++++++--
> >>  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > Dennis,
> > 
> > Do you plan to send anything to rdma-next which can potentially create
> > conflicts with netdev in this cycle?
> > 
> > If not, it will be safe to apply this patch directly to net-next.
> > 
> > Thanks
> 
> Nothing right now. Should be safe to sent to net-next.

Jakub, can you please take this patch?

> 
> FYI, since I talked about it publicly at the OFA Workshop recently. We will be
> starting the upstream of support for our new HW, soon.

Great, thanks

> 
> -Denny

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Jakub Kicinski]

On Tue, 30 Apr 2024 17:10:39 +0300 Leon Romanovsky wrote:
> > Nothing right now. Should be safe to sent to net-next.  
> 
> Jakub, can you please take this patch?

We'll need a repost, it wasn't CCed to netdev.

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Breno Leitao]

On Tue, Apr 30, 2024 at 07:55:34AM -0700, Jakub Kicinski wrote:
> On Tue, 30 Apr 2024 17:10:39 +0300 Leon Romanovsky wrote:
> > > Nothing right now. Should be safe to sent to net-next.  
> > 
> > Jakub, can you please take this patch?
> 
> We'll need a repost, it wasn't CCed to netdev.

I can repost it and CC netdev.

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Mon, Mar 11, 2024 at 08:32:03AM -0700, Breno Leitao wrote:
> Hello Dennis,
> 
> On Mon, Mar 11, 2024 at 08:05:45AM -0400, Dennis Dalessandro wrote:
> > On 3/8/24 1:29 PM, 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 iwl_trans_pcie by converting it
> > > into a pointer. Then use the leverage alloc_netdev() to allocate the
> > > net_device object at iwl_trans_pcie_alloc.
> > 
> > What does an Omni-Path Architecture driver from Cornelis Networks have to do
> > with an Intel wireless driver?
> 
> That is an oversight. I will fix it in v2. Sorry about it.
> 
> > > The private data of net_device becomes a pointer for the struct
> > > iwl_trans_pcie, so, it is easy to get back to the iwl_trans_pcie parent
> > > given the net_device object.
> > > 
> > > [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> > > 
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > ---
> > >  drivers/infiniband/hw/hfi1/netdev.h    | 2 +-
> > >  drivers/infiniband/hw/hfi1/netdev_rx.c | 9 +++++++--
> > >  2 files changed, 8 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..5c26a69fa2bb 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,11 @@ 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(sizeof(struct iwl_trans_pcie *),
> > > +				   "dummy", NET_NAME_UNKNOWN,
> > > +				   init_dummy_netdev);
> > 
> > Again with the iwl stuff? Please do not stuff to the mailing list that doesn't
> > even compile....
> > 
> >  CC [M]  drivers/infiniband/hw/hfi1/verbs.o
> >   CC [M]  drivers/infiniband/hw/hfi1/verbs_txreq.o
> >   CC [M]  drivers/infiniband/hw/hfi1/vnic_main.o
> > In file included from ./include/net/sock.h:46,
> >                  from ./include/linux/tcp.h:19,
> >                  from ./include/linux/ipv6.h:95,
> >                  from ./include/net/ipv6.h:12,
> >                  from ./include/rdma/ib_verbs.h:25,
> >                  from ./include/rdma/ib_hdrs.h:11,
> >                  from drivers/infiniband/hw/hfi1/hfi.h:29,
> >                  from drivers/infiniband/hw/hfi1/sdma.h:15,
> >                  from drivers/infiniband/hw/hfi1/netdev_rx.c:11:
> > drivers/infiniband/hw/hfi1/netdev_rx.c: In function ‘hfi1_alloc_rx’:
> > drivers/infiniband/hw/hfi1/netdev_rx.c:365:36: error: passing argument 4 of
> > ‘alloc_netdev_mqs’ from incompatible pointer type
> > [-Werror=incompatible-pointer-types]
> >   365 |                                    init_dummy_netdev);
> >       |                                    ^~~~~~~~~~~~~~~~~
> >       |                                    |
> >       |                                    int (*)(struct net_device *)
> > ./include/linux/netdevice.h:4632:63: note: in definition of macro ‘alloc_netdev’
> >  4632 |         alloc_netdev_mqs(sizeof_priv, name, name_assign_type, setup, 1, 1)
> >       |                                                               ^~~~~
> > ./include/linux/netdevice.h:4629:44: note: expected ‘void (*)(struct net_device
> > *)’ but argument is of type ‘int (*)(struct net_device *)’
> >  4629 |                                     void (*setup)(struct net_device *),
> >       |                                     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   CC [M]  drivers/infiniband/hw/hfi1/vnic_sdma.o
> 
> Sorry, this patch is against net-next and you probably tested in Linus'
> upstream.

Dennis tested against rdma-next git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git

Thanks

> 
> You need to have d160c66cda0ac8614 ("net: Do not return value from
> init_dummy_netdev()"), which is in net-next, and has this important
> change that is necessary for this patch:
> 
>     -int init_dummy_netdev(struct net_device *dev);
>     +void init_dummy_netdev(struct net_device *dev);
> 
> If you are OK with a v2, I will fix the topics reported in this thread.
> 
> Thank you
> Breno

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Mon, Mar 11, 2024 at 11:25:32AM -0700, Jakub Kicinski wrote:
> On Mon, 11 Mar 2024 12:22:51 +0200 Leon Romanovsky wrote:
> > > From my experience, you can leverage all the helpers to deal with the
> > > relationship between struct net_device and you private structure. Here
> > > are some examples that comes to my mind:
> > > 
> > > * alloc_netdev() allocates the private structure for you
> > > * netdev_priv() gets the private structure for you
> > > * dev->priv_destructor sets the destructure to be called when the
> > >   interface goes away or failures.  
> > 
> > Everything above is true, but it doesn't relevant to HFI1 devices which
> > are not netdev devices.
> 
> Why are they abusing struct net_device then?

Care to explain what is the abuse here?
HFI1 uses init_dummy_netdev() exactly as it should be used according to
the commit message.

This netdevice is controlled by HFI1 lifetime model and used for NAPI.
https://lore.kernel.org/netdev/1231906495.22571.79.camel@pasglop/

> If you're willing to take care of removing the use of NAPI from this
> driver completely, that'd be great.

I have no plans to remove NAPI from HFI1 driver. We are not against to
accept Bruno's removal patch, but we are asking for justification
in commit message.

> 
> > > > Will it create multiple "dummy" netdev in the system? Will all devices
> > > > have the same "dummy" name?  
> > > 
> > > Are these devices visible to userspace?  
> > 
> > HFI devices yes, dummy device no.
> > 
> > > 
> > > This allocation are using NET_NAME_UNKNOWN, which implies that the
> > > device is not expose to userspace.  
> > 
> > Great
> > 
> > > 
> > > Would you prefer a different name?  
> > 
> > I prefer to see some new wrapper over plain alloc_netdev, which will
> > create this dummy netdevice. For example, alloc_dummy_netdev(...).
> 
> Nope, no bona fide APIs for hacky uses.

Interesting position, instead of making simple and self descriptive API
which is impossible to misuse, you prefer complexity.

HFI1 is not alone in using init_dummy_netdev():
➜  kernel git:(rdma-next) ✗ git grep "init_dummy_netdev(&"
rivers/infiniband/hw/hfi1/netdev_rx.c: init_dummy_netdev(&rx->rx_napi);
drivers/net/ethernet/ibm/emac/mal.c:    init_dummy_netdev(&mal->dummy_dev);
drivers/net/ethernet/marvell/prestera/prestera_rxtx.c:  init_dummy_netdev(&sdma->napi_dev);
drivers/net/ethernet/mediatek/mtk_eth_soc.c:    init_dummy_netdev(&eth->dummy_dev);
drivers/net/ipa/gsi.c:  init_dummy_netdev(&gsi->dummy_dev);
drivers/net/wireless/ath/ath10k/core.c: init_dummy_netdev(&ar->napi_dev);
drivers/net/wireless/ath/ath11k/ahb.c:          init_dummy_netdev(&irq_grp->napi_ndev);
drivers/net/wireless/ath/ath11k/pcic.c:         init_dummy_netdev(&irq_grp->napi_ndev);
drivers/net/wireless/ath/ath12k/pci.c:          init_dummy_netdev(&irq_grp->napi_ndev);
drivers/net/wireless/ath/wil6210/netdev.c:      init_dummy_netdev(&wil->napi_ndev);
drivers/net/wireless/intel/iwlwifi/pcie/trans.c:                init_dummy_netdev(&trans_pcie->napi_dev);
drivers/net/wireless/mediatek/mt76/dma.c:       init_dummy_netdev(&dev->napi_dev);
drivers/net/wireless/mediatek/mt76/dma.c:       init_dummy_netdev(&dev->tx_napi_dev);
drivers/net/wireless/quantenna/qtnfmac/pcie/pcie.c:     init_dummy_netdev(&bus->mux_dev);
drivers/net/wireless/realtek/rtw88/pci.c:       init_dummy_netdev(&rtwpci->netdev);
drivers/net/wireless/realtek/rtw89/core.c:      init_dummy_netdev(&rtwdev->netdev);
drivers/net/wwan/t7xx/t7xx_netdev.c:    init_dummy_netdev(&ctlb->dummy_dev);
net/mptcp/protocol.c:   init_dummy_netdev(&mptcp_napi_dev);
net/xfrm/xfrm_input.c:  init_dummy_netdev(&xfrm_napi_dev);

Thanks

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Dennis Dalessandro]

On 3/11/24 11:32 AM, Breno Leitao wrote:
> Hello Dennis,
> 
> On Mon, Mar 11, 2024 at 08:05:45AM -0400, Dennis Dalessandro wrote:
>> On 3/8/24 1:29 PM, 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 iwl_trans_pcie by converting it
>>> into a pointer. Then use the leverage alloc_netdev() to allocate the
>>> net_device object at iwl_trans_pcie_alloc.
>>
>> What does an Omni-Path Architecture driver from Cornelis Networks have to do
>> with an Intel wireless driver?
> 
> That is an oversight. I will fix it in v2. Sorry about it.
> 
>>> The private data of net_device becomes a pointer for the struct
>>> iwl_trans_pcie, so, it is easy to get back to the iwl_trans_pcie parent
>>> given the net_device object.
>>>
>>> [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
>>>
>>> Signed-off-by: Breno Leitao <leitao@debian.org>
>>> ---
>>>  drivers/infiniband/hw/hfi1/netdev.h    | 2 +-
>>>  drivers/infiniband/hw/hfi1/netdev_rx.c | 9 +++++++--
>>>  2 files changed, 8 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..5c26a69fa2bb 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,11 @@ 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(sizeof(struct iwl_trans_pcie *),
>>> +				   "dummy", NET_NAME_UNKNOWN,
>>> +				   init_dummy_netdev);
>>
>> Again with the iwl stuff? Please do not stuff to the mailing list that doesn't
>> even compile....
>>
>>  CC [M]  drivers/infiniband/hw/hfi1/verbs.o
>>   CC [M]  drivers/infiniband/hw/hfi1/verbs_txreq.o
>>   CC [M]  drivers/infiniband/hw/hfi1/vnic_main.o
>> In file included from ./include/net/sock.h:46,
>>                  from ./include/linux/tcp.h:19,
>>                  from ./include/linux/ipv6.h:95,
>>                  from ./include/net/ipv6.h:12,
>>                  from ./include/rdma/ib_verbs.h:25,
>>                  from ./include/rdma/ib_hdrs.h:11,
>>                  from drivers/infiniband/hw/hfi1/hfi.h:29,
>>                  from drivers/infiniband/hw/hfi1/sdma.h:15,
>>                  from drivers/infiniband/hw/hfi1/netdev_rx.c:11:
>> drivers/infiniband/hw/hfi1/netdev_rx.c: In function ‘hfi1_alloc_rx’:
>> drivers/infiniband/hw/hfi1/netdev_rx.c:365:36: error: passing argument 4 of
>> ‘alloc_netdev_mqs’ from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
>>   365 |                                    init_dummy_netdev);
>>       |                                    ^~~~~~~~~~~~~~~~~
>>       |                                    |
>>       |                                    int (*)(struct net_device *)
>> ./include/linux/netdevice.h:4632:63: note: in definition of macro ‘alloc_netdev’
>>  4632 |         alloc_netdev_mqs(sizeof_priv, name, name_assign_type, setup, 1, 1)
>>       |                                                               ^~~~~
>> ./include/linux/netdevice.h:4629:44: note: expected ‘void (*)(struct net_device
>> *)’ but argument is of type ‘int (*)(struct net_device *)’
>>  4629 |                                     void (*setup)(struct net_device *),
>>       |                                     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   CC [M]  drivers/infiniband/hw/hfi1/vnic_sdma.o
> 
> Sorry, this patch is against net-next and you probably tested in Linus'
> upstream.
> 
> You need to have d160c66cda0ac8614 ("net: Do not return value from
> init_dummy_netdev()"), which is in net-next, and has this important
> change that is necessary for this patch:
> 
>     -int init_dummy_netdev(struct net_device *dev);
>     +void init_dummy_netdev(struct net_device *dev);
> 
> If you are OK with a v2, I will fix the topics reported in this thread.

You also use struct iwl_trans_pcie in hfi1 code. Fix that up, and make sure the
patch builds and I'll give it a test.

-Denny

> Thank you
> Breno

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Jakub Kicinski]

On Mon, 11 Mar 2024 12:22:51 +0200 Leon Romanovsky wrote:
> > From my experience, you can leverage all the helpers to deal with the
> > relationship between struct net_device and you private structure. Here
> > are some examples that comes to my mind:
> > 
> > * alloc_netdev() allocates the private structure for you
> > * netdev_priv() gets the private structure for you
> > * dev->priv_destructor sets the destructure to be called when the
> >   interface goes away or failures.  
> 
> Everything above is true, but it doesn't relevant to HFI1 devices which
> are not netdev devices.

Why are they abusing struct net_device then?
If you're willing to take care of removing the use of NAPI from this
driver completely, that'd be great.

> > > Will it create multiple "dummy" netdev in the system? Will all devices
> > > have the same "dummy" name?  
> > 
> > Are these devices visible to userspace?  
> 
> HFI devices yes, dummy device no.
> 
> > 
> > This allocation are using NET_NAME_UNKNOWN, which implies that the
> > device is not expose to userspace.  
> 
> Great
> 
> > 
> > Would you prefer a different name?  
> 
> I prefer to see some new wrapper over plain alloc_netdev, which will
> create this dummy netdevice. For example, alloc_dummy_netdev(...).

Nope, no bona fide APIs for hacky uses.

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Breno Leitao]

Hello Dennis,

On Mon, Mar 11, 2024 at 08:05:45AM -0400, Dennis Dalessandro wrote:
> On 3/8/24 1:29 PM, 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 iwl_trans_pcie by converting it
> > into a pointer. Then use the leverage alloc_netdev() to allocate the
> > net_device object at iwl_trans_pcie_alloc.
> 
> What does an Omni-Path Architecture driver from Cornelis Networks have to do
> with an Intel wireless driver?

That is an oversight. I will fix it in v2. Sorry about it.

> > The private data of net_device becomes a pointer for the struct
> > iwl_trans_pcie, so, it is easy to get back to the iwl_trans_pcie parent
> > given the net_device object.
> > 
> > [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  drivers/infiniband/hw/hfi1/netdev.h    | 2 +-
> >  drivers/infiniband/hw/hfi1/netdev_rx.c | 9 +++++++--
> >  2 files changed, 8 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..5c26a69fa2bb 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,11 @@ 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(sizeof(struct iwl_trans_pcie *),
> > +				   "dummy", NET_NAME_UNKNOWN,
> > +				   init_dummy_netdev);
> 
> Again with the iwl stuff? Please do not stuff to the mailing list that doesn't
> even compile....
> 
>  CC [M]  drivers/infiniband/hw/hfi1/verbs.o
>   CC [M]  drivers/infiniband/hw/hfi1/verbs_txreq.o
>   CC [M]  drivers/infiniband/hw/hfi1/vnic_main.o
> In file included from ./include/net/sock.h:46,
>                  from ./include/linux/tcp.h:19,
>                  from ./include/linux/ipv6.h:95,
>                  from ./include/net/ipv6.h:12,
>                  from ./include/rdma/ib_verbs.h:25,
>                  from ./include/rdma/ib_hdrs.h:11,
>                  from drivers/infiniband/hw/hfi1/hfi.h:29,
>                  from drivers/infiniband/hw/hfi1/sdma.h:15,
>                  from drivers/infiniband/hw/hfi1/netdev_rx.c:11:
> drivers/infiniband/hw/hfi1/netdev_rx.c: In function ‘hfi1_alloc_rx’:
> drivers/infiniband/hw/hfi1/netdev_rx.c:365:36: error: passing argument 4 of
> ‘alloc_netdev_mqs’ from incompatible pointer type
> [-Werror=incompatible-pointer-types]
>   365 |                                    init_dummy_netdev);
>       |                                    ^~~~~~~~~~~~~~~~~
>       |                                    |
>       |                                    int (*)(struct net_device *)
> ./include/linux/netdevice.h:4632:63: note: in definition of macro ‘alloc_netdev’
>  4632 |         alloc_netdev_mqs(sizeof_priv, name, name_assign_type, setup, 1, 1)
>       |                                                               ^~~~~
> ./include/linux/netdevice.h:4629:44: note: expected ‘void (*)(struct net_device
> *)’ but argument is of type ‘int (*)(struct net_device *)’
>  4629 |                                     void (*setup)(struct net_device *),
>       |                                     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   CC [M]  drivers/infiniband/hw/hfi1/vnic_sdma.o

Sorry, this patch is against net-next and you probably tested in Linus'
upstream.

You need to have d160c66cda0ac8614 ("net: Do not return value from
init_dummy_netdev()"), which is in net-next, and has this important
change that is necessary for this patch:

    -int init_dummy_netdev(struct net_device *dev);
    +void init_dummy_netdev(struct net_device *dev);

If you are OK with a v2, I will fix the topics reported in this thread.

Thank you
Breno

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Mon, Mar 11, 2024 at 08:05:45AM -0400, Dennis Dalessandro wrote:
> On 3/8/24 1:29 PM, 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 iwl_trans_pcie by converting it
> > into a pointer. Then use the leverage alloc_netdev() to allocate the
> > net_device object at iwl_trans_pcie_alloc.
> 
> What does an Omni-Path Architecture driver from Cornelis Networks have to do
> with an Intel wireless driver?
> 
> > The private data of net_device becomes a pointer for the struct
> > iwl_trans_pcie, so, it is easy to get back to the iwl_trans_pcie parent
> > given the net_device object.
> > 
> > [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  drivers/infiniband/hw/hfi1/netdev.h    | 2 +-
> >  drivers/infiniband/hw/hfi1/netdev_rx.c | 9 +++++++--
> >  2 files changed, 8 insertions(+), 3 deletions(-)

<...>

> 
> Leon, please don't accept this until the author resubmits a patch that I either
> Ack or Test.

Sure, I will wait for your response.

Thanks

> 
> Thanks
> 
> -Denny

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Dennis Dalessandro]

On 3/8/24 1:29 PM, 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 iwl_trans_pcie by converting it
> into a pointer. Then use the leverage alloc_netdev() to allocate the
> net_device object at iwl_trans_pcie_alloc.

What does an Omni-Path Architecture driver from Cornelis Networks have to do
with an Intel wireless driver?

> The private data of net_device becomes a pointer for the struct
> iwl_trans_pcie, so, it is easy to get back to the iwl_trans_pcie parent
> given the net_device object.
> 
> [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/infiniband/hw/hfi1/netdev.h    | 2 +-
>  drivers/infiniband/hw/hfi1/netdev_rx.c | 9 +++++++--
>  2 files changed, 8 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..5c26a69fa2bb 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,11 @@ 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(sizeof(struct iwl_trans_pcie *),
> +				   "dummy", NET_NAME_UNKNOWN,
> +				   init_dummy_netdev);

Again with the iwl stuff? Please do not stuff to the mailing list that doesn't
even compile....

 CC [M]  drivers/infiniband/hw/hfi1/verbs.o
  CC [M]  drivers/infiniband/hw/hfi1/verbs_txreq.o
  CC [M]  drivers/infiniband/hw/hfi1/vnic_main.o
In file included from ./include/net/sock.h:46,
                 from ./include/linux/tcp.h:19,
                 from ./include/linux/ipv6.h:95,
                 from ./include/net/ipv6.h:12,
                 from ./include/rdma/ib_verbs.h:25,
                 from ./include/rdma/ib_hdrs.h:11,
                 from drivers/infiniband/hw/hfi1/hfi.h:29,
                 from drivers/infiniband/hw/hfi1/sdma.h:15,
                 from drivers/infiniband/hw/hfi1/netdev_rx.c:11:
drivers/infiniband/hw/hfi1/netdev_rx.c: In function ‘hfi1_alloc_rx’:
drivers/infiniband/hw/hfi1/netdev_rx.c:365:36: error: passing argument 4 of
‘alloc_netdev_mqs’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
  365 |                                    init_dummy_netdev);
      |                                    ^~~~~~~~~~~~~~~~~
      |                                    |
      |                                    int (*)(struct net_device *)
./include/linux/netdevice.h:4632:63: note: in definition of macro ‘alloc_netdev’
 4632 |         alloc_netdev_mqs(sizeof_priv, name, name_assign_type, setup, 1, 1)
      |                                                               ^~~~~
./include/linux/netdevice.h:4629:44: note: expected ‘void (*)(struct net_device
*)’ but argument is of type ‘int (*)(struct net_device *)’
 4629 |                                     void (*setup)(struct net_device *),
      |                                     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
  CC [M]  drivers/infiniband/hw/hfi1/vnic_sdma.o


Leon, please don't accept this until the author resubmits a patch that I either
Ack or Test.

Thanks

-Denny

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Mon, Mar 11, 2024 at 03:08:54AM -0700, Breno Leitao wrote:
> Hello Leon,
> 
> On Sun, Mar 10, 2024 at 12:14:51PM +0200, Leon Romanovsky wrote:
> > On Fri, Mar 08, 2024 at 10:29:50AM -0800, 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.
> > 
> > Why?
> 
> From my experience, you can leverage all the helpers to deal with the
> relationship between struct net_device and you private structure. Here
> are some examples that comes to my mind:
> 
> * alloc_netdev() allocates the private structure for you
> * netdev_priv() gets the private structure for you
> * dev->priv_destructor sets the destructure to be called when the
>   interface goes away or failures.

Everything above is true, but it doesn't relevant to HFI1 devices which
are not netdev devices.

> 
> > > @@ -360,7 +360,11 @@ 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(sizeof(struct iwl_trans_pcie *),
> > > +				   "dummy", NET_NAME_UNKNOWN,
> > 
> > Will it create multiple "dummy" netdev in the system? Will all devices
> > have the same "dummy" name?
> 
> Are these devices visible to userspace?

HFI devices yes, dummy device no.

> 
> This allocation are using NET_NAME_UNKNOWN, which implies that the
> device is not expose to userspace.

Great

> 
> Would you prefer a different name?

I prefer to see some new wrapper over plain alloc_netdev, which will
create this dummy netdevice. For example, alloc_dummy_netdev(...).

> 
> > > +				   init_dummy_netdev); +	if
> > > (!rx->rx_napi) +		return -ENOMEM;
> > 
> > You forgot to release previously allocated "rx" here.
> 
> Good catch, I will update.

Thanks

> 
> Thanks

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Breno Leitao]

Hello Leon,

On Sun, Mar 10, 2024 at 12:14:51PM +0200, Leon Romanovsky wrote:
> On Fri, Mar 08, 2024 at 10:29:50AM -0800, 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.
> 
> Why?

From my experience, you can leverage all the helpers to deal with the
relationship between struct net_device and you private structure. Here
are some examples that comes to my mind:

* alloc_netdev() allocates the private structure for you
* netdev_priv() gets the private structure for you
* dev->priv_destructor sets the destructure to be called when the
  interface goes away or failures.

> > @@ -360,7 +360,11 @@ 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(sizeof(struct iwl_trans_pcie *),
> > +				   "dummy", NET_NAME_UNKNOWN,
> 
> Will it create multiple "dummy" netdev in the system? Will all devices
> have the same "dummy" name?

Are these devices visible to userspace?

This allocation are using NET_NAME_UNKNOWN, which implies that the
device is not expose to userspace.

Would you prefer a different name?

> > +				   init_dummy_netdev); +	if
> > (!rx->rx_napi) +		return -ENOMEM;
> 
> You forgot to release previously allocated "rx" here.

Good catch, I will update.

Thanks

Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically

[Leon Romanovsky]

On Fri, Mar 08, 2024 at 10:29:50AM -0800, 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.

Why?

> 
> 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 iwl_trans_pcie by converting it
> into a pointer. Then use the leverage alloc_netdev() to allocate the
> net_device object at iwl_trans_pcie_alloc.
> 
> The private data of net_device becomes a pointer for the struct
> iwl_trans_pcie, so, it is easy to get back to the iwl_trans_pcie parent
> given the net_device object.
> 
> [1] https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  drivers/infiniband/hw/hfi1/netdev.h    | 2 +-
>  drivers/infiniband/hw/hfi1/netdev_rx.c | 9 +++++++--
>  2 files changed, 8 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..5c26a69fa2bb 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,11 @@ 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(sizeof(struct iwl_trans_pcie *),
> +				   "dummy", NET_NAME_UNKNOWN,

Will it create multiple "dummy" netdev in the system? Will all devices
have the same "dummy" name?

> +				   init_dummy_netdev);
> +	if (!rx->rx_napi)
> +		return -ENOMEM;

You forgot to release previously allocated "rx" here.

Thanks

>  
>  	xa_init(&rx->dev_tbl);
>  	atomic_set(&rx->enabled, 0);
> @@ -374,6 +378,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
> 

[PATCH net-next] 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 iwl_trans_pcie by converting it
into a pointer. Then use the leverage alloc_netdev() to allocate the
net_device object at iwl_trans_pcie_alloc.

The private data of net_device becomes a pointer for the struct
iwl_trans_pcie, so, it is easy to get back to the iwl_trans_pcie parent
given the net_device object.

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

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/infiniband/hw/hfi1/netdev.h    | 2 +-
 drivers/infiniband/hw/hfi1/netdev_rx.c | 9 +++++++--
 2 files changed, 8 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..5c26a69fa2bb 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,11 @@ 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(sizeof(struct iwl_trans_pcie *),
+				   "dummy", NET_NAME_UNKNOWN,
+				   init_dummy_netdev);
+	if (!rx->rx_napi)
+		return -ENOMEM;
 
 	xa_init(&rx->dev_tbl);
 	atomic_set(&rx->enabled, 0);
@@ -374,6 +378,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