> -----Original Message-----
> From: Jason Gunthorpe
> Sent: Monday, March 12, 2018 10:27 AM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Hal Rosenstock
> <hal@xxxxxxxxxxxxxxxxxx>; Doug Ledford <dledford@xxxxxxxxxx>; RDMA
> mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens
> <danielj@xxxxxxxxxxxx>; Mark Bloch <markb@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next 04/13] IB/core: Honor port_num while resolving
> GID for IB link layer
>
> On Mon, Mar 12, 2018 at 08:40:50AM -0600, Parav Pandit wrote:
> > > On Mon, Mar 12, 2018 at 08:47:24AM -0400, Hal Rosenstock wrote:
> > > > On 3/11/2018 4:03 AM, Leon Romanovsky wrote:
> > > > > From: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > >
> > > > > Currently initialized ah_attr contains the port number to which
> > > > > cm_id is bound. However, while searching for GID table for
> > > > > matching GID entry, port number is ignored.
> > > > > It is better to honor the port number of the cm_id while
> > > > > searching GID table for IB link layer as well.
> > > > >
> > > > > Reviewed-by: Daniel Jurgens <danielj@xxxxxxxxxxxx>
> > > > > Signed-off-by: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > > Signed-off-by: Leon Romanovsky <leon@xxxxxxxxxx>
> > > > > drivers/infiniband/core/multicast.c | 24
> > > > > ++++++++++--------------
> > > > > 1 file changed, 10 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/core/multicast.c
> > > > > b/drivers/infiniband/core/multicast.c
> > > > > index 45f2f095f793..29bd3f1aff12 100644
> > > > > +++ b/drivers/infiniband/core/multicast.c
> > > > > @@ -724,21 +724,17 @@ int ib_init_ah_from_mcmember(struct
> > > > > ib_device *device, u8 port_num, {
> > > > > int ret;
> > > > > u16 gid_index;
> > > > > - u8 p;
> > > > > -
> > > > > - if (rdma_protocol_roce(device, port_num)) {
> > > > > - ret = ib_find_cached_gid_by_port(device, &rec->port_gid,
> > > > > - gid_type, port_num,
> > > > > - ndev,
> > > > > - &gid_index);
> > > > > - } else if (rdma_protocol_ib(device, port_num)) {
> > > > > - ret = ib_find_cached_gid(device, &rec->port_gid,
> > > > > - IB_GID_TYPE_IB, NULL, &p,
> > > > > - &gid_index);
> > > > > - } else {
> > > > > - ret = -EINVAL;
> > > > > - }
> > > > >
> > > > > + /* GID table is not based on the netdevice for IB link layer,
> > > > > + * so ignore ndev during search.
> > > > > + */
> > > > > + if (rdma_protocol_ib(device, port_num))
> > > > > + ndev = NULL;
> > > >
> > > > To be equivalent to what is being replaced, shouldn't there be:
> > > > else if (!rdma_protocol_roce(device, port_num))
> > > > return -EINVAL;
> > > > here (for iWarp) ?
> > >
> > > I'm sure that you are right.
> > >
> > ib_init_ah_from_mcmember() is called from multicast handler based on
> > SA path record entry. Multicast registration can only happen if the
> > HCA advertises capability via rdma_cap_ib_mcast().
> > RDMA_CORE_CAP_IB_SA capability is defined as RDMA_CORE_PORT_IBA_IB.
> > RDMA_CORE_PORT_IWARP doesn't have RDMA_CORE_CAP_IB_SA. So I don't
> see
> > a way that this code path can be executed for iWarp port until
> > RDMA_CORE_CAP_IB_SA gets added to RDMA_CORE_PORT_IWARP.
>
> Please try not to rely on these long chains of logic to explain this stuff..
>
> CM is already so complicated, better to explicitly provide the expected control
> flow in each function so someone can hope to understand it.
>
This function is supposed to be called only for IB and/or RoCE flows and not for iWarp link layers.
Are you suggesting that we add check in all such functions (if any) to return failure for iWarp ports? (as different path)
--
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
