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

On 1/25/2021 9:29 PM, Leon Romanovsky wrote:

On Mon, Jan 25, 2021 at 05:01:40PM -0800, Jacob Keller wrote:

quoted

On 1/25/2021 4:39 PM, 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
devlink instances, or at least someone from devlink land should approve of the
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.

I agree that plumbing these parameters at the PCI side in the devlink of
the parent device is weird. They don't seem to be parameters that the
parent driver cares about.

Maybe there is another mechanism that makes more sense? To me it is a
bit like if we were plumbing netdev specific paramters into devlink
instead of trying to expose them through netdevice specific interfaces
like iproute2 or ethtool.

I'm far from being expert in devlink, but for me separation is following:
1. devlink - operates on physical device level, when PCI device already initialized.
2. ethtool - changes needed to be done on netdev layer.
3. ip - upper layer of the netdev
4. rdmatool - RDMA specific when IB device already exists.

And the ENABLE_ROCE/ENABLE_RDMA thing shouldn't be in the RDMA driver at
all, because it is physical device property which once toggled will
prohibit creation of respective aux device.

Ok. I guess I hadn't looked quite as close at the specifics here. I
agree that ENABLE_RDMA should go in the PF devlink.

If there's any other sort of RDMA-specific configuration that ties to
the IB device, that should go somehow into rdmatool, rather than
devlink. And thus: I think I agree, we don't want the IB device or the
aux device to create a devlink instance.

Thanks