Received-SPF: pass (google.com: domain of linux-kernel+bounces-98799-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <6460dd4b-9b65-49df-beaf-05412e42f706@cornelisnetworks.com>
Date: Mon, 11 Mar 2024 08:05:45 -0400
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next] IB/hfi1: allocate dummy net_device dynamically
To: Breno Leitao <leitao@debian.org>, Jason Gunthorpe <jgg@ziepe.ca>,
 Leon Romanovsky <leon@kernel.org>
Cc: kuba@kernel.org, keescook@chromium.org,
 "open list:HFI1 DRIVER" <linux-rdma@vger.kernel.org>,
 open list <linux-kernel@vger.kernel.org>
References: <20240308182951.2137779-1-leitao@debian.org>
Content-Language: en-US
From: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
In-Reply-To: <20240308182951.2137779-1-leitao@debian.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?cWRhVjlhVWM2SXdKTVN2TXlSYXV6b1pVbDVTTXhJZ0FXZFNjRGJyOTBoSG4x?=
 =?utf-8?B?Z3FyV3p0MGdHN0dJMVh1TnBvVkt0aFpCM2FZZEhZdTAyTUdrcVY3ZU5nUUgr?=
 =?utf-8?B?d0NEMmd2ZjNONld5UHdHazJSR2RpRmZJc1h5T0lnbHlEV3RjcmQxek5DNGdn?=
 =?utf-8?B?RTZJVHVodjdNTUpmK2g5RHU2KzE1b1JPTmNFL3NQT0M3TlI2UzI0R3puU1I1?=
 =?utf-8?B?eHNHK3BPaXJHT0xRVkg5cEN5b0Vsc2VhREVhWTZxc2hBM05sbUpIRFBvR2ts?=
 =?utf-8?B?aFhCaEU5Q0syemZzeXRuQkJVVzVaYUttZ2dVTXhjTHVmMEIzcVBKbkxray92?=
 =?utf-8?B?RDBMOWJJMldlRWhtQXNrcWs4RFBXZHByajBSVUhGUjFScGx1ZWpmK0pDVENQ?=
 =?utf-8?B?aUJ3c1Z0ZmRXRUFZZkh1OEEwOU5xdUtvU1Vac2tiaG5MaFFDTUFTb1p0Z3hI?=
 =?utf-8?B?NHJ0Ty9QbUZxeWVvNlk2cjMwTTdLQUh4dlVwMjhGWThrVlU5c0NiT01TRUp3?=
 =?utf-8?B?d01ZWDdTWkE5V2JqYlJ3ai9lM3NUR0plaDlBTGNCWVBQWVhvREtUUHRlSVJN?=
 =?utf-8?B?VExGdEg4aUFYcTJWWU85bmRLQ0VueGpVTUp1VVQ1Wm5aY2pDZDJKZEMvSllP?=
 =?utf-8?B?MlNDVlhSZmVuK1VYczVkcTBNeW9SQk9rSlZjRWdRUkUrVXc4Y3doRjdkSHR1?=
 =?utf-8?B?MzIyZ2lhQ0xKOVAxZllJNFdZMnpvYW9VNHJHMnJrTWhwYXU4b1dFc055aGFF?=
 =?utf-8?B?SUpQZXpwcUhrdGdHRi9Jek9POXlvZU82OGtkVG1BbzRWb1VFMDFTVUpkR1U2?=
 =?utf-8?B?Rmlvai9pQVJlUlpEc1o1b2RNRHpjZ3E1WFZjM1pNajYzV0pqUG9Qa1Qycm55?=
 =?utf-8?B?UmUzSmQya0plaURuVThUdlhaSU9hNGluOGJKSnRsSEYzOVpocjZTaW1XRHFh?=
 =?utf-8?B?YllDcnJVazFPMGpNTnE3SXVGVFdlOFpSOW4rWFNxM1BSY2ZqK3dOOFZHTGhq?=
 =?utf-8?B?c2V5NGxDN0J4cUNJRWdCbzRwbmMrRnR5VzUrQTJsbVJrd3NzMEU1TlQ1SkFI?=
 =?utf-8?B?UFhWZzV6Y2puOTVvamE3SnZzOG9Zb2daZnZGOFZOYmo1QXN6dmVTTlA3cFl0?=
 =?utf-8?B?Z1l2L3dzZzNJUG4zaUd6Y0U1NER1QktuUGMzMWhFRWRsZkduTnUxSWl5SkUr?=
 =?utf-8?B?dnBHRlZGdlF2NnM3R2wrRDB5QXJOUFdWaHIwK3RtZDZPc29xcmk3OUo4MzJp?=
 =?utf-8?B?Um81QiswQjhUZTR6RTFCK3A4dFBjYjRaN3NNSkovY09EeC9XcWJkL2lzcGxz?=
 =?utf-8?B?TWp5QW5PcFRWSXFqVWxWUkNmRlM3YnY1SkpHTWFSWE8yRDBITUZERFkwUDhx?=
 =?utf-8?B?NEN6QlhLU0d5T2NQbUNJZlZqY0Via3JOOGhsMGd6VzFCaFFGd2czOFBYSzNK?=
 =?utf-8?B?MXk4QlQxYjNZcno3eGZVQ01wc1BJRGsrVXA1QWU1bk40N01CdGlZcGI1UEE0?=
 =?utf-8?B?MGoweHRmMzhzTDJCTXFUeWJuNU4wNWhjZnRNTGpjYUtpN21BVWJhc3dFTkV1?=
 =?utf-8?B?c01DNGNLU0ZCa21Pd0s2QVVpZzlEWlVHZHdydlA2ekdUK1g1NnVWZDIxMFA2?=
 =?utf-8?B?V21YUDZiNHE4cFZUSlI1UlYzZVBOQXZNWUx4ZXZpdis3U1JPZTdCUHZGMzVv?=
 =?utf-8?B?RjNuaXQvcTh5SXhpNDY1OEFacEFTT0c5bUhOVWRoSm9OZ2cyUUNKYXg3dXNI?=
 =?utf-8?B?TWZLRUpVZzF1aTNTY3hmUHgyckdnRVVxMSs4a0hUdCtkaVJkOVFBQTdueExs?=
 =?utf-8?B?L0NlTEg3elVxZERQOFZuQU14cGhHdHVsQ0hqN1ZxdzhmallLdWk1K2pPOVRn?=
 =?utf-8?B?LzdpYm5vVm9sQnlHNCtNRlAvYU9aTUlVSkRWN1dNL3BVZmlJVjdnMDVDaHoz?=
 =?utf-8?B?SjkzQjJGTzFxMGJnc1M3ZWZLVEtNVXJodTl2U3lHNUEzVGxvbERQZ0k1Ky9X?=
 =?utf-8?B?eFdpVU9ON29Pc2xzT2Nmc0Nxb3FKSFcyZzZXRVNjYzhaaEdzUnRZYVhWUTlE?=
 =?utf-8?B?WDJSWlVoUHY5V0hqTDdoaFBVZXMwcXlqRWJhSm9QY0gxYTFRTUNQdUE2T3A3?=
 =?utf-8?B?WFFUQXNsdW1oalRDN1QwU0ZZN1dKQ3l4dnRqUU5PS3VPRzlLQUExZjFJd1ZJ?=
 =?utf-8?Q?gGSgSaFcW5R50tHOUEZSIpo=3D?=
X-OriginatorOrg: cornelisnetworks.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ebbbc1af-8a2e-41b2-3d31-08dc41c396b4
X-MS-Exchange-CrossTenant-AuthSource: BL0PR01MB4817.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2024 12:05:48.3504
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4dbdb7da-74ee-4b45-8747-ef5ce5ebe68a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: JcBv8rg/vrCOaE8ZexDWbrVpa2mzdNCYRtSD4DKVhb9pnnJM7qRCzgDjDO5B8D/eSt+bWPScwAgqLWVbzSzd8rJXn82rtCHo2IeNtG08KEs/YTUDsoKfgf6As3Byug0N
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR01MB8183

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