[openib-general] more comments on cxgb3

[Michael S. Tsirkin]

OK, so I looked at cxgb3 some more.
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 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.

- 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?

- 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?
 
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 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);
}

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 have the same question for:
        /* TBD: check perms */
in iwch_bind_mw.

BTW, does TBD stand for "To Be Done" here?
google says:
>Definitions of TBD on the Web:

    * To Be Determined, Defined, Decided.
      www.csr.com/ptot.htm

    * to be determined
      www.liberalsagainstterrorism.com/wiki/index.php/Counterinsurgency_Operations/Glossary

    * Treasury Board (Secretariat)
      www.psc-cfp.gc.ca/centres/definitions_and_notes_e.htm

    * The three letter abbreviation TBD may be/mean, depending on context: * an  acronym for "To Be Determined" ("...at a later point in time.", typically)* the Douglas Devastator, a US Navy torpedo bomber of World War II
      en.wikipedia.org/wiki/TBD

What is to be determined here?
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,

-- 
MST