Re: [PATCH net-next 2/2] cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

Le vendredi 14 mars 2014 à 21:52 +0530, Hariprasad Shenai a écrit :
> From: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>

[...]

> Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> ---
>  drivers/infiniband/hw/cxgb4/device.c            | 177 ++++++++++++++----------
>  drivers/infiniband/hw/cxgb4/iw_cxgb4.h          |   9 +-
>  drivers/infiniband/hw/cxgb4/provider.c          |  43 +++++-
>  drivers/infiniband/hw/cxgb4/qp.c                | 140 +++++++++----------
>  drivers/infiniband/hw/cxgb4/t4.h                |   6 +
>  drivers/infiniband/hw/cxgb4/user.h              |   5 +
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4.h      |   1 +
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c |  87 +++++++-----
>  drivers/net/ethernet/chelsio/cxgb4/sge.c        |   8 +-
>  9 files changed, 286 insertions(+), 190 deletions(-)
> 

[...]

> diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c
> index 7e94c9a..e36d2a2 100644
> --- a/drivers/infiniband/hw/cxgb4/provider.c
> +++ b/drivers/infiniband/hw/cxgb4/provider.c
> @@ -106,15 +106,54 @@ 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;
> +	int ret = 0;
> +	struct c4iw_mm_entry *mm = NULL;
>  
>  	PDBG("%s ibdev %p\n", __func__, ibdev);
>  	context = kzalloc(sizeof(*context), GFP_KERNEL);
> -	if (!context)
> -		return ERR_PTR(-ENOMEM);
> +	if (!context) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
>  	c4iw_init_dev_ucontext(&rhp->rdev, &context->uctx);
>  	INIT_LIST_HEAD(&context->mmaps);
>  	spin_lock_init(&context->mmap_lock);
> +
> +	if (udata->outlen < sizeof(uresp)) {
> +		if (!warned++)
> +			pr_err(MOD "Warning - downlevel libcxgb4 (non-fatal), device status page disabled.");
> +		rhp->rdev.flags |= T4_STATUS_PAGE_DISABLED;
> +	} else {
> +		mm = kmalloc(sizeof(*mm), GFP_KERNEL);
> +		if (!mm)
> +			goto err_free;
> +

OK, that's the origin of the missing error I've noticed in my latest
review on linux-next.

See
<http://marc.info/?i=1395846311-29288-1-git-send-email-ydroneaud@opteya.com>
<http://marc.info/?i=005b01cf4907$9adfa320$d09ee960
$@opengridcomputing.com>

Sorry, I've missed the opportunity to report it.

> +		uresp.status_page_size = PAGE_SIZE;
> +
> +		spin_lock(&context->mmap_lock);
> +		uresp.status_page_key = context->key;
> +		context->key += PAGE_SIZE;
> +		spin_unlock(&context->mmap_lock);
> +

Is it really necessary to spinlock here since context is local to the
function ?

> +		ret = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> +		if (ret)
> +			goto err_mm;
> +
> +		mm->key = uresp.status_page_key;
> +		mm->addr = virt_to_phys(rhp->rdev.status_page);
> +		mm->len = PAGE_SIZE;
> +		insert_mmap(context, mm);
> +	}
>  	return &context->ibucontext;
> +err_mm:
> +	kfree(mm);
> +err_free:
> +	kfree(context);
> +err:
> +	return ERR_PTR(ret);
>  }
>  

[...]

> diff --git a/drivers/infiniband/hw/cxgb4/user.h b/drivers/infiniband/hw/cxgb4/user.h
> index 32b754c..11ccd27 100644
> --- a/drivers/infiniband/hw/cxgb4/user.h
> +++ b/drivers/infiniband/hw/cxgb4/user.h
> @@ -70,4 +70,9 @@ struct c4iw_create_qp_resp {
>  	__u32 qid_mask;
>  	__u32 flags;
>  };
> +
> +struct c4iw_alloc_ucontext_resp {
> +	__u64 status_page_key;
> +	__u32 status_page_size;
> +};

If this is going to be part of the ABI, mind add an explicit padding to
align the structure on 64bits.

Regards

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html