subject:"Re\: \[PATCH\] Add iSCSI IBFT Support \(v0.3\)"

On Wed, Nov 28, 2007 at 04:24:32PM -0400, [EMAIL PROTECTED] wrote:
> > But, why not just put it in a separate file, that is built in if the
> > user wants iscsi support?  That way the setup code can call it properly
> > if needed.
> 
> In what directory should I put that file? It can't be in the arch/*
> directories b/c the iscsi_ibft.c wouldn't build on all platforms.
> Should I put it in drivers/firmware ?

Put it in the same directory your other iscsi files are.  Or wherever
you feel it is best suited.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

> > > I didn't realize an external file, outside of your changes, needed this
> > > function.  If it does, then perhaps you need to just place it elsewhere.
> > 
> > The fundamental problem is that 'find_ibft' ought to be available
> > from anywhere (or at least from the iscsi_ibft.c) so that the iscsi_ibft
> > module can be loaded on any platform. But on x86, it needs to be called
> >  from setup_[32|64].c because the IBFT can be located anywhere
> > between 512KB and 1MB - and the E820 does not neccesarily have to
> > exclude that region. Hence the patch I proposed implements a
> > 'reserve_ibft_region' code which would reserve the region of memory
> > found by 'find_ibft' so that it can be preserved when iscsi_ibft
> > module is actually loaded.
> > 
> > It ends up that there are three consumers of 'find_ibft':
> >  a) the module itself (iscsi_ibft.c)
> >  b) setup_32.c
> >  c) setup_64.c
> > 
> > The first choice, which looked the most flexible, was to have it
> > in iscsi_ibft.h file.
> > The second one, which would inhibit the user from making iscsi_ibft
> > a module, would be to move it to iscsi_ibft.c and make it
> > EXPORT_SYMBOL(), but that seems wasteful from a memory foot-print
> > look.
> > A third option was to put in /lib, but that doesn't seem right - this
> > 'find_ibft' code is specific to this module.
> > 
> > Of all the options, the cleanest looks to be the first choice :-(
> > (I am not trying to be obstinate here - I just can't think of any
> > other reasonable place).
> 
> If you insist on putting it in a .h file, it needs to be marked "inline"
> at the least.

Ok.

> But, why not just put it in a separate file, that is built in if the
> user wants iscsi support?  That way the setup code can call it properly
> if needed.

In what directory should I put that file? It can't be in the arch/*
directories b/c the iscsi_ibft.c wouldn't build on all platforms.
Should I put it in drivers/firmware ?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Wed, Nov 28, 2007 at 03:21:40PM -0400, [EMAIL PROTECTED] wrote:
> On Tue, Nov 27, 2007 at 11:09:19AM -0800, Greg KH wrote:
> > On Tue, Nov 27, 2007 at 02:09:50PM -0400, [EMAIL PROTECTED] wrote:
> > > On Mon, Nov 26, 2007 at 09:29:55PM -0800, Greg KH wrote:
> > > > On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
> > > > > On Monday 26 November 2007 22:31:38 Greg KH wrote:
> > > > > > > +#if defined(CONFIG_ISCSI_IBFT) || 
> > > > > > > defined(CONFIG_ISCSI_IBFT_MODULE)
> > > > > ..snip..
> > > > > > > +static ssize_t find_ibft(void)
> > > > > > > +{
> > > > > ..snip..
> > > > > > > +}
> > > > > >
> > > > > > What is a function (not even an inline one) doing in a .h file?
> > > > > 
> > > > > I was not sure where to put it. This function (find_ibft) is used by 
> > > > > the 
> > > > > setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in 
> > > > > .c file, 
> > > > > but I am not sure exactly where? Should I make a new file in called 
> > > > > libs/iscsi_ibft_helper.c ?
> > > > 
> > > > Put it in your .c file and make it a global function to be called by
> > > > someone else if they need it.
> > > 
> > > If the kernel is built with CONFIG_ISCSI_IBFT=m, the
> > > setup_[32|64],c code would depend on the 'find_ibft' symbol which is
> > > in a module (in the iscsi_ibft.c), which is not available during
> > > the bootup phase and not linked to vmlinuz.
> > 
> > Ah, then don't allow that :)
> > 
> > > This isn't an issue if the module is built with CONFIG_ISCSI_IBFT=y of
> > > course.
> > > 
> > > Or did by 'your .c file' mean a new file in arch/x86/kernel directory?
> > 
> > I didn't realize an external file, outside of your changes, needed this
> > function.  If it does, then perhaps you need to just place it elsewhere.
> 
> The fundamental problem is that 'find_ibft' ought to be available
> from anywhere (or at least from the iscsi_ibft.c) so that the iscsi_ibft
> module can be loaded on any platform. But on x86, it needs to be called
>  from setup_[32|64].c because the IBFT can be located anywhere
> between 512KB and 1MB - and the E820 does not neccesarily have to
> exclude that region. Hence the patch I proposed implements a
> 'reserve_ibft_region' code which would reserve the region of memory
> found by 'find_ibft' so that it can be preserved when iscsi_ibft
> module is actually loaded.
> 
> It ends up that there are three consumers of 'find_ibft':
>  a) the module itself (iscsi_ibft.c)
>  b) setup_32.c
>  c) setup_64.c
> 
> The first choice, which looked the most flexible, was to have it
> in iscsi_ibft.h file.
> The second one, which would inhibit the user from making iscsi_ibft
> a module, would be to move it to iscsi_ibft.c and make it
> EXPORT_SYMBOL(), but that seems wasteful from a memory foot-print
> look.
> A third option was to put in /lib, but that doesn't seem right - this
> 'find_ibft' code is specific to this module.
> 
> Of all the options, the cleanest looks to be the first choice :-(
> (I am not trying to be obstinate here - I just can't think of any
> other reasonable place).

If you insist on putting it in a .h file, it needs to be marked "inline"
at the least.

But, why not just put it in a separate file, that is built in if the
user wants iscsi support?  That way the setup code can call it properly
if needed.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Tue, Nov 27, 2007 at 11:09:19AM -0800, Greg KH wrote:
> On Tue, Nov 27, 2007 at 02:09:50PM -0400, [EMAIL PROTECTED] wrote:
> > On Mon, Nov 26, 2007 at 09:29:55PM -0800, Greg KH wrote:
> > > On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
> > > > On Monday 26 November 2007 22:31:38 Greg KH wrote:
> > > > > > +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
> > > > ..snip..
> > > > > > +static ssize_t find_ibft(void)
> > > > > > +{
> > > > ..snip..
> > > > > > +}
> > > > >
> > > > > What is a function (not even an inline one) doing in a .h file?
> > > > 
> > > > I was not sure where to put it. This function (find_ibft) is used by 
> > > > the 
> > > > setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c 
> > > > file, 
> > > > but I am not sure exactly where? Should I make a new file in called 
> > > > libs/iscsi_ibft_helper.c ?
> > > 
> > > Put it in your .c file and make it a global function to be called by
> > > someone else if they need it.
> > 
> > If the kernel is built with CONFIG_ISCSI_IBFT=m, the
> > setup_[32|64],c code would depend on the 'find_ibft' symbol which is
> > in a module (in the iscsi_ibft.c), which is not available during
> > the bootup phase and not linked to vmlinuz.
> 
> Ah, then don't allow that :)
> 
> > This isn't an issue if the module is built with CONFIG_ISCSI_IBFT=y of
> > course.
> > 
> > Or did by 'your .c file' mean a new file in arch/x86/kernel directory?
> 
> I didn't realize an external file, outside of your changes, needed this
> function.  If it does, then perhaps you need to just place it elsewhere.

The fundamental problem is that 'find_ibft' ought to be available
from anywhere (or at least from the iscsi_ibft.c) so that the iscsi_ibft
module can be loaded on any platform. But on x86, it needs to be called
 from setup_[32|64].c because the IBFT can be located anywhere
between 512KB and 1MB - and the E820 does not neccesarily have to
exclude that region. Hence the patch I proposed implements a
'reserve_ibft_region' code which would reserve the region of memory
found by 'find_ibft' so that it can be preserved when iscsi_ibft
module is actually loaded.

It ends up that there are three consumers of 'find_ibft':
 a) the module itself (iscsi_ibft.c)
 b) setup_32.c
 c) setup_64.c

The first choice, which looked the most flexible, was to have it
in iscsi_ibft.h file.
The second one, which would inhibit the user from making iscsi_ibft
a module, would be to move it to iscsi_ibft.c and make it
EXPORT_SYMBOL(), but that seems wasteful from a memory foot-print
look.
A third option was to put in /lib, but that doesn't seem right - this
'find_ibft' code is specific to this module.

Of all the options, the cleanest looks to be the first choice :-(
(I am not trying to be obstinate here - I just can't think of any
other reasonable place).
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Tue, Nov 27, 2007 at 11:09:19AM -0800, Greg KH wrote:
 On Tue, Nov 27, 2007 at 02:09:50PM -0400, [EMAIL PROTECTED] wrote:
  On Mon, Nov 26, 2007 at 09:29:55PM -0800, Greg KH wrote:
   On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
On Monday 26 November 2007 22:31:38 Greg KH wrote:
  +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
..snip..
  +static ssize_t find_ibft(void)
  +{
..snip..
  +}

 What is a function (not even an inline one) doing in a .h file?

I was not sure where to put it. This function (find_ibft) is used by 
the 
setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c 
file, 
but I am not sure exactly where? Should I make a new file in called 
libs/iscsi_ibft_helper.c ?
   
   Put it in your .c file and make it a global function to be called by
   someone else if they need it.
  
  If the kernel is built with CONFIG_ISCSI_IBFT=m, the
  setup_[32|64],c code would depend on the 'find_ibft' symbol which is
  in a module (in the iscsi_ibft.c), which is not available during
  the bootup phase and not linked to vmlinuz.
 
 Ah, then don't allow that :)
 
  This isn't an issue if the module is built with CONFIG_ISCSI_IBFT=y of
  course.
  
  Or did by 'your .c file' mean a new file in arch/x86/kernel directory?
 
 I didn't realize an external file, outside of your changes, needed this
 function.  If it does, then perhaps you need to just place it elsewhere.

The fundamental problem is that 'find_ibft' ought to be available
from anywhere (or at least from the iscsi_ibft.c) so that the iscsi_ibft
module can be loaded on any platform. But on x86, it needs to be called
 from setup_[32|64].c because the IBFT can be located anywhere
between 512KB and 1MB - and the E820 does not neccesarily have to
exclude that region. Hence the patch I proposed implements a
'reserve_ibft_region' code which would reserve the region of memory
found by 'find_ibft' so that it can be preserved when iscsi_ibft
module is actually loaded.

It ends up that there are three consumers of 'find_ibft':
 a) the module itself (iscsi_ibft.c)
 b) setup_32.c
 c) setup_64.c

The first choice, which looked the most flexible, was to have it
in iscsi_ibft.h file.
The second one, which would inhibit the user from making iscsi_ibft
a module, would be to move it to iscsi_ibft.c and make it
EXPORT_SYMBOL(), but that seems wasteful from a memory foot-print
look.
A third option was to put in /lib, but that doesn't seem right - this
'find_ibft' code is specific to this module.

Of all the options, the cleanest looks to be the first choice :-(
(I am not trying to be obstinate here - I just can't think of any
other reasonable place).
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Wed, Nov 28, 2007 at 03:21:40PM -0400, [EMAIL PROTECTED] wrote:
 On Tue, Nov 27, 2007 at 11:09:19AM -0800, Greg KH wrote:
  On Tue, Nov 27, 2007 at 02:09:50PM -0400, [EMAIL PROTECTED] wrote:
   On Mon, Nov 26, 2007 at 09:29:55PM -0800, Greg KH wrote:
On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
 On Monday 26 November 2007 22:31:38 Greg KH wrote:
   +#if defined(CONFIG_ISCSI_IBFT) || 
   defined(CONFIG_ISCSI_IBFT_MODULE)
 ..snip..
   +static ssize_t find_ibft(void)
   +{
 ..snip..
   +}
 
  What is a function (not even an inline one) doing in a .h file?
 
 I was not sure where to put it. This function (find_ibft) is used by 
 the 
 setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in 
 .c file, 
 but I am not sure exactly where? Should I make a new file in called 
 libs/iscsi_ibft_helper.c ?

Put it in your .c file and make it a global function to be called by
someone else if they need it.
   
   If the kernel is built with CONFIG_ISCSI_IBFT=m, the
   setup_[32|64],c code would depend on the 'find_ibft' symbol which is
   in a module (in the iscsi_ibft.c), which is not available during
   the bootup phase and not linked to vmlinuz.
  
  Ah, then don't allow that :)
  
   This isn't an issue if the module is built with CONFIG_ISCSI_IBFT=y of
   course.
   
   Or did by 'your .c file' mean a new file in arch/x86/kernel directory?
  
  I didn't realize an external file, outside of your changes, needed this
  function.  If it does, then perhaps you need to just place it elsewhere.
 
 The fundamental problem is that 'find_ibft' ought to be available
 from anywhere (or at least from the iscsi_ibft.c) so that the iscsi_ibft
 module can be loaded on any platform. But on x86, it needs to be called
  from setup_[32|64].c because the IBFT can be located anywhere
 between 512KB and 1MB - and the E820 does not neccesarily have to
 exclude that region. Hence the patch I proposed implements a
 'reserve_ibft_region' code which would reserve the region of memory
 found by 'find_ibft' so that it can be preserved when iscsi_ibft
 module is actually loaded.
 
 It ends up that there are three consumers of 'find_ibft':
  a) the module itself (iscsi_ibft.c)
  b) setup_32.c
  c) setup_64.c
 
 The first choice, which looked the most flexible, was to have it
 in iscsi_ibft.h file.
 The second one, which would inhibit the user from making iscsi_ibft
 a module, would be to move it to iscsi_ibft.c and make it
 EXPORT_SYMBOL(), but that seems wasteful from a memory foot-print
 look.
 A third option was to put in /lib, but that doesn't seem right - this
 'find_ibft' code is specific to this module.
 
 Of all the options, the cleanest looks to be the first choice :-(
 (I am not trying to be obstinate here - I just can't think of any
 other reasonable place).

If you insist on putting it in a .h file, it needs to be marked inline
at the least.

But, why not just put it in a separate file, that is built in if the
user wants iscsi support?  That way the setup code can call it properly
if needed.

thanks,

greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Wed, Nov 28, 2007 at 04:24:32PM -0400, [EMAIL PROTECTED] wrote:
  But, why not just put it in a separate file, that is built in if the
  user wants iscsi support?  That way the setup code can call it properly
  if needed.
 
 In what directory should I put that file? It can't be in the arch/*
 directories b/c the iscsi_ibft.c wouldn't build on all platforms.
 Should I put it in drivers/firmware ?

Put it in the same directory your other iscsi files are.  Or wherever
you feel it is best suited.

thanks,

greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

   I didn't realize an external file, outside of your changes, needed this
   function.  If it does, then perhaps you need to just place it elsewhere.
  
  The fundamental problem is that 'find_ibft' ought to be available
  from anywhere (or at least from the iscsi_ibft.c) so that the iscsi_ibft
  module can be loaded on any platform. But on x86, it needs to be called
   from setup_[32|64].c because the IBFT can be located anywhere
  between 512KB and 1MB - and the E820 does not neccesarily have to
  exclude that region. Hence the patch I proposed implements a
  'reserve_ibft_region' code which would reserve the region of memory
  found by 'find_ibft' so that it can be preserved when iscsi_ibft
  module is actually loaded.
  
  It ends up that there are three consumers of 'find_ibft':
   a) the module itself (iscsi_ibft.c)
   b) setup_32.c
   c) setup_64.c
  
  The first choice, which looked the most flexible, was to have it
  in iscsi_ibft.h file.
  The second one, which would inhibit the user from making iscsi_ibft
  a module, would be to move it to iscsi_ibft.c and make it
  EXPORT_SYMBOL(), but that seems wasteful from a memory foot-print
  look.
  A third option was to put in /lib, but that doesn't seem right - this
  'find_ibft' code is specific to this module.
  
  Of all the options, the cleanest looks to be the first choice :-(
  (I am not trying to be obstinate here - I just can't think of any
  other reasonable place).
 
 If you insist on putting it in a .h file, it needs to be marked inline
 at the least.

Ok.

 But, why not just put it in a separate file, that is built in if the
 user wants iscsi support?  That way the setup code can call it properly
 if needed.

In what directory should I put that file? It can't be in the arch/*
directories b/c the iscsi_ibft.c wouldn't build on all platforms.
Should I put it in drivers/firmware ?

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

2007-11-27 Thread Greg KH

On Tue, Nov 27, 2007 at 02:09:50PM -0400, [EMAIL PROTECTED] wrote:
> On Mon, Nov 26, 2007 at 09:29:55PM -0800, Greg KH wrote:
> > On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
> > > On Monday 26 November 2007 22:31:38 Greg KH wrote:
> > > > > +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
> > > ..snip..
> > > > > +static ssize_t find_ibft(void)
> > > > > +{
> > > ..snip..
> > > > > +}
> > > >
> > > > What is a function (not even an inline one) doing in a .h file?
> > > 
> > > I was not sure where to put it. This function (find_ibft) is used by the 
> > > setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c 
> > > file, 
> > > but I am not sure exactly where? Should I make a new file in called 
> > > libs/iscsi_ibft_helper.c ?
> > 
> > Put it in your .c file and make it a global function to be called by
> > someone else if they need it.
> 
> If the kernel is built with CONFIG_ISCSI_IBFT=m, the
> setup_[32|64],c code would depend on the 'find_ibft' symbol which is
> in a module (in the iscsi_ibft.c), which is not available during
> the bootup phase and not linked to vmlinuz.

Ah, then don't allow that :)

> This isn't an issue if the module is built with CONFIG_ISCSI_IBFT=y of
> course.
> 
> Or did by 'your .c file' mean a new file in arch/x86/kernel directory?

I didn't realize an external file, outside of your changes, needed this
function.  If it does, then perhaps you need to just place it elsewhere.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

2007-11-27 Thread darnok

On Mon, Nov 26, 2007 at 09:29:55PM -0800, Greg KH wrote:
> On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
> > On Monday 26 November 2007 22:31:38 Greg KH wrote:
> > > > +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
> > ..snip..
> > > > +static ssize_t find_ibft(void)
> > > > +{
> > ..snip..
> > > > +}
> > >
> > > What is a function (not even an inline one) doing in a .h file?
> > 
> > I was not sure where to put it. This function (find_ibft) is used by the 
> > setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c 
> > file, 
> > but I am not sure exactly where? Should I make a new file in called 
> > libs/iscsi_ibft_helper.c ?
> 
> Put it in your .c file and make it a global function to be called by
> someone else if they need it.

If the kernel is built with CONFIG_ISCSI_IBFT=m, the
setup_[32|64],c code would depend on the 'find_ibft' symbol which is
in a module (in the iscsi_ibft.c), which is not available during
the bootup phase and not linked to vmlinuz.

This isn't an issue if the module is built with CONFIG_ISCSI_IBFT=y of
course.

Or did by 'your .c file' mean a new file in arch/x86/kernel directory?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

2007-11-27 Thread darnok

On Mon, Nov 26, 2007 at 09:29:55PM -0800, Greg KH wrote:
 On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
  On Monday 26 November 2007 22:31:38 Greg KH wrote:
+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
  ..snip..
+static ssize_t find_ibft(void)
+{
  ..snip..
+}
  
   What is a function (not even an inline one) doing in a .h file?
  
  I was not sure where to put it. This function (find_ibft) is used by the 
  setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c 
  file, 
  but I am not sure exactly where? Should I make a new file in called 
  libs/iscsi_ibft_helper.c ?
 
 Put it in your .c file and make it a global function to be called by
 someone else if they need it.

If the kernel is built with CONFIG_ISCSI_IBFT=m, the
setup_[32|64],c code would depend on the 'find_ibft' symbol which is
in a module (in the iscsi_ibft.c), which is not available during
the bootup phase and not linked to vmlinuz.

This isn't an issue if the module is built with CONFIG_ISCSI_IBFT=y of
course.

Or did by 'your .c file' mean a new file in arch/x86/kernel directory?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

2007-11-27 Thread Greg KH

On Tue, Nov 27, 2007 at 02:09:50PM -0400, [EMAIL PROTECTED] wrote:
 On Mon, Nov 26, 2007 at 09:29:55PM -0800, Greg KH wrote:
  On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
   On Monday 26 November 2007 22:31:38 Greg KH wrote:
 +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
   ..snip..
 +static ssize_t find_ibft(void)
 +{
   ..snip..
 +}
   
What is a function (not even an inline one) doing in a .h file?
   
   I was not sure where to put it. This function (find_ibft) is used by the 
   setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c 
   file, 
   but I am not sure exactly where? Should I make a new file in called 
   libs/iscsi_ibft_helper.c ?
  
  Put it in your .c file and make it a global function to be called by
  someone else if they need it.
 
 If the kernel is built with CONFIG_ISCSI_IBFT=m, the
 setup_[32|64],c code would depend on the 'find_ibft' symbol which is
 in a module (in the iscsi_ibft.c), which is not available during
 the bootup phase and not linked to vmlinuz.

Ah, then don't allow that :)

 This isn't an issue if the module is built with CONFIG_ISCSI_IBFT=y of
 course.
 
 Or did by 'your .c file' mean a new file in arch/x86/kernel directory?

I didn't realize an external file, outside of your changes, needed this
function.  If it does, then perhaps you need to just place it elsewhere.

thanks,

greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Mon, Nov 26, 2007 at 11:50:10PM -0500, Konrad Rzeszutek wrote:
> > >
> > > sysfs files have ONE VALUE PER FILE, not a whole bunch of different
> > > things in a single file.  Please fix this.
> >
> > The subparameters _are_ actually part of a single value, that value being
> > associated with the initiator instance.
> >
> > Konrad is trying to implement a "work-alike" for what open firmware does.
> > open-iscsi already has the ability to extract the same format
> > bits from real OFW.
> >
> > See open-iscsi.git/utils/fwparam_ppc.
> 
> 
> Greg,
> 
> In light of what Doug says (which is all true), should I go ahead with a new 
> version of this module which would export one value per file? The problem 
> that will be encountered is that a ethernetX sysfs directory would have (for 
> example):
> 
> /sys/firmware/ibft/ethernet0/pci-bdf
> 5:1:0
> /sys/firmware/ibft/ethernet0/mac
> 00:11:25:9d:8b:00
> /sys/firmware/ibft/ethernet0/vlan
> 0
> /sys/firmware/ibft/ethernet0/gateway
> 192.168.79.254
> /sys/firmware/ibft/ethernet0/origin
> 0
> /sys/firmware/ibft/ethernet0/subnet-mask
> 22
> /sys/firmware/ibft/ethernet0/ip-addr
> 192.168.77.41
> /sys/firmware/ibft/ethernet0/flags
> 7

Yes, that is the proper way to do this kind of thing in sysfs.

> And the flag would contain the value "7" which would mean the user would have 
> to parse what each bit means? (the v0.3 of the module does not export this 
> flag but uses it to figure out which is the boot iSCSI target).

Sure, as long as it means something to userspace, and is a single value,
and is documented, that's fine.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Mon, Nov 26, 2007 at 11:23:31PM -0500, Konrad Rzeszutek wrote:
> On Monday 26 November 2007 22:31:38 Greg KH wrote:
> > > +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
> ..snip..
> > > +static ssize_t find_ibft(void)
> > > +{
> ..snip..
> > > +}
> >
> > What is a function (not even an inline one) doing in a .h file?
> 
> I was not sure where to put it. This function (find_ibft) is used by the 
> setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c file, 
> but I am not sure exactly where? Should I make a new file in called 
> libs/iscsi_ibft_helper.c ?

Put it in your .c file and make it a global function to be called by
someone else if they need it.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

2007-11-26 Thread Doug Maxey


On Mon, 26 Nov 2007 19:31:38 PST, Greg KH wrote:
> On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:
> > +/*
> > + *  Routines for reading of the iBFT data in a human readable fashion.
> > + */
> > +ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
> > +struct ibft_attribute *attr,
> > +char *buf)
> > +{
> > +   struct ibft_initiator *initiator = attr->initiator;
> > +   void *ibft_loc = entry->data->hdr;
> > +   char *str = buf;
> > +
> > +   if (!initiator)
> > +   return 0;
> > +
> > +   str += sprintf_ipaddr(str, "isns", initiator->isns_server);
> > +   str += sprintf_ipaddr(str, "slp", initiator->slp_server);
> > +   str += sprintf_ipaddr(str, "primary_radius_server",
> > +   initiator->pri_radius_server);
> > +   str += sprintf_ipaddr(str, "secondary_radius_server",
> > +   initiator->sec_radius_server);
> > +   str += sprintf_string(str, "itname", initiator->initiator_name_len,
> > +   (char *)ibft_loc + initiator->initiator_name_off);
> > +   str--;
> > +
> > +   return str-buf;
> > +}
> 
> sysfs files have ONE VALUE PER FILE, not a whole bunch of different
> things in a single file.  Please fix this.

The subparameters _are_ actually part of a single value, that value being 
associated with the initiator instance.

Konrad is trying to implement a "work-alike" for what open firmware does.
open-iscsi already has the ability to extract the same format 
bits from real OFW.

See open-iscsi.git/utils/fwparam_ppc.

> 
> 
> > +
> > +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
> > +  struct ibft_attribute *attr,
> > +  char *buf)
> > +{
> > +   struct ibft_nic *nic = attr->nic;
> > +   void *ibft_loc = entry->data->hdr;
> > +   char *str = buf;
> > +
> > +   if (!nic)
> > +   return 0;
> > +   /*
> > +* Assume dhcp if any non-zero portions of its address are set.
> > +*/
> > +   if (memcmp(nic->dhcp, nulls, sizeof(nic->dhcp))) {
> > +   str += sprintf_ipaddr(str, "dhcp", nic->dhcp);
> > +   } else {
> > +   str += sprintf_ipaddr(str, "ciaddr", nic->ip_addr);
> > +   str += sprintf_ipaddr(str, "giaddr", nic->gateway);
> > +   str += sprintf_ipaddr(str, "dnsaddr1", nic->primary_dns);
> > +   str += sprintf_ipaddr(str, "dnsaddr2", nic->secondary_dns);
> > +   }
> > +   if (nic->hostname_len)
> > +   str += sprintf_string(str, "hostname", nic->hostname_len,
> > +   (char *)ibft_loc + nic->hostname_off);
> > +   /* Cut off the comma. */
> > +   str--;
> > +
> > +   return str-buf;
> > +}
> 
> Same here.
> 
> > +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
> > + struct ibft_attribute *attr,
> > + char *buf)
> > +{
> > +   struct ibft_tgt *tgt = attr->tgt;
> > +   void *ibft_loc = entry->data->hdr;
> > +   char *str = buf;
> > +   int i;
> > +
> > +   if (!tgt)
> > +   return 0;
> > +
> > +   str += sprintf_ipaddr(str, "siaddr", tgt->ip_addr);
> > +   str += sprintf(str, "iport=%d,", tgt->port);
> > +   str += sprintf(str, "ilun=");
> > +   for (i = 0; i < 8; i++)
> > +   str += sprintf(str, "%x", (u8)tgt->lun[i]);
> > +   str += sprintf(str, ",");
> > +
> > +   if (tgt->tgt_name_len)
> > +   str += sprintf_string(str, "iname", tgt->tgt_name_len,
> > +   (void *)ibft_loc + tgt->tgt_name_off);
> > +
> > +   if (tgt->chap_name_len)
> > +   str += sprintf_string(str, "chapid", tgt->chap_name_len,
> > +   (char *)ibft_loc + tgt->chap_name_off);
> > +   if (tgt->chap_secret_len)
> > +   str += sprintf_string(str, "chappw", tgt->chap_secret_len,
> > +   (char *)ibft_loc + tgt->chap_secret_off);
> > +   if (tgt->rev_chap_name_len)
> > +   str += sprintf_string(str, "ichapid", tgt->rev_chap_name_len,
> > +   (char *)ibft_loc + tgt->rev_chap_name_off);
> > +   if (tgt->rev_chap_secret_len)
> > +   str += sprintf_string(str, "ichappw", tgt->rev_chap_secret_len,
> > +   (char *)ibft_loc + tgt->rev_chap_secret_off);
> > +
> > +   /* Cut off the comma. */
> > +   str--;
> > +
> > +   return str-buf;
> > +}
> 
> Same here, are we writing a novella or something to userspace?  :)

Yep.  Just like real OFW.

> 
> > +ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
> > +   struct ibft_attribute *ibft_attr,
> > +   char *buf)
> > +{
> > +   char *str = buf;
> > +
> > +   str += sprintf(str, "//[EMAIL PROTECTED],%d:iscsi,", dev->data->index);
> > +   str += ibft_attr_show_initiator(dev, ibft_attr, str);
> > +   str += sprintf(str, ",");
> > +   str += ibft_attr_show_target(dev, ibft_attr, str);
> > +   str += sprintf(str, ",");
> > +   str += ibft_attr_show_nic(dev, ibft_attr, str);
> > +
> > +   return str-buf;
> > +}
> 
> And here, do I need to go

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

> >
> > sysfs files have ONE VALUE PER FILE, not a whole bunch of different
> > things in a single file.  Please fix this.
>
> The subparameters _are_ actually part of a single value, that value being
> associated with the initiator instance.
>
> Konrad is trying to implement a "work-alike" for what open firmware does.
> open-iscsi already has the ability to extract the same format
> bits from real OFW.
>
> See open-iscsi.git/utils/fwparam_ppc.


Greg,

In light of what Doug says (which is all true), should I go ahead with a new 
version of this module which would export one value per file? The problem 
that will be encountered is that a ethernetX sysfs directory would have (for 
example):

/sys/firmware/ibft/ethernet0/pci-bdf
5:1:0
/sys/firmware/ibft/ethernet0/mac
00:11:25:9d:8b:00
/sys/firmware/ibft/ethernet0/vlan
0
/sys/firmware/ibft/ethernet0/gateway
192.168.79.254
/sys/firmware/ibft/ethernet0/origin
0
/sys/firmware/ibft/ethernet0/subnet-mask
22
/sys/firmware/ibft/ethernet0/ip-addr
192.168.77.41
/sys/firmware/ibft/ethernet0/flags
7

And the flag would contain the value "7" which would mean the user would have 
to parse what each bit means? (the v0.3 of the module does not export this 
flag but uses it to figure out which is the boot iSCSI target).

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)


.. snip..
> > +#else
> > +static void __init reserve_ibft_region(void) { };
>
> No ending ; above.

Fixed.
>
..snip..
> > +static void __init reserve_ibft_region(void) { };
>
> Ditto.

Fixed.

.. snip..
> > +#include 
> > +
>
> No blank line here, please.

Why that creeps back in the code I am not sure myself. In your first review 
you mentioned this, I fixed it in my tree, and now it is back!? Either way, 
it is fixed.

>
> > +#include 

..snip..

> > +   printk(KERN_INFO  \
>
> Looks like this should use KERN_ERROR or KERN_WARNING?

Yes! Thanks for catching that.
>
> > +   "error, in IBFT structure (%s) expected %d but" \
> > +   return str-buf;
>
>   preferred form:
>   return str - buf;

Fixed.
>
..snip..
> > +
> > +   return str-buf;
>
>   Ditto.
Fixed.

>
..snip..
> > +   return str-buf;
>
>   Ditto.
Fixed.

..snip..
> > +   return str-buf;
>
>   Ditto.
Fixed.
..snip..
> > +   int len = 6;
>
> Could you just use ETH_ALEN instead of  and 6?
> and #include 

Yes. That makes much more sense.

>
> Or add a define for IBFT_ALEN (of 6) and use that?

Either one works. The first suggestion is much better.


..snip..
>
> > +
> > +   /* Based on the header index value find the data tuple,
> > +   if possibly. */
>
>  if possible. */
>
> or better:
>   /*
>* Based on the header index value, find the data tuple
>* if possible.
>*/

Yes, much more understandable - and now that I read I realized this was
not a proper assumption. One of the data structures (struct ibft_tgt) has a
'nic_assoc' value which makes a N-to-1 mapping to the NIC data structure,
so this will re-work. Thanks for catching a bug that early in the cycle!

..snip..
> > +  struct carry it for convience. */
>
>   convenience.
Fixed.

..snip..
> > + * Scan the IBFT table structure for the NIC and Target fields. When
> > + * found add them on the passed in list.
>
>   passed-in list.

Fixed.

>
> > + */
> > +static int ibft_scan_device(struct ibft_table_header *header,
> > +   struct list_head *list)
> > +{
> > +
> > +   /* We can have multiple NICs and multiple targets. The index in
> > +  their header defines their 1-to-1 correlation.

Not true. I will have to re-work this code to do a 1-to-N correlation.
> > +   */
> > +   for (ptr = >nic0_off; ptr <= end; ptr += sizeof(u16)) {
>
> In many searches,  would be the first address beyond the end of the
> table, so the loop-terminating condition test would be:
>
>   ptr < end;

Yes. That is correct. It did actually check the next offset, which fortunately 
had nothing in it.
>
> It looks like that should be the case here also

To check the offset to make sure it is within the full IBFT data structure? 
Yes, that is a good check - will implement.

>
..snip..
> > +   if (rc) break;
>
>   break;
>   on a separate line.
>
> Did you check this patch with scripts/checkpatch.pl ?

Yes. I ran it with check-patch-0.99.pl that I downloaded somewhere from Dave 
Jones web page. I hadn't realized that its home is now in 
scripts/checkpatch.pl - will make sure to use that improved-new version.

>
..snip..
> > +   printk(KERN_INFO "iBFT detected at 0x%lx.\n",
> > +  (unsigned long)ibft_phys);
>
> Use %p to print pointer values.

This is actually not a pointer yet. It is a true physical address which I 
thought might be useful for troubleshooting purposes.

>
..snip.
> > +   if (!rc)
> > +   return rc;
>
> Can't this always just be
>   return 0;
> ?

Yes, I was thinking that perhaps a more nicer way was to do 
"goto end;" where the end label is just "return rc;"  But this 
definitely trumps it.

>
> > +
..snip..
> > +
> > +struct ibft_tgt {
> > +   struct ibft_hdr hdr;
> > +   char ip_addr[16];
> > +   u16 port;
> > +   char lun[8];
> > +   u8 chap_type;
> > +   u8 nic_assoc;
> > +   u16 tgt_name_len;
> > +   u16 tgt_name_off;
> > +   u16 chap_name_len;
> > +   u16 chap_name_off;
> > +   u16 chap_secret_len;
> > +   u16 chap_secret_off;
> > +   u16 rev_chap_name_len;
> > +   u16 rev_chap_name_off;
> > +   u16 rev_chap_secret_len;
> > +   u16 rev_chap_secret_off;
> > +} __attribute__((__packed__));
> > +
> > +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
>
> Why is this #if line here instead of nearer the top of this header file?

My thought was that if other kernel users might want to include this header 
file they do not have to exposed to the semi-internal data structures of this 
header file. If that is not a concern then I think I can remove the 
conditional altogether.

>
> > +#define IBFT_SIGN "iBFT"
> > +#define IBFT_SIGN_LEN 4
> > +#define IBFT_START 0x8 /* 512kB */
> > +#define IBFT_END 0x10 /* 1MB */
> > +#define VGA_MEM 0xA /* VGA buffer */
> > +#define VGA_SIZE 0x2 /* 132kB */
>
> I'd say

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Monday 26 November 2007 22:31:38 Greg KH wrote:
> On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:
> > +/*
> > + *  Routines for reading of the iBFT data in a human readable fashion.
> > + */
> > +ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
> > +struct ibft_attribute *attr,
> > +char *buf)
> > +{
.. snip..
> > +
> > +   str += sprintf_ipaddr(str, "isns", initiator->isns_server);
> > +   str += sprintf_ipaddr(str, "slp", initiator->slp_server);
.. snip ..
>
> sysfs files have ONE VALUE PER FILE, not a whole bunch of different
> things in a single file.  Please fix this.

No problem. I will have that shortly posted.

>
> > +
> > +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
> > +  struct ibft_attribute *attr,
> > +  char *buf)
.. snip.. 
> > +   str += sprintf_ipaddr(str, "giaddr", nic->gateway);
> > +   str += sprintf_ipaddr(str, "dnsaddr1", nic->primary_dns);
>
> Same here.

Yup. 
>
> > +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
> > + struct ibft_attribute *attr,
> > + char *buf)
> > +{
.. snip..
> > +}
>
> Same here, are we writing a novella or something to userspace?  :)

Hehe.. I will make it simpler :-)

>
> > +ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
> > +   struct ibft_attribute *ibft_attr,
> > +   char *buf)
> > +{
.. snip ..
> > +}
>
> And here, do I need to go on?

I will have a new version posted quite shortly.

>
> > +ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
> > +  struct ibft_attribute *attr,
> > +  char *buf)
> > +{
..snip..
> > +
> > +   memcpy(buf, attr->nic->mac, len);
> > +
> > +   return len;
> > +}
>
> Is mac a user readable string?  Then perhaps a simple sprintf would work
> instead, as I doubt you are including a \n here...

It was meant to be as a binary value. But that doesn't fit in sysfs directory, 
so let me make it use sprintf here.

>
> > +/*
> > + * The main routine which allows the user to read the IBFT data.
> > + */
> > +static ssize_t ibft_show_attribute(struct kobject *kobj,
> > +  struct attribute *attr,
> > +  char *buf)
> > +{
..snip..
> > +
> > +static struct sysfs_ops ibft_attr_ops = {
> > +   .show = ibft_show_attribute,
> > +};
>
> I think this whole mess can go away in the new rework Kay and I have
> done, please document this whole thing and I'll see what I can do.

Absolutely.

>
> > +struct ibft_control {
> > +struct ibft_hdr hdr;
> > +u16 extensions;
> > +u16 initiator_off;
> > +u16 nic0_off;
> > +u16 tgt0_off;
> > +u16 nic1_off;
> > +u16 tgt1_off;
> > +} __attribute__((__packed__));
>
> Did we loose tabs for some reason?  I'm guessing your editor is not
> showing them properly, nor did you use scripts/checkpatch.pl :(

I did use checkpatch.pl v0.99 downloaded somewhere from the web. I hadn't
realized it was now residing in scripts/checkpatch.pl - and from now on I will 
use that.

>
> > +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
..snip..
> > +static ssize_t find_ibft(void)
> > +{
..snip..
> > +}
>
> What is a function (not even an inline one) doing in a .h file?

I was not sure where to put it. This function (find_ibft) is used by the 
setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c file, 
but I am not sure exactly where? Should I make a new file in called 
libs/iscsi_ibft_helper.c ?

>
..snip..
> > +struct ibft_kobject {
> > +   struct ibft_data *data;
> > +   char name[IBFT_ISCSI_KOBJECT_MAX_LEN];
>
> Why have this,
>
> > +   u8 type;
> > +   struct kobject kobj;
>
> When the kobject itself has an unlimited size name associated with it?

Absolutely no reason at all. It was a evolution vestige of the code that is
not needed anymore. 

>
..snip..
> > +   char name[IBFT_ISCSI_ATTR_MAX_LEN];
>
> Same here, an attribute already has a pointer to a name, no need to have
> another one in the same structure.

Thanks. Will remove it.
>
> > +   struct list_head node;
> > +};
>
> thanks,

Thank you for taking your time to review the code. I will have the new
version out shortly.
>
> greg k-h


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:
> +/*
> + *  Routines for reading of the iBFT data in a human readable fashion.
> + */
> +ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
> +  struct ibft_attribute *attr,
> +  char *buf)
> +{
> + struct ibft_initiator *initiator = attr->initiator;
> + void *ibft_loc = entry->data->hdr;
> + char *str = buf;
> +
> + if (!initiator)
> + return 0;
> +
> + str += sprintf_ipaddr(str, "isns", initiator->isns_server);
> + str += sprintf_ipaddr(str, "slp", initiator->slp_server);
> + str += sprintf_ipaddr(str, "primary_radius_server",
> + initiator->pri_radius_server);
> + str += sprintf_ipaddr(str, "secondary_radius_server",
> + initiator->sec_radius_server);
> + str += sprintf_string(str, "itname", initiator->initiator_name_len,
> + (char *)ibft_loc + initiator->initiator_name_off);
> + str--;
> +
> + return str-buf;
> +}

sysfs files have ONE VALUE PER FILE, not a whole bunch of different
things in a single file.  Please fix this.


> +
> +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
> +struct ibft_attribute *attr,
> +char *buf)
> +{
> + struct ibft_nic *nic = attr->nic;
> + void *ibft_loc = entry->data->hdr;
> + char *str = buf;
> +
> + if (!nic)
> + return 0;
> + /*
> +  * Assume dhcp if any non-zero portions of its address are set.
> +  */
> + if (memcmp(nic->dhcp, nulls, sizeof(nic->dhcp))) {
> + str += sprintf_ipaddr(str, "dhcp", nic->dhcp);
> + } else {
> + str += sprintf_ipaddr(str, "ciaddr", nic->ip_addr);
> + str += sprintf_ipaddr(str, "giaddr", nic->gateway);
> + str += sprintf_ipaddr(str, "dnsaddr1", nic->primary_dns);
> + str += sprintf_ipaddr(str, "dnsaddr2", nic->secondary_dns);
> + }
> + if (nic->hostname_len)
> + str += sprintf_string(str, "hostname", nic->hostname_len,
> + (char *)ibft_loc + nic->hostname_off);
> + /* Cut off the comma. */
> + str--;
> +
> + return str-buf;
> +}

Same here.

> +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
> +   struct ibft_attribute *attr,
> +   char *buf)
> +{
> + struct ibft_tgt *tgt = attr->tgt;
> + void *ibft_loc = entry->data->hdr;
> + char *str = buf;
> + int i;
> +
> + if (!tgt)
> + return 0;
> +
> + str += sprintf_ipaddr(str, "siaddr", tgt->ip_addr);
> + str += sprintf(str, "iport=%d,", tgt->port);
> + str += sprintf(str, "ilun=");
> + for (i = 0; i < 8; i++)
> + str += sprintf(str, "%x", (u8)tgt->lun[i]);
> + str += sprintf(str, ",");
> +
> + if (tgt->tgt_name_len)
> + str += sprintf_string(str, "iname", tgt->tgt_name_len,
> + (void *)ibft_loc + tgt->tgt_name_off);
> +
> + if (tgt->chap_name_len)
> + str += sprintf_string(str, "chapid", tgt->chap_name_len,
> + (char *)ibft_loc + tgt->chap_name_off);
> + if (tgt->chap_secret_len)
> + str += sprintf_string(str, "chappw", tgt->chap_secret_len,
> + (char *)ibft_loc + tgt->chap_secret_off);
> + if (tgt->rev_chap_name_len)
> + str += sprintf_string(str, "ichapid", tgt->rev_chap_name_len,
> + (char *)ibft_loc + tgt->rev_chap_name_off);
> + if (tgt->rev_chap_secret_len)
> + str += sprintf_string(str, "ichappw", tgt->rev_chap_secret_len,
> + (char *)ibft_loc + tgt->rev_chap_secret_off);
> +
> + /* Cut off the comma. */
> + str--;
> +
> + return str-buf;
> +}

Same here, are we writing a novella or something to userspace?  :)

> +ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
> + struct ibft_attribute *ibft_attr,
> + char *buf)
> +{
> + char *str = buf;
> +
> + str += sprintf(str, "//[EMAIL PROTECTED],%d:iscsi,", dev->data->index);
> + str += ibft_attr_show_initiator(dev, ibft_attr, str);
> + str += sprintf(str, ",");
> + str += ibft_attr_show_target(dev, ibft_attr, str);
> + str += sprintf(str, ",");
> + str += ibft_attr_show_nic(dev, ibft_attr, str);
> +
> + return str-buf;
> +}

And here, do I need to go on?

> +ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
> +struct ibft_attribute *attr,
> +char *buf)
> +{
> + struct ibft_nic *nic = attr->nic;
> + int len = 6;
> +
> + if (!nic)
> + return 0;
> +
> + memcpy(buf, attr->nic->mac, len);
> +
> + return len;
> +}

Is mac a user readable string?  Then perhaps a simple sprintf would work
instead, as I doubt you are including a \n here...

> +/*

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:
> 
> This patch adds /sysfs/firmware/ibft/[chosen|aliases|[EMAIL 
> PROTECTED],X|[EMAIL PROTECTED],X]
> directories along with text properties which export the the iSCSI Boot
> Firmware Table (iBFT) structure. The layout of the directories mirrors
> how PowerPC OpenBoot exports this data.
> 
> What is iSCSI Boot Firmware Table? It is a mechanism for the iSCSI
> tools to extract from the machine NICs the iSCSI connection information
> so that they can automagically mount the iSCSI share/target. Currently
> the iSCSI information is hard-coded in th initrd.
> 
> For full details of the IBFT structure please take a look at:
> ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf

As you are adding sysfs files in /sys/firmware, please add documentation
to Documentation/ABI as to what these files are, what they do, what is
in them, and what they are to be used for.

> + rc = firmware_register(_subsys);
> + if (rc)
> + return rc;

This function, as well as the whole decl_subsys() stuff is gone in my
tree and in -mm.  /sys/firmware is now just a simple kobject that you
are free to chain off of.  If you describe just what these sysfs
subdirectories and files are for and how they are going to be used, I'd
be glad to rework this patch to use the new interfaces.

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

2007-11-26 Thread Randy Dunlap


Konrad Rzeszutek wrote:

This patch adds /sysfs/firmware/ibft/[chosen|aliases|[EMAIL PROTECTED],X|[EMAIL 
PROTECTED],X]
directories along with text properties which export the the iSCSI Boot
Firmware Table (iBFT) structure. The layout of the directories mirrors
how PowerPC OpenBoot exports this data.

What is iSCSI Boot Firmware Table? It is a mechanism for the iSCSI
tools to extract from the machine NICs the iSCSI connection information
so that they can automagically mount the iSCSI share/target. Currently
the iSCSI information is hard-coded in th initrd.

For full details of the IBFT structure please take a look at:
ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf


Signed-off-by: Konrad Rzeszutek <[EMAIL PROTECTED]>
Signed-off-by: Peter Jones <[EMAIL PROTECTED]>

diff --git a/arch/x86/kernel/setup_32.c b/arch/x86/kernel/setup_32.c
index e1e18c3..e3ed866 100644
--- a/arch/x86/kernel/setup_32.c
+++ b/arch/x86/kernel/setup_32.c
@@ -148,6 +149,23 @@ static inline void copy_edd(void)
 }
 #endif
 
+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

+void *ibft_phys;
+#if defined(CONFIG_ISCSI_IBFT_MODULE)
+EXPORT_SYMBOL(ibft_phys);
+#endif
+static void __init reserve_ibft_region(void)
+{
+   unsigned int ibft_len;
+   ibft_len = find_ibft();
+   if (ibft_len)
+   reserve_bootmem((unsigned int)ibft_phys, PAGE_ALIGN(ibft_len));
+}
+
+#else
+static void __init reserve_ibft_region(void) { };


No ending ; above.


+#endif
+
 int __initdata user_defined_memmap = 0;
 
 /*



diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index 30d94d1..ba6fd21 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -198,6 +199,23 @@ static inline void copy_edd(void)
 }
 #endif
 
+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

+void *ibft_phys;
+#if defined(CONFIG_ISCSI_IBFT_MODULE)
+EXPORT_SYMBOL(ibft_phys);
+#endif
+static void __init reserve_ibft_region(void)
+{
+   unsigned int ibft_len;
+   ibft_len = find_ibft();
+   if (ibft_len)
+   reserve_bootmem_generic(ibft_phys, PAGE_ALIGN(ibft_len));
+}
+
+#else
+static void __init reserve_ibft_region(void) { };


Ditto.


+#endif
+
 #ifdef CONFIG_KEXEC
 static void __init reserve_crashkernel(void)
 {



diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
new file mode 100644
index 000..7e4e117
--- /dev/null
+++ b/drivers/firmware/iscsi_ibft.c
@@ -0,0 +1,612 @@
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+


No blank line here, please.


+#include 
+
+#define IBFT_ISCSI_VERSION  "0.3"
+#define IBFT_ISCSI_DATE "2007-Nov-21"
+
+MODULE_AUTHOR("Peter Jones <[EMAIL PROTECTED]> and \
+Konrad Rzeszutek <[EMAIL PROTECTED]>");
+MODULE_DESCRIPTION("sysfs interface to BIOS iBFT information");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(IBFT_ISCSI_VERSION);
+
+static LIST_HEAD(ibft_attr_list);
+static LIST_HEAD(ibft_kobject_list);
+static LIST_HEAD(ibft_data_list);
+
+static const char nulls[16];
+static struct ibft_table_header *ibft_device;
+



+/*
+ * Helper function to verify the IBFT header.
+ */
+static int ibft_verify_hdr(struct ibft_hdr *hdr, int id, int length)
+{
+#define IBFT_VERIFY_HDR_FIELD(hdr2, val, name) \
+   if (hdr2->val != val) { \
+   printk(KERN_INFO  \


Looks like this should use KERN_ERROR or KERN_WARNING?


+   "error, in IBFT structure (%s) expected %d but" \
+   " found %d\n", \
+   name, val, hdr2->val); \
+   return -ENODEV; \
+   }
+   IBFT_VERIFY_HDR_FIELD(hdr, id, "ID");
+   IBFT_VERIFY_HDR_FIELD(hdr, length, "Length");
+#undef IBFT_VERIFY_HDR_FIELD
+   return 0;
+}
+
+static void ibft_release(struct kobject *kobj)
+{
+   struct ibft_kobject *ibft =
+   container_of(kobj, struct ibft_kobject, kobj);
+   kfree(ibft);
+}
+
+/*
+ *  Routines for reading of the iBFT data in a human readable fashion.
+ */
+ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
+struct ibft_attribute *attr,
+char *buf)
+{
+   struct ibft_initiator *initiator = attr->initiator;
+   void *ibft_loc = entry->data->hdr;
+   char *str = buf;
+
+   if (!initiator)
+   return 0;
+
+   str += sprintf_ipaddr(str, "isns", initiator->isns_server);
+   str += sprintf_ipaddr(str, "slp", initiator->slp_server);
+   str += sprintf_ipaddr(str, "primary_radius_server",
+   initiator->pri_radius_server);
+   str += sprintf_ipaddr(str, "secondary_radius_server",
+   initiator->sec_radius_server);
+   str += sprintf_string(str, "itname", initiator->initiator_name_len,
+   (char *)ibft_loc + initiator->initiator_name_off);
+   str--;
+
+

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

2007-11-26 Thread Randy Dunlap


Konrad Rzeszutek wrote:

This patch adds /sysfs/firmware/ibft/[chosen|aliases|[EMAIL PROTECTED],X|[EMAIL 
PROTECTED],X]
directories along with text properties which export the the iSCSI Boot
Firmware Table (iBFT) structure. The layout of the directories mirrors
how PowerPC OpenBoot exports this data.

What is iSCSI Boot Firmware Table? It is a mechanism for the iSCSI
tools to extract from the machine NICs the iSCSI connection information
so that they can automagically mount the iSCSI share/target. Currently
the iSCSI information is hard-coded in th initrd.

For full details of the IBFT structure please take a look at:
ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf


Signed-off-by: Konrad Rzeszutek [EMAIL PROTECTED]
Signed-off-by: Peter Jones [EMAIL PROTECTED]

diff --git a/arch/x86/kernel/setup_32.c b/arch/x86/kernel/setup_32.c
index e1e18c3..e3ed866 100644
--- a/arch/x86/kernel/setup_32.c
+++ b/arch/x86/kernel/setup_32.c
@@ -148,6 +149,23 @@ static inline void copy_edd(void)
 }
 #endif
 
+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

+void *ibft_phys;
+#if defined(CONFIG_ISCSI_IBFT_MODULE)
+EXPORT_SYMBOL(ibft_phys);
+#endif
+static void __init reserve_ibft_region(void)
+{
+   unsigned int ibft_len;
+   ibft_len = find_ibft();
+   if (ibft_len)
+   reserve_bootmem((unsigned int)ibft_phys, PAGE_ALIGN(ibft_len));
+}
+
+#else
+static void __init reserve_ibft_region(void) { };


No ending ; above.


+#endif
+
 int __initdata user_defined_memmap = 0;
 
 /*



diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index 30d94d1..ba6fd21 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -198,6 +199,23 @@ static inline void copy_edd(void)
 }
 #endif
 
+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

+void *ibft_phys;
+#if defined(CONFIG_ISCSI_IBFT_MODULE)
+EXPORT_SYMBOL(ibft_phys);
+#endif
+static void __init reserve_ibft_region(void)
+{
+   unsigned int ibft_len;
+   ibft_len = find_ibft();
+   if (ibft_len)
+   reserve_bootmem_generic(ibft_phys, PAGE_ALIGN(ibft_len));
+}
+
+#else
+static void __init reserve_ibft_region(void) { };


Ditto.


+#endif
+
 #ifdef CONFIG_KEXEC
 static void __init reserve_crashkernel(void)
 {



diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
new file mode 100644
index 000..7e4e117
--- /dev/null
+++ b/drivers/firmware/iscsi_ibft.c
@@ -0,0 +1,612 @@
+
+#include linux/module.h
+#include linux/string.h
+#include linux/types.h
+#include linux/init.h
+#include linux/stat.h
+#include linux/err.h
+#include linux/ctype.h
+#include linux/slab.h
+#include linux/limits.h
+#include linux/device.h
+#include linux/pci.h
+#include linux/blkdev.h
+


No blank line here, please.


+#include linux/iscsi_ibft.h
+
+#define IBFT_ISCSI_VERSION  0.3
+#define IBFT_ISCSI_DATE 2007-Nov-21
+
+MODULE_AUTHOR(Peter Jones [EMAIL PROTECTED] and \
+Konrad Rzeszutek [EMAIL PROTECTED]);
+MODULE_DESCRIPTION(sysfs interface to BIOS iBFT information);
+MODULE_LICENSE(GPL);
+MODULE_VERSION(IBFT_ISCSI_VERSION);
+
+static LIST_HEAD(ibft_attr_list);
+static LIST_HEAD(ibft_kobject_list);
+static LIST_HEAD(ibft_data_list);
+
+static const char nulls[16];
+static struct ibft_table_header *ibft_device;
+



+/*
+ * Helper function to verify the IBFT header.
+ */
+static int ibft_verify_hdr(struct ibft_hdr *hdr, int id, int length)
+{
+#define IBFT_VERIFY_HDR_FIELD(hdr2, val, name) \
+   if (hdr2-val != val) { \
+   printk(KERN_INFO  \


Looks like this should use KERN_ERROR or KERN_WARNING?


+   error, in IBFT structure (%s) expected %d but \
+found %d\n, \
+   name, val, hdr2-val); \
+   return -ENODEV; \
+   }
+   IBFT_VERIFY_HDR_FIELD(hdr, id, ID);
+   IBFT_VERIFY_HDR_FIELD(hdr, length, Length);
+#undef IBFT_VERIFY_HDR_FIELD
+   return 0;
+}
+
+static void ibft_release(struct kobject *kobj)
+{
+   struct ibft_kobject *ibft =
+   container_of(kobj, struct ibft_kobject, kobj);
+   kfree(ibft);
+}
+
+/*
+ *  Routines for reading of the iBFT data in a human readable fashion.
+ */
+ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
+struct ibft_attribute *attr,
+char *buf)
+{
+   struct ibft_initiator *initiator = attr-initiator;
+   void *ibft_loc = entry-data-hdr;
+   char *str = buf;
+
+   if (!initiator)
+   return 0;
+
+   str += sprintf_ipaddr(str, isns, initiator-isns_server);
+   str += sprintf_ipaddr(str, slp, initiator-slp_server);
+   str += sprintf_ipaddr(str, primary_radius_server,
+   initiator-pri_radius_server);
+   str += sprintf_ipaddr(str, secondary_radius_server,
+   initiator-sec_radius_server);
+   str +=

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:

This patch adds /sysfs/firmware/ibft/[chosen|aliases|[EMAIL
PROTECTED],X|[EMAIL PROTECTED],X]
directories along with text properties which export the the iSCSI Boot
Firmware Table (iBFT) structure. The layout of the directories mirrors
how PowerPC OpenBoot exports this data.

What is iSCSI Boot Firmware Table? It is a mechanism for the iSCSI
tools to extract from the machine NICs the iSCSI connection information
so that they can automagically mount the iSCSI share/target. Currently
the iSCSI information is hard-coded in th initrd.

For full details of the IBFT structure please take a look at:
ftp://ftp.software.ibm.com/systems/support/system_x_pdf/ibm_iscsi_boot_firmware_table_v1.02.pdf

As you are adding sysfs files in /sys/firmware, please add documentation
to Documentation/ABI as to what these files are, what they do, what is
in them, and what they are to be used for.

+ rc = firmware_register(ibft_subsys);
+ if (rc)
+ return rc;

This function, as well as the whole decl_subsys() stuff is gone in my
tree and in -mm. /sys/firmware is now just a simple kobject that you
are free to chain off of. If you describe just what these sysfs
subdirectories and files are for and how they are going to be used, I'd
be glad to rework this patch to use the new interfaces.

thanks,

greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:
 +/*
 + *  Routines for reading of the iBFT data in a human readable fashion.
 + */
 +ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
 +  struct ibft_attribute *attr,
 +  char *buf)
 +{
 + struct ibft_initiator *initiator = attr-initiator;
 + void *ibft_loc = entry-data-hdr;
 + char *str = buf;
 +
 + if (!initiator)
 + return 0;
 +
 + str += sprintf_ipaddr(str, isns, initiator-isns_server);
 + str += sprintf_ipaddr(str, slp, initiator-slp_server);
 + str += sprintf_ipaddr(str, primary_radius_server,
 + initiator-pri_radius_server);
 + str += sprintf_ipaddr(str, secondary_radius_server,
 + initiator-sec_radius_server);
 + str += sprintf_string(str, itname, initiator-initiator_name_len,
 + (char *)ibft_loc + initiator-initiator_name_off);
 + str--;
 +
 + return str-buf;
 +}

sysfs files have ONE VALUE PER FILE, not a whole bunch of different
things in a single file.  Please fix this.


 +
 +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
 +struct ibft_attribute *attr,
 +char *buf)
 +{
 + struct ibft_nic *nic = attr-nic;
 + void *ibft_loc = entry-data-hdr;
 + char *str = buf;
 +
 + if (!nic)
 + return 0;
 + /*
 +  * Assume dhcp if any non-zero portions of its address are set.
 +  */
 + if (memcmp(nic-dhcp, nulls, sizeof(nic-dhcp))) {
 + str += sprintf_ipaddr(str, dhcp, nic-dhcp);
 + } else {
 + str += sprintf_ipaddr(str, ciaddr, nic-ip_addr);
 + str += sprintf_ipaddr(str, giaddr, nic-gateway);
 + str += sprintf_ipaddr(str, dnsaddr1, nic-primary_dns);
 + str += sprintf_ipaddr(str, dnsaddr2, nic-secondary_dns);
 + }
 + if (nic-hostname_len)
 + str += sprintf_string(str, hostname, nic-hostname_len,
 + (char *)ibft_loc + nic-hostname_off);
 + /* Cut off the comma. */
 + str--;
 +
 + return str-buf;
 +}

Same here.

 +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
 +   struct ibft_attribute *attr,
 +   char *buf)
 +{
 + struct ibft_tgt *tgt = attr-tgt;
 + void *ibft_loc = entry-data-hdr;
 + char *str = buf;
 + int i;
 +
 + if (!tgt)
 + return 0;
 +
 + str += sprintf_ipaddr(str, siaddr, tgt-ip_addr);
 + str += sprintf(str, iport=%d,, tgt-port);
 + str += sprintf(str, ilun=);
 + for (i = 0; i  8; i++)
 + str += sprintf(str, %x, (u8)tgt-lun[i]);
 + str += sprintf(str, ,);
 +
 + if (tgt-tgt_name_len)
 + str += sprintf_string(str, iname, tgt-tgt_name_len,
 + (void *)ibft_loc + tgt-tgt_name_off);
 +
 + if (tgt-chap_name_len)
 + str += sprintf_string(str, chapid, tgt-chap_name_len,
 + (char *)ibft_loc + tgt-chap_name_off);
 + if (tgt-chap_secret_len)
 + str += sprintf_string(str, chappw, tgt-chap_secret_len,
 + (char *)ibft_loc + tgt-chap_secret_off);
 + if (tgt-rev_chap_name_len)
 + str += sprintf_string(str, ichapid, tgt-rev_chap_name_len,
 + (char *)ibft_loc + tgt-rev_chap_name_off);
 + if (tgt-rev_chap_secret_len)
 + str += sprintf_string(str, ichappw, tgt-rev_chap_secret_len,
 + (char *)ibft_loc + tgt-rev_chap_secret_off);
 +
 + /* Cut off the comma. */
 + str--;
 +
 + return str-buf;
 +}

Same here, are we writing a novella or something to userspace?  :)

 +ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
 + struct ibft_attribute *ibft_attr,
 + char *buf)
 +{
 + char *str = buf;
 +
 + str += sprintf(str, //[EMAIL PROTECTED],%d:iscsi,, dev-data-index);
 + str += ibft_attr_show_initiator(dev, ibft_attr, str);
 + str += sprintf(str, ,);
 + str += ibft_attr_show_target(dev, ibft_attr, str);
 + str += sprintf(str, ,);
 + str += ibft_attr_show_nic(dev, ibft_attr, str);
 +
 + return str-buf;
 +}

And here, do I need to go on?

 +ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
 +struct ibft_attribute *attr,
 +char *buf)
 +{
 + struct ibft_nic *nic = attr-nic;
 + int len = 6;
 +
 + if (!nic)
 + return 0;
 +
 + memcpy(buf, attr-nic-mac, len);
 +
 + return len;
 +}

Is mac a user readable string?  Then perhaps a simple sprintf would work
instead, as I doubt you are including a \n here...

 +/*
 + * The main routine which allows the user to read the IBFT data.
 + */
 +static ssize_t ibft_show_attribute(struct kobject *kobj,
 +struct attribute *attr,
 +

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Monday 26 November 2007 22:31:38 Greg KH wrote:
 On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:
  +/*
  + *  Routines for reading of the iBFT data in a human readable fashion.
  + */
  +ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
  +struct ibft_attribute *attr,
  +char *buf)
  +{
.. snip..
  +
  +   str += sprintf_ipaddr(str, isns, initiator-isns_server);
  +   str += sprintf_ipaddr(str, slp, initiator-slp_server);
.. snip ..

 sysfs files have ONE VALUE PER FILE, not a whole bunch of different
 things in a single file.  Please fix this.

No problem. I will have that shortly posted.


  +
  +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
  +  struct ibft_attribute *attr,
  +  char *buf)
.. snip.. 
  +   str += sprintf_ipaddr(str, giaddr, nic-gateway);
  +   str += sprintf_ipaddr(str, dnsaddr1, nic-primary_dns);

 Same here.

Yup. 

  +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
  + struct ibft_attribute *attr,
  + char *buf)
  +{
.. snip..
  +}

 Same here, are we writing a novella or something to userspace?  :)

Hehe.. I will make it simpler :-)


  +ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
  +   struct ibft_attribute *ibft_attr,
  +   char *buf)
  +{
.. snip ..
  +}

 And here, do I need to go on?

I will have a new version posted quite shortly.


  +ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
  +  struct ibft_attribute *attr,
  +  char *buf)
  +{
..snip..
  +
  +   memcpy(buf, attr-nic-mac, len);
  +
  +   return len;
  +}

 Is mac a user readable string?  Then perhaps a simple sprintf would work
 instead, as I doubt you are including a \n here...

It was meant to be as a binary value. But that doesn't fit in sysfs directory, 
so let me make it use sprintf here.


  +/*
  + * The main routine which allows the user to read the IBFT data.
  + */
  +static ssize_t ibft_show_attribute(struct kobject *kobj,
  +  struct attribute *attr,
  +  char *buf)
  +{
..snip..
  +
  +static struct sysfs_ops ibft_attr_ops = {
  +   .show = ibft_show_attribute,
  +};

 I think this whole mess can go away in the new rework Kay and I have
 done, please document this whole thing and I'll see what I can do.

Absolutely.


  +struct ibft_control {
  +struct ibft_hdr hdr;
  +u16 extensions;
  +u16 initiator_off;
  +u16 nic0_off;
  +u16 tgt0_off;
  +u16 nic1_off;
  +u16 tgt1_off;
  +} __attribute__((__packed__));

 Did we loose tabs for some reason?  I'm guessing your editor is not
 showing them properly, nor did you use scripts/checkpatch.pl :(

I did use checkpatch.pl v0.99 downloaded somewhere from the web. I hadn't
realized it was now residing in scripts/checkpatch.pl - and from now on I will 
use that.


  +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
..snip..
  +static ssize_t find_ibft(void)
  +{
..snip..
  +}

 What is a function (not even an inline one) doing in a .h file?

I was not sure where to put it. This function (find_ibft) is used by the 
setup_[32|64].c and the iscsi_ibft.c code. Randy suggested I put in .c file, 
but I am not sure exactly where? Should I make a new file in called 
libs/iscsi_ibft_helper.c ?


..snip..
  +struct ibft_kobject {
  +   struct ibft_data *data;
  +   char name[IBFT_ISCSI_KOBJECT_MAX_LEN];

 Why have this,

  +   u8 type;
  +   struct kobject kobj;

 When the kobject itself has an unlimited size name associated with it?

Absolutely no reason at all. It was a evolution vestige of the code that is
not needed anymore. 


..snip..
  +   char name[IBFT_ISCSI_ATTR_MAX_LEN];

 Same here, an attribute already has a pointer to a name, no need to have
 another one in the same structure.

Thanks. Will remove it.

  +   struct list_head node;
  +};

 thanks,

Thank you for taking your time to review the code. I will have the new
version out shortly.

 greg k-h


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)


.. snip..
  +#else
  +static void __init reserve_ibft_region(void) { };

 No ending ; above.

Fixed.

..snip..
  +static void __init reserve_ibft_region(void) { };

 Ditto.

Fixed.

.. snip..
  +#include linux/blkdev.h
  +

 No blank line here, please.

Why that creeps back in the code I am not sure myself. In your first review 
you mentioned this, I fixed it in my tree, and now it is back!? Either way, 
it is fixed.


  +#include linux/iscsi_ibft.h

..snip..

  +   printk(KERN_INFO  \

 Looks like this should use KERN_ERROR or KERN_WARNING?

Yes! Thanks for catching that.

  +   error, in IBFT structure (%s) expected %d but \
  +   return str-buf;

   preferred form:
   return str - buf;

Fixed.

..snip..
  +
  +   return str-buf;

   Ditto.
Fixed.


..snip..
  +   return str-buf;

   Ditto.
Fixed.

..snip..
  +   return str-buf;

   Ditto.
Fixed.
..snip..
  +   int len = 6;

 Could you just use ETH_ALEN instead of len and 6?
 and #include linux/if_ether.h

Yes. That makes much more sense.


 Or add a define for IBFT_ALEN (of 6) and use that?

Either one works. The first suggestion is much better.


..snip..

  +
  +   /* Based on the header index value find the data tuple,
  +   if possibly. */

  if possible. */

 or better:
   /*
* Based on the header index value, find the data tuple
* if possible.
*/

Yes, much more understandable - and now that I read I realized this was
not a proper assumption. One of the data structures (struct ibft_tgt) has a
'nic_assoc' value which makes a N-to-1 mapping to the NIC data structure,
so this will re-work. Thanks for catching a bug that early in the cycle!

..snip..
  +  struct carry it for convience. */

   convenience.
Fixed.

..snip..
  + * Scan the IBFT table structure for the NIC and Target fields. When
  + * found add them on the passed in list.

   passed-in list.

Fixed.


  + */
  +static int ibft_scan_device(struct ibft_table_header *header,
  +   struct list_head *list)
  +{
  +
  +   /* We can have multiple NICs and multiple targets. The index in
  +  their header defines their 1-to-1 correlation.

Not true. I will have to re-work this code to do a 1-to-N correlation.
  +   */
  +   for (ptr = control-nic0_off; ptr = end; ptr += sizeof(u16)) {

 In many searches, end would be the first address beyond the end of the
 table, so the loop-terminating condition test would be:

   ptr  end;

Yes. That is correct. It did actually check the next offset, which fortunately 
had nothing in it.

 It looks like that should be the case here also

To check the offset to make sure it is within the full IBFT data structure? 
Yes, that is a good check - will implement.


..snip..
  +   if (rc) break;

   break;
   on a separate line.

 Did you check this patch with scripts/checkpatch.pl ?

Yes. I ran it with check-patch-0.99.pl that I downloaded somewhere from Dave 
Jones web page. I hadn't realized that its home is now in 
scripts/checkpatch.pl - will make sure to use that improved-new version.


..snip..
  +   printk(KERN_INFO iBFT detected at 0x%lx.\n,
  +  (unsigned long)ibft_phys);

 Use %p to print pointer values.

This is actually not a pointer yet. It is a true physical address which I 
thought might be useful for troubleshooting purposes.


..snip.
  +   if (!rc)
  +   return rc;

 Can't this always just be
   return 0;
 ?

Yes, I was thinking that perhaps a more nicer way was to do 
goto end; where the end label is just return rc;  But this 
definitely trumps it.


  +
..snip..
  +
  +struct ibft_tgt {
  +   struct ibft_hdr hdr;
  +   char ip_addr[16];
  +   u16 port;
  +   char lun[8];
  +   u8 chap_type;
  +   u8 nic_assoc;
  +   u16 tgt_name_len;
  +   u16 tgt_name_off;
  +   u16 chap_name_len;
  +   u16 chap_name_off;
  +   u16 chap_secret_len;
  +   u16 chap_secret_off;
  +   u16 rev_chap_name_len;
  +   u16 rev_chap_name_off;
  +   u16 rev_chap_secret_len;
  +   u16 rev_chap_secret_off;
  +} __attribute__((__packed__));
  +
  +#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

 Why is this #if line here instead of nearer the top of this header file?

My thought was that if other kernel users might want to include this header 
file they do not have to exposed to the semi-internal data structures of this 
header file. If that is not a concern then I think I can remove the 
conditional altogether.


  +#define IBFT_SIGN iBFT
  +#define IBFT_SIGN_LEN 4
  +#define IBFT_START 0x8 /* 512kB */
  +#define IBFT_END 0x10 /* 1MB */
  +#define VGA_MEM 0xA /* VGA buffer */
  +#define VGA_SIZE 0x2 /* 132kB */

 I'd say that if 0x8 is 512kB, then 0x2 is 128kB.

Yes. Did the decimal conversion and forgot about the 1000 != 1024!


 Add blank line here,

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

 
  sysfs files have ONE VALUE PER FILE, not a whole bunch of different
  things in a single file.  Please fix this.

 The subparameters _are_ actually part of a single value, that value being
 associated with the initiator instance.

 Konrad is trying to implement a work-alike for what open firmware does.
 open-iscsi already has the ability to extract the same format
 bits from real OFW.

 See open-iscsi.git/utils/fwparam_ppc.


Greg,

In light of what Doug says (which is all true), should I go ahead with a new 
version of this module which would export one value per file? The problem 
that will be encountered is that a ethernetX sysfs directory would have (for 
example):

/sys/firmware/ibft/ethernet0/pci-bdf
5:1:0
/sys/firmware/ibft/ethernet0/mac
00:11:25:9d:8b:00
/sys/firmware/ibft/ethernet0/vlan
0
/sys/firmware/ibft/ethernet0/gateway
192.168.79.254
/sys/firmware/ibft/ethernet0/origin
0
/sys/firmware/ibft/ethernet0/subnet-mask
22
/sys/firmware/ibft/ethernet0/ip-addr
192.168.77.41
/sys/firmware/ibft/ethernet0/flags
7

And the flag would contain the value 7 which would mean the user would have 
to parse what each bit means? (the v0.3 of the module does not export this 
flag but uses it to figure out which is the boot iSCSI target).

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

2007-11-26 Thread Doug Maxey


On Mon, 26 Nov 2007 19:31:38 PST, Greg KH wrote:
 On Mon, Nov 26, 2007 at 06:56:42PM -0400, Konrad Rzeszutek wrote:
  +/*
  + *  Routines for reading of the iBFT data in a human readable fashion.
  + */
  +ssize_t ibft_attr_show_initiator(struct ibft_kobject *entry,
  +struct ibft_attribute *attr,
  +char *buf)
  +{
  +   struct ibft_initiator *initiator = attr-initiator;
  +   void *ibft_loc = entry-data-hdr;
  +   char *str = buf;
  +
  +   if (!initiator)
  +   return 0;
  +
  +   str += sprintf_ipaddr(str, isns, initiator-isns_server);
  +   str += sprintf_ipaddr(str, slp, initiator-slp_server);
  +   str += sprintf_ipaddr(str, primary_radius_server,
  +   initiator-pri_radius_server);
  +   str += sprintf_ipaddr(str, secondary_radius_server,
  +   initiator-sec_radius_server);
  +   str += sprintf_string(str, itname, initiator-initiator_name_len,
  +   (char *)ibft_loc + initiator-initiator_name_off);
  +   str--;
  +
  +   return str-buf;
  +}
 
 sysfs files have ONE VALUE PER FILE, not a whole bunch of different
 things in a single file.  Please fix this.

The subparameters _are_ actually part of a single value, that value being 
associated with the initiator instance.

Konrad is trying to implement a work-alike for what open firmware does.
open-iscsi already has the ability to extract the same format 
bits from real OFW.

See open-iscsi.git/utils/fwparam_ppc.

 
 
  +
  +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
  +  struct ibft_attribute *attr,
  +  char *buf)
  +{
  +   struct ibft_nic *nic = attr-nic;
  +   void *ibft_loc = entry-data-hdr;
  +   char *str = buf;
  +
  +   if (!nic)
  +   return 0;
  +   /*
  +* Assume dhcp if any non-zero portions of its address are set.
  +*/
  +   if (memcmp(nic-dhcp, nulls, sizeof(nic-dhcp))) {
  +   str += sprintf_ipaddr(str, dhcp, nic-dhcp);
  +   } else {
  +   str += sprintf_ipaddr(str, ciaddr, nic-ip_addr);
  +   str += sprintf_ipaddr(str, giaddr, nic-gateway);
  +   str += sprintf_ipaddr(str, dnsaddr1, nic-primary_dns);
  +   str += sprintf_ipaddr(str, dnsaddr2, nic-secondary_dns);
  +   }
  +   if (nic-hostname_len)
  +   str += sprintf_string(str, hostname, nic-hostname_len,
  +   (char *)ibft_loc + nic-hostname_off);
  +   /* Cut off the comma. */
  +   str--;
  +
  +   return str-buf;
  +}
 
 Same here.
 
  +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
  + struct ibft_attribute *attr,
  + char *buf)
  +{
  +   struct ibft_tgt *tgt = attr-tgt;
  +   void *ibft_loc = entry-data-hdr;
  +   char *str = buf;
  +   int i;
  +
  +   if (!tgt)
  +   return 0;
  +
  +   str += sprintf_ipaddr(str, siaddr, tgt-ip_addr);
  +   str += sprintf(str, iport=%d,, tgt-port);
  +   str += sprintf(str, ilun=);
  +   for (i = 0; i  8; i++)
  +   str += sprintf(str, %x, (u8)tgt-lun[i]);
  +   str += sprintf(str, ,);
  +
  +   if (tgt-tgt_name_len)
  +   str += sprintf_string(str, iname, tgt-tgt_name_len,
  +   (void *)ibft_loc + tgt-tgt_name_off);
  +
  +   if (tgt-chap_name_len)
  +   str += sprintf_string(str, chapid, tgt-chap_name_len,
  +   (char *)ibft_loc + tgt-chap_name_off);
  +   if (tgt-chap_secret_len)
  +   str += sprintf_string(str, chappw, tgt-chap_secret_len,
  +   (char *)ibft_loc + tgt-chap_secret_off);
  +   if (tgt-rev_chap_name_len)
  +   str += sprintf_string(str, ichapid, tgt-rev_chap_name_len,
  +   (char *)ibft_loc + tgt-rev_chap_name_off);
  +   if (tgt-rev_chap_secret_len)
  +   str += sprintf_string(str, ichappw, tgt-rev_chap_secret_len,
  +   (char *)ibft_loc + tgt-rev_chap_secret_off);
  +
  +   /* Cut off the comma. */
  +   str--;
  +
  +   return str-buf;
  +}
 
 Same here, are we writing a novella or something to userspace?  :)

Yep.  Just like real OFW.

 
  +ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
  +   struct ibft_attribute *ibft_attr,
  +   char *buf)
  +{
  +   char *str = buf;
  +
  +   str += sprintf(str, //[EMAIL PROTECTED],%d:iscsi,, dev-data-index);
  +   str += ibft_attr_show_initiator(dev, ibft_attr, str);
  +   str += sprintf(str, ,);
  +   str += ibft_attr_show_target(dev, ibft_attr, str);
  +   str += sprintf(str, ,);
  +   str += ibft_attr_show_nic(dev, ibft_attr, str);
  +
  +   return str-buf;
  +}
 
 And here, do I need to go on?
 
  +ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
  +  struct ibft_attribute *attr,
  +  char *buf)
  +{
  +   struct ibft_nic *nic = attr-nic;
  +   int len = 6;
  +
  +   if (!nic)
  +   return 0;
  +
  +   memcpy(buf, attr-nic-mac, len);
  +
  +   return len;
  +}
 
 Is

Re: [PATCH] Add iSCSI IBFT Support (v0.3)

On Mon, Nov 26, 2007 at 11:50:10PM -0500, Konrad Rzeszutek wrote:
  
   sysfs files have ONE VALUE PER FILE, not a whole bunch of different
   things in a single file.  Please fix this.
 
  The subparameters _are_ actually part of a single value, that value being
  associated with the initiator instance.
 
  Konrad is trying to implement a work-alike for what open firmware does.
  open-iscsi already has the ability to extract the same format
  bits from real OFW.
 
  See open-iscsi.git/utils/fwparam_ppc.
 
 
 Greg,
 
 In light of what Doug says (which is all true), should I go ahead with a new 
 version of this module which would export one value per file? The problem 
 that will be encountered is that a ethernetX sysfs directory would have (for 
 example):
 
 /sys/firmware/ibft/ethernet0/pci-bdf
 5:1:0
 /sys/firmware/ibft/ethernet0/mac
 00:11:25:9d:8b:00
 /sys/firmware/ibft/ethernet0/vlan
 0
 /sys/firmware/ibft/ethernet0/gateway
 192.168.79.254
 /sys/firmware/ibft/ethernet0/origin
 0
 /sys/firmware/ibft/ethernet0/subnet-mask
 22
 /sys/firmware/ibft/ethernet0/ip-addr
 192.168.77.41
 /sys/firmware/ibft/ethernet0/flags
 7

Yes, that is the proper way to do this kind of thing in sysfs.

 And the flag would contain the value 7 which would mean the user would have 
 to parse what each bit means? (the v0.3 of the module does not export this 
 flag but uses it to figure out which is the boot iSCSI target).

Sure, as long as it means something to userspace, and is a single value,
and is documented, that's fine.

thanks,

greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add iSCSI IBFT Support (v0.3)