Received-SPF: pass (google.com: domain of linux-kernel+bounces-98176-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sun, 10 Mar 2024 12:14:51 +0200
From: Leon Romanovsky <leon@kernel.org>
To: Breno Leitao <leitao@debian.org>
Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
	Jason Gunthorpe <jgg@ziepe.ca>, kuba@kernel.org,
	keescook@chromium.org,
	"open list:HFI1 DRIVER" <linux-rdma@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically
Message-ID: <20240310101451.GD12921@unreal>
References: <20240308182951.2137779-1-leitao@debian.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240308182951.2137779-1-leitao@debian.org>

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
>