re: mlx5: Add driver for Mellanox Connect-IB adapters

re: mlx5: Add driver for Mellanox Connect-IB adapters

[Dan Carpenter]

Hello Eli Cohen,

This is a semi-automatic email about new static checker warnings.

The patch e126ba97dba9: "mlx5: Add driver for Mellanox Connect-IB 
adapters" from Jul 7, 2013, leads to the following Smatch complaint:

drivers/infiniband/hw/mlx5/qp.c:979 create_qp_common()
	 error: we previously assumed 'pd->uobject' could be null (see line 847)

drivers/infiniband/hw/mlx5/qp.c
   846		if (pd) {
   847			if (pd->uobject) {
                            ^^^^^^^^^^^
There are a bunch of checks for pd->uobject.

   848				mlx5_ib_dbg(dev, "requested sq_wqe_count (%d)\n", ucmd.sq_wqe_count);
   849				if (ucmd.rq_wqe_shift != qp->rq.wqe_shift ||
   850				    ucmd.rq_wqe_count != qp->rq.wqe_cnt) {
   851					mlx5_ib_dbg(dev, "invalid rq params\n");
   852					return -EINVAL;
   853				}
   854				if (ucmd.sq_wqe_count > dev->mdev.caps.max_wqes) {
   855					mlx5_ib_dbg(dev, "requested sq_wqe_count (%d) > max allowed (%d)\n",

[ snip ]

   971		qp->doorbell_qpn = swab32(qp->mqp.qpn << 8);
   972	
   973		qp->mqp.event = mlx5_ib_qp_event;
   974	
   975		return 0;
   976	
   977	err_create:
   978		if (qp->create_type == MLX5_QP_USER)
   979			destroy_qp_user(pd, qp);
                        ^^^^^^^^^^^^^^^
But we dereference it inside destroy_qp_user() unconditionally.

   980		else if (qp->create_type == MLX5_QP_KERNEL)
   981			destroy_qp_kernel(dev, qp);

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: mlx5: Add driver for Mellanox Connect-IB adapters

[Eli Cohen]

On Fri, Jun 20, 2014 at 08:58:32PM +0300, Dan Carpenter wrote:
>    976	
>    977	err_create:
>    978		if (qp->create_type == MLX5_QP_USER)
>    979			destroy_qp_user(pd, qp);
>                         ^^^^^^^^^^^^^^^
> But we dereference it inside destroy_qp_user() unconditionally.
> 
>    980		else if (qp->create_type == MLX5_QP_KERNEL)
>    981			destroy_qp_kernel(dev, qp);
> 

When we get here we already updated create_type and since all
MLX5_QP_USER type QPs have PD the logic is not broken.
--
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: mlx5: Add driver for Mellanox Connect-IB adapters

[Dan Carpenter]

Hello Eli Cohen,

The patch e126ba97dba9: "mlx5: Add driver for Mellanox Connect-IB
adapters" from Jul 7, 2013, leads to the following Smatch warning:
"drivers/net/ethernet/mellanox/mlx5/core/cmd.c:822
mlx5_alloc_cmd_msg()
	 warn: use 'flags' here instead of GFP_XXX?"

   811  static struct mlx5_cmd_msg *mlx5_alloc_cmd_msg(struct mlx5_core_dev *dev,
   812                                                 gfp_t flags, int size)
                                                       ^^^^^^^^^^^

   813  {
   814          struct mlx5_cmd_mailbox *tmp, *head = NULL;
   815          struct mlx5_cmd_prot_block *block;
   816          struct mlx5_cmd_msg *msg;
   817          int blen;
   818          int err;
   819          int n;
   820          int i;
   821  
   822          msg = kzalloc(sizeof(*msg), GFP_KERNEL);
                                            ^^^^^^^^^^
   823          if (!msg)
   824                  return ERR_PTR(-ENOMEM);
   825  
   826          blen = size - min_t(int, sizeof(msg->first.data), size);
   827          n = (blen + MLX5_CMD_DATA_BLOCK_SIZE - 1) / MLX5_CMD_DATA_BLOCK_SIZE;
   828  
   829          for (i = 0; i < n; i++) {
   830                  tmp = alloc_cmd_box(dev, flags);
                                                 ^^^^^
There is a kmalloc() in alloc_cmd_box() that uses flags as well as a
pci_pool_alloc() that uses it.

   831                  if (IS_ERR(tmp)) {
   832                          mlx5_core_warn(dev, "failed allocating block\n");

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: mlx5: Add driver for Mellanox Connect-IB adapters

[Or Gerlitz]

On 10/07/2013 13:55, Dan Carpenter wrote:
> Hello Eli Cohen,
>
> The patch e126ba97dba9: "mlx5: Add driver for Mellanox Connect-IB
> adapters" from Jul 7, 2013, leads to the following Smatch warning:
> "drivers/net/ethernet/mellanox/mlx5/core/cmd.c:822
> mlx5_alloc_cmd_msg()
> 	 warn: use 'flags' here instead of GFP_XXX?"
>
>     811  static struct mlx5_cmd_msg *mlx5_alloc_cmd_msg(struct mlx5_core_dev *dev,
>     812                                                 gfp_t flags, int size)
>                                                         ^^^^^^^^^^^
>
>     813  {
>     814          struct mlx5_cmd_mailbox *tmp, *head = NULL;
>     815          struct mlx5_cmd_prot_block *block;
>     816          struct mlx5_cmd_msg *msg;
>     817          int blen;
>     818          int err;
>     819          int n;
>     820          int i;
>     821
>     822          msg = kzalloc(sizeof(*msg), GFP_KERNEL);
>                                              ^^^^^^^^^^
>     823          if (!msg)
>     824                  return ERR_PTR(-ENOMEM);
>     825
>     826          blen = size - min_t(int, sizeof(msg->first.data), size);
>     827          n = (blen + MLX5_CMD_DATA_BLOCK_SIZE - 1) / MLX5_CMD_DATA_BLOCK_SIZE;
>     828
>     829          for (i = 0; i < n; i++) {
>     830                  tmp = alloc_cmd_box(dev, flags);
>                                                   ^^^^^
> There is a kmalloc() in alloc_cmd_box() that uses flags as well as a
> pci_pool_alloc() that uses it.
>
>     831                  if (IS_ERR(tmp)) {
>     832                          mlx5_core_warn(dev, "failed allocating block\n");
>
>

Hi,

I think Eli has some problems with his email account for mailing lists, 
so sorry for the delay. Once this is fixed, he should
be addressing your email.

Or.


--
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: mlx5: Add driver for Mellanox Connect-IB adapters

[Eli Cohen]

Hi Dan,
thanks for cathing this. I will fix this an a subsequent patch.
Currently we can consider this non critical as we don't use commands
from interrupt context.

On Wed, Jul 10, 2013 at 01:55:38PM +0300, Dan Carpenter wrote:
> Hello Eli Cohen,
> 
> The patch e126ba97dba9: "mlx5: Add driver for Mellanox Connect-IB
> adapters" from Jul 7, 2013, leads to the following Smatch warning:
> "drivers/net/ethernet/mellanox/mlx5/core/cmd.c:822
> mlx5_alloc_cmd_msg()
> 	 warn: use 'flags' here instead of GFP_XXX?"
> 
>    811  static struct mlx5_cmd_msg *mlx5_alloc_cmd_msg(struct mlx5_core_dev *dev,
>    812                                                 gfp_t flags, int size)
>                                                        ^^^^^^^^^^^
> 
>    813  {
>    814          struct mlx5_cmd_mailbox *tmp, *head = NULL;
>    815          struct mlx5_cmd_prot_block *block;
>    816          struct mlx5_cmd_msg *msg;
>    817          int blen;
>    818          int err;
>    819          int n;
>    820          int i;
>    821  
>    822          msg = kzalloc(sizeof(*msg), GFP_KERNEL);
>                                             ^^^^^^^^^^
>    823          if (!msg)
>    824                  return ERR_PTR(-ENOMEM);
>    825  
>    826          blen = size - min_t(int, sizeof(msg->first.data), size);
>    827          n = (blen + MLX5_CMD_DATA_BLOCK_SIZE - 1) / MLX5_CMD_DATA_BLOCK_SIZE;
>    828  
>    829          for (i = 0; i < n; i++) {
>    830                  tmp = alloc_cmd_box(dev, flags);
>                                                  ^^^^^
> There is a kmalloc() in alloc_cmd_box() that uses flags as well as a
> pci_pool_alloc() that uses it.
> 
>    831                  if (IS_ERR(tmp)) {
>    832                          mlx5_core_warn(dev, "failed allocating block\n");
> 
> 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
--
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: mlx5: Add driver for Mellanox Connect-IB adapters

[Dan Carpenter]

---
Side note: Sparse should warn about endian bugs but in linux-next
endian checking is disabled because we hit:

include/uapi/linux/swab.h:71:16: error: undefined identifier '__builtin_bswap64'
include/uapi/linux/swab.h:71:33: error: not a function <noident>

do_error() in Sparse disables warning messages.  I feel like we
shouldn't do that.

        /* Shut up warnings after an error */
        max_warnings = 0;
---

Hello Eli Cohen,

The patch e126ba97dba9: "mlx5: Add driver for Mellanox Connect-IB 
adapters" from Jul 7, 2013, has an endian related bug:

drivers/net/ethernet/mellanox/mlx5/core/main.c
   214          memset(&set_out, 0, sizeof(set_out));
   215          set_ctx->hca_cap.uar_page_sz = cpu_to_be16(PAGE_SHIFT - 12);
                         ^^^^^^^^^^^^^^^^^^^
This is defined in the header as be32 but we are saving a be16 to it.
My guess is the header is correct and the be16 is wrong.

   216          set_ctx->hdr.opcode = cpu_to_be16(MLX5_CMD_OP_SET_HCA_CAP);
   217          err = mlx5_cmd_exec(dev, set_ctx, sizeof(*set_ctx),
   218                                   &set_out, sizeof(set_out));

regards,
dan carpenter

Re: mlx5: Add driver for Mellanox Connect-IB adapters

[Or Gerlitz]

On 10/07/2013 13:54, Dan Carpenter wrote:
> ---
> Side note: Sparse should warn about endian bugs but in linux-next
> endian checking is disabled because we hit:
>
> include/uapi/linux/swab.h:71:16: error: undefined identifier '__builtin_bswap64'
> include/uapi/linux/swab.h:71:33: error: not a function <noident>
>
> do_error() in Sparse disables warning messages.  I feel like we
> shouldn't do that.
>
>          /* Shut up warnings after an error */
>          max_warnings = 0;
> ---
>
> Hello Eli Cohen,
>
> The patch e126ba97dba9: "mlx5: Add driver for Mellanox Connect-IB
> adapters" from Jul 7, 2013, has an endian related bug:
>
> drivers/net/ethernet/mellanox/mlx5/core/main.c
>     214          memset(&set_out, 0, sizeof(set_out));
>     215          set_ctx->hca_cap.uar_page_sz = cpu_to_be16(PAGE_SHIFT - 12);
>                           ^^^^^^^^^^^^^^^^^^^
> This is defined in the header as be32 but we are saving a be16 to it.
> My guess is the header is correct and the be16 is wrong.
>
>     216          set_ctx->hdr.opcode = cpu_to_be16(MLX5_CMD_OP_SET_HCA_CAP);
>     217          err = mlx5_cmd_exec(dev, set_ctx, sizeof(*set_ctx),
>     218                                   &set_out, sizeof(set_out));
>
> regards,
> dan carpenter
>

Dan, this sparse catch was reported earlier by Fengguang Wu 
<fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, we have a fix, will send now

Or.
--
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: mlx5: Add driver for Mellanox Connect-IB adapters

[Josh Triplett]

On Wed, Jul 10, 2013 at 01:54:15PM +0300, Dan Carpenter wrote:
> ---
> Side note: Sparse should warn about endian bugs but in linux-next
> endian checking is disabled because we hit:
> 
> include/uapi/linux/swab.h:71:16: error: undefined identifier '__builtin_bswap64'
> include/uapi/linux/swab.h:71:33: error: not a function <noident>
> 
> do_error() in Sparse disables warning messages.  I feel like we
> shouldn't do that.

No, I think that's the correct behavior: sparse should only generate
*errors* rather than warnings when it hits something that prevents it
from handling some chunk of code, in which case we'd get a huge number
of spurious warnings if not suppressed.

In any case, current Sparse has __builtin_bswap64, so you shouldn't get
that error.

- Josh Triplett
--
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