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/[email protected]/
Signed-off-by: Breno Leitao <[email protected]>
---
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
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/[email protected]/
>
> Signed-off-by: Breno Leitao <[email protected]>
> ---
> 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
>
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
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
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/[email protected]/
>
> Signed-off-by: Breno Leitao <[email protected]>
> ---
> 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
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/[email protected]/
> >
> > Signed-off-by: Breno Leitao <[email protected]>
> > ---
> > 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
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/[email protected]/
> >
> > Signed-off-by: Breno Leitao <[email protected]>
> > ---
> > 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
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.
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/[email protected]/
>>>
>>> Signed-off-by: Breno Leitao <[email protected]>
>>> ---
>>> 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
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(ð->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
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/[email protected]/
> > >
> > > Signed-off-by: Breno Leitao <[email protected]>
> > > ---
> > > 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