RE: [PATCH 07/22] RDMA/irdma: Register an auxiliary driver and implement private channel OPs

-----Original Message-----
From: Jason Gunthorpe <jgg@nvidia.com>
Sent: Monday, January 25, 2021 4:48 PM
To: Saleem, Shiraz <redacted>
Cc: Leon Romanovsky <leon@kernel.org>; dledford@redhat.com;
kuba@kernel.org; davem@davemloft.net; linux-rdma@vger.kernel.org;
gregkh@linuxfoundation.org; netdev@vger.kernel.org; Ertman, David M
[off-list ref]; Nguyen, Anthony L
[off-list ref]; Ismail, Mustafa [off-list ref];
Keller, Jacob E [off-list ref]; jiri@nvidia.com; Samudrala, Sridhar
[off-list ref]; Williams, Dan J [off-list ref]
Subject: Re: [PATCH 07/22] RDMA/irdma: Register an auxiliary driver and
implement private channel OPs

On Tue, Jan 26, 2021 at 12:39:53AM +0000, Saleem, Shiraz wrote:

quoted

quoted

Subject: Re: [PATCH 07/22] RDMA/irdma: Register an auxiliary driver and
implement private channel OPs

On Sun, Jan 24, 2021 at 03:45:51PM +0200, Leon Romanovsky wrote:

quoted

On Fri, Jan 22, 2021 at 05:48:12PM -0600, Shiraz Saleem wrote:

quoted

From: Mustafa Ismail <redacted>

Register irdma as an auxiliary driver which can attach to auxiliary
RDMA devices from Intel PCI netdev drivers i40e and ice. Implement
the private channel ops, add basic devlink support in the driver and
register net notifiers.

Devlink part in "the RDMA client" is interesting thing.

The idea behind auxiliary bus was that PCI logic will stay at one
place and devlink considered as the tool to manage that.

Yes, this doesn't seem right, I don't think these auxiliary bus objects should

have

quoted

quoted

devlink instances, or at least someone from devlink land should approve of

the

quoted

quoted

idea.

In our model, we have one auxdev (for RDMA) per PCI device function
owned by netdev driver and one devlink instance per auxdev. Plus
there is an Intel netdev driver for each HW generation.  Moving the
devlink logic to the PCI netdev driver would mean duplicating the
same set of RDMA params in each Intel netdev driver. Additionally,
plumbing RDMA specific params in the netdev driver sort of seems
misplaced to me.

That's because it is not supposed to be "the netdev driver" but the
shared physical PCI function driver, and devlink is a shared part of
the PCI function.

Well, at least in Intel ice driver case, we have multiple PCI functions, and each function gets its own devlink (as opposed to a single devlink instance). There's no separation between the netdev driver and the PCI function driver today.

quoted

devlink is on a per 'struct device' object right? Should we be
limiting ourselves in its usage to only the PCI driver and PCI dev?
And not other devices like auxdev?

The devlink should not be created on the aux device, devlink should be
created against PCI functions.

It is important to follow establish convention here, otherwise it is a
user mess to know what to do

Jason