Re: mlx5: Add driver for Mellanox Connect-IB adapters
On Fri, Jun 20, 2014 at 08:58:32PM +0300, Dan Carpenter wrote: >976 >977err_create: >978if (qp->create_type == MLX5_QP_USER) >979destroy_qp_user(pd, qp); > ^^^ > But we dereference it inside destroy_qp_user() unconditionally. > >980else if (qp->create_type == MLX5_QP_KERNEL) >981destroy_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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
re: mlx5: Add driver for Mellanox Connect-IB adapters
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mlx5: Add driver for Mellanox Connect-IB adapters
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 majord...@vger.kernel.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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mlx5: Add driver for Mellanox Connect-IB adapters
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mlx5: Add driver for Mellanox Connect-IB adapters
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 > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: mlx5: Add driver for Mellanox Connect-IB adapters
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 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 , 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
re: mlx5: Add driver for Mellanox Connect-IB adapters
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
re: mlx5: Add driver for Mellanox Connect-IB adapters
--- 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html