On Tue, Nov 01, 2016 at 02:42:27PM +0000, Amrani, Ram wrote:
> > While looking on this patch and associated code to it, I noticed the
> > following code stack:
> >
> > qedr_create_qp
> > -->
> > dev->ops->rdma_destroy_qp(dev->rdma_ctx, qp->qed_qp);
> > -->
> > qed_rdma_destroy_qp
> > -->
> > qed_roce_destroy_qp
> > This function will check the QP state and return -EINVAL and comment
> > that this QP needs to be prepared before destroying it.
> >
> > However immediately after returning, you are calling to kfree(qp)
> > without any checks.
> >
> > It looks like an error and it is worth to take a look on it.
> >
>
> That's a deep level of reading... thanks.
You are welcome.
>
> When the QP is created its state is set in ecore_rdma_create_qp():
> qp->cur_state = ECORE_ROCE_QP_STATE_RESET;
>
> When it is ecore_roce_destroy_qp() is invoked, the function *must* be in either RESET or two other states:
> if ((qp->cur_state != QED_ROCE_QP_STATE_RESET) &&
> (qp->cur_state != QED_ROCE_QP_STATE_ERR) &&
> (qp->cur_state != QED_ROCE_QP_STATE_INIT)) {
> DP_NOTICE(p_hwfn,
> "QP must be in error, reset or init state before destroying it\n");
> return -EINVAL;
> }
> So actually, we won't return -EINVAL here.
>
> The bug I see is that I see in our upstream code is that for RESET the normal "destroy" operations continue. But they shouldn't.
> We need here something like this:
> if (qp->cur_state == ECORE_ROCE_QP_STATE_RESET)
> return 0;
>
> Flow will return to qed_rdma_destroy_qp() that will release the qp resource in the qed_roce scope (our real purpose).
> And then return to qedr_create_qp() where the qp resource will be released in the qedr scope.
>
> And as a side issue - an improvement that can be added is to return the error code of the QP create and not of the QP destroy.
I'm happy to hear that it helped.
>
> I'll the first later on and probably the second too.
>
>
>
> > And did I miss the fix to memory leak posted during code review?
> >
> As far as I know, I have supplied patches for all memory leaks. Can you direct me to a specific e-mail?
I failed to find the relevant thread now, so forget it, probably it is
my fault.
>
> Thanks,
> Ram
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Attachment:
signature.asc
Description: PGP signature
