Re: iser-target: Add iSCSI Extensions for RDMA (iSER) target driver
I see now that there is documentation in ib_req_notify_cq(). 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: iser-target: Add iSCSI Extensions for RDMA (iSER) target driver
Hello Nicholas Bellinger,
The patch b8d26b3be8b3: "iser-target: Add iSCSI Extensions for RDMA
(iSER) target driver" from Mar 7, 2013, leads to the following static
checker warning:
drivers/infiniband/ulp/isert/ib_isert.c:423 isert_device_get()
error: passing non negative 1 to ERR_PTR
drivers/infiniband/ulp/isert/ib_isert.c
417
418 device->ib_device = cma_id->device;
419 ret = isert_create_device_ib_res(device);
420 if (ret) {
421 kfree(device);
422 mutex_unlock(&device_list_mutex);
423 return ERR_PTR(ret);
The warning here is because isert_create_device_ib_res() returns either
a negative error code, zero or one. The documentation is not clear what
that means. AHAHAHAHAHAHAHAH. I joke. There is no documentation.
Anyway, it's definitely a bug and it leads to a NULL dereference in the
caller.
424 }
425
426 device->refcount++;
427 list_add_tail(&device->dev_node, &device_list);
428 isert_info("Created a new iser device %p refcount %d\n",
429 device, device->refcount);
430 mutex_unlock(&device_list_mutex);
431
432 return device;
433 }
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: [PATCH 2/3] staging/rdma/hfi1: Fix module parameter spelling
Nice. Whenever you see a bug like this, you should report it to kernel-janitors because you know that 10 other people have made the same mistake. I will take care of it. 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: [PATCH 3/3] staging/rdma/hfi1: check for ARMED->ACTIVE transition in receive interrupt
Possible off by one, but mostly the whitespace makes me itch.
Jim was not on the CC list.
On Thu, Dec 17, 2015 at 07:24:15PM -0500, Jubin John wrote:
> From: Jim Snow
>
> The link state will transition from ARMED to ACTIVE when a non-SC15
> packet arrives, but the driver might not notice the change. With this
> fix, if the slowpath receive interrupt handler sees a non-SC15 packet
> while in the ARMED state, we queue work to call linkstate_active_work
> from process context to promote it to ACTIVE.
>
> Signed-off-by: Jim Snow
> Signed-off-by: Brendan Cunningham
> Reviewed-by: Dean Luick
> Signed-off-by: Jubin John
> ---
> drivers/staging/rdma/hfi1/chip.c |5 ++-
> drivers/staging/rdma/hfi1/chip.h |2 +
> drivers/staging/rdma/hfi1/driver.c | 66
>
> drivers/staging/rdma/hfi1/hfi.h| 12 ++
> drivers/staging/rdma/hfi1/init.c |1 +
> 5 files changed, 84 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rdma/hfi1/chip.c
> b/drivers/staging/rdma/hfi1/chip.c
> index 78a5f08..f82b848 100644
> --- a/drivers/staging/rdma/hfi1/chip.c
> +++ b/drivers/staging/rdma/hfi1/chip.c
> @@ -4606,7 +4606,7 @@ static inline void clear_recv_intr(struct hfi1_ctxtdata
> *rcd)
> }
>
> /* force the receive interrupt */
> -static inline void force_recv_intr(struct hfi1_ctxtdata *rcd)
> +void force_recv_intr(struct hfi1_ctxtdata *rcd)
> {
> write_csr(rcd->dd, CCE_INT_FORCE + (8 * rcd->ireg), rcd->imask);
> }
> @@ -4705,7 +4705,7 @@ u32 read_physical_state(struct hfi1_devdata *dd)
> & DC_DC8051_STS_CUR_STATE_PORT_MASK;
> }
>
> -static u32 read_logical_state(struct hfi1_devdata *dd)
> +u32 read_logical_state(struct hfi1_devdata *dd)
> {
> u64 reg;
>
> @@ -6658,6 +6658,7 @@ int set_link_state(struct hfi1_pportdata *ppd, u32
> state)
> ppd->link_enabled = 1;
> }
>
> + set_all_slowpath(ppd->dd);
> ret = set_local_link_attributes(ppd);
> if (ret)
> break;
> diff --git a/drivers/staging/rdma/hfi1/chip.h
> b/drivers/staging/rdma/hfi1/chip.h
> index d74aed8..620cf08 100644
> --- a/drivers/staging/rdma/hfi1/chip.h
> +++ b/drivers/staging/rdma/hfi1/chip.h
> @@ -691,6 +691,8 @@ u64 read_dev_cntr(struct hfi1_devdata *dd, int index, int
> vl);
> u64 write_dev_cntr(struct hfi1_devdata *dd, int index, int vl, u64 data);
> u64 read_port_cntr(struct hfi1_pportdata *ppd, int index, int vl);
> u64 write_port_cntr(struct hfi1_pportdata *ppd, int index, int vl, u64 data);
> +u32 read_logical_state(struct hfi1_devdata *dd);
> +void force_recv_intr(struct hfi1_ctxtdata *rcd);
>
> /* Per VL indexes */
> enum {
> diff --git a/drivers/staging/rdma/hfi1/driver.c
> b/drivers/staging/rdma/hfi1/driver.c
> index 4c52e78..16b1be1 100644
> --- a/drivers/staging/rdma/hfi1/driver.c
> +++ b/drivers/staging/rdma/hfi1/driver.c
> @@ -862,6 +862,15 @@ static inline void set_all_dma_rtail(struct hfi1_devdata
> *dd)
> &handle_receive_interrupt_dma_rtail;
> }
>
> +void set_all_slowpath(struct hfi1_devdata *dd)
> +{
> + int i;
> +
> + for (i = HFI1_CTRL_CTXT + 1; i < dd->first_user_ctxt; i++)
> + dd->rcd[i]->do_interrupt =
> + &handle_receive_interrupt;
This fits within the 80 character limit.
We start counting from HFI1_CTRL_CTXT + 1 but in receive_interrupt_work()
we start counting from HFI1_CTRL_CTXT. What's the story? It's either a
bug or needs much better documentation.
> +}
> +
> /*
> * handle_receive_interrupt - receive a packet
> * @rcd: the context
> @@ -929,6 +938,27 @@ int handle_receive_interrupt(struct hfi1_ctxtdata *rcd,
> int thread)
> last = skip_rcv_packet(&packet, thread);
> skip_pkt = 0;
> } else {
> + /* Auto activate link on non-SC15 packet receive */
> + if (unlikely(rcd->ppd->host_link_state ==
> + HLS_UP_ARMED)) {
> + struct work_struct *lsaw =
> + &rcd->ppd->linkstate_active_work;
> + struct hfi1_message_header *hdr =
> + hfi1_get_msgheader(packet.rcd->dd,
> + packet.rhf_addr);
> + if (hdr2sc(hdr, packet.rhf) != 0xf) {
> + int hwstate = read_logical_state(dd);
> +
> + if (hwstate != LSTATE_ACTIVE) {
> + dd_dev_info(dd, "Unexpected
> link state %d\n",
> + hwstate);
> + } else {
> + queue_work(
> + rcd->ppd->hfi1_wq
Re: [PATCH 13/14] staging/rdma/hfi1: Add TID entry program function body
On Thu, Dec 17, 2015 at 02:00:23AM -0500, ira.we...@intel.com wrote:
> + mutex_lock(&uctxt->exp_lock);
> + /*
> + * The first step is to program the RcvArray entries which are complete
> + * groups.
> + */
> + while (ngroups && uctxt->tid_group_list.count) {
> + struct tid_group *grp =
> + tid_group_pop(&uctxt->tid_group_list);
> +
> + ret = program_rcvarray(fp, vaddr, grp, pagesets,
> +pageidx, dd->rcv_entries.group_size,
> +pages, tidlist, &tididx, &mapped);
> + /*
> + * If there was a failure to program the RcvArray
> + * entries for the entire group, reset the grp fields
> + * and add the grp back to the free group list.
> + */
> + if (ret <= 0) {
> + tid_group_add_tail(grp, &uctxt->tid_group_list);
> + hfi1_cdbg(TID,
> + "Failed to program RcvArray group %d", ret);
> + goto unlock;
We print a failure message but we still return success when ret == 0.
> + }
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: [PATCH 08/14] staging/rdma/hfi1: Start adding building blocks for TID caching
On Thu, Dec 17, 2015 at 02:00:18AM -0500, ira.we...@intel.com wrote:
> +}
> +
> +static int program_rcvarray(struct file *fp, unsigned long vaddr,
> + struct tid_group *grp,
> + struct tid_pageset *sets,
> + unsigned start, u16 count, struct page **pages,
> + u32 *tidlist, unsigned *tididx, unsigned *pmapped)
> +{
It's not clear what a zero return from this function means. Could we
add some documentation?
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: [PATCH 08/14] staging/rdma/hfi1: Start adding building blocks for TID caching
On Thu, Dec 17, 2015 at 02:00:18AM -0500, ira.we...@intel.com wrote:
> From: Mitko Haralanov
> +static int unprogram_rcvarray(struct file *fp, u32 tidinfo,
> + struct tid_group **grp)
> +{
> + struct hfi1_filedata *fd = fp->private_data;
> + struct hfi1_ctxtdata *uctxt = fd->uctxt;
> + struct hfi1_devdata *dd = uctxt->dd;
> + struct mmu_rb_node *node;
> + u8 tidctrl = EXP_TID_GET(tidinfo, CTRL);
> + u32 tidbase = uctxt->expected_base,
> + tididx = EXP_TID_GET(tidinfo, IDX) << 1, rcventry;
> +
> + if (tididx > uctxt->expected_count) {
Should this be >= ? I don't think it makes that much difference since
we're not using it as an offset.
> + dd_dev_err(dd, "Invalid RcvArray entry (%u) index for ctxt
> %u\n",
> +tididx, uctxt->ctxt);
> + return -EINVAL;
> + }
> +
> + if (tidctrl == 0x3)
> + return -EINVAL;
> +
> + rcventry = tidbase + tididx + (tidctrl - 1);
> +
> + spin_lock(&fd->rb_lock);
> + node = mmu_rb_search_by_entry(&fd->tid_rb_root, rcventry);
> + if (!node) {
> + spin_unlock(&fd->rb_lock);
> + return -EBADF;
> + }
> + rb_erase(&node->rbnode, &fd->tid_rb_root);
> + spin_unlock(&fd->rb_lock);
> + if (grp)
> + *grp = node->grp;
> + clear_tid_node(fd, fd->subctxt, node);
> + return 0;
> +}
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: [PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc
On Mon, Dec 14, 2015 at 05:41:23PM +, Nicholas Mc Guire wrote: > I obviously made a real mess here. > I incorrectly concluded that rxcontext is 0 which it is not in some cases Yep. Plus you build tested it but assumed that the unused variable warning must have been there in the original... I've done that for static checker warnings. Lesson learned, hopefully. 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: net/mlx5: E-Switch, Add SR-IOV (FDB) support
Hello Saeed Mahameed,
The patch 81848731ff40: "net/mlx5: E-Switch, Add SR-IOV (FDB)
support" from Dec 1, 2015, leads to the following static checker
warning:
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:579
esw_fdb_set_vport_rule()
warn: passing zero to 'PTR_ERR'
drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
568 esw_debug(esw->dev,
569"\tFDB add rule dmac_v(%pM) dmac_c(%pM) ->
vport(%d)\n",
570dmac_v, dmac_c, vport);
571 flow_rule =
572 mlx5_add_flow_rule(esw,
573 match_header,
574 match_c,
575 match_v,
576 MLX5_FLOW_CONTEXT_ACTION_FWD_DEST,
577 0, &dest);
578 if (IS_ERR_OR_NULL(flow_rule)) {
mlx5_add_flow_rule() only returns NULL on error. It never returns
ERR_PTRs.
579 pr_warn(
580 "FDB: Failed to add flow rule: dmac_v(%pM)
dmac_c(%pM) -> vport(%d), err(%ld)\n",
It's not a terrible bug but this always says "err(0)" which is not very
useful, and it causes this static checker warning.
581 dmac_v, dmac_c, vport, PTR_ERR(flow_rule));
582 flow_rule = NULL;
583 }
584 out:
585 kfree(match_v);
586 kfree(match_c);
587 return flow_rule;
588 }
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: [PATCH 5/5] staging/rdma/hfi1: Add page lock limit check for SDMA requests
On Wed, Dec 02, 2015 at 03:56:27PM -0500, ira.we...@intel.com wrote:
> - for (i = tx->idx; i >= 0; i--) {
> - if (tx->iovecs[i].flags & TXREQ_FLAGS_IOVEC_LAST_PKT)
> - unpin_vector_pages(tx->iovecs[i].vec);
> + /*
> + * If we have any io vectors associated with this txreq,
> + * check whether they need to be 'freed'. We can't free them
> + * here because the unpin function needs to be able to sleep.
> + */
> + i = tx->idx;
> + while (i)
> + if (tx->iovecs[i--].flags & TXREQ_FLAGS_IOVEC_LAST_PKT) {
> + defer = true;
> + break;
> }
In the original code, we checked tx->iovecs[0].flags but now we skip
that one. Going back to the original for loop is probably the right way
to fix this.
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: staging/rdma/hfi1: pre-compute sc and sde for RC/UC QPs
Hello Mike Marciniszyn,
The patch d7b8ba5121e8: "staging/rdma/hfi1: pre-compute sc and sde
for RC/UC QPs" from Nov 9, 2015, leads to the following static
checker warning:
drivers/staging/rdma/hfi1/verbs.c:1657 ah_to_sc()
error: buffer overflow 'ibp->sl_to_sc' 32 <= 255
drivers/staging/rdma/hfi1/qp.c
817 if (attr_mask & IB_QP_PATH_MIG_STATE) {
818 qp->s_mig_state = attr->path_mig_state;
819 if (mig) {
820 qp->remote_ah_attr = qp->alt_ah_attr;
821 qp->port_num = qp->alt_ah_attr.port_num;
822 qp->s_pkey_index = qp->s_alt_pkey_index;
823 qp->s_flags |= HFI1_S_AHG_CLEAR;
824 qp->s_sc = ah_to_sc(ibqp->device,
&qp->remote_ah_attr);
Do we need to verify (hfi1_check_ah(ibqp->device, &qp->remote_ah_attr))
before calling ah_to_sc()?
825 qp->s_sde = qp_to_sdma_engine(qp, qp->s_sc);
826 }
827 }
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: HFI1 code duplication todo
On Fri, Nov 20, 2015 at 10:41:16AM -0500, Doug Ledford wrote: > So, as to the hfi1/qib/rxe transport library. The qib driver is in the > rdma tree, and we aren't going to move it to staging just because it > depends on something in staging, so we need to start adding the library > in the core tree to avoid dependency issues. As for the hfi1 driver, > I'm of the opinion that putting it in staging because of the code > duplication issue was probably not the right thing to do. It's > benefited from the extra eyes on it to help make it a better driver, but > I think I'm ready to move it to the main RDMA tree and start working on > it from there where there won't be any cross tree dependency issues. You could leave it staging but manage it from the same git tree. It avoids the cross tree dependencies. This is what we do for drivers/staging/media/, all that stuff goes through the linux-media tree. 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: [PATCH 07/18] staging/rdma/hfi1: Extend quiet timeout
On Fri, Nov 20, 2015 at 03:21:14PM +, Marciniszyn, Mike wrote: > > > The longest quiet timeout is now 6s. Extend the driver wait. > > > > To what? And why? What does this fix? > > > > The driver wasn't following the our internal specification: 6 seconds. > > The patch corrects that issue. Does that mean the patch fixes something which is visible to the user? A better description would be: We added XXX long timeout so now the longest timeout is 6 seconds in our spec. It shouldn't affect most users but something something prevents a hang. I had all the same issues that Greg did with these patch descriptions so it was predictable that he was going to complain. 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: HFI1 code duplication todo
At the beginning when hfi1 was put into staging, then it was easy enough for Greg to take those patches but now it feels awkward. Probably Doug and the linux-rdma@vger.kernel.org people should start maintaining the drivers/staging/rdma directory. Like merge Greg's tree and pull in whatever checkpatch fixes are still on the list and then take over from there... 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: [PATCH 12/13] staging/rdma/hfi1: Read EFI variable for device description
On Wed, Nov 11, 2015 at 02:03:02PM +, Luick, Dean wrote:
>
>
> > -Original Message-
> > From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> > ow...@vger.kernel.org] On Behalf Of Dan Carpenter
> > Sent: Wednesday, November 11, 2015 2:45 AM
> > To: John, Jubin
> > Cc: gre...@linuxfoundation.org; de...@driverdev.osuosl.org; linux-
> > r...@vger.kernel.org; dledf...@redhat.com
> > Subject: Re: [PATCH 12/13] staging/rdma/hfi1: Read EFI variable for device
> > description
> >
> > On Wed, Nov 11, 2015 at 02:33:32AM -0500, Jubin John wrote:
> > > +static int read_efi_var(const char *name, unsigned long *size,
> > > + void **return_data)
> > > +{
> > > + int ret;
> > > +
> > > + /* set failure return values */
> > > + *size = 0;
> > > + *return_data = NULL;
> > > +
> > > + /*
> > > + * Use EFI run-time support to obtain an EFI variable. Support may
> > > + * be compiled out, so declare all variables inside.
> > > + */
> > > + if (efi_enabled(EFI_RUNTIME_SERVICES)) {
> >
> >
> > Flip this around:
> >
> > if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > return -ENOSYS;
>
> The style here is very deliberate.
>
> The issue is how efi_enabled() is defined via CONFIG options.
> The function can be turned into a 0 if certain CONFIG variables are
> not set. The code is structured to make all of the dependent
> variables disappear if efi_enabled() becomes 0.
This all understand.
> If the code is shifted as you suggest, we will get builds from the
> automatic builders that try all combinations with unused variables.
> This was done to avoid that.
I'm not sure I understand. You are doing this to try tricking the
autobuilders into not testind certain configs? What? I don't
understand what you mean by unused variables. There shouldn't be any
unused variable warnings. If you are getting unused variable warnings
can you post one so that I can take a look?
Maybe you are worried the function is a waste of memory if you declare
the variables earlier before the if enabled check? It's not a problem.
The compiler is smart enough to see the immediate return and removes
the dead code.
Perhaps I am not seeing your concern.
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: [PATCH v4 5/9] staging/rdma/hfi1: Add function stubs for TID caching
On Wed, Nov 11, 2015 at 01:10:40AM -0500, ira.weiny wrote: > The original author and I have been going through the code to see what we can > do. We have identified a couple of other pieces which can be split. > > One question. Is it ok to have functionality which is added which is unused > in > a preliminary patch? I believe this is ok as long as the code compiles but I > just wanted to make sure. While there are different operations added in this > patch it is broken to not use them as a set. So we need to have a series > which > implement the pieces with a final patch which exposes the set of operations. > > Is this acceptable? Yeah. It's fine. Don't add warnings about unused static functions though. Also we like to see a user in the same patchset so don't add infrastructure first and the user code the next year or whatever. 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: [PATCH 5/8] staging/rdma/hfi1: return early if setlink state was specified
On Wed, Nov 11, 2015 at 12:43:06AM -0500, ira.we...@intel.com wrote:
> From: Ira Weiny
>
> Set link state was not supported and so we can return early in the parameter
> checks rather than falling through the switch clause.
>
> Signed-off-by: Dennis Dalessandro
> Signed-off-by: Ira Weiny
> ---
> drivers/staging/rdma/hfi1/diag.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/rdma/hfi1/diag.c
> b/drivers/staging/rdma/hfi1/diag.c
> index 556a47591989..a489a79dd3b6 100644
> --- a/drivers/staging/rdma/hfi1/diag.c
> +++ b/drivers/staging/rdma/hfi1/diag.c
> @@ -999,16 +999,14 @@ static long hfi1_ioctl(struct file *fp, unsigned int
> cmd, unsigned long arg)
>* Other are invalid.
>*/
> return -EINVAL;
> + } else if (cmd == HFI1_SNOOP_IOCSETLINKSTATE) {
> + /* We do not support the old setlink state */
> + return -EINVAL;
Just delete it and let the default in the switch statement return
-ENOTTY.
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: [PATCH 4/8] staging/rdma/hfi1: remove unneeded goto done
On Wed, Nov 11, 2015 at 12:43:05AM -0500, ira.we...@intel.com wrote: > From: Ira Weiny > > This goto done is followed by an if (ret) break in the outer switch clause. > It > is unnecessary. > > Signed-off-by: Dennis Dalessandro > Signed-off-by: Ira Weiny Also these sign offs don't really make sense. Signed-off-by is basically like signing a legal document saying the code in this patch was not stolen from SCO UnixWare. You only need to sign if you have handled the patch. Did Dennis write this patch or did he Ack it or Review it somehow? 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: [PATCH 4/8] staging/rdma/hfi1: remove unneeded goto done
On Wed, Nov 11, 2015 at 12:43:05AM -0500, ira.we...@intel.com wrote: > From: Ira Weiny > > This goto done is followed by an if (ret) break in the outer switch clause. > It > is unnecessary. > > Signed-off-by: Dennis Dalessandro > Signed-off-by: Ira Weiny > --- > drivers/staging/rdma/hfi1/diag.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/rdma/hfi1/diag.c > b/drivers/staging/rdma/hfi1/diag.c > index 2bb857b2a103..556a47591989 100644 > --- a/drivers/staging/rdma/hfi1/diag.c > +++ b/drivers/staging/rdma/hfi1/diag.c > @@ -1053,7 +1053,6 @@ static long hfi1_ioctl(struct file *fp, unsigned int > cmd, unsigned long arg) > break; > default: > ret = -EINVAL; > - goto done; > } > ret = set_link_state(ppd, dev_state); > break; Wut? No it's not. 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: [PATCH 12/13] staging/rdma/hfi1: Read EFI variable for device description
/ > + snprintf(name, sizeof(name), "%04x:%02x:%02x.%x-%s", > + pci_domain_nr(dd->pcidev->bus), > + dd->pcidev->bus->number, > + PCI_SLOT(dd->pcidev->devfn), > + PCI_FUNC(dd->pcidev->devfn), > + kind); > + name[sizeof(name) - 1] = 0; /* make sure the string is terminated */ No need. snprintf() always puts a NUL terminator (technically it doesn't if the sizeof(name) is zero, I suppose). > + > + return read_efi_var(name, size, return_data); > +} 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: [PATCH 10/13] staging/rdma/hfi1: adding per SDMA engine stats to hfistats
On Wed, Nov 11, 2015 at 02:33:30AM -0500, Jubin John wrote:
> @@ -8288,6 +8367,21 @@ static int init_cntrs(struct hfi1_devdata *dd)
> dd->ndevcntrs++;
> index++;
> }
> + } else if (dev_cntrs[i].flags & CNTR_SDMA) {
> + hfi1_dbg_early(
> +"\tProcessing per SDE counters chip
> enginers %u\n",
> +dd->chip_sdma_engines);
> + dev_cntrs[i].offset = index;
> + for (j = 0; j < dd->chip_sdma_engines; j++) {
> + memset(name, '\0', C_MAX_NAME);
This patch is ok, but none of the memsets in this function are needed.
You could remove them in a later patch.
> + snprintf(name, C_MAX_NAME, "%s%d",
> + dev_cntrs[i].name, j);
> + sz += strlen(name);
> + sz++;
> + hfi1_dbg_early("\t\t%s\n", name);
We're basically just trying to calculate a bunch of strlen()s but there
is a lot of extra code to generate debug output. It would be better to
remove it in a later patch.
> + dd->ndevcntrs++;
> + index++;
> + }
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: [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices
That's fine. I feel like the first option is better because those long names really are cumbersome. I sometimes cut and paste bugs caused by using really long names because it's hard to spot the differences between two really long names which are 80% the same filler text. (I'm using a static checker so the checker finds the bugs and I have to stare at it for a very long time to spot the characters which are different). 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: [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices
Gar... No. Please please get rid of the PC() macro. It makes the code impossible to understand because instead of hitting CTRL-[ you have decode it and then manually type out :cs find g CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT which is the length of a typical college essay. I meant just put a comment like this: /* * In the hardware spec these are prefixed with: * CCE_PCIE_CTRL_... * But it is too long to use in code. */ #define XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK 0x1ull Or probably even better: #define CCE_PCIE_CTRL (CCE + 0x00C0) #define LANE_BUNDLE_MASK0x3ull /* CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK */ #define LANE_BUNDLE_SHIFT 0 /* CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT */ #define LANE_DELAY_MASK 0xFull /* CCE_PCIE_CTRL_PCIE_LANE_DELAY_MASK */ #define LANE_DELAY_SHIFT2 /* CCE_PCIE_CTRL_PCIE_LANE_DELAY_SHIFT */ #define MARGIN_OVERWRITE_SHIFT 8 /* CCE_PCIE_CTRL_XMT_MARGIN_OVERWRITE_ENABLE_SHIFT */ #define MARGIN_SHIFT9 /* CCE_PCIE_CTRL_XMT_MARGIN_SHIFT */ #define MARGIN_G1G2_OVERWRITE_MASK 0x1ull /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK */ #define MARGIN_G1G2_OVERWRITE_SHIFT 12/* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT */ #define MARGIN_G1G2_MASK0x7ull /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_MASK */ #define MARGIN_G1G2_SHIFT 13 /* CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_SHIFT */ Those lines go over the 80 character limit but it's fine. 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: [PATCH] staging/rdma/hfi1: set Gen3 half-swing for integrated devices
On Thu, Nov 05, 2015 at 11:41:23AM -0500, ira.weiny wrote: > Is this an example where we should ignore the line lengths of checkpatch to > preserve the readability of the code? Honestly, you can't fight checkpatch, especially in staging. Rejecting patches is a drain on your emotions and your energy. We accept all checkpatch fixes if they are half way decent. Maybe just put a comment in the header about the full hardware name? This patch breaks cscope and we don't end up actually using the hardware name in the end... 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: [PATCH] staging/rdma/hfi1: Convert dd_dev_info() to hfi1_cdbg() in process startup
On Thu, Nov 05, 2015 at 09:51:20AM -0500, Dennis Dalessandro wrote: > >/me chants, "delete. delete. delete." > > I would tend to agree with that, but in this case we want to keep a > way to get this information without making code changes. We just > don't want it to spew to the console/syslog all the time. Instead we > are using the trace mechanism which lets the user selectively turn > on the messages when needed. > > Perhaps we should expand on the commit message to make this more clear? > No no. It's fine. So long as you aren't just keeping it around because you are afraid to delete things. 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: [PATCH] staging/rdma/hfi1: Delete unnecessary checks before two function calls
On Thu, Nov 05, 2015 at 09:10:47AM +0100, SF Markus Elfring wrote: > diff --git a/drivers/staging/rdma/hfi1/file_ops.c > b/drivers/staging/rdma/hfi1/file_ops.c > index aae9826..204d1d0 100644 > --- a/drivers/staging/rdma/hfi1/file_ops.c > +++ b/drivers/staging/rdma/hfi1/file_ops.c > @@ -313,7 +313,7 @@ static ssize_t hfi1_file_write(struct file *fp, const > char __user *data, > case HFI1_CMD_SDMA_STATUS_UPD: > break; > case HFI1_CMD_CREDIT_UPD: > - if (uctxt && uctxt->sc) > + if (uctxt) > sc_return_credits(uctxt->sc); Relying on hidden sanity checks makes the code harder to read. 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: [PATCH] staging/rdma/hfi1: Disable thermal polling before sensor initialization
On Wed, Nov 04, 2015 at 11:44:17PM -0500, jubin.j...@intel.com wrote: > From: jareer.h.abdel-qader This should be "Jareer Abdel-Qader ". 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: [PATCH] staging/rdma/hfi1: Convert dd_dev_info() to hfi1_cdbg() in process startup
On Wed, Nov 04, 2015 at 11:14:57PM -0500, jubin.j...@intel.com wrote: > From: Sebastian Sanchez > > Replacing dd_dev_info() for hfi1_cdbg() to avoid generating syslog > output for every context that is open by PSM. > Just delete it... People get scared about deleting debug code but you can add it back if there is really a bug. /me chants, "delete. delete. delete." 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: [PATCH 3/3] staging/rdma/hfi1: Method to toggle "fast ECN" detection
On Wed, Nov 04, 2015 at 09:10:11PM -0500, ira.we...@intel.com wrote: > From: Vennila Megavannan > > Add a module paramter to toggle prescan/Fast ECN Detection. > > In addition change the PRESCAN_RXQ Kconfig default to "yes". > > Reviewed-by: Arthur Kepner > Reviewed-by: Mike Marciniszyn > Signed-off-by: Vennila Megavannan > Signed-off-by: Ira Weiny Hm... In the original code we had the config entry but the code to support this was actually disabled. Now we are turning on the config entry by default but the module parameter defaults to disabled. I think the documentation is a bit tricky because it should say that actually enabling the config is not enough, it's still disabled. Do we really need the config to be there? Distros are going to enable it. Who is it who wants to disable the config? Also is it better to default to off or on for this code, what are the upsides and downsides of that choice? 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: [PATCH] staging/rdma/hfi1: set Gen3 half-swing for integrated devices
On Wed, Nov 04, 2015 at 09:06:08PM -0500, ira.we...@intel.com wrote: > +#define CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK 0x1ull There is absolutely no point to having a variable name which is nine bajillion characters long since we are not able to use it and have to instead use macro majic to hide the first twelve zillion characters. If we can't even see the name when it is used, then why does it exist? Also macro magic breaks grep and cscope. Just think of a better name to begin with and get rid of the PC macro. 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: IB/core: Use GID table in AH creation and dmac resolution
Hello Matan Barak,
This is a semi-automatic email about new static checker warnings.
The patch dbf727de7440: "IB/core: Use GID table in AH creation and
dmac resolution" from Oct 15, 2015, leads to the following Smatch
complaint:
drivers/infiniband/hw/ocrdma/ocrdma_ah.c:157 ocrdma_create_ah()
error: we previously assumed 'sgid_attr.ndev' could be null (see line
146)
drivers/infiniband/hw/ocrdma/ocrdma_ah.c
145 }
146 if (sgid_attr.ndev) {
^^
Patch introduces a NULL check.
147 if (is_vlan_dev(sgid_attr.ndev))
148 vlan_tag = vlan_dev_vlan_id(sgid_attr.ndev);
149 dev_put(sgid_attr.ndev);
150 }
151
152 if ((pd->uctx) &&
153 (!rdma_is_multicast_addr((struct in6_addr
*)attr->grh.dgid.raw)) &&
154 (!rdma_link_local_addr((struct in6_addr
*)attr->grh.dgid.raw))) {
155 status = rdma_addr_find_dmac_by_grh(&sgid,
&attr->grh.dgid,
156 attr->dmac,
&vlan_tag,
157
sgid_attr.ndev->ifindex);
Patch introduces this new dereference. The warning might be a false
positive if "pd->uctx" or rdma_is_multicast_addr() imply it's non-NULL
but I don't know this code well enough to say for sure. Hence this
email. :)
158 if (status) {
159 pr_err("%s(): Failed to resolve dmac from gid."
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: [PATCH 1/4 v2] staging: ipath: ipath_driver: Use setup_timer
On Tue, Oct 27, 2015 at 11:45:18AM +0200, Leon Romanovsky wrote: > On Tue, Oct 27, 2015 at 11:19 AM, Dan Carpenter > wrote: > > On Sun, Oct 25, 2015 at 01:21:11PM +0200, Leon Romanovsky wrote: > >> On Sun, Oct 25, 2015 at 12:17 PM, Muhammad Falak R Wani > >> wrote: > >> Please follow standard naming convention for the patches. > >> It should be [PATCH v2 1/4] and not [PATCH 1/4 v2]. > > > > Does this matter? It's in a thread so it sorts fine either way. > It will be wise if people read guides and follow examples. > > [1] https://www.kernel.org/doc/Documentation/SubmittingPatches That document doesn't really specify one way or the other. And even if it did then why would you care? Stop being so picky for no reason. 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: [PATCH 1/4 v2] staging: ipath: ipath_driver: Use setup_timer
On Sun, Oct 25, 2015 at 01:21:11PM +0200, Leon Romanovsky wrote: > On Sun, Oct 25, 2015 at 12:17 PM, Muhammad Falak R Wani > wrote: > Please follow standard naming convention for the patches. > It should be [PATCH v2 1/4] and not [PATCH 1/4 v2]. Does this matter? It's in a thread so it sorts fine either way. 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: [PATCH v2 14/22] staging/rdma/hfi1: Implement Expected Receive TID caching
On Thu, Oct 22, 2015 at 07:18:19PM -0400, ira.weiny wrote:
> This follows the rest of the style of the case statement in this function. We
> prefer to leave this as is for a number of reasons.
>
> 1) This is consistent with the coding style elsewhere in this driver.
Don't try to match existing style if it is wrong. If 99 lines are
consistent and 1 line is correct style then at least that's better than
no lines being correct. I am worried that you will feel you have to do
this the wrong way forever for a silly reason...
> 2) It is functionally equivalent.
It is a style issue and I only complained about it because in the
next lines the bad style causes a bug. If anyone finds this kind of
info leak in released code, then we always give them a CVE for it btw.
It's a headache.
> 3) I have a long list of patches which need to be processed and this may cause
>later merge conflicts.
>
Yes, that's fine. I'm not insisting that you redo everything because of
a style issue.
Let me explain a little more why success handling is an anti-pattern.
Failure handling looks like this:
ret = one();
if (ret)
return ret;
ret = two();
if (ret)
goto undo_one;
ret = three();
if (ret)
goto undo_two;
return 0;
undo_two:
undo_two();
undo_one:
undo_one();
return ret;
In this example the success path is always at indent level one. The
code is a series of statements with no if conditions or indenting. This
is how most kernel code looks.
With success handling it looks like:
ret = one();
if (!ret) {
ret = two();
if (!ret)
ret = three();
}
return ret;
It is fewer lines but it is way more complicated. It very quickly
starts to bump into the 80 character limit.
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: [PATCH v2 15/22] staging/rdma/hfi1: Allow tuning of SDMA interrupt rate
What values work well for this parameter? 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: [PATCH v2 14/22] staging/rdma/hfi1: Implement Expected Receive TID caching
On Mon, Oct 19, 2015 at 10:11:29PM -0400, ira.we...@intel.com wrote:
> + case HFI1_CMD_TID_INVAL_READ:
> + ret = hfi1_user_exp_rcv_invalid(fp, &tinfo);
> + if (!ret) {
> + addr = (unsigned long)cmd.addr +
> + offsetof(struct hfi1_tid_info, tidcnt);
> + if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
> + sizeof(tinfo.tidcnt)))
> + ret = -EFAULT;
> + }
> + break;
This switch statement uses success handling throughtout instead of
error handling. It's better if you write it like this:
case HFI1_CMD_TID_INVAL_READ:
ret = hfi1_user_exp_rcv_invalid(fp, &tinfo);
if (ret)
break;
addr = (unsigned long)cmd.addr +
offsetof(struct hfi1_tid_info, tidcnt);
if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
sizeof(tinfo.tidcnt)))
ret = -EFAULT;
break;
The casting is sort of ugly... It would be better to make address a
pointer. It does cut down on the lines of code but at least the cast is
all done at once and really "addr" is actually a pointer.
case HFI1_CMD_TID_INVAL_READ:
ret = hfi1_user_exp_rcv_invalid(fp, &tinfo);
if (ret)
break;
addr = (void __user *)(unsigned long)cmd.addr +
offsetof(struct hfi1_tid_info, tidcnt);
if (copy_to_user(addr, &tinfo.tidcnt, sizeof(tinfo.tidcnt)))
ret = -EFAULT;
break;
> case HFI1_CMD_TID_FREE:
> - ret = exp_tid_free(fp, &tinfo);
> + ret = hfi1_user_exp_rcv_clear(fp, &tinfo);
> + addr = (unsigned long)cmd.addr +
> + offsetof(struct hfi1_tid_info, tidcnt);
> + if (copy_to_user((void __user *)addr, &tinfo.tidcnt,
> + sizeof(tinfo.tidcnt)))
> + ret = -EFAULT;
> break;
This is an information leak. We should break if
hfi1_user_exp_rcv_clear() fails, but instead we copy uninitialized
variables to the user.
> case HFI1_CMD_RECV_CTRL:
> ret = manage_rcvq(uctxt, subctxt_fp(fp), (int)user_val);
> @@ -607,9 +603,9 @@ static int hfi1_file_mmap(struct file *fp, struct
> vm_area_struct *vma)
>* Use the page where this context's flags are. User level
>* knows where it's own bitmap is within the page.
>*/
> - memaddr = ((unsigned long)dd->events +
> -((uctxt->ctxt - dd->first_user_ctxt) *
> - HFI1_MAX_SHARED_CTXTS)) & PAGE_MASK;
> + memaddr = (unsigned long)(dd->events +
> + ((uctxt->ctxt - dd->first_user_ctxt) *
> +HFI1_MAX_SHARED_CTXTS)) & PAGE_MASK;
I am too lazy to investigate the types of all these variables but I'm
instead going to assert that moving the cast to later is an unrelated
white space change. Don't mix white space changes in with a behavior
change patch because it makes it hard to review.
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: [PATCH v2 07/22] staging/rdma/hfi1: Fix sparse error in sdma.h file
On Wed, Oct 21, 2015 at 04:29:35PM +, Weiny, Ira wrote: > > > > On Mon, Oct 19, 2015 at 10:11:22PM -0400, ira.we...@intel.com wrote: > > > From: Niranjana Vishwanathapura > > > > > > Use NULL instead of 0 for pointer argument to fix the sparse error. > > > > > > Reviewed-by: Mike Marciniszyn > > > Reviewed-by: Mitko Haralanov > > > Reviewed-by: Dennis Dalessandro > > > Signed-off-by: Niranjana Vishwanathapura > > > > > > Signed-off-by: Ira Weiny > > > > This should have just been folded in with the previous patch. Don't > > introduce > > problems and fix them in the patchset. > > My bad, I will fix in v3. I don't think it's worth redoing the patchset over because it's just a Sparse warning, not a bug. But for the future. 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: [PATCH v2 07/22] staging/rdma/hfi1: Fix sparse error in sdma.h file
On Mon, Oct 19, 2015 at 10:11:22PM -0400, ira.we...@intel.com wrote: > From: Niranjana Vishwanathapura > > Use NULL instead of 0 for pointer argument to fix the sparse error. > > Reviewed-by: Mike Marciniszyn > Reviewed-by: Mitko Haralanov > Reviewed-by: Dennis Dalessandro > Signed-off-by: Niranjana Vishwanathapura > Signed-off-by: Ira Weiny This should have just been folded in with the previous patch. Don't introduce problems and fix them in the patchset. 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: [PATCH v2 01/22] staging/rdma/hfi1: Fix regression in send performance
On Mon, Oct 19, 2015 at 10:11:16PM -0400, ira.we...@intel.com wrote: > From: Mike Marciniszyn > > This additional call is a regression from qib. For small messages the > progress > routine always builds one and clears out the ahg state when the queue has gone > to empty which is the predominant case for small messages. > > Inline the routine to mitigate the call and move the routine to qp.h for scope > reasons. > > Reviewed-by: Dennis Dalessandro > Signed-off-by: Mike Marciniszyn > Signed-off-by: Ira Weiny > --- The whole point of this patch is to do: > - if (qp->s_sde) > + if (qp->s_sde && qp->s_ahgidx >= 0) It was not at all clear from the changelog and it was difficult to tell because we moved code around as well. 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
[patch] IB/hfi1: clean up some defines
I added spaces around operators so it matches kernel style because normally "-1ULL" is a number and " - 1" is a subtract operation. Also removed some superflous "ULL" types so "1ULL" becomes "1". Signed-off-by: Dan Carpenter diff --git a/drivers/staging/rdma/hfi1/sdma.h b/drivers/staging/rdma/hfi1/sdma.h index 1e613fc..4960869 100644 --- a/drivers/staging/rdma/hfi1/sdma.h +++ b/drivers/staging/rdma/hfi1/sdma.h @@ -109,53 +109,53 @@ /* * Bits defined in the send DMA descriptor. */ -#define SDMA_DESC0_FIRST_DESC_FLAG (1ULL<<63) -#define SDMA_DESC0_LAST_DESC_FLAG (1ULL<<62) +#define SDMA_DESC0_FIRST_DESC_FLAG (1ULL << 63) +#define SDMA_DESC0_LAST_DESC_FLAG (1ULL << 62) #define SDMA_DESC0_BYTE_COUNT_SHIFT 48 #define SDMA_DESC0_BYTE_COUNT_WIDTH 14 #define SDMA_DESC0_BYTE_COUNT_MASK \ - ((1ULL<http://vger.kernel.org/majordomo-info.html
[patch] IB/hfi1: mask vs shift confusion
We are shifting by the _MASK macros instead of the _SHIFT ones. Signed-off-by: Dan Carpenter diff --git a/drivers/staging/rdma/hfi1/sdma.c b/drivers/staging/rdma/hfi1/sdma.c index a8c903c..3a457d2 100644 --- a/drivers/staging/rdma/hfi1/sdma.c +++ b/drivers/staging/rdma/hfi1/sdma.c @@ -1848,7 +1848,7 @@ static void dump_sdma_state(struct sdma_engine *sde) dd_dev_err(sde->dd, "\taidx: %u amode: %u alen: %u\n", (u8)((desc[1] & SDMA_DESC1_HEADER_INDEX_SMASK) - >> SDMA_DESC1_HEADER_INDEX_MASK), + >> SDMA_DESC1_HEADER_INDEX_SHIFT), (u8)((desc[1] & SDMA_DESC1_HEADER_MODE_SMASK) >> SDMA_DESC1_HEADER_MODE_SHIFT), (u8)((desc[1] & SDMA_DESC1_HEADER_DWS_SMASK) @@ -1926,7 +1926,7 @@ void sdma_seqfile_dump_sde(struct seq_file *s, struct sdma_engine *sde) if (desc[0] & SDMA_DESC0_FIRST_DESC_FLAG) seq_printf(s, "\t\tahgidx: %u ahgmode: %u\n", (u8)((desc[1] & SDMA_DESC1_HEADER_INDEX_SMASK) - >> SDMA_DESC1_HEADER_INDEX_MASK), + >> SDMA_DESC1_HEADER_INDEX_SHIFT), (u8)((desc[1] & SDMA_DESC1_HEADER_MODE_SMASK) >> SDMA_DESC1_HEADER_MODE_SHIFT)); head = (head + 1) & sde->sdma_mask; -- 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: IB/cma: Fix net_dev reference leak with failed requests
Hello Haggai Eran,
The patch be688195bd08: "IB/cma: Fix net_dev reference leak with
failed requests" from Aug 27, 2015, leads to the following static
checker warning:
drivers/infiniband/core/cma.c:1306 cma_id_from_event()
error: potential NULL dereference '*net_dev'.
drivers/infiniband/core/cma.c
1279 static struct rdma_id_private *cma_id_from_event(struct ib_cm_id *cm_id,
1280 struct ib_cm_event
*ib_event,
1281 struct net_device
**net_dev)
1282 {
1283 struct cma_req_info req;
1284 struct rdma_bind_list *bind_list;
1285 struct rdma_id_private *id_priv;
1286 int err;
1287
1288 err = cma_save_req_info(ib_event, &req);
1289 if (err)
1290 return ERR_PTR(err);
1291
1292 *net_dev = cma_get_net_dev(ib_event, &req);
1293 if (IS_ERR(*net_dev)) {
1294 if (PTR_ERR(*net_dev) == -EAFNOSUPPORT) {
1295 /* Assuming the protocol is AF_IB */
1296 *net_dev = NULL;
Set to NULL here.
1297 } else {
1298 return ERR_CAST(*net_dev);
1299 }
1300 }
1301
1302 bind_list = cma_ps_find(rdma_ps_from_service_id(req.service_id),
1303
cma_port_from_service_id(req.service_id));
1304 id_priv = cma_find_listener(bind_list, cm_id, ib_event, &req,
*net_dev);
1305 if (IS_ERR(id_priv)) {
1306 dev_put(*net_dev);
Dereferenced inside function call. This warning is from a work in
progress Smatch check (unpublished).
1307 *net_dev = NULL;
1308 }
1309
1310 return id_priv;
1311 }
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: IB/hfi1: add driver files
Hello Mike Marciniszyn,
The patch 7724105686e7: "IB/hfi1: add driver files" from Jul 30,
2015, leads to the following static checker warning:
drivers/staging/rdma/hfi1/rc.c:2399 hfi1_rc_hdrerr()
warn: right shift assign to zero
drivers/staging/rdma/hfi1/rc.c
2376 void hfi1_rc_hdrerr(
2377 struct hfi1_ctxtdata *rcd,
2378 struct hfi1_ib_header *hdr,
2379 u32 rcv_flags,
2380 struct hfi1_qp *qp)
2381 {
2382 int has_grh = rcv_flags & HFI1_HAS_GRH;
2383 struct hfi1_other_headers *ohdr;
2384 struct hfi1_ibport *ibp = to_iport(qp->ibqp.device,
qp->port_num);
2385 int diff;
2386 u8 opcode;
2387 u32 psn;
2388
2389 /* Check for GRH */
2390 ohdr = &hdr->u.oth;
2391 if (has_grh)
2392 ohdr = &hdr->u.l.oth;
2393
2394 opcode = be32_to_cpu(ohdr->bth[0]);
2395 if (hfi1_ruc_check_hdr(ibp, hdr, has_grh, qp, opcode))
2396 return;
2397
2398 psn = be32_to_cpu(ohdr->bth[2]);
2399 opcode >>= 24;
2400
opcode should probably be a u32 instead of a u8.
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: IB/hfi1: add driver files
Hello Mike Marciniszyn,
The patch 7724105686e7: "IB/hfi1: add driver files" from Jul 30,
2015, leads to the following static checker warning:
drivers/staging/rdma/hfi1/user_sdma.c:1349 set_txreq_header_ahg()
warn: mask and shift to zero
drivers/staging/rdma/hfi1/user_sdma.c
1347 /* Clear KDETH.SH on last packet */
1348 if (unlikely(tx->flags &
USER_SDMA_TXREQ_FLAGS_LAST_PKT)) {
1349 val |=
cpu_to_le16(KDETH_GET(hdr->kdeth.ver_tid_offset,
1350 INTR)
>> 16);
KDETH_GET(hdr->kdeth.ver_tid_offset, INTR) is zero or one. 1 >> 16 is
zero. This line is a no-op.
1351 val &= cpu_to_le16(~(1U << 13));
1352 AHG_HEADER_SET(req->ahg, diff, 7, 16, 14, val);
1353 } else
1354 AHG_HEADER_SET(req->ahg, diff, 7, 16, 12, val);
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
[patch v2] IB/hfi1: info leak in get_ctxt_info()
The cinfo struct has a hole after the last struct member so we need to zero it out. Otherwise we disclose some uninitialized stack data. Signed-off-by: Dan Carpenter --- v2: typo in changelog diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c index 4698617..2c43ca5 100644 --- a/drivers/staging/rdma/hfi1/file_ops.c +++ b/drivers/staging/rdma/hfi1/file_ops.c @@ -1181,6 +1181,7 @@ static int get_ctxt_info(struct file *fp, void __user *ubase, __u32 len) struct hfi1_filedata *fd = fp->private_data; int ret = 0; + memset(&cinfo, 0, sizeof(cinfo)); ret = hfi1_get_base_kinfo(uctxt, &cinfo); if (ret < 0) goto done; -- 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: [patch] IB/hfi1: info leak in get_ctxt_info()
On Wed, Sep 16, 2015 at 08:25:00AM +0200, Julia Lawall wrote: > On Wed, 16 Sep 2015, Dan Carpenter wrote: > > > The cinfo struct has a hole after the last struct member so we need to > > zero it out. Otherwise we don't disclose some uninitialized stack data. > > I think the "don't" wasn't intended in the second sentence? > Derp... I will resend. 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: IB/ipath: infiniband verbs support
Hello Bryan O'Sullivan,
The patch 6522108f19a9: "IB/ipath: infiniband verbs support" from Mar
29, 2006, leads to the following static checker warning:
drivers/staging/rdma/ipath/ipath_verbs.c:2289 show_hca()
warn: bool is not less than zero.
drivers/staging/rdma/ipath/ipath_verbs.c
2281 static ssize_t show_hca(struct device *device, struct device_attribute
*attr,
2282 char *buf)
2283 {
2284 struct ipath_ibdev *dev =
2285 container_of(device, struct ipath_ibdev, ibdev.dev);
2286 int ret;
2287
2288 ret = dev->dd->ipath_f_get_boardname(dev->dd, buf, 128);
2289 if (ret < 0)
ret is either zero or one, not negative. There is some dead code in
ipath_ht_boardname() which indicates that it might have returned error
codes at some point as well.
This warning is from a too many false positives to publish Smatch check.
2290 goto bail;
2291 strcat(buf, "\n");
2292 ret = strlen(buf);
2293
2294 bail:
2295 return ret;
2296 }
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
[patch] IB/hfi1: fix a locking bug
mutex_trylock() returns zero on failure, not EBUSY.
Signed-off-by: Dan Carpenter
diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index 654eafe..aa58e59 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -2710,7 +2710,7 @@ int acquire_lcb_access(struct hfi1_devdata *dd, int
sleep_ok)
if (sleep_ok) {
mutex_lock(&ppd->hls_lock);
} else {
- while (mutex_trylock(&ppd->hls_lock) == EBUSY)
+ while (!mutex_trylock(&ppd->hls_lock))
udelay(1);
}
@@ -2758,7 +2758,7 @@ int release_lcb_access(struct hfi1_devdata *dd, int
sleep_ok)
if (sleep_ok) {
mutex_lock(&dd->pport->hls_lock);
} else {
- while (mutex_trylock(&dd->pport->hls_lock) == EBUSY)
+ while (!mutex_trylock(&dd->pport->hls_lock))
udelay(1);
}
--
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
[patch] IB/hfi1: info leak in get_ctxt_info()
The cinfo struct has a hole after the last struct member so we need to zero it out. Otherwise we don't disclose some uninitialized stack data. Signed-off-by: Dan Carpenter diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c index 4698617..2c43ca5 100644 --- a/drivers/staging/rdma/hfi1/file_ops.c +++ b/drivers/staging/rdma/hfi1/file_ops.c @@ -1181,6 +1181,7 @@ static int get_ctxt_info(struct file *fp, void __user *ubase, __u32 len) struct hfi1_filedata *fd = fp->private_data; int ret = 0; + memset(&cinfo, 0, sizeof(cinfo)); ret = hfi1_get_base_kinfo(uctxt, &cinfo); if (ret < 0) goto done; -- 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
[patch] IB/hfi1: checking for NULL instead of IS_ERR
__get_txreq() returns an ERR_PTR() but this checks for NULL so it would
oops on failure.
Signed-off-by: Dan Carpenter
diff --git a/drivers/staging/rdma/hfi1/verbs.c
b/drivers/staging/rdma/hfi1/verbs.c
index 53ac214..41bb59e 100644
--- a/drivers/staging/rdma/hfi1/verbs.c
+++ b/drivers/staging/rdma/hfi1/verbs.c
@@ -749,11 +749,13 @@ static inline struct verbs_txreq *get_txreq(struct
hfi1_ibdev *dev,
struct verbs_txreq *tx;
tx = kmem_cache_alloc(dev->verbs_txreq_cache, GFP_ATOMIC);
- if (!tx)
+ if (!tx) {
/* call slow path to get the lock */
tx = __get_txreq(dev, qp);
- if (tx)
- tx->qp = qp;
+ if (IS_ERR(tx))
+ return tx;
+ }
+ tx->qp = qp;
return tx;
}
--
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
[patch] IB/hfi1: fix copy_to/from_user() error handling
copy_to/from_user() returns the number of bytes which we were not able
to copy. It doesn't return an error code.
Also a couple places had a printk() on error and I removed that because
people can take advantage of it to fill /var/log/messages with spam.
Signed-off-by: Dan Carpenter
---
This patch has some checkpatch warnings because the alignment is off.
That was a problem in the original code and needs to be fixed in a separate
patch.
Also there is another bug:
drivers/staging/rdma/hfi1/diag.c:1160 hfi1_ioctl() error: scheduling with locks
held: 'spin_lock:snoop_lock'
That function needs to be re-worked and simplified and technically I'm
still on vacation so I didn't address it.
diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c
index 6777d6b..ce01dee 100644
--- a/drivers/staging/rdma/hfi1/diag.c
+++ b/drivers/staging/rdma/hfi1/diag.c
@@ -1012,11 +1012,10 @@ static long hfi1_ioctl(struct file *fp, unsigned int
cmd, unsigned long arg)
case HFI1_SNOOP_IOCSETLINKSTATE_EXTRA:
memset(&link_info, 0, sizeof(link_info));
- ret = copy_from_user(&link_info,
+ if (copy_from_user(&link_info,
(struct hfi1_link_info __user *)arg,
- sizeof(link_info));
- if (ret)
- break;
+ sizeof(link_info)))
+ ret = -EFAULT;
value = link_info.port_state;
index = link_info.port_number;
@@ -1080,9 +1079,10 @@ static long hfi1_ioctl(struct file *fp, unsigned int
cmd, unsigned long arg)
case HFI1_SNOOP_IOCGETLINKSTATE_EXTRA:
if (cmd == HFI1_SNOOP_IOCGETLINKSTATE_EXTRA) {
memset(&link_info, 0, sizeof(link_info));
- ret = copy_from_user(&link_info,
+ if (copy_from_user(&link_info,
(struct hfi1_link_info __user *)arg,
- sizeof(link_info));
+ sizeof(link_info)))
+ ret = -EFAULT;
index = link_info.port_number;
} else {
ret = __get_user(index, (int __user *) arg);
@@ -1114,9 +1114,10 @@ static long hfi1_ioctl(struct file *fp, unsigned int
cmd, unsigned long arg)
ppd->link_speed_active;
link_info.link_width_active =
ppd->link_width_active;
- ret = copy_to_user(
+ if (copy_to_user(
(struct hfi1_link_info __user *)arg,
- &link_info, sizeof(link_info));
+ &link_info, sizeof(link_info)))
+ ret = -EFAULT;
} else {
ret = __put_user(value, (int __user *)arg);
}
@@ -1142,10 +1143,9 @@ static long hfi1_ioctl(struct file *fp, unsigned int
cmd, unsigned long arg)
snoop_dbg("Setting filter");
/* just copy command structure */
argp = (unsigned long *)arg;
- ret = copy_from_user(&filter_cmd, (void __user *)argp,
-sizeof(filter_cmd));
- if (ret < 0) {
- pr_alert("Error copying filter command\n");
+ if (copy_from_user(&filter_cmd, (void __user *)argp,
+sizeof(filter_cmd))) {
+ ret = -EFAULT;
break;
}
if (filter_cmd.opcode >= HFI1_MAX_FILTERS) {
@@ -1167,12 +1167,11 @@ static long hfi1_ioctl(struct file *fp, unsigned int
cmd, unsigned long arg)
break;
}
/* copy remaining data from userspace */
- ret = copy_from_user((u8 *)filter_value,
+ if (copy_from_user((u8 *)filter_value,
(void __user *)filter_cmd.value_ptr,
- filter_cmd.length);
- if (ret < 0) {
+ filter_cmd.length)) {
kfree(filter_value);
- pr_alert("Error copy
[patch] IB/core: off by one in error handling
This is a zero offset array. The current code could try to free random
memory and crash. Also it leaks the first element.
Fixes: 230145ff8124 ('IB/core: Add RoCE GID table management')
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index a9d5c70..f5d14a7 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -582,7 +582,7 @@ static int _gid_table_setup_one(struct ib_device *ib_dev)
return 0;
rollback_table_setup:
- for (port = 1; port <= ib_dev->phys_port_cnt; port++)
+ for (port = 0; port < ib_dev->phys_port_cnt; port++)
free_gid_table(ib_dev, port, table[port]);
kfree(table);
--
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
[patch] IB/core: missing curly braces in ib_find_gid()
Smatch says that, based on the indenting, we should probably add curly
braces here.
Fixes: 230145ff8124 ('IB/core: Add RoCE GID table management')
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 258b3f7..5d5bbae 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -807,9 +807,10 @@ int ib_find_gid(struct ib_device *device, union ib_gid
*gid,
for (port = rdma_start_port(device); port <= rdma_end_port(device);
++port) {
if (rdma_cap_roce_gid_table(device, port)) {
if (!ib_cache_gid_find_by_port(device, gid, port,
- NULL, index))
+ NULL, index)) {
*port_num = port;
return 0;
+ }
}
for (i = 0; i < device->port_immutable[port].gid_tbl_len; ++i) {
--
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: IB/mlx5: Support IB_WR_REG_SIG_MR
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:
drivers/infiniband/ulp/ipoib/ipoib_cm.c:764 ipoib_cm_send()
warn: 'rc' can be either negative or positive
drivers/infiniband/hw/mlx5/qp.c
2417 static int set_psv_wr(struct ib_sig_domain *domain,
2418u32 psv_idx, void **seg, int *size)
2419 {
2420 struct mlx5_seg_set_psv *psv_seg = *seg;
2421
2422 memset(psv_seg, 0, sizeof(*psv_seg));
2423 psv_seg->psv_num = cpu_to_be32(psv_idx);
2424 switch (domain->sig_type) {
2425 case IB_SIG_TYPE_NONE:
2426 break;
2427 case IB_SIG_TYPE_T10_DIF:
2428 psv_seg->transient_sig = cpu_to_be32(domain->sig.dif.bg
<< 16 |
2429
domain->sig.dif.app_tag);
2430 psv_seg->ref_tag = cpu_to_be32(domain->sig.dif.ref_tag);
2431 break;
2432 default:
2433 pr_err("Bad signature type given.\n");
2434 return 1;
Probably we should return -EINVAL here? It actually really upsets the
static checker because all the callers expect negative error codes and
there are lot of callers.
2435 }
2436
2437 *seg += sizeof(*psv_seg);
2438 *size += sizeof(*psv_seg) / 16;
2439
2440 return 0;
2441 }
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: net/mlx5e: Allocate DMA coherent memory on reader NUMA node
[ You didn't introduce this, you just renamed the function so now it
shows up as a new warning. - dan ]
Hello Saeed Mahameed,
The patch 311c7c71c9bb: "net/mlx5e: Allocate DMA coherent memory on
reader NUMA node" from Jul 23, 2015, leads to the following static
checker warning:
drivers/net/ethernet/mellanox/mlx5/core/alloc.c:156 mlx5_db_alloc_node()
warn: missing error code here? 'mlx5_alloc_db_from_pgdir()' failed.
'ret' = '0'
drivers/net/ethernet/mellanox/mlx5/core/alloc.c
147 int mlx5_db_alloc_node(struct mlx5_core_dev *dev, struct mlx5_db *db,
int node)
148 {
149 struct mlx5_db_pgdir *pgdir;
150 int ret = 0;
151
152 mutex_lock(&dev->priv.pgdir_mutex);
153
154 list_for_each_entry(pgdir, &dev->priv.pgdir_list, list)
155 if (!mlx5_alloc_db_from_pgdir(pgdir, db))
156 goto out;
There should probably be a "ret = -ENOMEM;" here.
157
158 pgdir = mlx5_alloc_db_pgdir(dev, node);
159 if (!pgdir) {
160 ret = -ENOMEM;
161 goto out;
162 }
163
164 list_add(&pgdir->list, &dev->priv.pgdir_list);
165
166 /* This should never fail -- we just allocated an empty page: */
167 WARN_ON(mlx5_alloc_db_from_pgdir(pgdir, db));
168
169 out:
170 mutex_unlock(&dev->priv.pgdir_mutex);
171
172 return ret;
173 }
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
[patch -next] net/mlx5_core: fix an error code
We return success if mlx5e_alloc_sq_db() fails but we should return an
error code.
Fixes: f62b8bb8f2d3 ('net/mlx5: Extend mlx5_core to support ConnectX-4 Ethernet
functionality')
Signed-off-by: Dan Carpenter
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 7348c51..075e517 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -525,7 +525,8 @@ static int mlx5e_create_sq(struct mlx5e_channel *c,
sq->uar_map = sq->uar.map;
sq->bf_buf_size = (1 << MLX5_CAP_GEN(mdev, log_bf_reg_size)) / 2;
- if (mlx5e_alloc_sq_db(sq, cpu_to_node(c->cpu)))
+ err = mlx5e_alloc_sq_db(sq, cpu_to_node(c->cpu));
+ if (err)
goto err_sq_wq_destroy;
sq->txq = netdev_get_tx_queue(priv->netdev,
--
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
[patch] IB/usnic: clean up some error handling code
This code causes a static checker warning:
drivers/infiniband/hw/usnic/usnic_uiom.c:476 usnic_uiom_alloc_pd()
warn: passing zero to 'PTR_ERR'
This code isn't buggy, but iommu_domain_alloc() doesn't return an error
pointer so we can simplify the error handling and silence the static
checker warning.
The static checker warning is to catch place which do:
if (!ptr)
return ERR_PTR(ptr);
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c
b/drivers/infiniband/hw/usnic/usnic_uiom.c
index 417de1f..cb2337f 100644
--- a/drivers/infiniband/hw/usnic/usnic_uiom.c
+++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
@@ -472,11 +472,10 @@ struct usnic_uiom_pd *usnic_uiom_alloc_pd(void)
return ERR_PTR(-ENOMEM);
pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
- if (IS_ERR_OR_NULL(domain)) {
- usnic_err("Failed to allocate IOMMU domain with err %ld\n",
- PTR_ERR(pd->domain));
+ if (!domain) {
+ usnic_err("Failed to allocate IOMMU domain");
kfree(pd);
- return ERR_PTR(domain ? PTR_ERR(domain) : -ENOMEM);
+ return ERR_PTR(-ENOMEM);
}
iommu_set_fault_handler(pd->domain, usnic_uiom_dma_fault, NULL);
--
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: net/mlx5: Ethernet resource handling files
Hello Amir Vadai,
The patch afb736e9330a: "net/mlx5: Ethernet resource handling files"
from May 28, 2015, leads to the following static checker warning:
drivers/net/ethernet/mellanox/mlx5/core/en_flow_table.c:726
mlx5e_create_main_flow_table()
error: potential null dereference 'g'. (kcalloc returns null)
drivers/net/ethernet/mellanox/mlx5/core/en_flow_table.c
719 static int mlx5e_create_main_flow_table(struct mlx5e_priv *priv)
720 {
721 struct mlx5_flow_table_group *g;
722 u8 *dmac;
723
724 g = kcalloc(9, sizeof(*g), GFP_KERNEL);
It should check for allocation failure.
725
726 g[0].log_sz = 2;
727 g[0].match_criteria_enable = MLX5_MATCH_OUTER_HEADERS;
728 MLX5_SET_TO_ONES(fte_match_param, g[0].match_criteria,
729 outer_headers.ethertype);
730 MLX5_SET_TO_ONES(fte_match_param, g[0].match_criteria,
731 outer_headers.ip_protocol);
732
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
[patch] IB/ipath: remove some left over code
"ret" is always zero here after commit c4bce8032ef4 ('IB/ipath: Add new
chip-specific functions to older chips, consistent init').
Signed-off-by: Dan Carpenter
---
I just happened to spot this dead code when I was reviewing a different
static checker warning:
drivers/infiniband/hw/ipath/ipath_verbs.c:2266 show_hca()
warn: bool is not less than zero.
It looks to me that this code returns 0 on success and 1 on failure so
the warning is correct. This is very strange. The documentation
doesn't say anything about what the return codes mean so it is all very
puzzling.
diff --git a/drivers/infiniband/hw/ipath/ipath_iba6110.c
b/drivers/infiniband/hw/ipath/ipath_iba6110.c
index 7cc3054..3de90e9 100644
--- a/drivers/infiniband/hw/ipath/ipath_iba6110.c
+++ b/drivers/infiniband/hw/ipath/ipath_iba6110.c
@@ -721,10 +721,6 @@ static int ipath_ht_boardname(struct ipath_devdata *dd,
char *name,
if (n)
snprintf(name, namelen, "%s", n);
- if (ret) {
- ipath_dev_err(dd, "Unsupported InfiniPath board %s!\n", name);
- goto bail;
- }
if (dd->ipath_majrev != 3 || (dd->ipath_minrev < 2 ||
dd->ipath_minrev > 4)) {
/*
--
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: RDMA/ocrdma: Add driver for Emulex OneConnect IBoE RDMA adapter
Hello Parav Pandit,
The patch fe2caefcdf58: "RDMA/ocrdma: Add driver for Emulex
OneConnect IBoE RDMA adapter" from Mar 21, 2012, leads to the
following static checker warning:
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:1426 _ocrdma_modify_qp()
warn: bool is not less than zero.
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c
1411 int _ocrdma_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr,
1412int attr_mask)
1413 {
1414 int status = 0;
1415 struct ocrdma_qp *qp;
1416 struct ocrdma_dev *dev;
1417 enum ib_qp_state old_qps;
1418
1419 qp = get_ocrdma_qp(ibqp);
1420 dev = get_ocrdma_dev(ibqp->device);
1421 if (attr_mask & IB_QP_STATE)
1422 status = ocrdma_qp_state_change(qp, attr->qp_state,
&old_qps);
1423 /* if new and previous states are same hw doesn't need to
1424 * know about it.
1425 */
1426 if (status < 0)
This check is never true. Based on the comment then the check should
be:
if (status == 1)
return SOMETHING;
1427 return status;
1428 status = ocrdma_mbx_modify_qp(dev, qp, attr, attr_mask);
1429
1430 return status;
1431 }
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
[patch] RDMA/nes: remove a stray indent
This line was indented two tabs too far so it looks weird.
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/nes/nes_nic.c
b/drivers/infiniband/hw/nes/nes_nic.c
index 70acda9..1f74ae2 100644
--- a/drivers/infiniband/hw/nes/nes_nic.c
+++ b/drivers/infiniband/hw/nes/nes_nic.c
@@ -929,7 +929,7 @@ static void nes_netdev_set_multicast_list(struct net_device
*netdev)
nesadapter->pft_mcast_map[mc_index] !=
nesvnic->nic_index &&
mc_index < max_pft_entries_avaiable) {
- nes_debug(NES_DBG_NIC_RX,
+ nes_debug(NES_DBG_NIC_RX,
"mc_index=%d skipping nic_index=%d, "
"used for=%d \n", mc_index,
nesvnic->nic_index,
--
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
[patch] IB/qib: fix a do nothing for loop
We accidentally deleted a semi-colon here and changed this do-nothing
loop into a do-something loop.
Fixes: ff93905fb92d ('IB/qib: Fix checkpatch warnings')
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/qib/qib_wc_x86_64.c
b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
index 4fcf2459..81b225f 100644
--- a/drivers/infiniband/hw/qib/qib_wc_x86_64.c
+++ b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
@@ -92,7 +92,7 @@ int qib_enable_wc(struct qib_devdata *dd)
}
for (bits = 0; !(piolen & (1ULL << bits)); bits++)
- /* do nothing */
+ ; /* do nothing */
if (piolen != (1ULL << bits)) {
piolen >>= bits;
--
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
[patch] RDMA/ocrdma: off by one in ocrdma_query_gid()
The ->sgid_tbl[] array has OCRDMA_MAX_SGID number of elements so this test is off by one. ->sgid_tbl is allocated in ocrdma_alloc_resources(). Signed-off-by: Dan Carpenter diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c index fb8d8c4..18ea619 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c @@ -53,7 +53,7 @@ int ocrdma_query_gid(struct ib_device *ibdev, u8 port, dev = get_ocrdma_dev(ibdev); memset(sgid, 0, sizeof(*sgid)); - if (index > OCRDMA_MAX_SGID) + if (index >= OCRDMA_MAX_SGID) return -EINVAL; memcpy(sgid, &dev->sgid_tbl[index], sizeof(*sgid)); -- 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
[patch v2] IB/mlx5: fix error code in get_port_caps()
The current code returns success when kmalloc() fails. It should return
an error code, -ENOMEM.
Fixes: e126ba97dba9 ('mlx5: Add driver for Mellanox Connect-IB adapters')
Signed-off-by: Dan Carpenter
---
v2: re-write changelog
diff --git a/drivers/infiniband/hw/mlx5/main.c
b/drivers/infiniband/hw/mlx5/main.c
index 8a87404..cc4ac1e 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -997,7 +997,7 @@ static int get_port_caps(struct mlx5_ib_dev *dev)
struct ib_device_attr *dprops = NULL;
struct ib_port_attr *pprops = NULL;
struct mlx5_general_caps *gen;
- int err = 0;
+ int err = -ENOMEM;
int port;
gen = &dev->mdev->caps.gen;
--
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
[patch] IB/mlx5: fix error code in get_port_caps()
We should return -ENOMEM on allocation failure instead of success.
Fixes: e126ba97dba9 ('mlx5: Add driver for Mellanox Connect-IB adapters')
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/mlx5/main.c
b/drivers/infiniband/hw/mlx5/main.c
index 8a87404..cc4ac1e 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -997,7 +997,7 @@ static int get_port_caps(struct mlx5_ib_dev *dev)
struct ib_device_attr *dprops = NULL;
struct ib_port_attr *pprops = NULL;
struct mlx5_general_caps *gen;
- int err = 0;
+ int err = -ENOMEM;
int port;
gen = &dev->mdev->caps.gen;
--
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_core: Fix PowerPC support
Ah. Thanks for the explanation. This was on x86. 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_core: Fix PowerPC support
Hello Eli Cohen,
The patch 05bdb2ab6b09: "mlx5_core: Fix PowerPC support" from Jan 14,
2014, leads to the following static checker warning:
drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c:225 free_4k()
warn: right shifting to zero
drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
214 static void free_4k(struct mlx5_core_dev *dev, u64 addr)
215 {
216 struct fw_page *fwp;
217 int n;
218
219 fwp = find_fw_page(dev, addr & PAGE_MASK);
220 if (!fwp) {
221 mlx5_core_warn(dev, "page not found\n");
222 return;
223 }
224
225 n = (addr & ~PAGE_MASK) >> MLX5_ADAPTER_PAGE_SHIFT;
Maybe n = addr >> MLX5_ADAPTER_PAGE_SHIFT; was intended? (Totally
random guess).
226 fwp->free_count++;
227 set_bit(n, &fwp->bitmask);
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: IB/srp: Fix deadlock between host removal and multipathd
Hello Bart Van Assche,
The patch bcc059103591: "IB/srp: Fix deadlock between host removal
and multipathd" from Jul 9, 2014, leads to the following static
checker warning:
drivers/infiniband/ulp/srp/ib_srp.c:3323 srp_init_module()
warn: 'srp_remove_wq' isn't an ERR_PTR
drivers/infiniband/ulp/srp/ib_srp.c
3321
3322 srp_remove_wq = create_workqueue("srp_remove");
Returns NULL on failure and not an ERR_PTR.
3323 if (IS_ERR(srp_remove_wq)) {
3324 ret = PTR_ERR(srp_remove_wq);
3325 goto out;
3326 }
3327
3328 ret = -ENOMEM;
3329 ib_srp_transport_template =
3330 srp_attach_transport(&ib_srp_transport_functions);
3331 if (!ib_srp_transport_template)
3332 goto destroy_wq;
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
[patch] RDMA/amso1100: integer overflow in c2_alloc_cq_buf()
This is a static checker fix. The static checker says that q_size comes
from the user and can be any 32 bit value. The call tree is:
--> ib_uverbs_create_cq()
--> c2_create_cq()
--> c2_init_cq()
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/amso1100/c2_cq.c
b/drivers/infiniband/hw/amso1100/c2_cq.c
index 49e0e85..1b63185 100644
--- a/drivers/infiniband/hw/amso1100/c2_cq.c
+++ b/drivers/infiniband/hw/amso1100/c2_cq.c
@@ -260,11 +260,14 @@ static void c2_free_cq_buf(struct c2_dev *c2dev, struct
c2_mq *mq)
mq->msg_pool.host, dma_unmap_addr(mq, mapping));
}
-static int c2_alloc_cq_buf(struct c2_dev *c2dev, struct c2_mq *mq, int q_size,
- int msg_size)
+static int c2_alloc_cq_buf(struct c2_dev *c2dev, struct c2_mq *mq,
+ size_t q_size, size_t msg_size)
{
u8 *pool_start;
+ if (q_size > SIZE_MAX / msg_size)
+ return -EINVAL;
+
pool_start = dma_alloc_coherent(&c2dev->pcidev->dev, q_size * msg_size,
&mq->host_dma, GFP_KERNEL);
if (!pool_start)
--
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: IB/ipath: sysfs and ipathfs support for core driver
Hello Bryan O'Sullivan,
This is a semi-automatic email about new static checker warnings.
The patch 3e9b4a5eb4ae: "IB/ipath: sysfs and ipathfs support for core
driver" from Mar 29, 2006, leads to the following Smatch complaint:
drivers/infiniband/hw/ipath/ipath_fs.c:285 remove_file()
error: we previously assumed 'tmp->d_inode' could be null (see line
281)
drivers/infiniband/hw/ipath/ipath_fs.c
280 spin_lock(&tmp->d_lock);
281 if (!(d_unhashed(tmp) && tmp->d_inode)) {
We allow that ->d_inode to be NULL.
282 dget_dlock(tmp);
283 __d_drop(tmp);
284 spin_unlock(&tmp->d_lock);
285 simple_unlink(parent->d_inode, tmp);
^^^
But we dereference it inside the call to simple_unlink(). This code is
probably cut-and-pasted from configfs because I reported a bug in that
code as well yesterday but haven't heard back.
286 } else
287 spin_unlock(&tmp->d_lock);
drivers/infiniband/hw/qib/qib_fs.c
458 spin_lock(&tmp->d_lock);
459 if (!(d_unhashed(tmp) && tmp->d_inode)) {
460 __d_drop(tmp);
461 spin_unlock(&tmp->d_lock);
462 simple_unlink(parent->d_inode, tmp);
^^^
And another one here in qib_fs.c.
463 } else {
464 spin_unlock(&tmp->d_lock);
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,
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: IB/mlx4: Implement IB_QP_CREATE_USE_GFP_NOIO
Hello Jiri Kosina,
The patch 40f2287bd583: "IB/mlx4: Implement
IB_QP_CREATE_USE_GFP_NOIO" from May 11, 2014, leads to the following
static checker warning:
drivers/infiniband/hw/mlx4/qp.c:677 create_qp_common()
warn: use 'gfp' here instead of GFP_XXX?
drivers/infiniband/hw/mlx4/qp.c
673 if (!*caller_qp) {
674 if (qp_type == MLX4_IB_QPT_SMI || qp_type ==
MLX4_IB_QPT_GSI ||
675 (qp_type & (MLX4_IB_QPT_PROXY_SMI |
MLX4_IB_QPT_PROXY_SMI_OWNER |
676 MLX4_IB_QPT_PROXY_GSI |
MLX4_IB_QPT_TUN_SMI_OWNER))) {
677 sqp = kzalloc(sizeof (struct mlx4_ib_sqp),
GFP_KERNEL);
^^
678 if (!sqp)
679 return -ENOMEM;
680 qp = &sqp->qp;
681 qp->pri.vid = 0x;
682 qp->alt.vid = 0x;
683 } else {
684 qp = kzalloc(sizeof (struct mlx4_ib_qp),
GFP_KERNEL);
^^
685 if (!qp)
686 return -ENOMEM;
687 qp->pri.vid = 0x;
688 qp->alt.vid = 0x;
689 }
690 } else
691 qp = *caller_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
[patch] RDMA/cxgb3: information leak in send_abort()
The cpl_abort_req struct has several reserved members which need to be cleared to avoid disclosing kernel information. I have added a memset() so now it matches the cxgb4 version of this function. Signed-off-by: Dan Carpenter diff --git a/drivers/infiniband/hw/cxgb3/iwch_cm.c b/drivers/infiniband/hw/cxgb3/iwch_cm.c index 095bb04..cb78b1e 100644 --- a/drivers/infiniband/hw/cxgb3/iwch_cm.c +++ b/drivers/infiniband/hw/cxgb3/iwch_cm.c @@ -418,6 +418,7 @@ static int send_abort(struct iwch_ep *ep, struct sk_buff *skb, gfp_t gfp) skb->priority = CPL_PRIORITY_DATA; set_arp_failure_handler(skb, abort_arp_failure); req = (struct cpl_abort_req *) skb_put(skb, sizeof(*req)); + memset(req, 0, sizeof(*req)); req->wr.wr_hi = htonl(V_WR_OP(FW_WROPCODE_OFLD_HOST_ABORT_CON_REQ)); req->wr.wr_lo = htonl(V_WR_TID(ep->hwtid)); OPCODE_TID(req) = htonl(MK_OPCODE_TID(CPL_ABORT_REQ, ep->hwtid)); -- 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: [PATCH 4/4] IB: Remove redundant error check
On Sat, May 17, 2014 at 08:23:00PM +0200, Peter Senna Tschudin wrote:
> ---
> drivers/infiniband/hw/ocrdma/ocrdma_hw.c |6 +++---
> drivers/infiniband/ulp/srpt/ib_srpt.c|8
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c
> b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c
> index 3bbf201..f085a51 100644
> --- a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c
> +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c
> @@ -1839,11 +1839,11 @@ int ocrdma_reg_mr(struct ocrdma_dev *dev,
>
> status = ocrdma_mbx_reg_mr_cont(dev, hwmr, cur_pbl_cnt,
> pbl_offset, last);
> - if (status)
> + if (status) {
> + pr_err("%s() err. status=%d\n", __func__, status);
> break;
return status;
> + }
> }
> - if (status)
> - pr_err("%s() err. status=%d\n", __func__, status);
>
> return status;
return 0;
> }
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: [patch] RDMA/cxgb4: info leak in c4iw_alloc_ucontext()
On Fri, Mar 28, 2014 at 11:27:48AM +0100, Yann Droneaud wrote: > Unfortunately, it's not the only structure which has this problem. I'm > currently preparing a report on this issue for this driver (cxgb4) and > another. > This information leak is still present in linux-next. These days we count those things as security vulnerabilities with CVEs and everything. When is it likely to get fixed? 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
[patch] RDMA/cxgb4: release mutex on error in c4iw_reject_cr()
We recently added some locking to this function and there was an error
path which missed the unlock.
Fixes: 9306dcbc96f3 ('RDMA/cxgb4: Lock around accept/reject downcalls')
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index 303e29c..f3753a1 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -2524,6 +2524,7 @@ int c4iw_reject_cr(struct iw_cm_id *cm_id, const void
*pdata, u8 pdata_len)
mutex_lock(&ep->com.mutex);
if (ep->com.state == DEAD) {
+ mutex_unlock(&ep->com.mutex);
c4iw_put_ep(&ep->com);
return -ECONNRESET;
}
--
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
[patch] RDMA/cxgb4: info leak in c4iw_alloc_ucontext()
The c4iw_alloc_ucontext_resp struct has a 4 byte hole after the last
member and we should clear it before passing it to the user.
Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes')
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/cxgb4/provider.c
b/drivers/infiniband/hw/cxgb4/provider.c
index e36d2a2..a72aaa7 100644
--- a/drivers/infiniband/hw/cxgb4/provider.c
+++ b/drivers/infiniband/hw/cxgb4/provider.c
@@ -107,7 +107,7 @@ static struct ib_ucontext *c4iw_alloc_ucontext(struct
ib_device *ibdev,
struct c4iw_ucontext *context;
struct c4iw_dev *rhp = to_c4iw_dev(ibdev);
static int warned;
- struct c4iw_alloc_ucontext_resp uresp;
+ struct c4iw_alloc_ucontext_resp uresp = {};
int ret = 0;
struct c4iw_mm_entry *mm = NULL;
--
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
[patch 2/2] net/mlx4: make buffer larger to avoid overflow warning
My static checker complains that the sprintf() here can overflow.
drivers/infiniband/hw/mlx4/main.c:1836 mlx4_ib_alloc_eqs()
error: format string overflow. buf_size: 32 length: 69
This seems like a valid complaint. The "dev->pdev->bus->name" string
can be 48 characters long. I just made the buffer 80 characters instead
of 69 and I changed the sprintf() to snprintf().
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/mlx4/main.c
b/drivers/infiniband/hw/mlx4/main.c
index 8d9d6b8..1b6dbe15 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1803,7 +1803,7 @@ static void init_pkeys(struct mlx4_ib_dev *ibdev)
static void mlx4_ib_alloc_eqs(struct mlx4_dev *dev, struct mlx4_ib_dev *ibdev)
{
- char name[32];
+ char name[80];
int eq_per_port = 0;
int added_eqs = 0;
int total_eqs = 0;
@@ -1833,8 +1833,8 @@ static void mlx4_ib_alloc_eqs(struct mlx4_dev *dev,
struct mlx4_ib_dev *ibdev)
eq = 0;
mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_IB) {
for (j = 0; j < eq_per_port; j++) {
- sprintf(name, "mlx4-ib-%d-%d@%s",
- i, j, dev->pdev->bus->name);
+ snprintf(name, sizeof(name), "mlx4-ib-%d-%d@%s",
+i, j, dev->pdev->bus->name);
/* Set IRQ for specific name (per ring) */
if (mlx4_assign_eq(dev, name, NULL,
&ibdev->eq_table[eq])) {
--
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
[patch 1/2] net/mlx4: fix some indenting in mlx4_ib_add()
The code was indented too far and also kernel style says we should have
curly braces.
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/mlx4/main.c
b/drivers/infiniband/hw/mlx4/main.c
index 6cb8546..8d9d6b8 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -2048,8 +2048,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
err = mlx4_counter_alloc(ibdev->dev,
&ibdev->counters[i]);
if (err)
ibdev->counters[i] = -1;
- } else
- ibdev->counters[i] = -1;
+ } else {
+ ibdev->counters[i] = -1;
+ }
}
mlx4_foreach_port(i, dev, MLX4_PORT_TYPE_IB)
--
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: IB/mlx4: Build the port IBoE GID table properly under bonding
On Tue, Feb 18, 2014 at 04:13:28PM +0200, Moni Shoua wrote:
> On 2/17/2014 1:52 PM, Dan Carpenter wrote:
> >Hello Moni Shoua,
> >
> >This is a semi-automatic email about new static checker warnings.
> >
> >The patch ad4885d279b6: "IB/mlx4: Build the port IBoE GID table
> >properly under bonding" from Feb 5, 2014, leads to the following
> >Smatch complaint:
> >
> >drivers/infiniband/hw/mlx4/main.c:1749 mlx4_ib_scan_netdevs()
> > error: we previously assumed 'curr_netdev' could be null (see line
> > 1722)
> >
> >drivers/infiniband/hw/mlx4/main.c
> > 1721
> > 1722 if (curr_netdev) {
> > ^^^
> >Check.
> >
> > 1723 port_state =
> > (netif_running(curr_netdev) && netif_carrier_ok(curr_netdev)) ?
> > 1724 IB_PORT_ACTIVE
> > : IB_PORT_DOWN;
> > 1725 mlx4_ib_set_default_gid(ibdev,
> > curr_netdev, port);
> > 1726 } else {
> > 1727 reset_gid_table(ibdev, port);
> > 1728 }
> > 1729 /* if using bonding/team and a slave port is
> > down, we don't the bond IP
> > 1730 * based gids in the table since flows that
> > select port by gid may get
> > 1731 * the down port.
> > 1732 */
> > 1733 if (curr_master && (port_state ==
> > IB_PORT_DOWN)) {
> > 1734 reset_gid_table(ibdev, port);
> > 1735 mlx4_ib_set_default_gid(ibdev,
> > curr_netdev, port);
> > 1736 }
> > 1737 /* if bonding is used it is possible that we
> > add it to masters
> > 1738 * only after IP address is assigned to the net
> > bonding
> > 1739 * interface.
> > 1740 */
> > 1741 if (curr_master && (old_master != curr_master))
> > {
> > 1742 reset_gid_table(ibdev, port);
> > 1743 mlx4_ib_set_default_gid(ibdev,
> > curr_netdev, port);
> > 1744 mlx4_ib_get_dev_addr(curr_master,
> > ibdev, port);
> > 1745 }
> > 1746
> > 1747 if (!curr_master && (old_master !=
> > curr_master)) {
> > 1748 reset_gid_table(ibdev, port);
> > 1749 mlx4_ib_set_default_gid(ibdev,
> > curr_netdev, port);
> >^^^
> >Dereferenced without checking.
> >
> > 1750 mlx4_ib_get_dev_addr(curr_netdev,
> > ibdev, port);
> > 1751 }
> >
> >regards,
> >dan carpenter
> Not really a bug. (curr_master != NULL) implies (curr_netdev != NULL)
Ha ha. Take another look. That's what I was just explaining about! :)
On line 1743 when curr_master is non-NULL then Smatch doesn't complain
because it understands about the relationship between curr_master and
curr_netdev.
But here it is complaining about line 1749 where curr_master is NULL so
the implication doesn't apply.
Nice, huh?
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: IB/mlx4: Build the port IBoE GID table properly under bonding
Hello Moni Shoua,
This is a semi-automatic email about new static checker warnings.
The patch ad4885d279b6: "IB/mlx4: Build the port IBoE GID table
properly under bonding" from Feb 5, 2014, leads to the following
Smatch complaint:
drivers/infiniband/hw/mlx4/main.c:1749 mlx4_ib_scan_netdevs()
error: we previously assumed 'curr_netdev' could be null (see line
1722)
drivers/infiniband/hw/mlx4/main.c
1721
1722 if (curr_netdev) {
^^^
Check.
1723 port_state = (netif_running(curr_netdev) &&
netif_carrier_ok(curr_netdev)) ?
1724 IB_PORT_ACTIVE :
IB_PORT_DOWN;
1725 mlx4_ib_set_default_gid(ibdev, curr_netdev,
port);
1726 } else {
1727 reset_gid_table(ibdev, port);
1728 }
1729 /* if using bonding/team and a slave port is down, we
don't the bond IP
1730 * based gids in the table since flows that select port
by gid may get
1731 * the down port.
1732 */
1733 if (curr_master && (port_state == IB_PORT_DOWN)) {
1734 reset_gid_table(ibdev, port);
1735 mlx4_ib_set_default_gid(ibdev, curr_netdev,
port);
1736 }
1737 /* if bonding is used it is possible that we add it to
masters
1738 * only after IP address is assigned to the net bonding
1739 * interface.
1740 */
1741 if (curr_master && (old_master != curr_master)) {
1742 reset_gid_table(ibdev, port);
1743 mlx4_ib_set_default_gid(ibdev, curr_netdev,
port);
1744 mlx4_ib_get_dev_addr(curr_master, ibdev, port);
1745 }
1746
1747 if (!curr_master && (old_master != curr_master)) {
1748 reset_gid_table(ibdev, port);
1749 mlx4_ib_set_default_gid(ibdev, curr_netdev,
port);
^^^
Dereferenced without checking.
1750 mlx4_ib_get_dev_addr(curr_netdev, ibdev, port);
1751 }
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
[patch] IB/qib: remove duplicative check in get_a_ctxt()
We already know "pusable" is non-zero, no need to check again.
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c
b/drivers/infiniband/hw/qib/qib_file_ops.c
index 275f247f9fca..cff8f830492c 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -1459,7 +1459,7 @@ static int get_a_ctxt(struct file *fp, const struct
qib_user_info *uinfo,
cused++;
else
cfree++;
- if (pusable && cfree && cused < inuse) {
+ if (cfree && cused < inuse) {
udd = dd;
inuse = cused;
}
--
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: [PATCH] RDMA/ocrdma: fix traffic class shift
On Mon, Feb 10, 2014 at 01:48:58PM +0530, Devesh Sharma wrote: > Use correct value for obtaining traffic class from device > response for Query QP request. > > Signed-off-by: Devesh Sharma Reported-by: Dan Carpenter regards, dan carpenter > --- > drivers/infiniband/hw/ocrdma/ocrdma_verbs.c |2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c > b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c > index 7686dce..998f0de 100644 > --- a/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_verbs.c > @@ -1415,7 +1415,7 @@ int ocrdma_query_qp(struct ib_qp *ibqp, > OCRDMA_QP_PARAMS_HOP_LMT_MASK) >> > OCRDMA_QP_PARAMS_HOP_LMT_SHIFT; > qp_attr->ah_attr.grh.traffic_class = (params.tclass_sq_psn & > - OCRDMA_QP_PARAMS_SQ_PSN_MASK) >> > + OCRDMA_QP_PARAMS_TCLASS_MASK) >> > OCRDMA_QP_PARAMS_TCLASS_SHIFT; > > qp_attr->ah_attr.ah_flags = IB_AH_GRH; > -- > 1.7.1 -- 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: RDMA/ocrdma: Add driver for Emulex OneConnect IBoE RDMA adapter
Hello Parav Pandit, The patch fe2caefcdf58: "RDMA/ocrdma: Add driver for Emulex OneConnect IBoE RDMA adapter" from Mar 21, 2012, leads to the following static checker warning: drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:1419 ocrdma_query_qp() warn: right shifting to zero drivers/infiniband/hw/ocrdma/ocrdma_verbs.c 1414 qp_attr->ah_attr.grh.sgid_index = qp->sgid_idx; 1415 qp_attr->ah_attr.grh.hop_limit = (params.hop_lmt_rq_psn & 1416 OCRDMA_QP_PARAMS_HOP_LMT_MASK) >> 1417 OCRDMA_QP_PARAMS_HOP_LMT_SHIFT; 1418 qp_attr->ah_attr.grh.traffic_class = (params.tclass_sq_psn & 1419 OCRDMA_QP_PARAMS_SQ_PSN_MASK) >> 1420 OCRDMA_QP_PARAMS_TCLASS_SHIFT; We always set traffic_class to zero. Maybe OCRDMA_QP_PARAMS_TCLASS_MASK was intended instead of OCRDMA_QP_PARAMS_SQ_PSN_MASK? Btw, the reason we are seing this bug is because the names are horrible. Try reading the variable names out loud. OCRDMA_QP_PARAMS_SQ_PSN_MASK is 6 words. It's 16 syllables long. It takes me about 4 seconds to just pronounce it. The next one is almost identical except for one or two characters in the middle. http://www.spotthedifference.com Even though the variable names are longer than a Tolstoy novel, I still have no idea what they mean. I know that the "T" in OCRDMA_QP_PARAMS_TCLASS_SHIFT stands for "traffic" but when it comes to the "SQ_PSN" then I have no idea about that. 1421 1422 qp_attr->ah_attr.ah_flags = IB_AH_GRH; 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
[patch] RDMA/nes: clean up a conditon
We don't need to test "ret" twice and also the white space is messed up.
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/nes/nes_verbs.c
b/drivers/infiniband/hw/nes/nes_verbs.c
index 8308e3634767..2db2b58f70d9 100644
--- a/drivers/infiniband/hw/nes/nes_verbs.c
+++ b/drivers/infiniband/hw/nes/nes_verbs.c
@@ -3134,9 +3134,7 @@ int nes_modify_qp(struct ib_qp *ibqp, struct ib_qp_attr
*attr,
" original_last_aeq = 0x%04X. last_aeq =
0x%04X.\n",
nesqp->hwqp.qp_id,
atomic_read(&nesqp->refcount),
original_last_aeq, nesqp->last_aeq);
- if ((!ret) ||
- ((original_last_aeq !=
NES_AEQE_AEID_RDMAP_ROE_BAD_LLP_CLOSE) &&
- (ret))) {
+ if (!ret || original_last_aeq !=
NES_AEQE_AEID_RDMAP_ROE_BAD_LLP_CLOSE) {
if (dont_wait) {
if (nesqp->cm_id && nesqp->hw_tcp_state != 0) {
nes_debug(NES_DBG_MOD_QP, "QP%u Queuing
fake disconnect for QP refcount (%d),"
--
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
[patch] RDMA/cxgb3: remove a couple unneeded conditions
We know that "reset_tpt_entry" is false on this side of the if else statement so there is no need to check again. Signed-off-by: Dan Carpenter diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.c b/drivers/infiniband/hw/cxgb3/cxio_hal.c index c3f5aca4ef00..de1c61b417d6 100644 --- a/drivers/infiniband/hw/cxgb3/cxio_hal.c +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c @@ -735,14 +735,12 @@ static int __cxio_tpt_op(struct cxio_rdev *rdev_p, u32 reset_tpt_entry, ((perm & TPT_MW_BIND) ? F_TPT_MW_BIND_ENABLE : 0) | V_TPT_ADDR_TYPE((zbva ? TPT_ZBTO : TPT_VATO)) | V_TPT_PAGE_SIZE(page_size)); - tpt.rsvd_pbl_addr = reset_tpt_entry ? 0 : - cpu_to_be32(V_TPT_PBL_ADDR(PBL_OFF(rdev_p, pbl_addr)>>3)); + tpt.rsvd_pbl_addr = cpu_to_be32(V_TPT_PBL_ADDR(PBL_OFF(rdev_p, pbl_addr)>>3)); tpt.len = cpu_to_be32(len); tpt.va_hi = cpu_to_be32((u32) (to >> 32)); tpt.va_low_or_fbo = cpu_to_be32((u32) (to & 0xULL)); tpt.rsvd_bind_cnt_or_pstag = 0; - tpt.rsvd_pbl_size = reset_tpt_entry ? 0 : - cpu_to_be32(V_TPT_PBL_SIZE(pbl_size >> 2)); + tpt.rsvd_pbl_size = cpu_to_be32(V_TPT_PBL_SIZE(pbl_size >> 2)); } err = cxio_hal_ctrl_qp_write_mem(rdev_p, stag_idx + -- 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
[patch] IB/qib: cleanup qib_register_observer()
Returning directly is easier to read than do-nothing gotos. Remove the
duplicative check on "olp" and pulled the code in one indent level.
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/qib/qib_diag.c
b/drivers/infiniband/hw/qib/qib_diag.c
index 1686fd4bda87..52601ae4b327 100644
--- a/drivers/infiniband/hw/qib/qib_diag.c
+++ b/drivers/infiniband/hw/qib/qib_diag.c
@@ -689,28 +689,23 @@ int qib_register_observer(struct qib_devdata *dd,
const struct diag_observer *op)
{
struct diag_observer_list_elt *olp;
- int ret = -EINVAL;
+ unsigned long flags;
if (!dd || !op)
- goto bail;
- ret = -ENOMEM;
+ return -EINVAL;
olp = vmalloc(sizeof *olp);
if (!olp) {
pr_err("vmalloc for observer failed\n");
- goto bail;
+ return -ENOMEM;
}
- if (olp) {
- unsigned long flags;
- spin_lock_irqsave(&dd->qib_diag_trans_lock, flags);
- olp->op = op;
- olp->next = dd->diag_observer_list;
- dd->diag_observer_list = olp;
- spin_unlock_irqrestore(&dd->qib_diag_trans_lock, flags);
- ret = 0;
- }
-bail:
- return ret;
+ spin_lock_irqsave(&dd->qib_diag_trans_lock, flags);
+ olp->op = op;
+ olp->next = dd->diag_observer_list;
+ dd->diag_observer_list = olp;
+ spin_unlock_irqrestore(&dd->qib_diag_trans_lock, flags);
+
+ return 0;
}
/* Remove all registered observers when device is closed */
--
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
[patch] IB/iser: use after free in iser_snd_completion()
We use "tx_desc" again after we free it.
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c
b/drivers/infiniband/ulp/iser/iser_initiator.c
index 538822684d5b..334f34b1cd46 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -610,11 +610,12 @@ void iser_snd_completion(struct iser_tx_desc *tx_desc,
ib_dma_unmap_single(device->ib_device, tx_desc->dma_addr,
ISER_HEADERS_LEN, DMA_TO_DEVICE);
kmem_cache_free(ig.desc_cache, tx_desc);
+ tx_desc = NULL;
}
atomic_dec(&ib_conn->post_send_buf_count);
- if (tx_desc->type == ISCSI_TX_CONTROL) {
+ if (tx_desc && tx_desc->type == ISCSI_TX_CONTROL) {
/* this arithmetic is legal by libiscsi dd_data allocation */
task = (void *) ((long)(void *)tx_desc -
sizeof(struct iscsi_task));
--
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
[patch] IB/usnic: use GFP_ATOMIC under spinlock
This is called from qp_grp_and_vf_bind() and we are holding the
"vf->lock" so the allocation can't sleep.
Fixes: e3cf00d0a87f ('IB/usnic: Add Cisco VIC low-level hardware driver')
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c
b/drivers/infiniband/hw/usnic/usnic_uiom.c
index ae6934c0d05a..16755cdab2c0 100644
--- a/drivers/infiniband/hw/usnic/usnic_uiom.c
+++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
@@ -498,7 +498,7 @@ int usnic_uiom_attach_dev_to_pd(struct usnic_uiom_pd *pd,
struct device *dev)
struct usnic_uiom_dev *uiom_dev;
int err;
- uiom_dev = kzalloc(sizeof(*uiom_dev), GFP_KERNEL);
+ uiom_dev = kzalloc(sizeof(*uiom_dev), GFP_ATOMIC);
if (!uiom_dev)
return -ENOMEM;
uiom_dev->dev = dev;
--
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: IB/usnic: Add UDP support in usnic_ib_qp_grp.[hc]
Hello Upinder Malhi,
The patch e45e614e4015: "IB/usnic: Add UDP support in
usnic_ib_qp_grp.[hc]" from Jan 9, 2014, leads to the following
static checker warning:
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c:638 qp_grp_id_from_flow()
warn: passing casted pointer 'id' to 'usnic_transport_sock_get_addr()'
32 vs 16.
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c
627 static int qp_grp_id_from_flow(struct usnic_ib_qp_grp_flow *qp_flow,
628 uint32_t *id)
629 {
630 enum usnic_transport_type trans_type = qp_flow->trans_type;
631 int err;
632
633 switch (trans_type) {
634 case USNIC_TRANSPORT_ROCE_CUSTOM:
635 *id = qp_flow->usnic_roce.port_num;
636 break;
637 case USNIC_TRANSPORT_IPV4_UDP:
638 err = usnic_transport_sock_get_addr(qp_flow->udp.sock,
639 NULL, NULL,
640 (uint16_t *)
id);
^^^
This doesn't work on big endian systems.
641 if (err)
642 return err;
643 break;
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: IB/usnic: Add Cisco VIC low-level hardware driver
On Thu, Dec 12, 2013 at 03:03:15AM +, Upinder Malhi (umalhi) wrote: > Why not? > > We can switch to WARN_ON. The kernel provides this macro - > assert_spin_locked - which does asserts via BUG_ON that > spin_is_locked, which makes me think that using BUG_ON in conjunction > with spin_is_locked is legal. We should use this macro instead of > manually asserting, unless that it is not legal, in which case WARN_ON > will do. It's better to not halt the kernel. That way you can do some debugging. But the main thing is the scheduling with a spin lock held. The other I don't care about. 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: IB/usnic: Add Cisco VIC low-level hardware driver
On Thu, Dec 12, 2013 at 01:59:00AM +, Upinder Malhi (umalhi) wrote:
> Hi Dan,
> Can I ask what static checker you are using for these?
>
In this email, the released version of Smatch will only warn about the
actual bug and not the others. It requires that you build the cross
function database though before you test the code.
~/smatch/smatch_scripts/build_kernel_data.sh
> > drivers/infiniband/hw/usnic/usnic_uiom.c
> > 469 pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
> > ^^^
> > This function returns NULL on error not error pointers.
> >
> > 470 if (IS_ERR_OR_NULL(domain)) {
> > 471 usnic_err("Failed to allocate IOMMU domain with err
> > %ld\n",
> > 472 PTR_ERR(pd->domain));
> > 473 kfree(pd);
> > 474 return ERR_PTR(domain ? PTR_ERR(domain) : -ENOMEM);
> > 475 }
> >
> > Similar harmless but crappy slop in:
> [UM] - Unless I'm missing something, I think this is OK and preferred
> in case iommu_domain_alloc in future starts returning ERR_PTRs.
It doesn't cause a bug, but it's unusual and considered sloppy. APIs
should be clear about what error codes they return and they should
almost always return either ERR_PTRs *or* NULL but not *both*. The
exceptions are when NULL is a valid return which means something. For
example, maybe you are looking for a file and you return NULL if the
file is not found but ERR_PTR(-ENOMEM) if you have an error.
In linux if we decide that iommu_domain_alloc() should return ERR_PTRs
instead of NULL pointers, then it's up to the person who decides that
to change all the callers as well.
Also if you trust the lower levels to print their error messages
correctly then the error handling here could be much simpler. kzalloc()
for example, has very extensive error messages.
pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
if (!domain) {
kfree(pd);
return -ENOMEM;
}
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: IB/usnic: Add Cisco VIC low-level hardware driver
Hello Upinder Malhi,
The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following static checker
warning:
drivers/infiniband/hw/usnic/usnic_ib_verbs.c:114
usnic_ib_fill_create_qp_resp()
warn: check that 'resp' doesn't leak information (struct has
a hole after 'transport')
drivers/infiniband/hw/usnic/usnic_ib_verbs.c
109 WARN_ON(chunk->type != USNIC_VNIC_RES_TYPE_CQ);
110 resp.cq_cnt = chunk->cnt;
111 for (i = 0; i < chunk->cnt; i++)
112 resp.cq_idx[i] = chunk->res[i]->vnic_idx;
113
114 err = ib_copy_to_udata(udata, &resp, sizeof(resp));
^
The "resp" struct has a struct hole and uninitialized struct members so
it leaks uninitialized stack information to the user (information
disclosure security bug).
115 if (err) {
116 usnic_err("Failed to copy udata for %s",
us_ibdev->ib_dev.name);
117 return err;
118 }
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: IB/usnic: Add Cisco VIC low-level hardware driver
Hello Upinder Malhi,
The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following Smatch
warning:
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c:467
usnic_ib_qp_grp_create()
error: scheduling with locks held: 'spin_lock:lock'
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c
449 BUG_ON(!spin_is_locked(&vf->lock));
Holding lock.
Don't call BUG_ON(), use WARN_ON()?
450
451 err =
usnic_vnic_res_spec_satisfied(&min_transport_spec[transport],
452 res_spec);
453 if (err) {
454 usnic_err("Spec does not meet miniumum req for
transport %d\n",
455 transport);
456 log_spec(res_spec);
457 return ERR_PTR(err);
458 }
459
460 port_num = usnic_transport_rsrv_port(transport, 0);
461 if (!port_num) {
462 usnic_err("Unable to allocate port for %s\n",
463 netdev_name(ufdev->netdev));
464 return ERR_PTR(-EINVAL);
465 }
466
467 qp_grp = kzalloc(sizeof(*qp_grp), GFP_KERNEL);
^^
Sleeping allocation.
468 if (!qp_grp) {
469 usnic_err("Unable to alloc qp_grp - Out of memory\n");
470 return NULL;
471 }
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: IB/usnic: Add Cisco VIC low-level hardware driver
Hello Upinder Malhi,
The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following Smatch
warning:
drivers/infiniband/hw/usnic/usnic_uiom.c:560
usnic_uiom_get_dev_list()
error: scheduling with locks held: 'spin_lock:lock'
drivers/infiniband/hw/usnic/usnic_uiom.c
553 struct device **usnic_uiom_get_dev_list(struct usnic_uiom_pd *pd)
554 {
555 struct usnic_uiom_dev *uiom_dev;
556 struct device **devs;
557 int i = 0;
558
559 spin_lock(&pd->lock);
560 devs = kzalloc(sizeof(*devs)*(pd->dev_cnt + 1), GFP_KERNEL);
You can't do blocking allocations with a spin_lock held.
561 if (!devs) {
562 devs = ERR_PTR(-ENOMEM);
563 goto out;
564 }
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: IB/usnic: Add Cisco VIC low-level hardware driver
Hello Upinder Malhi,
The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following static checker
warning:
drivers/infiniband/hw/usnic/usnic_uiom.c:47 usnic_uiom_alloc_pd()
warn: passing zero to 'PTR_ERR'"
drivers/infiniband/hw/usnic/usnic_uiom.c
469 pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
^^^
This function returns NULL on error not error pointers.
470 if (IS_ERR_OR_NULL(domain)) {
471 usnic_err("Failed to allocate IOMMU domain with err
%ld\n",
472 PTR_ERR(pd->domain));
473 kfree(pd);
474 return ERR_PTR(domain ? PTR_ERR(domain) : -ENOMEM);
475 }
Similar harmless but crappy slop in:
vers/infiniband/hw/usnic/usnic_ib_main.c
249 us_ibdev = (struct usnic_ib_dev
*)ib_alloc_device(sizeof(*us_ibdev));
250 if (IS_ERR_OR_NULL(us_ibdev)) {
251 usnic_err("Device %s context alloc failed\n",
252 netdev_name(pci_get_drvdata(dev)));
253 return ERR_PTR(us_ibdev ? PTR_ERR(us_ibdev) : -EFAULT);
254 }
255
256 us_ibdev->ufdev = usnic_fwd_dev_alloc(dev);
257 if (IS_ERR_OR_NULL(us_ibdev->ufdev)) {
^^^
258 usnic_err("Failed to alloc ufdev for %s with err %ld\n",
259 pci_name(dev),
PTR_ERR(us_ibdev->ufdev));
260 goto err_dealloc;
261 }
The general confusing about what return values are leads to a bug later
on:
vers/infiniband/hw/usnic/usnic_ib_main.c
462 pf = usnic_ib_discover_pf(vf->vnic);
^^
This function returns ERR_PTRs. Also it has a bug and can return freed
pointers. Oops... :(
463 if (!pf) {
464 usnic_err("Failed to discover pf of vnic %s with
err%d\n",
465 pci_name(pdev), err);
466 goto out_clean_vnic;
467 }
468
469 vf->pf = pf;
470 spin_lock_init(&vf->lock);
471 mutex_lock(&pf->usdev_lock);
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
[PATCH] mlx5: shift wrapping bug in mlx5_ib_get_buf_offset()
"page_size" is declared as u64 but the (1 << page_shift) shift will wrap
at 32 bits.
Fixes: e126ba97dba9 ('mlx5: Add driver for Mellanox Connect-IB adapters')
Signed-off-by: Dan Carpenter
diff --git a/drivers/infiniband/hw/mlx5/mem.c b/drivers/infiniband/hw/mlx5/mem.c
index 3a53228..3020ec2 100644
--- a/drivers/infiniband/hw/mlx5/mem.c
+++ b/drivers/infiniband/hw/mlx5/mem.c
@@ -148,7 +148,7 @@ int mlx5_ib_get_buf_offset(u64 addr, int page_shift, u32
*offset)
u64 off_mask;
u64 buf_off;
- page_size = 1 << page_shift;
+ page_size = 1ULL << page_shift;
page_mask = page_size - 1;
buf_off = addr & page_mask;
off_size = page_size >> 6;
--
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
[patch v2] mlx5_core: delete some dead code
The printk() looks like it is left over debug code. I have removed it.
Signed-off-by: Dan Carpenter
---
v2: Remove the printk instead of moving it infront of the return.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
index ba816c2..9b79067 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
@@ -192,10 +192,8 @@ static int alloc_4k(struct mlx5_core_dev *dev, u64 *addr)
struct fw_page *fp;
unsigned n;
- if (list_empty(&dev->priv.free_list)) {
+ if (list_empty(&dev->priv.free_list))
return -ENOMEM;
- mlx5_core_warn(dev, "\n");
- }
fp = list_entry(dev->priv.free_list.next, struct fw_page, list);
n = find_first_bit(&fp->bitmask, 8 * sizeof(fp->bitmask));
--
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: [patch -next] mlx5_core: warn if no space left in alloc_4k()
On Sun, Nov 03, 2013 at 09:03:27AM +0200, Eli Cohen wrote:
> On Fri, Nov 01, 2013 at 01:20:44PM +0300, Dan Carpenter wrote:
> > The warning was unreachable. In the original code, it would print the
> > line number and the function but I have added an error message.
> >
> > Signed-off-by: Dan Carpenter
> > ---
> > I haven't tested this, hopefully the warning is not annoying.
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > index ba816c2..cb86265 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > @@ -193,8 +193,8 @@ static int alloc_4k(struct mlx5_core_dev *dev, u64
> > *addr)
> > unsigned n;
> >
> > if (list_empty(&dev->priv.free_list)) {
> > + mlx5_core_warn(dev, "no available space\n");
> > return -ENOMEM;
> > - mlx5_core_warn(dev, "\n");
> > }
>
> What we really need to do here is silently return -ENOMEM. The list
> can be found empty on a regular basis so we don't want to flood dmesg
> with annoying messages. The statement was probably left there from a
> previous debug cycle.
Ah, ok. Thanks for the review. I will remove the message.
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
[patch -next] mlx5_core: warn if no space left in alloc_4k()
The warning was unreachable. In the original code, it would print the
line number and the function but I have added an error message.
Signed-off-by: Dan Carpenter
---
I haven't tested this, hopefully the warning is not annoying.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
index ba816c2..cb86265 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
@@ -193,8 +193,8 @@ static int alloc_4k(struct mlx5_core_dev *dev, u64 *addr)
unsigned n;
if (list_empty(&dev->priv.free_list)) {
+ mlx5_core_warn(dev, "no available space\n");
return -ENOMEM;
- mlx5_core_warn(dev, "\n");
}
fp = list_entry(dev->priv.free_list.next, struct fw_page, list);
--
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
