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

[Michael S. Tsirkin]

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

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

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

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

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

-- 
MST