On Fri, Mar 27, 2015 at 10:28:20AM -0600, Jason Gunthorpe wrote:

On Fri, Mar 27, 2015 at 04:46:57PM +0100, Michael Wang wrote:

quoted

Introduce helper has_mcast() and cap_mcast() to help us check if an
IB device or it's port support Multicast.

Cc: Jason Gunthorpe <redacted>
Cc: Doug Ledford <redacted>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Sean Hefty <redacted>
Signed-off-by: Michael Wang <redacted>
 drivers/infiniband/core/cma.c       |  2 +-
 drivers/infiniband/core/multicast.c |  8 ++++----
 include/rdma/ib_verbs.h             | 28 ++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 276fb76..cbbc85b 100644
+++ b/drivers/infiniband/core/cma.c

@@ -3398,7 +3398,7 @@ void rdma_leave_multicast(struct rdma_cm_id *id, struct sockaddr *addr)
                 ib_detach_mcast(id->qp,
                         &mc->multicast.ib->rec.mgid,
                         be16_to_cpu(mc->multicast.ib->rec.mlid));
-            if (rdma_transport_is_ib(id_priv->cma_dev->device)) {
+            if (has_mcast(id_priv->cma_dev->device)) {

This might make more sense as cap_ib_multicast / cap_ip_multicast

Agreed.

quoted

                 switch (rdma_port_get_link_layer(id->device, id->port_num)) {
                 case IB_LINK_LAYER_INFINIBAND:
                     ib_sa_free_multicast(mc->multicast.ib);

quoted

diff --git a/drivers/infiniband/core/multicast.c b/drivers/infiniband/core/multicast.c
index 17573ff..ffeaf27 100644
+++ b/drivers/infiniband/core/multicast.c

@@ -780,7 +780,7 @@ static void mcast_event_handler(struct ib_event_handler *handler,
     int index;
 
     dev = container_of(handler, struct mcast_device, event_handler);
-    if (!rdma_port_ll_is_ib(dev->device, event->element.port_num))
+    if (!cap_mcast(dev->device, event->element.port_num))
         return;

These should probably be cap_ib_sa - that is what they are guarding
against.

But it seems redudent, since mcast_add_one will already not add a port that is
not IB, so mcast_event_handler is not callable. Something to do with
rocee/ib switching?

I'm not sure about this either.  This check seems to be necessary only on a
per-port level.  It does seem apparent that one can't go from Eth to IB.  What
happens if you go from IB to Eth on the port?

quoted

     index = event->element.port_num - dev->start_port;

@@ -807,7 +807,7 @@ static void mcast_add_one(struct ib_device *device)
     int i;
     int count = 0;
 
-    if (!rdma_transport_is_ib(device))
+    if (!has_mcast(device))
         return;

Again, this seems redundant, every port is tested directly below, why
is this check needed?

Agreed.  Same as my comments about the SA support.  This is really only
needed on ports which need to register with the SA (or perhaps some future
entity) for Mcast support.

Also this is part of the ib_sa module and exports the function
ib_sa_join_multicast.  So that this point it is covered under the
cap_sa(device, port) call.

So the implementation of cap_mcast at this point is:

cap_mcast(device, port)
{
	return cap_sa(device,port);
}

Looking at this, I do wonder how a port can dynamically change between
rocee and IB.. If the link value changes then mcast_remove_one will
not be a perfect reversal of mcast_add_one. Bug?

It feels necessary to understand what happens when a port dynamically
switches to ethernet on mlx hardware to validate these patches :(

Agreed.

:-(

-- Ira

Jason