[openib-general] [PATCH v2 1/2] ofed_1_2 Changes to kernel_patches/ for Chelsio T3 Support.

[Steve Wise]

On Fri, 2007-01-12 at 01:06 +0200, Michael S. Tsirkin wrote:
> > Quoting Steve Wise <swise at opengridcomputing.com>:
> > Subject: Re: [PATCH v2 1/2] ofed_1_2 Changes to kernel_patches/ for Chelsio T3 Support.
> > 
> > On Fri, 2007-01-12 at 00:33 +0200, Michael S. Tsirkin wrote:
> > > For genalloc,
> > > 1. It's a backport, why do you put it in fixes?   
> > 
> > Because its not exactly a backport.  The exact file exists in 2.6.20 but
> > it is only conditionally compiled into the kernel.
> 
> I don't really understand. Maybe I am missing something?
> So you want it for older kernels, right?
> So how is it not a backport?
> 

The issue is slightly different for genalloc.  In 2.6.18 and later
genalloc.c exists in lib/genalloc.c.  But it is configured into the
kernel build ONLY if somebody else in the kernel wants it by having a
config dependency on CONFIG_GENERIC_ALLOCATOR.  So on any given customer
system with 2.6.19, for instance, they may or may not have built this in
based on what other modules they've built that may or may not have a
config dependency on CONFIG_GENERIC_ALLOCATOR.  Now, If they DID build
it in, then we cannot add in our own genalloc functions with the same
symbol names (I think) because they are exported from that kernel.  But
if they are _not_ exported on the customer's system/kernel, then we need
the functionality for sure.

I chose to simply always add the genalloc service _and_ change the
function names so that I wouldn't collide with kernels that have it
configured in.

Does this make sense?

So I'm claiming that under my current scheme/design (which is of course
can change :) I need these services in every build regardless of which
kernel we're building against.  




> > But I can put it
> > backport if you want.  That makes more sense I guess.
> > 
> > > 2. I think a better way would be to stick it under kernel_addons/backports than
> > >    as a patch. Please see my earlier mail on how this works.
> > >  
> > 
> > Um, what earlier mail?
> > 
> > >    It is *much* easier to maintain this way (you see the full file).
> > >    And also benefits all the subsystem and not just chelsio.
> > >    We currently have infrastructure to add headers only, so what you do
> > >    for C is stick iit under backports/<kernel>/src/genpool.c and
> > >    add a backport patch that just adds a small file pulling
> > >    in the real code, like this:
> > > 
> > > +#include "src/genpool.c"
> > > 
> > >   and also adds the relevant line to Makefile to stick it in core.
> > > 
> > 
> > Ok, so add this into ib_core then?
> 
> Yes. But the bulk of the code goes under
> kernel_addons/backports/<version>/include/src/genpool.c
> and kernel_addons/<version>/include/src/genpool.h
> 
> > >   Look at how we did this for
> > >   kernel_addons/backport/2.6.5_sles9_sp3/include/src/stream.c
> > > 
> > 
> > ok.
> > 
> > > 
> > > Or, if you have an idea how to get rid of the two-line patch
> > > that would be fine too. Sticking all these things in core is ugly
> > > and creates Makefile patches that often trigger conflicts.
> > > Maybe we should have an ib_backports module built?
> > > This might be a good idea, I'll think about it next week.
> > > 
> > 
> > A backports modules sounds interesting...
> 
> In a good way?
> 

Yea. :-)

> > > 3. Is there a real reason to call it iwch_gen_pool and not just gen_pool
> > > as in 2.6.20? The patch would be much smaller then.
> > > 
> > 
> > What if its already built in and export in the kernel we're trying to
> > load into?  Will this cause a load problem?  I was assuming it
> > would...that's why I changed the names.  Am I wrong?
> 
> Yes. that's why kernel_addons/backports/<xxx> has lots of directories
> for each kernel version, and same for kernel_patches/backports.
> Backports (patches and addons) are per kernel. So for kernels which have genpool
> (e.g. 2.6.20) you do not add this file and so there will be no conflict.
> For older kernels you don't.
> 

See my explanation above. 

> > > 4. I think you want to remove playing with EXTRA_CFLAGS
> > > for your driver: these things should be controlled centrally.
> > > Instead of -DDEBUG just do
> > > #ifdef CONFIG_INFINIBAND_CXGB3_DEBUG in your code.
> > > you really do not want to touch the global DEBUG macro.
> > > 
> > 
> > This was feedback we got from reviewers on the amso driver.  They said
> > use DEBUG.
> 
> Hmm, sounds weird, and that's not what we do for other modules.
> the question is not ofed specific, same applies to upstream.
> Roland?
> 

This review comment came from lklm.  They said "dont create new debug
#defines...use -DDEBUG...

> > > And -g is something user should ask explicitly.
> > > 
> > 
> > The user asks explicitly by requesting to compile the debug module.
> 
> Kernel has a global option "compile with debug symbols" which adds -g.
> Why does your driver need a separate one?
> 

Perhaps not.