[openib-general] more comments on cxgb3

[Steve Wise]

On Thu, 2007-02-08 at 08:40 +0200, Michael S. Tsirkin wrote:
> OK, so I looked at cxgb3 some more.

Thanks!

> To summarise my previous comments, I think the cxio hal layer needs to go to
> make the code readable - if I understand correctly it is there for historical
> reasons only.
> 

I can do this but its low on the list of todos.


> I started looking at userspace/kernel interaction, and then
> went over to other code under cxgb3 (but not core/).
> 
> - Consider a user that does e.g. create QP, but never calls mmap.
>   Is there some code that will clean out the unclamed mmap object?
>   I couldn't find it, and iwch_dealloc_ucontext does not seem to
>   do anything with it.
> 

This is a bug.  I've got a fix for it too.  

> - Passing physical address to userspace and back looks suspicios.
>   Especially this:
>                 uresp.physaddr = virt_to_phys(chp->cq.queue);
>   Could you elaborate on the design here? What are these phy addresses
>   and how come userspace needs to know the phy address?
>   You are not doing DMA by this address, by any chance?
> 

No, Its not used for DMA by the HW.  The physaddr is passed up to the
user, and the user then mmaps() using that as the offset.

I took this code from the ipath driver.  It has been pointed out to me
that this is broken for 32b userspace on a 64 kernel because mmap2()
cannot pass down 64 bits.  So I need to rework this code.

> - It seems that by passing in huge resource sizes, userspace will be able to
>   drink up unlimited amounts of kernel memory.
>   mthca handles this by using the mlock rlimit, should something be done here
>   as well?

Hmm.  That's a good point.  I'll put this on the todo as well.  So mthca
adds to process's rlimit value as things are allocated out of kernel
memory (or maybe even anything that gets pinned)?

>  
> A couple of comments on PDBG macro.
> - I'd like to suggest following the practice of prefixing macro names with module name
>   (same goes for functions like get_mhp really) - unless they are local to file.
> 
> - You are using __FUNCTION__ a lot - it might be to just to kill it,
>   messages are unique so you'll be able to locate the msg source anyway,
>   save some kernel text and logs will be shorter. In any case I think
>   __func__ is the recommended gcc way to get the name currently.
> 
> - comment near pr_debug definition in include/linux/kernel.h says:
> 	/* If you are writing a driver, please use dev_dbg instead */
>   so it might be a good idea for PDBG to follow this rule.
> 
> - log messages do not look very informative to me.
>   I also think they are a bit too many of them currently.
>   For example, I do not think it is a good idea to print
>   the kernel pointers out.
> 
>   For example, in code like the following:
> 	mhp = get_mhp(rhp, (sg_list[i].lkey) >> 8);
> 	if (!mhp) {
> 		PDBG("%s %d\n", __FUNCTION__, __LINE__);
> 		return -EIO;
> 	}
> 
>   might be better to say 
>   "MR key XXX does not exist. Exiting.".
>   -EIO also looks like a strange error code to return here, does it not?
>   Maybe something like EINVAL would be more appropriate?
> 

I'll take a todo to clean up the debug stuff. 

> - I wonder about the names like get_mhp - what does "mhp" mean?
> static inline struct iwch_mr *get_mhp(struct iwch_dev *rhp, u32 mmid)
> {
>         return idr_find(&rhp->mmidr, mmid);
> }
> 

Memory Handle Pointer.


> Looks like it looks up an mr. Is that right? Maybe the name shouldbe changed
> to convey this meaning.
> 
> - In the following code, what does "missing pdid check" mean?
> /*
>  * TBD: this is going to be moved to firmware. Missing pdid/qpid check for now.
>  */
> This sounds interesting.
> Does this mean the code does not validate the PD currently?
> 

I need firmware support for this.  It will be done asap and I can remove
this code entirely.

> I have the same question for:
>         /* TBD: check perms */
> in iwch_bind_mw.
> 
> BTW, does TBD stand for "To Be Done" here?

Yes.

> Do you mean TODO really?
> 
> - iwch_sgl2pbl_map is used in several places, and seems a bit too big to be inline
> 
> Well, it's time to go do my day job now :)
> 
> Hope this helps,
> 

Thanks again Michael!

Steve.