Re: [PATCH] Add iSCSI iBFT support.

From: Konrad Rzeszutek
Date: Wed Sep 26 2007 - 20:54:00 EST

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

> > + * drivers/firmware/iscsi_ibft.c
> Don't repeat the file name.

> > + * This code exposes the the iSCSI Boot Format Table to userland via
> > sysfs.
> ~~~~~~~
> yes.

> > +
> no blank line here.

> > +static ssize_t
> > +ibft_read_binary(struct kobject *kobj, struct bin_attribute *attr, char
> > *buf, + loff_t off, size_t count)
> Put 'static ssize_t' on same line as function name, then put parameters
> on following lines as needed.

> > +static int
> > +ibft_mmap_binary(struct kobject *kobj, struct bin_attribute *attr,
> > + struct vm_area_struct *vma)
> ditto.

> > + /* Need PAGE_ALING for mmap functionality. */

> > + printk(KERN_INFO "BIOS iBFT unloaded.\n");
> Drop the unload message. ibft_init() is also quite noisy.

> 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.
> > + unsigned long pos;
> add blank line here between data / code.



Thank you for taking time to do such thoroughly review.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at