Re: [PATCH] Add iSCSI iBFT support.

[Randy Dunlap <randy.dunlap@oracle.com>]

On Wed, 26 Sep 2007 20:52:43 -0400 Konrad Rzeszutek wrote:

> > > +config ISCSI_IBFT
> > > +	tristate "iSCSI Boot Firmware Table Attributes"
> > > +	depends on X86
> >
> > why only on X86?
> 
> PowerPC exports this data via the OpenFirmware so it already shows in 
> the /sysfs entries. I was thinking to combine those sysfs entries under this 
> code, but that is something in the future.
> 
> In regards to all other platforms, I figured I would only make it supported 
> under platforms that have been tested. Is there anything that stops this from 
> working for example of IA64? Well no. The hardware that supports the iBFT is 
> either in the BIOS or in NICs - so the SGI or HP boxes _should_ work, however 
> I am not comfortable to make it supported unless I've tested it.

That's not how Linux development works.  You (we) have a huge
test lab around the world.  You practice "release early, release
often" and get testing/feedback on it.  Maybe even patches.  :)


> > Need blank line here... except why is this function in the header
> 
> Fixed blank line.
> > file?  and why is it inline?
> 
> Q: "Why is this function in the header file"
> If the find_ibft() was to be implemented in firmware/iscsi_ibft.c code the 
> firmware-driver couldn't be compiled as a module (or rather it could, but 
> when the vmlinuz was to be linked it would complain about missing symbol - 
> find_ibft). I was thinking to let the user give the choice whether they want 
> to load this firmware driver or have it built-in the kernel.
> 
> Q:"Why is it inline"
> Uhh. It does not need to. I will remove the 'inline' part. 

But we strongly prefer not to have non-inline C code in header files,
[and the function does not need to be inline]
so find_ibft() needs a home in some source file/code that won't be built
as a loadable module, right?  And preferably not duplicated (i386 &
x86_64 versions; but we should see a merged x86/ arch soon, so it
sounds).  Would ia64 have its own version of find_ibft() or use this
same code?

I think that for now you can put find_ibft() in both setup.c files
and the merged x86/ arch tree can eliminate one of them.

On looking back at the patch, why aren't the ibft_phys and find_ibft()
parts of both setup.c patches surrounded by #ifdef CONFIG_ISCSI_IBFT &
#endif ?


---
~Randy
Phaedrus says that Quality is about caring.