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
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
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
--- 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 -- 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
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...@intel.com, 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
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
