[openib-general] [PATCH 02/18] [RFC] Provider Device Discovery

[Steve Wise]

On Mon, 2006-03-06 at 14:28 -0800, Roland Dreier wrote:
>  > +t3c_cpl_handler_t t3c_handlers[NUM_CPL_CMDS];
> 

ok.

> Can we kill the _t suffix here?  I had to search through the code to
> see that this is a function type, so it would be better to call this
> something like t3c_cpl_handler_func.
> 
>  > +DECLARE_MUTEX(dev_mutex);
> 
> static DEFINE_MUTEX() instead?  (ie make it static since it's only
> used in this function, and make it a real mutex_lock/mutex_unlock
> mutex instead of a (deprecated) semaphore).
> 

Ok.  I'll change all semaphores to mutex.


>  > +	if (!(rnicp->pdid2hlp = kzalloc(sizeof(void *) * T3_MAX_NUM_PD, 
>  > +	                                GFP_KERNEL)))
>  > +		goto pdid_err;
> 
> kernel idiom is to do
> 
> 	rnicp->pdid2hlp = kzalloc(sizeof(void *) * T3_MAX_NUM_PD, GFP_KERNEL);
> 	if (!rnicp->pdid2hlp)
> 		goto pdid_err;
> 
> it's the same number of lines and it's easier to read.
> 

agreed.  I'll fix this everywhere.

>  > +	rnicp->attr.maxPDs = T3_MAX_NUM_PD - 1;
>  > +	rnicp->attr.memPgSizesBitMask = 0x7FFF;	/* 4KB-128MB */
>  > +	rnicp->attr.canResizeWQ = 0;
>  > +	rnicp->attr.maxRDMAreadsPerQP = 8;
>  > +	rnicp->attr.maxRDMAreadResources =
>  > +	    rnicp->attr.maxRDMAreadsPerQP * rnicp->attr.maxQPs;
>  > +	rnicp->attr.maxRDMAreadQPdepth = 8;	/* IRD */
>  > +	rnicp->attr.maxRDMAreaddepth =
>  > +	    rnicp->attr.maxRDMAreadQPdepth * rnicp->attr.maxQPs;
> 
> Please switch from StudlyCAPS to underscore_between_words style for
> these member names.
> 

SorryAboutThat. will_change.

:-)

>  > +	list_for_each_entry(dev, &dev_list, entry) {
>  > +		if (dev->rdev.t3cdev_p == tdev) {
>  > +			list_del(&dev->entry);
>  > +			iwch_unregister_device(dev);
>  > +			cxio_rdev_close(&dev->rdev);
>  > +			kfree(dev->pdid2hlp);
>  > +			kfree(dev->cqid2hlp);
>  > +			kfree(dev->stag2hlp);
>  > +			kfree(dev->qpid2hlp);
>  > +			ib_dealloc_device(&dev->ibdev);
> 
> This needs to be list_for_each_entry_safe(), because
> ib_dealloc_device() is freeing dev and then list_for_each_entry
> dereferences it the next time through the loop.
> 

Yup!  

Is it SOP to post the resulting patch for these changes?

Steve.