Re: [RFC PATCH 06/11] IB/Verbs: Use management helper has_sa() and cap_sa(), for sa-check

[Doug Ledford <dledford@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5948 bytes --]

On Fri, 2015-03-27 at 12:47 -0400, ira.weiny wrote:
> 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?

Yes.

>   Do they show up as 2 devices?

No.  They are two ports of the same device with different link layers.
See here:

hca_id:	mlx4_1
	transport:			InfiniBand (0)
	fw_ver:				2.31.5050
	node_guid:			f452:1403:007b:cba0
	sys_image_guid:			f452:1403:007b:cba3
	vendor_id:			0x02c9
	vendor_part_id:			4099
	hw_ver:				0x0
	board_id:			MT_1090120019
	phys_port_cnt:			2
		port:	1
			state:			PORT_ACTIVE (4)
			max_mtu:		2048 (4)
			active_mtu:		2048 (4)
			sm_lid:			2
			port_lid:		2
			port_lmc:		0x01
			link_layer:		InfiniBand

		port:	2
			state:			PORT_ACTIVE (4)
			max_mtu:		4096 (5)
			active_mtu:		4096 (5)
			sm_lid:			0
			port_lid:		0
			port_lmc:		0x00
			link_layer:		Ethernet



> Regardless I think we should define the SA access on a per port basis.

Yes.

> > 
> > Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Cc: Doug Ledford <dledford@redhat.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Cc: Sean Hefty <sean.hefty@intel.com>
> > Signed-off-by: Michael Wang <yun.wang@profitbricks.com>
> > ---
> >  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
> > 


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]