[ofw][mlx4] SRQ attached QP issue

[Alex Estrin]

Do you think it makes sense to propagate the fix to 2.0 branch?
Thanks,
Alex.

> -----Original Message-----
> From: Leonid Keller [mailto:leonid at mellanox.co.il] 
> Sent: Thursday, October 02, 2008 5:27 AM
> To: Alex Estrin
> Cc: ofw at lists.openfabrics.org
> Subject: RE: [ofw][mlx4] SRQ attached QP issue
> 
> Right. 
> Returned to your variant.
> Fixed in 1627, thank you. 
> 
> > -----Original Message-----
> > From: Alex Estrin [mailto:alex.estrin at qlogic.com] 
> > Sent: Monday, September 29, 2008 3:01 PM
> > To: Leonid Keller
> > Cc: ofw at lists.openfabrics.org
> > Subject: RE: [ofw][mlx4] SRQ attached QP issue
> > 
> > Hello,
> > 
> > Your version of the patch won't work because for the SRQ case 
> > this will always be 'TRUE':
> > > -		if (!qp->sq.wrid || !qp->rq.wrid) {
> > > -			err = -ENOMEM;
> > > -			goto err_wrid;
> > 
> > Thanks,
> > Alex.
> > 
> > > -----Original Message-----
> > > From: Leonid Keller [mailto:leonid at mellanox.co.il]
> > > Sent: Sunday, September 28, 2008 11:13 AM
> > > To: Alex Estrin
> > > Cc: ofw at lists.openfabrics.org
> > > Subject: RE: [ofw][mlx4] SRQ attached QP issue
> > > 
> > > Applied with some changes in 1625, thank you. 
> > > 
> > > > -----Original Message-----
> > > > From: Alex Estrin [mailto:alex.estrin at qlogic.com]
> > > > Sent: Saturday, September 27, 2008 1:20 AM
> > > > To: Leonid Keller
> > > > Cc: ofw at lists.openfabrics.org
> > > > Subject: [ofw][mlx4] SRQ attached QP issue
> > > > 
> > > > Hello,
> > > > 
> > > > This patch seem fixed the create-QP-attached-to-SRQ issue.
> > > > I didn't verify though, if code allowes send queue zero size. 
> > > > Included check anyway.
> > > > Please review.
> > > > 
> > > > Thanks,
> > > > Alex.
> > > > 
> > > > Index: mlx4/kernel/bus/ib/qp.c
> > > > 
> > ===================================================================
> > > > --- mlx4/kernel/bus/ib/qp.c	(revision 1622)
> > > > +++ mlx4/kernel/bus/ib/qp.c	(working copy)
> > > > @@ -411,14 +411,20 @@
> > > >  		err = mlx4_buf_write_mtt(dev->dev, 
> &qp->mtt, &qp->buf);
> > > >  		if (err)
> > > >  			goto err_mtt;
> > > > -
> > > > -		qp->sq.wrid  = kmalloc(qp->sq.wqe_cnt * 
> sizeof (u64),
> > > > GFP_KERNEL);
> > > > -		qp->rq.wrid  = kmalloc(qp->rq.wqe_cnt * 
> sizeof (u64),
> > > > GFP_KERNEL);
> > > > -
> > > > -		if (!qp->sq.wrid || !qp->rq.wrid) {
> > > > -			err = -ENOMEM;
> > > > -			goto err_wrid;
> > > > +		if (qp->sq.wqe_cnt) {
> > > > +			qp->sq.wrid  = 
> kmalloc(qp->sq.wqe_cnt * sizeof
> > > > (u64), GFP_KERNEL);
> > > > +			if (!qp->sq.wrid) {
> > > > +				err = -ENOMEM;
> > > > +				goto err_wrid;
> > > > +			}
> > > >  		}
> > > > +		if (qp->rq.wqe_cnt) {
> > > > +			qp->rq.wrid  = 
> kmalloc(qp->rq.wqe_cnt * sizeof
> > > > (u64), GFP_KERNEL);
> > > > +			if (!qp->rq.wrid) {
> > > > +				err = -ENOMEM;
> > > > +				goto err_wrid;
> > > > +			}
> > > > +		}
> > > >  	}
> > > >  
> > > >  	if (!sqpn)
> > > > @@ -452,8 +458,10 @@
> > > >  			
> mlx4_ib_db_unmap_user(to_mucontext(pd->p_uctx),
> > > >  					      &qp->db);
> > > >  	} else {
> > > > -		kfree(qp->sq.wrid);
> > > > -		kfree(qp->rq.wrid);
> > > > +		if (qp->sq.wrid)
> > > > +			kfree(qp->sq.wrid);
> > > > +		if (qp->rq.wrid)
> > > > +			kfree(qp->rq.wrid);
> > > >  	}
> > > >  
> > > >  err_mtt:
> > > > 
> > > > 
> > > > 
> > > > > Hello,
> > > > > 
> > > > > It seem there is a controversy exist in mlx4 driver that
> > > > won't allow
> > > > > to create QP if it was attached to SRQ.
> > > > > 
> > > > > At first it doesn't allow to set any recv queue size
> > > (which is fine)
> > > > > 
> > > > > mlx4\kernel\bus\ib\qp.c@ line 219:
> > > > > 		if (cap->max_recv_wr)
> > > > > 			return -EINVAL;
> > > > > 
> > > > > 		qp->rq.wqe_cnt = qp->rq.max_gs = 0;
> > > > > 
> > > > > But then there is assertion for zero size memory request in
> > > > 'kmalloc':
> > > > > mlx4\kernel\bus\ib\qp.c @ line 416:
> > > > > 
> > > > > 		qp->rq.wrid  = kmalloc(qp->rq.wqe_cnt * sizeof
> > > > (u64), GFP_KERNEL);
> > > > > 		
> > > > > mlx4\kernel\inc\l2w_memory.h @ line 81:
> > > > > 
> > > > > 		static inline void * kmalloc( SIZE_T bsize,
> > > > gfp_t gfp_mask)
> > > > > 		{
> > > > > 			void *ptr;
> > > > > 			ASSERT( KeGetCurrentIrql() <= 
> > DISPATCH_LEVEL);
> > > > > 			ASSERT(bsize);
> > > > > 		.....
> > > > > 
> > > > > I believe it was added for a reason.
> > > > > Please let me know if I missed anything.
> > > > > 
> > > > > Thanks,
> > > > > Alex.
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > 
> > 
>