Re: [PATCH for-next V2 3/4] RDMA/core: Add common iWARP query port

[Kamal Heib <kamalheib1@gmail.com>]

On Thu, Aug 01, 2019 at 11:10:11AM +0000, Michal Kalderon wrote:
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Kamal Heib
> > 
> > Add support for a common iWARP query port function, the new function
> > includes a common code that is used by the iWARP devices to update the
> > port attributes like max_mtu, active_mtu, state, and phys_state, the
> > function also includes a call for the driver-specific query_port callback to
> > query the device-specific port attributes.
> > 
> Thanks, the qedr is also a iWARP device but it has most of the code common with
> The RoCE part, so we'll need to split the code earlier between the protocols. 

Yes, That's why I decided not to touch it.

> However, why not make the code for port-state and mtu common for Both iWARP + RoCE? 
> 

I don't think that this is a good idea because they don't share the same
code for determining the port-state and mtu (please see mlx4/mlx5 v.s.
other iWARP drivers), while it is the same code for all iWARP drivers.

> > Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
> > ---
> >  drivers/infiniband/core/device.c | 87 ++++++++++++++++++++++++++------
> >  1 file changed, 71 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/infiniband/core/device.c
> > b/drivers/infiniband/core/device.c
> > index 9773145dee09..860c08ca49e7 100644
> > --- a/drivers/infiniband/core/device.c
> > +++ b/drivers/infiniband/core/device.c
> > @@ -1940,31 +1940,64 @@ void ib_dispatch_event(struct ib_event *event)
> > }  EXPORT_SYMBOL(ib_dispatch_event);
> > 
> > -/**
> > - * ib_query_port - Query IB port attributes
> > - * @device:Device to query
> > - * @port_num:Port number to query
> > - * @port_attr:Port attributes
> > - *
> > - * ib_query_port() returns the attributes of a port through the
> > - * @port_attr pointer.
> > - */
> > -int ib_query_port(struct ib_device *device,
> > -		  u8 port_num,
> > -		  struct ib_port_attr *port_attr)
> > +static int iw_query_port(struct ib_device *device,
> > +			   u8 port_num,
> > +			   struct ib_port_attr *port_attr)
> >  {
> > -	union ib_gid gid;
> > +	struct in_device *inetdev;
> > +	struct net_device *netdev;
> >  	int err;
> > 
> > -	if (!rdma_is_port_valid(device, port_num))
> > -		return -EINVAL;
> > +	memset(port_attr, 0, sizeof(*port_attr));
> > +
> > +	netdev = ib_device_get_netdev(device, port_num);
> > +	if (!netdev)
> > +		return -ENODEV;
> > +
> > +	dev_put(netdev);
> > +
> > +	port_attr->max_mtu = IB_MTU_4096;
> > +	port_attr->active_mtu = ib_mtu_int_to_enum(netdev->mtu);
> > +
> > +	if (!netif_carrier_ok(netdev)) {
> > +		port_attr->state = IB_PORT_DOWN;
> > +		port_attr->phys_state = IB_PORT_PHYS_STATE_DISABLED;
> > +	} else {
> > +		inetdev = in_dev_get(netdev);
> > +
> > +		if (inetdev && inetdev->ifa_list) {
> > +			port_attr->state = IB_PORT_ACTIVE;
> > +			port_attr->phys_state =
> > IB_PORT_PHYS_STATE_LINK_UP;
> > +			in_dev_put(inetdev);
> > +		} else {
> > +			port_attr->state = IB_PORT_INIT;
> > +			port_attr->phys_state =
> > +
> > 	IB_PORT_PHYS_STATE_PORT_CONFIGURATION_TRAINING;
> > +		}
> > +	}
> > +
> > +	err = device->ops.query_port(device, port_num, port_attr);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __ib_query_port(struct ib_device *device,
> > +			   u8 port_num,
> > +			   struct ib_port_attr *port_attr)
> > +{
> > +	union ib_gid gid = {};
> > +	int err;
> > 
> >  	memset(port_attr, 0, sizeof(*port_attr));
> > +
> >  	err = device->ops.query_port(device, port_num, port_attr);
> >  	if (err || port_attr->subnet_prefix)
> >  		return err;
> > 
> > -	if (rdma_port_get_link_layer(device, port_num) !=
> > IB_LINK_LAYER_INFINIBAND)
> > +	if (rdma_port_get_link_layer(device, port_num) !=
> > +	    IB_LINK_LAYER_INFINIBAND)
> >  		return 0;
> > 
> >  	err = device->ops.query_gid(device, port_num, 0, &gid); @@ -1974,6
> > +2007,28 @@ int ib_query_port(struct ib_device *device,
> >  	port_attr->subnet_prefix = be64_to_cpu(gid.global.subnet_prefix);
> >  	return 0;
> >  }
> > +
> > +/**
> > + * ib_query_port - Query IB port attributes
> > + * @device:Device to query
> > + * @port_num:Port number to query
> > + * @port_attr:Port attributes
> > + *
> > + * ib_query_port() returns the attributes of a port through the
> > + * @port_attr pointer.
> > + */
> > +int ib_query_port(struct ib_device *device,
> > +		  u8 port_num,
> > +		  struct ib_port_attr *port_attr)
> > +{
> > +	if (!rdma_is_port_valid(device, port_num))
> > +		return -EINVAL;
> > +
> > +	if (rdma_node_get_transport(device->node_type) ==
> > RDMA_TRANSPORT_IWARP)
> Raising a question, in some places we use the macro above and in others
> rdma_protocol_iwarp(device, port_num), any reason to prefer one over the other ?

I thought it's more readable to use rdma_node_get_transport() in this
case than using rdma_protocol_iwarp().

>  thanks,
> 
> > +		return iw_query_port(device, port_num, port_attr);
> > +	else
> > +		return __ib_query_port(device, port_num, port_attr); }
> >  EXPORT_SYMBOL(ib_query_port);
> > 
> >  static void add_ndev_hash(struct ib_port_data *pdata)
> > --
> > 2.20.1
>