OK, the "real" patches are coming now. I just thought I would let you
know which of your comments I've acted on and which are still pending....
Christoph> Please avoid -I statements for kernel code wherever
Christoph> possible. Just put the Infiniband Kernel Internal API
Christoph> into $(TOPDIR)/include/infiniband/
This is no problem to do. However, I'd like to see whether we can get
a consensus about where to put .h files.
Christoph> This should read more like:
OK, Makefile is fixed. I didn't realize foo-objs and foo-y worked the
same, thanks for teaching me.
Christoph> Also ib_ipoib is a rather strange name, the double ib
Christoph> doesn't make much sense.
True. We use ib_xxx for all our module names, so ib_ipoib is
consistent, but changing to ipoib for the IPoIB driver is easy to do
if that's the right way to go.
Christoph> Please don't refer to licenses at urls as they can
Christoph> changed easily. There's a toplevel COPYING file you
Christoph> can reference, and if you want additional license bits
Christoph> I'd suggets to keep them simple enough to be in the
Christoph> file header. Or better just stop the dual-licensing
Christoph> sillyness - you'll copy code from other parts of the
Christoph> kernel sooner or later that are GPL-only.
License is not fixed yet (needs to be cleared with all contributors).
Christoph> Please make this and inline so you have proper
Christoph> typechecking. (And give it a less shouting name)
Done.
Christoph> Please try to avoid global lists. Looking at the code
Christoph> this is used in two places:
Christoph> (1) the debugging pseudo fs (2) ipoib_remove_one
Christoph> the latter would be much better served with a per
Christoph> ib_device list anyway, and for (1) there must be a
Christoph> better way than the global list either.
OK, I got rid of the global list, although the debug fs keeps a list
of devices added before the fs was mounted (static to the file with
debugging code). I could get rid of that list by just creating the fs
superblock etc. at startup rather than waiting for a mount request,
but I didn't do that yet. (I don't want to call kern_mount because
then the module use count gets bumped and the module can't be
unloaded)
Christoph> Please don't use PCI dma calls in highlevel protocol
Christoph> handlers. At least use the dma_* calls or even better
Christoph> restructure the code to avoid dma mapping outside the
Christoph> HCA driver.
We can switch to dma_* (and in fact I see some reasons to do so).
However Greg KH suggested sticking with pci_* stuff until there's a
non-PCI device to deal with.
I'm pretty convinced that the DMA mapping needs to be exposed outside
the low-level driver. Otherwise you're limiting what upper level code
can do with reusing DMA mappings, DMA pools, etc.
Thanks,
Roland
|