On Mon, May 14, 2018 at 11:11:13AM +0300, Leon Romanovsky wrote: > > [1] https://www.spinics.net/lists/linux-rdma/msg58148.html > > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx> > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> Extra space between signed offs > /** > - * ib_find_cached_gid - Returns the port number and GID table index where > - * a specified GID value occurs. > + * rdma_find_gid - Returns SGID attributes if the matching GID is found. > * @device: The device to query. > * @gid: The GID value to search for. > * @gid_type: The GID type to search for. > * @ndev: In RoCE, the net device of the device. NULL means ignore. > - * @port_num: The port number of the device where the GID value was found. > - * @index: The index into the cached GID table where the GID was found. This > - * parameter may be NULL. > * > - * ib_find_cached_gid() searches for the specified GID value in > + * rdma_find_gid() searches for the specified GID value in > * the local software cache. > + * > + * Returns sgid attributes if the GID is found with valid reference. > + * Or returns ERR_PTR for the error. > + * Caller must invoke ib_put_cached_gid_attr() to release the reference. > + * > */ > -int ib_find_cached_gid(struct ib_device *device, > - const union ib_gid *gid, > - enum ib_gid_type gid_type, > - struct net_device *ndev, > - u8 *port_num, > - u16 *index) > + Extra space between kdoc and function > +const struct ib_gid_attr * > +rdma_find_gid(struct ib_device *device, > + const union ib_gid *gid, > + enum ib_gid_type gid_type, > + struct net_device *ndev) > { > - return ib_cache_gid_find(device, gid, gid_type, ndev, port_num, index); > + return ib_cache_gid_find(device, gid, gid_type, ndev); > } > -EXPORT_SYMBOL(ib_find_cached_gid); > +EXPORT_SYMBOL(rdma_find_gid); Afer all the patches are applied this is the only call site for ib_cache_gid_find, no sense in having a wrapper like this. > -int ib_find_gid_by_filter(struct ib_device *device, > - const union ib_gid *gid, > - u8 port_num, > - bool (*filter)(const union ib_gid *gid, > - const struct ib_gid_attr *, > - void *), > - void *context, u16 *index) > +const struct ib_gid_attr * > +rdma_find_gid_by_filter(struct ib_device *device, > + const union ib_gid *gid, > + u8 port_num, > + bool (*filter)(const union ib_gid *gid, > + const struct ib_gid_attr *, > + void *), > + void *context) > { > /* Only RoCE GID table supports filter function */ > if (!rdma_protocol_roce(device, port_num) && filter) > - return -EPROTONOSUPPORT; > + return ERR_PTR(-EPROTONOSUPPORT); > > return ib_cache_gid_find_by_filter(device, gid, > port_num, filter, > - context, index); > + context); > } > > int ib_get_cached_pkey(struct ib_device *device, > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index 7df4c7173607..0efad89cfe22 100644 > +++ b/drivers/infiniband/core/cm.c > @@ -474,6 +474,11 @@ static int cm_init_av_for_lap(struct cm_port *port, struct ib_wc *wc, > if (ret) > return ret; > > + /* > + * Now that ah attribute is initialized based new wc, > + * old ah attribute can be discarded. > + */ > + rdma_cleanup_ah_attr_gid_attr(&av->ah_attr); > memcpy(&av->ah_attr, &new_ah_attr, sizeof(new_ah_attr)); > return 0; > } > @@ -508,31 +513,50 @@ static int add_cm_id_to_port_list(struct cm_id_private *cm_id_priv, > return ret; > } > > -static struct cm_port *get_cm_port_from_path(struct sa_path_rec *path) > +static struct cm_port * > +get_cm_port_from_path(struct sa_path_rec *path, const struct ib_gid_attr *attr) > { > struct cm_device *cm_dev; > struct cm_port *port = NULL; > unsigned long flags; > - u8 p; > - struct net_device *ndev = ib_get_ndev_from_path(path); > - > - read_lock_irqsave(&cm.device_lock, flags); > - list_for_each_entry(cm_dev, &cm.device_list, list) { > - if (!ib_find_cached_gid(cm_dev->ib_device, &path->sgid, > - sa_conv_pathrec_to_gid_type(path), > - ndev, &p, NULL)) { > - port = cm_dev->port[p - 1]; > - break; > + > + if (attr) { > + read_lock_irqsave(&cm.device_lock, flags); > + list_for_each_entry(cm_dev, &cm.device_list, list) { > + if (cm_dev->ib_device == attr->device) { > + port = cm_dev->port[attr->port_num - 1]; > + break; > + } > } > + read_unlock_irqrestore(&cm.device_lock, flags); > + } else { > + /* SGID attribute can be NULL in following > + * conditions. > + * (a) Alternative path > + * (b) IB link layer without GRH > + * (c) LAP send messages > + */ This doesn't seem right.. Why wouldn't alternate path have a gid_attr too? If we are proposing or loading an alt path then we need to hold the reference as well, so it certainly should exist, and if it exists it should be used here. The IB case should also have a SGID attr, IMHO. On the REQ side the attr should arise when the cm_id is bound to an IP, otherwise it comes from the path (eg AF_IB style) the GID is already mandatory in the paths even if the hop limit is 0. On the RESP side it comes from the IP or some random appropriate GID for the Rxing namespace. APM paths are similar. It just makes everything cleaner and far more symmetric.. So this function shouldn't even accept a path.. As is this function is confusing because it does require the caller to pass a path that perfectly matches the attr if attr is non NULL. At the very least this should be a WARN_ON that the path->sgid and git_attr->gid are the same until it can be cleaned up as in the above. Overall things are very confusing if there is both a gid attr and a path involved, more so because the attr is not stored as part of the path structure... Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
