| Message ID | 20190731202459.19570-4-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA: Cleanups and improvements | expand |
> 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. However, why not make the code for port-state and mtu common for Both iWARP + RoCE? > 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 ? 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
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 >
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) + 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)
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. Signed-off-by: Kamal Heib <kamalheib1@gmail.com> --- drivers/infiniband/core/device.c | 87 ++++++++++++++++++++++++++------ 1 file changed, 71 insertions(+), 16 deletions(-)