Add iSCSI IBFT Support (v0.3)

This patch adds /sysfs/firmware/ibft/[chosen|aliases|ethernet@0,X|target@0,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]>

arch/x86/kernel/setup_32.c +5 -0

index e1e18c3..e3ed866 100644
@@ -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

arch/x86/kernel/setup_32.c +2 -0

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

arch/x86/kernel/setup_64.c +5 -0

index 30d94d1..ba6fd21 100644
@@ -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

arch/x86/kernel/setup_64.c +2 -0

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

drivers/firmware/iscsi_ibft.c new file +4 -0

new file mode 100644
index 0000000..7e4e117
@@ -0,0 +1,612 @@
+
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/types.h>

drivers/firmware/iscsi_ibft.c new file +17 -0

+#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;

drivers/firmware/iscsi_ibft.c new file +33 -0

+			"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);

drivers/firmware/iscsi_ibft.c new file +20 -0

+}
+
+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);

drivers/firmware/iscsi_ibft.c new file +32 -0

+}
+
+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)

drivers/firmware/iscsi_ibft.c new file +6 -0

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

drivers/firmware/iscsi_ibft.c new file +76 -0

+
+	if (!nic)
+		return 0;
+
+	memcpy(buf, attr->nic->mac, len);
+
+	return len;
+}
+
+/*
+ * 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)
+{
+	struct ibft_kobject *dev =
+		container_of(kobj, struct ibft_kobject, kobj);
+	struct ibft_attribute *ibft_attr =
+		container_of(attr, struct ibft_attribute, attr);
+	ssize_t ret = -EIO;
+	char *str = buf;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;
+
+	if (ibft_attr->show)
+		ret = ibft_attr->show(dev, ibft_attr, str);
+
+	return ret;
+}
+
+static struct sysfs_ops ibft_attr_ops = {
+	.show = ibft_show_attribute,
+};
+
+static struct kobj_type ktype_ibft = {
+	.release = ibft_release,
+	.sysfs_ops = &ibft_attr_ops,
+};
+
+static decl_subsys(ibft, &ktype_ibft, NULL);
+
+
+static int ibft_alloc_device(void *idev)
+{
+	int len;
+	struct ibft_table_header *hdr;
+
+	if (!idev)
+		return -ENOENT;
+
+	hdr = (struct ibft_table_header *)phys_to_virt(
+			(unsigned long)ibft_phys);
+
+	len = hdr->length;
+
+	ibft_device = kzalloc(len, GFP_KERNEL);
+
+	if (!ibft_device)
+		return -ENOMEM;
+
+	memcpy(ibft_device, hdr, len);
+
+	return 0;
+}
+
+static void ibft_free_device(struct ibft_table_header *hdr)
+{
+	kfree(hdr);
+};
+
+
+/*
+ * Helper function for ibft_scan_device.
+ */

drivers/firmware/iscsi_ibft.c new file +15 -0

+		data->initiator = initiator;
+		data->hdr = header;
+		data->index = hdr->index;
+		list_add_tail(&data->node, list);
+	}
+
+	switch (hdr->id) {
+	case id_nic:
+		data->nic = (struct ibft_nic *)hdr;
+		rc = ibft_verify_hdr(hdr, id_nic, sizeof(*data->nic));
+		break;
+	case id_target:
+		data->tgt = (struct ibft_tgt *)hdr;
+		rc = ibft_verify_hdr(hdr, id_target, sizeof(*data->tgt));
+		break;

+ */

drivers/firmware/iscsi_ibft.c new file +8 -0

+static int ibft_scan_device(struct ibft_table_header *header,
+			    struct list_head *list)
+{
+	struct ibft_control *control = NULL;
+	struct ibft_initiator *initiator = NULL;
+	void *ptr, *end;
+	int rc = 0;
+	u16 offset;

drivers/firmware/iscsi_ibft.c new file +64 -0

+		offset = *(u16 *)ptr;
+		if (offset) {
+			rc = ibft_populate_data(header,
+					(void *)header + offset,
+					initiator, list);
+			if (rc)
+				return rc;
+		}
+	}
+	return rc;
+};
+
+static void ibft_free_data(struct list_head *list)
+{
+	struct ibft_data *data = NULL, *n;
+
+	list_for_each_entry_safe(data, n, list, node) {
+		list_del(&data->node);
+		kfree(data);
+	}
+	list_del_init(list);
+};
+
+/*
+ * Helper function for ibft_register_kobjects.
+ */
+static int ibft_create_kobject(struct ibft_data *data,
+			       u8 type, const char *name,
+			       struct list_head *list)
+{
+	struct ibft_kobject *obj = NULL;
+	int rc = 0;
+
+	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+	if (!obj)
+		return -ENOMEM;
+
+	snprintf(obj->name, IBFT_ISCSI_KOBJECT_MAX_LEN, name, data->index);
+
+	obj->data = data;
+	obj->type = type;
+
+	kobject_set_name(&obj->kobj, obj->name);
+	kobj_set_kset_s(obj, ibft_subsys);
+
+	rc = kobject_register(&obj->kobj);
+	if (rc)
+		return rc;
+
+	list_add_tail(&obj->node, list);
+	return rc;
+}
+
+static int ibft_register_kobjects(struct list_head *data_list,
+				  struct list_head *kobj_list)
+{
+	struct ibft_data *data = NULL, *n = NULL;
+	int rc = 0;
+
+	list_for_each_entry_safe(data, n, data_list, node) {
+		if (data->tgt)
+			rc |= ibft_create_kobject(data, kobject_type_target,
+				IBFT_ISCSI_KOBJECT_TARGET_NAME, kobj_list);
+		if (data->nic)

drivers/firmware/iscsi_ibft.c new file +116 -0

+	}
+	return rc;
+};
+
+static void ibft_unregister_kobjects(struct list_head *list)
+{
+	struct ibft_kobject *data = NULL, *n;
+	list_for_each_entry_safe(data, n, list, node) {
+		list_del(&data->node);
+		kobject_unregister(&data->kobj);
+	};
+	list_del_init(list);
+};
+
+static int ibft_create_attribute(struct ibft_kobject *kobj_data,
+				 const char *name,
+				 ssize_t (*show) (struct  ibft_kobject *entry,
+						  struct ibft_attribute *attr,
+						  char *buf),
+				 struct list_head *list)
+{
+	struct ibft_attribute *attr = NULL;
+
+	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+
+	snprintf(attr->name, IBFT_ISCSI_ATTR_MAX_LEN, name,
+		kobj_data->data->index);
+
+	attr->attr.name = attr->name;
+	attr->attr.mode = S_IRUSR;
+	attr->attr.owner = THIS_MODULE;
+
+	attr->nic = kobj_data->data->nic;
+	attr->tgt = kobj_data->data->tgt;
+	attr->initiator = kobj_data->data->initiator;
+
+	attr->show = show;
+	attr->kobj = &kobj_data->kobj;
+
+	list_add_tail(&attr->node, list);
+	return 0;
+}
+/*
+ *  Register the attributes for all of the kobjects.
+ */
+static int ibft_register_attributes(struct list_head *kobject_list,
+				    struct list_head *attr_list)
+{
+	int rc = 0;
+	struct ibft_kobject *data = NULL;
+	struct ibft_attribute *attr = NULL, *m;
+
+	list_for_each_entry(data, kobject_list, node) {
+		switch (data->type) {
+		case kobject_type_chosen:
+			rc = ibft_create_attribute(data,
+				IBFT_ISCSI_ATTR_BOOTPATH_NAME,
+				ibft_attr_show_disk, attr_list);
+			break;
+		case kobject_type_ethernet:
+			rc |= ibft_create_attribute(data,
+					IBFT_ISCSI_ATTR_LOCAL_MAC_ADDRESS_NAME,
+					ibft_attr_show_mac, attr_list);
+			rc |= ibft_create_attribute(data,
+					IBFT_ISCSI_ATTR_PROPERTY_NAME,
+					ibft_attr_show_nic, attr_list);
+			break;
+		case kobject_type_target:
+			rc |= ibft_create_attribute(data,
+					IBFT_ISCSI_ATTR_PROPERTY_NAME,
+					ibft_attr_show_target, attr_list);
+			break;
+		case kobject_type_aliases:
+			rc |= ibft_create_attribute(data,
+					IBFT_ISCSI_ATTR_DISK_NAME,
+					ibft_attr_show_disk, attr_list);
+			break;
+		default:
+			break;
+		}
+		if (rc)
+			break;
+	}
+	list_for_each_entry_safe(attr, m, attr_list, node) {
+		rc = sysfs_create_file(attr->kobj, &attr->attr);
+		if (rc) {
+			list_del(&attr->node);
+			kfree(attr);
+			break;
+		}
+	}
+	return rc;
+};
+
+static void ibft_unregister_attributes(struct list_head *list)
+{
+	struct ibft_attribute *attr = NULL, *m;
+
+	list_for_each_entry_safe(attr, m, list, node) {
+		sysfs_remove_file(attr->kobj, &attr->attr);
+		list_del(&attr->node);
+		kfree(attr);
+	}
+	list_del_init(list);
+};
+
+
+/*
+ * ibft_init() - creates sysfs tree entries for iBFT data.
+ */
+static int __init ibft_init(void)
+{
+	int rc = 0;
+

drivers/firmware/iscsi_ibft.c new file +16 -0

+
+		rc = ibft_alloc_device(ibft_phys);
+		if (rc)
+			goto out_firmware_unregister;
+
+		/* Scan the IBFT for data. */
+		rc = ibft_scan_device(ibft_device, &ibft_data_list);
+		if (rc)
+			goto out_free;
+
+		/* Register the kobjects based on the ibft_data_list */
+		rc = ibft_register_kobjects(&ibft_data_list,
+				&ibft_kobject_list);
+		if (rc)
+			goto out_free;
+

drivers/firmware/iscsi_ibft.c new file +25 -0

+
+out_free:
+	ibft_unregister_attributes(&ibft_attr_list);
+	ibft_unregister_kobjects(&ibft_kobject_list);
+	ibft_free_data(&ibft_data_list);
+	ibft_free_device(ibft_device);
+	ibft_device = NULL;
+out_firmware_unregister:
+	firmware_unregister(&ibft_subsys);
+	return rc;
+}
+
+static void __exit ibft_exit(void)
+{
+	ibft_unregister_attributes(&ibft_attr_list);
+	ibft_unregister_kobjects(&ibft_kobject_list);
+	ibft_free_data(&ibft_data_list);
+	ibft_free_device(ibft_device);
+	ibft_device = NULL;
+	firmware_unregister(&ibft_subsys);
+	ibft_phys = 0;
+}
+
+module_init(ibft_init);
+module_exit(ibft_exit);

include/linux/iscsi_ibft.h new file +79 -0

new file mode 100644
index 0000000..bbbb53c
@@ -0,0 +1,198 @@
+#ifndef ISCSI_IBFT_H
+#define ISCSI_IBFT_H
+
+extern void *ibft_phys;
+
+struct ibft_table_header {
+	char signature[4];
+	u32 length;
+	u8 revision;
+	u8 checksum;
+	char oem_id[6];
+	char oem_table_id[8];
+	char reserved[24];
+} __attribute__((__packed__));
+
+#define ISCSI_IBFT_INIT_FLAG_FW_SEL_BOOT 2
+
+enum ibft_id {
+    id_reserved = 0,
+    id_control = 1,
+    id_initiator = 2,
+    id_nic = 3,
+    id_target = 4,
+    id_extensions,
+};
+struct ibft_hdr {
+	u8 id;
+	u8 version;
+	u16 length;
+	u8 index;
+	u8 flags;
+} __attribute__((__packed__));
+
+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__));
+
+struct ibft_initiator {
+	struct ibft_hdr hdr;
+	char isns_server[16];
+	char slp_server[16];
+	char pri_radius_server[16];
+	char sec_radius_server[16];
+	u16 initiator_name_len;
+	u16 initiator_name_off;
+} __attribute__((__packed__));
+
+struct ibft_nic {
+	struct ibft_hdr hdr;
+	char ip_addr[16];
+	u8 subnet_mask_prefix;
+	u8 origin;
+	char gateway[16];
+	char primary_dns[16];
+	char secondary_dns[16];
+	char dhcp[16];
+	u16 vlan;
+	char mac[6];
+	u16 pci_bdf;
+	u16 hostname_len;
+	u16 hostname_off;
+} __attribute__((__packed__));
+
+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;

include/linux/iscsi_ibft.h new file +26 -0

+static ssize_t find_ibft(void)
+{
+	unsigned long pos;
+
+	for (pos = IBFT_START; pos < IBFT_END; pos += 16) {
+		void *virt;
+		/* The table can't be inside the VGA BIOS reserved space,
+		 * so skip that area */
+		if (pos == VGA_MEM)
+			pos += VGA_SIZE;
+		virt = phys_to_virt(pos);
+		if (memcmp(virt, IBFT_SIGN, IBFT_SIGN_LEN) == 0) {
+			unsigned long *addr =
+			    (unsigned long *)phys_to_virt(pos + 4);
+			unsigned int len = *addr;
+			/* if the length of the table extends past 1M,
+			 * the table cannot be valid. */
+			if (pos + len <= (IBFT_END-1)) {
+				ibft_phys = (void *)pos;
+				return len;
+			}
+		}
+	}
+	return 0;
+}
+

include/linux/iscsi_ibft.h new file +5 -0

+};
+
+#endif
+
+#endif /* ISCSI_IBFT_H */

Review

Quoted 2 lines

(quoted diff) +2 -0

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

No ending ; above.

Quoted 1 line

+static void __init reserve_ibft_region(void) { };

Ditto.

Review

Quoted 2 lines

(quoted diff) +2 -0

+#include <linux/blkdev.h>
+

No blank line here, please.

Quoted 2 lines

+#include <linux/iscsi_ibft.h>

drivers/firmware/iscsi_ibft.c line 80

Quoted 1 line

drivers/firmware/iscsi_ibft.c new file +1 -0

+		printk(KERN_INFO  \

Looks like this should use KERN_ERROR or KERN_WARNING?

drivers/firmware/iscsi_ibft.c line 123

Quoted 3 lines

drivers/firmware/iscsi_ibft.c new file +2 -0

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

	preferred form:
	return str - buf;

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	return str-buf;

Ditto.

Review

Quoted 1 line

(quoted diff) +1 -0

+	return str-buf;

Ditto.

Review

Quoted 1 line

(quoted diff) +1 -0

+	return str-buf;

Ditto.

drivers/firmware/iscsi_ibft.c line 219

Quoted 1 line

drivers/firmware/iscsi_ibft.c new file +1 -0

+	int len = 6;

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

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

drivers/firmware/iscsi_ibft.c line 305

Quoted 4 lines

drivers/firmware/iscsi_ibft.c new file +3 -0

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

drivers/firmware/iscsi_ibft.c line 317

Quoted 1 line

drivers/firmware/iscsi_ibft.c new file +1 -0

+		   struct carry it for convience. */

convenience.

Quoted 2 lines

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

passed-in list.

drivers/firmware/iscsi_ibft.c line 359

Quoted 8 lines

+ */

drivers/firmware/iscsi_ibft.c new file +6 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

+	*/
+	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;

It looks like that should be the case here also....

drivers/firmware/iscsi_ibft.c line 435

Quoted 1 line

drivers/firmware/iscsi_ibft.c new file +1 -0

+		if (rc) break;

	break;
		on a separate line.

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

drivers/firmware/iscsi_ibft.c line 561

Quoted 2 lines

drivers/firmware/iscsi_ibft.c new file +2 -0

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

Use %p to print pointer values.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (!rc)
+		return rc;

Can't this always just be

	return 0;

?

Quoted 2 lines

+

Quoted 15 lines

include/linux/iscsi_ibft.h new file +15 -0

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

include/linux/iscsi_ibft.h line 89

Quoted 6 lines

include/linux/iscsi_ibft.h new file +6 -0

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

include/linux/iscsi_ibft.h line 96

Quoted 7 lines

include/linux/iscsi_ibft.h new file +6 -0

+#define IBFT_SIGN "iBFT"
+#define IBFT_SIGN_LEN 4
+#define IBFT_START 0x80000 /* 512kB */
+#define IBFT_END 0x100000 /* 1MB */
+#define VGA_MEM 0xA0000 /* VGA buffer */
+#define VGA_SIZE 0x20000 /* 132kB */

I'd say that if 0x80000 is 512kB, then 0x20000 is 128kB.

Add blank line here, please.

This patch adds /sysfs/firmware/ibft/[chosen|aliases|ethernet@0,X|target@0,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

drivers/firmware/iscsi_ibft.c new file +16 -0

+/*
+ *  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);

drivers/firmware/iscsi_ibft.c new file +20 -0

+
+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);

drivers/firmware/iscsi_ibft.c new file +31 -0

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

drivers/firmware/iscsi_ibft.c new file +5 -0

+ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
+			    struct ibft_attribute *ibft_attr,
+			    char *buf)
+{
+	char *str = buf;

drivers/firmware/iscsi_ibft.c new file +4 -0

+ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
+			   struct ibft_attribute *attr,
+			   char *buf)
+{

drivers/firmware/iscsi_ibft.c new file +16 -0

+/*
+ * 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)
+{
+	struct ibft_kobject *dev =
+		container_of(kobj, struct ibft_kobject, kobj);
+	struct ibft_attribute *ibft_attr =
+		container_of(attr, struct ibft_attribute, attr);
+	ssize_t ret = -EIO;
+	char *str = buf;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EACCES;

include/linux/iscsi_ibft.h new file +23 -0

+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)
+
+#define IBFT_SIGN "iBFT"
+#define IBFT_SIGN_LEN 4
+#define IBFT_START 0x80000 /* 512kB */
+#define IBFT_END 0x100000 /* 1MB */
+#define VGA_MEM 0xA0000 /* VGA buffer */
+#define VGA_SIZE 0x20000 /* 132kB */
+static ssize_t find_ibft(void)
+{
+	unsigned long pos;
+
+	for (pos = IBFT_START; pos < IBFT_END; pos += 16) {
+		void *virt;
+		/* The table can't be inside the VGA BIOS reserved space,
+		 * so skip that area */
+		if (pos == VGA_MEM)
+			pos += VGA_SIZE;
+		virt = phys_to_virt(pos);
+		if (memcmp(virt, IBFT_SIGN, IBFT_SIGN_LEN) == 0) {
+			unsigned long *addr =
+			    (unsigned long *)phys_to_virt(pos + 4);
+			unsigned int len = *addr;

include/linux/iscsi_ibft.h new file +15 -0

+	struct list_head node;
+};
+
+/*
+ * The text attributes, which can be:
+ *  - local-mac-address (many of them)
+ *  - property (many of them)
+ *  - bootpath (one) , iscsi-diskX (many)
+ *
+*/
+#define IBFT_ISCSI_ATTR_DISK_NAME "iscsi-disk%d"
+#define IBFT_ISCSI_ATTR_PROPERTY_NAME "property"
+#define IBFT_ISCSI_ATTR_BOOTPATH_NAME "bootpath"
+#define IBFT_ISCSI_ATTR_LOCAL_MAC_ADDRESS_NAME "local-mac-address"
+#define IBFT_ISCSI_ATTR_MAX_LEN 18

Quoted 7 lines · Konrad Rzeszutek

drivers/firmware/iscsi_ibft.c new file +7 -0

+/*
+ *  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)
+{

Quoted 3 lines

drivers/firmware/iscsi_ibft.c new file +3 -0

+
+	str += sprintf_ipaddr(str, "isns", initiator->isns_server);
+	str += sprintf_ipaddr(str, "slp", initiator->slp_server);

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

Quoted 5 lines

drivers/firmware/iscsi_ibft.c new file +4 -0

+
+ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
+			   struct ibft_attribute *attr,
+			   char *buf)

drivers/firmware/iscsi_ibft.c line 144

Quoted 2 lines

drivers/firmware/iscsi_ibft.c new file +2 -0

+		str += sprintf_ipaddr(str, "giaddr", nic->gateway);
+		str += sprintf_ipaddr(str, "dnsaddr1", nic->primary_dns);

Same here.

Quoted 5 lines

drivers/firmware/iscsi_ibft.c new file +4 -0

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

Quoted 1 line

+}

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

Quoted 5 lines

drivers/firmware/iscsi_ibft.c new file +4 -0

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

Quoted 1 line

+}

And here, do I need to go on?

Quoted 5 lines

drivers/firmware/iscsi_ibft.c new file +4 -0

+ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
+			   struct ibft_attribute *attr,
+			   char *buf)
+{

drivers/firmware/iscsi_ibft.c line 227

Quoted 5 lines

drivers/firmware/iscsi_ibft.c new file +5 -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...

Quoted 8 lines

drivers/firmware/iscsi_ibft.c new file +7 -0

+/*
+ * 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)
+{

drivers/firmware/iscsi_ibft.c line 254

Quoted 4 lines

drivers/firmware/iscsi_ibft.c new file +4 -0

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

include/linux/iscsi_ibft.h line 42

Quoted 10 lines

include/linux/iscsi_ibft.h new file +9 -0

+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 :(

Quoted 2 lines

+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

Quoted 2 lines

include/linux/iscsi_ibft.h new file +2 -0

+static ssize_t find_ibft(void)
+{

Quoted 1 line

+}

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

include/linux/iscsi_ibft.h line 165

Quoted 3 lines

include/linux/iscsi_ibft.h new file +3 -0

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

Why have this,

include/linux/iscsi_ibft.h line 167

Quoted 2 lines

include/linux/iscsi_ibft.h new file +2 -0

+	u8 type;
+	struct kobject kobj;

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

include/linux/iscsi_ibft.h line 192

Quoted 1 line

include/linux/iscsi_ibft.h new file +1 -0

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

Review

Quoted 3 lines

(quoted diff) +2 -0

+	struct list_head node;
+};

thanks,

greg k-h

Quoted 1 line · On Monday 26 November 2007 22:31:38 Greg KH wrote:

Quoted 1 line

+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

..snip..

Quoted 2 lines

Quoted 2 lines

include/linux/iscsi_ibft.h new file +2 -0

+static ssize_t find_ibft(void)
+{

..snip..

Quoted 3 lines

Quoted 1 line

+}

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 ?

Quoted 10 lines · Konrad Rzeszutek

Quoted 1 line · On Monday 26 November 2007 22:31:38 Greg KH wrote:

Quoted 1 line

+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

..snip..

Quoted 2 lines

Quoted 2 lines

include/linux/iscsi_ibft.h new file +2 -0

+static ssize_t find_ibft(void)
+{

..snip..

Quoted 3 lines

Quoted 1 line

+}

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

Quoted 4 lines

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.

Quoted 16 lines · Greg KH

Quoted 14 lines · Konrad Rzeszutek

Quoted 1 line · On Monday 26 November 2007 22:31:38 Greg KH wrote:

Quoted 1 line

+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

..snip..

Quoted 2 lines

Quoted 2 lines

include/linux/iscsi_ibft.h new file +2 -0

+static ssize_t find_ibft(void)
+{

..snip..

Quoted 3 lines

Quoted 1 line

+}

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 ?

Quoted 2 lines

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?

Quoted 17 lines · On Tue, Nov 27, 2007 at 02:09:50PM -0400, [email protected] wrote:

Quoted 16 lines · Greg KH

Quoted 14 lines · Konrad Rzeszutek

Quoted 1 line · On Monday 26 November 2007 22:31:38 Greg KH wrote:

Quoted 1 line

+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

..snip..

Quoted 2 lines

Quoted 2 lines

include/linux/iscsi_ibft.h new file +2 -0

+static ssize_t find_ibft(void)
+{

..snip..

Quoted 3 lines

Quoted 1 line

+}

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 ?

Quoted 7 lines

Quoted 2 lines

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 :)

Quoted 4 lines

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.

Quoted 28 lines · Greg KH

Quoted 17 lines · On Tue, Nov 27, 2007 at 02:09:50PM -0400, [email protected] wrote:

Quoted 16 lines · Greg KH

Quoted 14 lines · Konrad Rzeszutek

Quoted 1 line · On Monday 26 November 2007 22:31:38 Greg KH wrote:

Quoted 1 line

+#if defined(CONFIG_ISCSI_IBFT) || defined(CONFIG_ISCSI_IBFT_MODULE)

..snip..

Quoted 2 lines

Quoted 2 lines

(quoted diff) +2 -0

+static ssize_t find_ibft(void)
+{

..snip..

Quoted 3 lines

Quoted 1 line

+}

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 ?

Quoted 7 lines

Quoted 2 lines

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 :)

Quoted 7 lines

Quoted 4 lines

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

Quoted 27 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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 ?

Quoted 21 lines · Konrad Rzeszutek

drivers/firmware/iscsi_ibft.c new file +21 -0

+/*
+ *  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,

drivers/firmware/iscsi_ibft.c line 124

Quoted 5 lines

drivers/firmware/iscsi_ibft.c new file +5 -0

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

Quoted 22 lines

drivers/firmware/iscsi_ibft.c new file +20 -0

+
+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);

drivers/firmware/iscsi_ibft.c line 154

Quoted 10 lines

drivers/firmware/iscsi_ibft.c new file +10 -0

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

Quoted 35 lines

drivers/firmware/iscsi_ibft.c new file +35 -0

+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);

drivers/firmware/iscsi_ibft.c line 154

Quoted 6 lines

drivers/firmware/iscsi_ibft.c new file +6 -0

+
+	/* Cut off the comma. */
+	str--;
+
+	return str-buf;
+}

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

Quoted 6 lines

drivers/firmware/iscsi_ibft.c new file +5 -0

+ssize_t ibft_attr_show_disk(struct ibft_kobject *dev,
+			    struct ibft_attribute *ibft_attr,
+			    char *buf)
+{
+	char *str = buf;

drivers/firmware/iscsi_ibft.c line 212

Quoted 10 lines

drivers/firmware/iscsi_ibft.c new file +10 -0

+
+	str += sprintf(str, "//ethernet@0,%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?

Quoted 4 lines

drivers/firmware/iscsi_ibft.c new file +4 -0

+ssize_t ibft_attr_show_mac(struct ibft_kobject *entry,
+			   struct ibft_attribute *attr,
+			   char *buf)
+{

drivers/firmware/iscsi_ibft.c line 227

Quoted 10 lines

drivers/firmware/iscsi_ibft.c new file +10 -0

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

Quoted 3 lines

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.

Quoted 4 lines

Quoted 3 lines

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

Quoted 8 lines

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

Quoted 4 lines

Quoted 3 lines

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

Quoted 8 lines

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

Quoted 3 lines

/sys/firmware/ibft/ethernet0/pci-bdf
5:1:0

shouldn't this somehow also have a symlink to the kernels ethX view of ethernet devices?
(and if so.. how much of the info is duplicated..)

Quoted 7 lines

Quoted 3 lines

/sys/firmware/ibft/ethernet0/pci-bdf
5:1:0

shouldn't this somehow also have a symlink to the kernels ethX view of ethernet devices?
(and if so.. how much of the info is duplicated..)

That NIC is used by the NIC firmware (or the BIOS) to negotiate the iSCSI target. The information that is in this directory is what the BIOS used (note past tense) which might very well be completly different from what Linux uses.

My rationale behind _not_ linking to ethX view was that it could be confusing and not entirely useful for users: "It says that _this_ NIC, with this IP, uses this iSCSI target. But Linux is using a completly different NIC (and IP) to setup a iSCSI connection to the same iSCSI target!?"