Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> It does returns ocrdma_mr* on success. Why do you think it doesn't return? > Below is the snippet. Sorry, you're absoutely right. I was looking at ocrdma_mbx_alloc_lkey(). - R. -- 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 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> -Original Message-
> From: Roland Dreier [mailto:rol...@purestorage.com]
> Sent: Wednesday, March 21, 2012 10:13 PM
> To: Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> > +struct ib_pd *ocrdma_alloc_pd(struct ib_device *ibdev,
> > + struct ib_ucontext *context,
> > + struct ib_udata *udata) {
> > + struct ocrdma_dev *dev = get_ocrdma_dev(ibdev);
> > + struct ocrdma_pd *pd;
> > + int status;
> > +
> > + pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> > + if (!pd)
> > + return ERR_PTR(-ENOMEM);
> > + pd->dev = dev;
> > + if (udata && context) {
> > + pd->dpp_enabled = (dev->nic_info.dev_family ==
> > + OCRDMA_GEN2_FAMILY) ? true :
> > + false;
>
> Writing
>
> () ? true : false
>
> is pretty silly, since it's just an obfuscated way of writing
>
>
>
> IOW, you can just write
>
> pd->dpp_enabled = (dev->nic_info.dev_family ==
> OCRDMA_GEN2_FAMILY);
>
>
> > +int ocrdma_dealloc_pd(struct ib_pd *ibpd) {
> > + struct ocrdma_pd *pd = get_ocrdma_pd(ibpd);
> > + struct ocrdma_dev *dev = pd->dev;
> > + int status;
> > + u64 usr_db;
> > +
> > + if (atomic_read(&pd->use_cnt)) {
> > + ocrdma_err("%s(%d) pd=0x%x is in use.\n",
> > + __func__, dev->id, pd->id);
> > + status = -EFAULT;
> > + goto dealloc_err;
> > + }
>
> all of the use_cnt tracking in this driver seems to duplicate what the rdma
> midlayer already does... is there any reason we need that in the low-level
> hardware driver too, or can we just get rid of the various use_cnt members?
This use_cnt can be removed from low-level hardware driver. I'll remove it.
--
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 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
Inline.
> -Original Message-
> From: Roland Dreier [mailto:rol...@purestorage.com]
> Sent: Wednesday, March 21, 2012 11:27 PM
> To: frank zago
> Cc: Pandit, Parav; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> On Wed, Mar 21, 2012 at 10:42 AM, frank zago
> wrote:
> > On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
> >> +struct ib_mr *ocrdma_get_dma_mr(struct ib_pd *ibpd, int acc) {
> >> + struct ocrdma_mr *mr;
> >> +
> >> + mr = ocrdma_alloc_lkey(ibpd, acc, 0,
> >> + OCRDMA_ADDR_CHECK_DISABLE);
> >> + if (!mr)
> >> + return ERR_PTR(-ENOMEM);
> >
> > ocrdma_alloc_lkey does not return NULL on error.
>
I'll fix this part.
> Good catch! Even more to the point, ocrdma_alloc_lkey() doesn't return a
> struct ocrdma_mr* on success. So this function is totally broken.
>
It does returns ocrdma_mr* on success. Why do you think it doesn't return?
Below is the snippet.
status = ocrdma_mbx_alloc_lkey(dev, &mr->hwmr, pd->id, addr_check);
if (status) {
kfree(mr);
return ERR_PTR(-ENOMEM);
}
mr->pd = pd;
atomic_inc(&pd->use_cnt);
mr->ibmr.lkey = mr->hwmr.lkey;
if (mr->hwmr.remote_wr || mr->hwmr.remote_rd)
mr->ibmr.rkey = mr->hwmr.lkey;
return mr;
> - R.
--
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 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
> +
> +int ocrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
> + struct ib_send_wr **bad_wr)
> +{
> + int status = 0;
> + struct ocrdma_qp *qp = get_ocrdma_qp(ibqp);
> + struct ocrdma_hdr_wqe *hdr;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&qp->q_lock, flags);
> + if (qp->state != OCRDMA_QPS_RTS && qp->state != OCRDMA_QPS_SQD) {
> + spin_unlock_irqrestore(&qp->q_lock, flags);
> + return -EINVAL;
> + }
There, and in several places in this function, you return an error
without setting bad_wr.
Frank.
--
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
Fwd: Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
(Resent becasue the first one got lost)
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
> +
> +int ocrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
> + struct ib_send_wr **bad_wr)
> +{
> + int status = 0;
> + struct ocrdma_qp *qp = get_ocrdma_qp(ibqp);
> + struct ocrdma_hdr_wqe *hdr;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&qp->q_lock, flags);
> + if (qp->state != OCRDMA_QPS_RTS && qp->state != OCRDMA_QPS_SQD) {
> + spin_unlock_irqrestore(&qp->q_lock, flags);
> + return -EINVAL;
> + }
There, and in several places in this function, you return an error
without setting bad_wr.
Frank.
--
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 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Wed, Mar 21, 2012 at 10:42 AM, frank zago
wrote:
> On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
>> +struct ib_mr *ocrdma_get_dma_mr(struct ib_pd *ibpd, int acc)
>> +{
>> + struct ocrdma_mr *mr;
>> +
>> + mr = ocrdma_alloc_lkey(ibpd, acc, 0, OCRDMA_ADDR_CHECK_DISABLE);
>> + if (!mr)
>> + return ERR_PTR(-ENOMEM);
>
> ocrdma_alloc_lkey does not return NULL on error.
Good catch! Even more to the point, ocrdma_alloc_lkey() doesn't return a
struct ocrdma_mr* on success. So this function is totally broken.
- R.
--
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 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
> +struct ib_mr *ocrdma_get_dma_mr(struct ib_pd *ibpd, int acc)
> +{
> + struct ocrdma_mr *mr;
> +
> + mr = ocrdma_alloc_lkey(ibpd, acc, 0, OCRDMA_ADDR_CHECK_DISABLE);
> + if (!mr)
> + return ERR_PTR(-ENOMEM);
ocrdma_alloc_lkey does not return NULL on error.
--
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 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> +struct ib_pd *ocrdma_alloc_pd(struct ib_device *ibdev,
> + struct ib_ucontext *context,
> + struct ib_udata *udata)
> +{
> + struct ocrdma_dev *dev = get_ocrdma_dev(ibdev);
> + struct ocrdma_pd *pd;
> + int status;
> +
> + pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + return ERR_PTR(-ENOMEM);
> + pd->dev = dev;
> + if (udata && context) {
> + pd->dpp_enabled = (dev->nic_info.dev_family ==
> + OCRDMA_GEN2_FAMILY) ? true : false;
Writing
() ? true : false
is pretty silly, since it's just an obfuscated way of writing
IOW, you can just write
pd->dpp_enabled = (dev->nic_info.dev_family == OCRDMA_GEN2_FAMILY);
> +int ocrdma_dealloc_pd(struct ib_pd *ibpd)
> +{
> + struct ocrdma_pd *pd = get_ocrdma_pd(ibpd);
> + struct ocrdma_dev *dev = pd->dev;
> + int status;
> + u64 usr_db;
> +
> + if (atomic_read(&pd->use_cnt)) {
> + ocrdma_err("%s(%d) pd=0x%x is in use.\n",
> + __func__, dev->id, pd->id);
> + status = -EFAULT;
> + goto dealloc_err;
> + }
all of the use_cnt tracking in this driver seems to duplicate what the rdma
midlayer already does... is there any reason we need that in the low-level
hardware driver too, or can we just get rid of the various use_cnt members?
--
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
