-----Original Message-----
From: Leon Romanovsky <leon@kernel.org>
Sent: Tuesday, August 24, 2021 3:25 PM
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Ariel Elior <redacted>; Shai Malin <redacted>;
davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org;
malin1024@gmail.com; RDMA mailing list [off-list ref]
Subject: Re: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering

On Mon, Aug 23, 2021 at 12:17:42PM -0300, Jason Gunthorpe wrote:

quoted

On Mon, Aug 23, 2021 at 02:54:13PM +0000, Ariel Elior wrote:

quoted

quoted

From: Jason Gunthorpe <jgg@ziepe.ca>
Sent: Monday, August 23, 2021 4:34 PM
To: Leon Romanovsky <leon@kernel.org>
Cc: Shai Malin <redacted>; davem@davemloft.net;
kuba@kernel.org; netdev@vger.kernel.org; Ariel Elior
[off-list ref]; malin1024@gmail.com; RDMA mailing list
<linux- rdma@vger.kernel.org>
Subject: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering

External Email

On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:

quoted

+RDMA

Jakub, David

Can we please ask that everything directly or indirectly related
to RDMA will be sent to linux-rdma@ too?

On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:

quoted

Enable the RoCE and iWARP FW relaxed ordering.

Signed-off-by: Ariel Elior <redacted>
Signed-off-by: Shai Malin <redacted>
drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c

b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
index 4f4b79250a2b..496092655f26 100644

+++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c

@@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct

qed_hwfn

*p_hwfn,

quoted

quoted

 				    cnq_id);
 	}

+	p_params_header->relaxed_ordering = 1;

Maybe it is only description that needs to be updated, but I
would expect to see call to pcie_relaxed_ordering_enabled()
before setting relaxed_ordering to always true.

If we are talking about RDMA, the IB_ACCESS_RELAXED_ORDERING
flag should be taken into account too.

Why does this file even exist in netdev? This whole struct
qed_rdma_ops mess looks like another mis-design to support out of

tree modules??

quoted

quoted

quoted

Jason

Hi Jason,
qed_rdma_ops is not related to in tree / out of tree drivers. The
qed is the core module which is used by the protocol drivers which drive

this type of nic:

quoted

quoted

qede, qedr, qedi and qedf for ethernet, rdma, iscsi and fcoe respectively.
qed mostly serves as a HW abstraction layer, hiding away the details
of FW interaction and device register usage, and may also hold Linux
specific things which are protocol agnostic, such as dcbx, sriov,
debug data collection logic, etc. qed interacts with the protocol
drivers through ops structs for many purposes (dcbx, ptp, sriov,
etc). And also for rdma. It's just a way for us to separate the modules in a

clean way.

quoted

Delete the ops struct.

Move the RDMA functions to the RDMA module

Directly export the core functions needed to make that work

Two halfs of the same dirver do not and should not have an ops
structure ABI between them.

Yea, I read drivers/net/ethernet/qlogic/qed/qed_rdma.c and have hard time
to believe that hiding RDMA objects and code from the RDMA community
can be counted as "a clean way".

Hi Jason, Leon

I certainly see your point, and understand the motivation to have rdma content
in the rdma tree. We will start work on refactoring the (day 1) driver design to
have more rdma logic in the rdma driver and invoke the core module when needed.
Changing ops to exported functions will also be part of that.

But realistically I don't think we can move it all. Please understand that the
core module has many responsibilities which must take RDMA components into
account, but are not just rdma specific (the same logic is applied for the other
protocol drivers).

A few examples for this might be laying out host memory for connection contexts,
computing bar offsets, computing resource amounts and allocating resources for
VFs/PFs, etc.

I think we can definitely move some of the RDMA logic from core module to the
rdma driver (as in the case of this patchset) and I understand it makes more
sense that way. But I can think of multiple code areas where this would be very
difficult.

The current design is for the core module to own data structures and logic for
device configuration and manipulation. These flows/data-structures are
triggered/populated from multiple entry points: some at the low level part of
protocol flows (e.g. create QP) which can easily transition to be an exported
function as you directed, but other entry points may also be activated earlier
e.g. when device is initialized, even before rdma driver is loaded (based on
device configuration information, for example) in which case we would not be
able to invoke functionality residing in the rdma driver, but still have to
populate data structures, invoke FW flows, configure registers, etc. which have
to do with RDMA. 

Additionally, the qedr RDMA driver services both roce and iwarp, which means
there are TCP related flows/data structures which are shared between it and our
iSCSI driver qedi. This is nicely handled in the common module avoiding any code
duplication. Ripping it out and locating it in the protocol driver would be
difficult to perform and hard to maintain across the two trees.

Likewise functionality like light l2 queues, dcbx, sriov are shared amongst all
the protocol drivers. Exposing the functionality through export instead of ops
is no problem, but moving the logic outside of the core module to a specific
protocol is both a considerable design change and may lead to code duplication
or some very convoluted flows.  

In our view the qed/qede/qedr/qedi/qedf are separate drivers, hence we used
function pointer structures for the communication between them. We use
hierarchies of structures of function pointers to group toghether those which
have common purposes (dcbx, ll2, Ethernet, rdma). Changing that to flat exported
functions for the RDMA protocol is no problem if it is preferred by you.

In summary - we got the message and will work on it, but this is no small task
and may take some time, and will likely not result in total removal of any
mention whatsoever of rdma from the core module (but will reduce it
considerably).

Thanks

quoted

Jason