Return-Path: Received: from mga09.intel.com ([134.134.136.24]:22572 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752396AbbC0QsH (ORCPT ); Fri, 27 Mar 2015 12:48:07 -0400 Date: Fri, 27 Mar 2015 12:47:56 -0400 From: "ira.weiny" To: Michael Wang Cc: Roland Dreier , Sean Hefty , Hal Rosenstock , linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org, linux-nfs@vger.kernel.org, netdev@vger.kernel.org, "J. Bruce Fields" , Trond Myklebust , "David S. Miller" , Or Gerlitz , Moni Shoua , PJ Waskiewicz , Tatyana Nikolova , Yan Burman , Jack Morgenstein , Bart Van Assche , Yann Droneaud , Colin Ian King , Majd Dibbiny , Jiri Kosina , Matan Barak , Alex Estrin , Doug Ledford , Eric Dumazet , Erez Shitrit , Sagi Grimberg , Haggai Eran , Shachar Raindel , Mike Marciniszyn , Steve Wise , Tom Tucker , Chuck Lever Subject: Re: [RFC PATCH 06/11] IB/Verbs: Use management helper has_sa() and cap_sa(), for sa-check Message-ID: <20150327164756.GA27862@phlsvsds.ph.intel.com> References: <551579CA.4030901@profitbricks.com> <55157B43.6060507@profitbricks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <55157B43.6060507@profitbricks.com> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, Mar 27, 2015 at 04:46:11PM +0100, Michael Wang wrote: > > Introduce helper has_sa() and cap_sa() to help us check if an IB device > or it's port support Subnet Administrator. I think these 2 should be combined. The question is if a port requires the use of the SA depending on the network it is connected to. Aren't some dual port Mellanox cards capable of doing IB on 1 port and Eth on the other? Do they show up as 2 devices? Regardless I think we should define the SA access on a per port basis. > > Cc: Jason Gunthorpe > Cc: Doug Ledford > Cc: Ira Weiny > Cc: Sean Hefty > Signed-off-by: Michael Wang > --- > drivers/infiniband/core/sa_query.c | 12 ++++++------ > include/rdma/ib_verbs.h | 28 ++++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c > index d95d25f..89c27da 100644 > --- a/drivers/infiniband/core/sa_query.c > +++ b/drivers/infiniband/core/sa_query.c > @@ -450,7 +450,7 @@ static void ib_sa_event(struct ib_event_handler *handler, struct ib_event *event > struct ib_sa_port *port = > &sa_dev->port[event->element.port_num - sa_dev->start_port]; > > - if (!rdma_port_ll_is_ib(handler->device, port->port_num)) > + if (!cap_sa(handler->device, port->port_num)) > return; > > spin_lock_irqsave(&port->ah_lock, flags); > @@ -1154,7 +1154,7 @@ static void ib_sa_add_one(struct ib_device *device) > struct ib_sa_device *sa_dev; > int s, e, i; > > - if (!rdma_transport_is_ib(device)) > + if (!has_sa(device)) The logic here should be: if (no ports of this device need sa access) return; So why not eliminate this check and allow the cap_sa(s) to handle the support? -- Ira > return; > > if (device->node_type == RDMA_NODE_IB_SWITCH) > @@ -1175,7 +1175,7 @@ static void ib_sa_add_one(struct ib_device *device) > > for (i = 0; i <= e - s; ++i) { > spin_lock_init(&sa_dev->port[i].ah_lock); > - if (!rdma_port_ll_is_ib(device, i + 1)) > + if (!cap_sa(device, i + 1)) > continue; > > sa_dev->port[i].sm_ah = NULL; > @@ -1205,14 +1205,14 @@ static void ib_sa_add_one(struct ib_device *device) > goto err; > > for (i = 0; i <= e - s; ++i) > - if (rdma_port_ll_is_ib(device, i + 1)) > + if (cap_sa(device, i + 1)) > update_sm_ah(&sa_dev->port[i].update_task); > > return; > > err: > while (--i >= 0) > - if (rdma_port_ll_is_ib(device, i + 1)) > + if (cap_sa(device, i + 1)) > ib_unregister_mad_agent(sa_dev->port[i].agent); > > kfree(sa_dev); > @@ -1233,7 +1233,7 @@ static void ib_sa_remove_one(struct ib_device *device) > flush_workqueue(ib_wq); > > for (i = 0; i <= sa_dev->end_port - sa_dev->start_port; ++i) { > - if (rdma_port_ll_is_ib(device, i + 1)) { > + if (cap_sa(device, i + 1)) { > ib_unregister_mad_agent(sa_dev->port[i].agent); > if (sa_dev->port[i].sm_ah) > kref_put(&sa_dev->port[i].sm_ah->ref, free_sm_ah); > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > index c0a63f8..fa8ffa3 100644 > --- a/include/rdma/ib_verbs.h > +++ b/include/rdma/ib_verbs.h > @@ -1810,6 +1810,19 @@ static inline int has_cm(struct ib_device *device) > } > > /** > + * has_sa - Check if a device support Subnet Administrator. > + * > + * @device: Device to be checked > + * > + * Return 0 when a device has none port to support > + * Subnet Administrator. > + */ > +static inline int has_sa(struct ib_device *device) > +{ > + return rdma_transport_is_ib(device); > +} > + > +/** > * cap_smi - Check if the port of device has the capability > * Subnet Management Interface. > * > @@ -1824,6 +1837,21 @@ static inline int cap_smi(struct ib_device *device, u8 port_num) > return rdma_port_ll_is_ib(device, port_num); > } > > +/** > + * cap_sa - Check if the port of device has the capability > + * Subnet Administrator. > + * > + * @device: Device to be checked > + * @port_num: Port number of the device > + * > + * Return 0 when port of the device don't support > + * Subnet Administrator. > + */ > +static inline int cap_sa(struct ib_device *device, u8 port_num) > +{ > + return rdma_port_ll_is_ib(device, port_num); > +} > + > int ib_query_gid(struct ib_device *device, > u8 port_num, int index, union ib_gid *gid); > > -- > 2.1.0 >