[rdma-next,v1,13/22] RDMA/hns: Remove empty functions

Commit Message

Selvin Xavier Aug. 18, 2017, 9:46 a.m. UTC

Removing the "modify_port" hook is causing some issues with ib_cm module.
While adding a mad agent for the new device, ib_cm invokes
ib_modify_port  which fails
with return value -ENOSYS. So the mad agent gets unregistered. This
break the connection
establishment. It is broken for bnxt_re also.

The following patch helps. But it is like a workaround to solve the problem.


Or should we have the modify_port hook with some basic checks?

Thanks,
Selvin

On Sun, Aug 13, 2017 at 3:48 PM, Leon Romanovsky <[e-mail]> wrote:
> From: Leon Romanovsky <[e-mail]>
>
> The functions which are not implemented can be simply ignored
> instead of defining empty function. This patch removes such functions
> from hns driver.
>
> Signed-off-by: Leon Romanovsky <[e-mail]>
> Reviewed-by: Dennis Dalessandro <[e-mail]>
> ---
>  drivers/infiniband/hw/hns/hns_roce_main.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c
> index d9777b662eba..250e2059ef07 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_main.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_main.c
> @@ -285,12 +285,6 @@ static enum rdma_link_layer hns_roce_get_link_layer(struct ib_device *device,
>         return IB_LINK_LAYER_ETHERNET;
>  }
>
> -static int hns_roce_query_gid(struct ib_device *ib_dev, u8 port_num, int index,
> -                             union ib_gid *gid)
> -{
> -       return 0;
> -}
> -
>  static int hns_roce_query_pkey(struct ib_device *ib_dev, u8 port, u16 index,
>                                u16 *pkey)
>  {
> @@ -316,12 +310,6 @@ static int hns_roce_modify_device(struct ib_device *ib_dev, int mask,
>         return 0;
>  }
>
> -static int hns_roce_modify_port(struct ib_device *ib_dev, u8 port_num, int mask,
> -                               struct ib_port_modify *props)
> -{
> -       return 0;
> -}
> -
>  static struct ib_ucontext *hns_roce_alloc_ucontext(struct ib_device *ib_dev,
>                                                    struct ib_udata *udata)
>  {
> @@ -462,10 +450,8 @@ static int hns_roce_register_device(struct hns_roce_dev *hr_dev)
>         ib_dev->modify_device           = hns_roce_modify_device;
>         ib_dev->query_device            = hns_roce_query_device;
>         ib_dev->query_port              = hns_roce_query_port;
> -       ib_dev->modify_port             = hns_roce_modify_port;
>         ib_dev->get_link_layer          = hns_roce_get_link_layer;
>         ib_dev->get_netdev              = hns_roce_get_netdev;
> -       ib_dev->query_gid               = hns_roce_query_gid;
>         ib_dev->add_gid                 = hns_roce_add_gid;
>         ib_dev->del_gid                 = hns_roce_del_gid;
>         ib_dev->query_pkey              = hns_roce_query_pkey;
> --
> 2.14.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Leon Romanovsky Aug. 18, 2017, 3:40 p.m. UTC | #1

On Fri, Aug 18, 2017 at 03:16:12PM +0530, Selvin Xavier wrote:
>  Removing the "modify_port" hook is causing some issues with ib_cm module.
> While adding a mad agent for the new device, ib_cm invokes
> ib_modify_port  which fails
> with return value -ENOSYS. So the mad agent gets unregistered. This
> break the connection
> establishment. It is broken for bnxt_re also.
>
> The following patch helps. But it is like a workaround to solve the problem.
>
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index d5ca101..59911dd 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -4189,7 +4189,7 @@ static void cm_add_one(struct ib_device *ib_device)
>                         goto error2;
>
>                 ret = ib_modify_port(ib_device, i, 0, &port_modify);
> -               if (ret)
> +               if (ret && ret != -ENOSYS)
>                         goto error3;
>
>                 count++;
>
> Or should we have the modify_port hook with some basic checks?

I think that your proposal is right thing to do. The driver should
properly return the status of its callbacks (-ENOSYS) and it is
responsibility of the caller to decide what to do in such case.

The dummy function (return 0) hides the fact that modify_port do
nothing and it can be wrong for some callers.

Thanks

Selvin Xavier Aug. 21, 2017, 4:39 a.m. UTC | #2

On Fri, Aug 18, 2017 at 9:10 PM, Leon Romanovsky <[e-mail]> wrote:

>>
>> The following patch helps. But it is like a workaround to solve the problem.
>>
>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>> index d5ca101..59911dd 100644
>> --- a/drivers/infiniband/core/cm.c
>> +++ b/drivers/infiniband/core/cm.c
>> @@ -4189,7 +4189,7 @@ static void cm_add_one(struct ib_device *ib_device)
>>                         goto error2;
>>
>>                 ret = ib_modify_port(ib_device, i, 0, &port_modify);
>> -               if (ret)
>> +               if (ret && ret != -ENOSYS)
>>                         goto error3;
>>
>>                 count++;
>>
>> Or should we have the modify_port hook with some basic checks?
>
> I think that your proposal is right thing to do. The driver should
> properly return the status of its callbacks (-ENOSYS) and it is
> responsibility of the caller to decide what to do in such case.
>

> The dummy function (return 0) hides the fact that modify_port do
> nothing and it can be wrong for some callers.
>
I am bit confused. You support the proposal to have a modify_port with
some basic check.. right?
> Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky Aug. 21, 2017, 5:16 a.m. UTC | #3

On Mon, Aug 21, 2017 at 10:09:10AM +0530, Selvin Xavier wrote:
> On Fri, Aug 18, 2017 at 9:10 PM, Leon Romanovsky <[e-mail]> wrote:
>
> >>
> >> The following patch helps. But it is like a workaround to solve the problem.
> >>
> >> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> >> index d5ca101..59911dd 100644
> >> --- a/drivers/infiniband/core/cm.c
> >> +++ b/drivers/infiniband/core/cm.c
> >> @@ -4189,7 +4189,7 @@ static void cm_add_one(struct ib_device *ib_device)
> >>                         goto error2;
> >>
> >>                 ret = ib_modify_port(ib_device, i, 0, &port_modify);
> >> -               if (ret)
> >> +               if (ret && ret != -ENOSYS)
> >>                         goto error3;
> >>
> >>                 count++;
> >>
> >> Or should we have the modify_port hook with some basic checks?
> >
> > I think that your proposal is right thing to do. The driver should
> > properly return the status of its callbacks (-ENOSYS) and it is
> > responsibility of the caller to decide what to do in such case.
> >
>
> > The dummy function (return 0) hides the fact that modify_port do
> > nothing and it can be wrong for some callers.
> >
> I am bit confused. You support the proposal to have a modify_port with
> some basic check.. right?

Sorry for not being clear,

Yes, I fully support your suggestion to improve modify_port function.

At least that common modify_port function should check the type of port
and return success for RoCE (see mlx4_ib_modify_port), but need to check
that mlx5_ib_modify_port won't break after that.

Thanks

> > Thanks
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index d5ca101..59911dd 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -4189,7 +4189,7 @@  static void cm_add_one(struct ib_device *ib_device)
                        goto error2;

                ret = ib_modify_port(ib_device, i, 0, &port_modify);
-               if (ret)
+               if (ret && ret != -ENOSYS)
                        goto error3;

                count++;