[bug report] IB/mlx5: Support IB_WR_REG_SIG_MR

[bug report] IB/mlx5: Support IB_WR_REG_SIG_MR

[Dan Carpenter]

Hello Sagi Grimberg,

The patch e6631814fb3a: "IB/mlx5: Support IB_WR_REG_SIG_MR" from Feb
23, 2014, leads to the following static checker warning:

	net/rds/ib_recv.c:672 rds_ib_send_ack()
	warn: 'ret' can be either negative or positive

drivers/infiniband/hw/mlx5/qp.c
  3614  static int set_psv_wr(struct ib_sig_domain *domain,
  3615                        u32 psv_idx, void **seg, int *size)
  3616  {
  3617          struct mlx5_seg_set_psv *psv_seg = *seg;
  3618  
  3619          memset(psv_seg, 0, sizeof(*psv_seg));
  3620          psv_seg->psv_num = cpu_to_be32(psv_idx);
  3621          switch (domain->sig_type) {
  3622          case IB_SIG_TYPE_NONE:
  3623                  break;
  3624          case IB_SIG_TYPE_T10_DIF:
  3625                  psv_seg->transient_sig = cpu_to_be32(domain->sig.dif.bg << 16 |
  3626                                                       domain->sig.dif.app_tag);
  3627                  psv_seg->ref_tag = cpu_to_be32(domain->sig.dif.ref_tag);
  3628                  break;
  3629          default:
  3630                  pr_err("Bad signature type given.\n");
  3631                  return 1;

Why do we return 1 here instead of a negative error code?

  3632          }
  3633  
  3634          *seg += sizeof(*psv_seg);
  3635          *size += sizeof(*psv_seg) / 16;
  3636  
  3637          return 0;
  3638  }

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] IB/mlx5: Support IB_WR_REG_SIG_MR

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

On Fri, Dec 09, 2016 at 12:20:44PM +0300, Dan Carpenter wrote:
> Hello Sagi Grimberg,
>
> The patch e6631814fb3a: "IB/mlx5: Support IB_WR_REG_SIG_MR" from Feb
> 23, 2014, leads to the following static checker warning:
>
> 	net/rds/ib_recv.c:672 rds_ib_send_ack()
> 	warn: 'ret' can be either negative or positive
>
> drivers/infiniband/hw/mlx5/qp.c
>   3614  static int set_psv_wr(struct ib_sig_domain *domain,
>   3615                        u32 psv_idx, void **seg, int *size)
>   3616  {
>   3617          struct mlx5_seg_set_psv *psv_seg = *seg;
>   3618
>   3619          memset(psv_seg, 0, sizeof(*psv_seg));
>   3620          psv_seg->psv_num = cpu_to_be32(psv_idx);
>   3621          switch (domain->sig_type) {
>   3622          case IB_SIG_TYPE_NONE:
>   3623                  break;
>   3624          case IB_SIG_TYPE_T10_DIF:
>   3625                  psv_seg->transient_sig = cpu_to_be32(domain->sig.dif.bg << 16 |
>   3626                                                       domain->sig.dif.app_tag);
>   3627                  psv_seg->ref_tag = cpu_to_be32(domain->sig.dif.ref_tag);
>   3628                  break;
>   3629          default:
>   3630                  pr_err("Bad signature type given.\n");
>   3631                  return 1;
>
> Why do we return 1 here instead of a negative error code?

Yes, probably better to return something like EINVAL, instead of 1.
do you want us to handle it?

Thanks

>
>   3632          }
>   3633
>   3634          *seg += sizeof(*psv_seg);
>   3635          *size += sizeof(*psv_seg) / 16;
>   3636
>   3637          return 0;
>   3638  }
>
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bug report] IB/mlx5: Support IB_WR_REG_SIG_MR

[Dan Carpenter]

On Sun, Dec 11, 2016 at 11:23:26AM +0200, Leon Romanovsky wrote:
> >   3624          case IB_SIG_TYPE_T10_DIF:
> >   3625                  psv_seg->transient_sig = cpu_to_be32(domain->sig.dif.bg << 16 |
> >   3626                                                       domain->sig.dif.app_tag);
> >   3627                  psv_seg->ref_tag = cpu_to_be32(domain->sig.dif.ref_tag);
> >   3628                  break;
> >   3629          default:
> >   3630                  pr_err("Bad signature type given.\n");
> >   3631                  return 1;
> >
> > Why do we return 1 here instead of a negative error code?
> 
> Yes, probably better to return something like EINVAL, instead of 1.
> do you want us to handle it?
> 

Yes, please.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] IB/mlx5: Support IB_WR_REG_SIG_MR

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Mon, Dec 12, 2016 at 12:33:55PM +0300, Dan Carpenter wrote:
> On Sun, Dec 11, 2016 at 11:23:26AM +0200, Leon Romanovsky wrote:
> > >   3624          case IB_SIG_TYPE_T10_DIF:
> > >   3625                  psv_seg->transient_sig = cpu_to_be32(domain->sig.dif.bg << 16 |
> > >   3626                                                       domain->sig.dif.app_tag);
> > >   3627                  psv_seg->ref_tag = cpu_to_be32(domain->sig.dif.ref_tag);
> > >   3628                  break;
> > >   3629          default:
> > >   3630                  pr_err("Bad signature type given.\n");
> > >   3631                  return 1;
> > >
> > > Why do we return 1 here instead of a negative error code?
> >
> > Yes, probably better to return something like EINVAL, instead of 1.
> > do you want us to handle it?
> >
>
> Yes, please.

Thanks,
We will send it as part of our fixes series for 4.10.

>
> regards,
> dan carpenter
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [bug report] IB/mlx5: Support IB_WR_REG_SIG_MR

[Sagi Grimberg]

>>>>   3624          case IB_SIG_TYPE_T10_DIF:
>>>>   3625                  psv_seg->transient_sig = cpu_to_be32(domain->sig.dif.bg << 16 |
>>>>   3626                                                       domain->sig.dif.app_tag);
>>>>   3627                  psv_seg->ref_tag = cpu_to_be32(domain->sig.dif.ref_tag);
>>>>   3628                  break;
>>>>   3629          default:
>>>>   3630                  pr_err("Bad signature type given.\n");
>>>>   3631                  return 1;
>>>>
>>>> Why do we return 1 here instead of a negative error code?
>>>
>>> Yes, probably better to return something like EINVAL, instead of 1.
>>> do you want us to handle it?
>>>
>>
>> Yes, please.
>
> Thanks,
> We will send it as part of our fixes series for 4.10.

Thanks Leon. Thanks Dan for reporting.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html