[PATCH] Add iSCSI iBFT support (v0.4.5) - Kernel

This is a discussion on [PATCH] Add iSCSI iBFT support (v0.4.5) - Kernel ; Hey Andrew, Please add this patch along with Greg KH's kobject fixes. This module is dependent on the fixes that Greg KH has in his patches git tree. This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX] directories along with text properties which export ...

+ Reply to Thread
Results 1 to 13 of 13

Thread: [PATCH] Add iSCSI iBFT support (v0.4.5)

  1. [PATCH] Add iSCSI iBFT support (v0.4.5)

    Hey Andrew,

    Please add this patch along with Greg KH's kobject fixes. This module
    is dependent on the fixes that Greg KH has in his patches git tree.

    This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX]
    directories along with text properties which export the the iSCSI
    Boot Firmware Table (iBFT) structure.

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

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

    Signed-off-by: Konrad Rzeszutek

    diff --git a/Documentation/ABI/testing/sysfs-ibft b/Documentation/ABI/testing/sysfs-ibft
    new file mode 100644
    index 0000000..062c009
    --- /dev/null
    +++ b/Documentation/ABI/testing/sysfs-ibft
    @@ -0,0 +1,23 @@
    +What: /sys/firmware/ibft/initiator
    +Date: November 2007
    +Contact: Konrad Rzeszutek
    +Description: The /sys/firmware/ibft/initiator directory will contain
    + files that expose the iSCSI Boot Firmware Table initiator data.
    + Usually this contains the Initiator name.
    +
    +What: /sys/firmware/ibft/targetX
    +Date: November 2007
    +Contact: Konrad Rzeszutek
    +Description: The /sys/firmware/ibft/targetX directory will contain
    + files that expose the iSCSI Boot Firmware Table target data.
    + Usually this contains the target's IP address, boot LUN,
    + target name, and what NIC it is associated with. It can also
    + contain the CHAP name (and password), the reverse CHAP
    + name (and password)
    +
    +What: /sys/firmware/ibft/ethernetX
    +Date: November 2007
    +Contact: Konrad Rzeszutek
    +Description: The /sys/firmware/ibft/ethernetX directory will contain
    + files that expose the iSCSI Boot Firmware Table NIC data.
    + This can this can the IP address, MAC, and gateway of the NIC.
    diff --git a/arch/x86/kernel/setup_32.c b/arch/x86/kernel/setup_32.c
    index 9c24b45..8127cad 100644
    --- a/arch/x86/kernel/setup_32.c
    +++ b/arch/x86/kernel/setup_32.c
    @@ -39,6 +39,7 @@
    #include
    #include
    #include
    +#include
    #include
    #include
    #include
    @@ -148,6 +149,20 @@ static inline void copy_edd(void)
    }
    #endif

    +#if defined(CONFIG_ISCSI_IBFT_FIND)
    +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) { }
    +#endif
    +
    int __initdata user_defined_memmap = 0;

    /*
    @@ -496,6 +511,8 @@ void __init setup_bootmem_allocator(void)
    }
    #endif
    reserve_crashkernel();
    +
    + reserve_ibft_region();
    }

    /*
    diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
    index 30d94d1..307fb99 100644
    --- a/arch/x86/kernel/setup_64.c
    +++ b/arch/x86/kernel/setup_64.c
    @@ -33,6 +33,7 @@
    #include
    #include
    #include
    +#include
    #include
    #include
    #include
    @@ -198,6 +199,20 @@ static inline void copy_edd(void)
    }
    #endif

    +#if defined(CONFIG_ISCSI_IBFT_FIND)
    +static void __init reserve_ibft_region(void)
    +{
    + unsigned int ibft_len;
    +
    + ibft_len = find_ibft();
    + if (ibft_len)
    + reserve_bootmem_generic((unsigned long)ibft_phys,
    + PAGE_ALIGN(ibft_len));
    +}
    +#else
    +static void __init reserve_ibft_region(void) { }
    +#endif
    +
    #ifdef CONFIG_KEXEC
    static void __init reserve_crashkernel(void)
    {
    @@ -403,6 +418,9 @@ void __init setup_arch(char **cmdline_p)
    }
    #endif
    reserve_crashkernel();
    +
    + reserve_ibft_region();
    +
    paging_init();

    #ifdef CONFIG_PCI
    diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
    index 05f02a3..e2f7208 100644
    --- a/drivers/firmware/Kconfig
    +++ b/drivers/firmware/Kconfig
    @@ -93,4 +93,23 @@ config DMIID
    information from userspace through /sys/class/dmi/id/ or if you want
    DMI-based module auto-loading.

    +config ISCSI_IBFT_FIND
    + bool "iSCSI Boot Firmware Table Attributes"
    + default n
    + help
    + This option enables the kernel to find the region of memory
    + in which the ISCSI Boot Firmware Table (iBFT) resides. This
    + is necessary for iSCSI Boot Firmware Table Attributes module to work
    + properly.
    +
    +config ISCSI_IBFT
    + tristate "iSCSI Boot Firmware Table Attributes module"
    + depends on ISCSI_IBFT_FIND
    + default n
    + help
    + This option enables support for detection and exposing of iSCSI
    + Boot Firmware Table (iBFT) via sysfs to userspace. If you wish to
    + detect iSCSI boot parameters dynamically during system boot, say Y.
    + Otherwise, say N.
    +
    endmenu
    diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
    index 8d4ebc8..4c91471 100644
    --- a/drivers/firmware/Makefile
    +++ b/drivers/firmware/Makefile
    @@ -8,3 +8,5 @@ obj-$(CONFIG_EFI_PCDP) += pcdp.o
    obj-$(CONFIG_DELL_RBU) += dell_rbu.o
    obj-$(CONFIG_DCDBAS) += dcdbas.o
    obj-$(CONFIG_DMIID) += dmi-id.o
    +obj-$(CONFIG_ISCSI_IBFT_FIND) += iscsi_ibft_find.o
    +obj-$(CONFIG_ISCSI_IBFT) += iscsi_ibft.o
    diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
    new file mode 100644
    index 0000000..00de4ea
    --- /dev/null
    +++ b/drivers/firmware/iscsi_ibft.c
    @@ -0,0 +1,972 @@
    +/*
    + * Copyright 2007 Red Hat, Inc.
    + * by Peter Jones
    + * Copyright 2008 IBM, Inc.
    + * by Konrad Rzeszutek
    + *
    + * This code exposes the iSCSI Boot Format Table to userland via sysfs.
    + *
    + * This program is free software; you can redistribute it and/or modify
    + * it under the terms of the GNU General Public License v2.0 as published by
    + * the Free Software Foundation
    + *
    + * This program is distributed in the hope that it will be useful,
    + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    + * GNU General Public License for more details.
    + *
    + * Changelog:
    + *
    + * 25 Jan 2008 - Konrad Rzeszutek
    + * Added logic to handle badly not-to-spec iBFTs. (v0.4.5)
    + *
    + * 4 Jan 2008 - Konrad Rzeszutek
    + * Added __init to function declarations. (v0.4.4)
    + *
    + * 21 Dec 2007 - Konrad Rzeszutek
    + * Updated kobject registration, combined unregister functions in one
    + * and code and style cleanup. (v0.4.3)
    + *
    + * 5 Dec 2007 - Konrad Rzeszutek
    + * Added end-markers to enums and re-organized kobject registration. (v0.4.2)
    + *
    + * 4 Dec 2007 - Konrad Rzeszutek
    + * Created 'device' sysfs link to the NIC and style cleanup. (v0.4.1)
    + *
    + * 28 Nov 2007 - Konrad Rzeszutek
    + * Added sysfs-ibft documentation, moved 'find_ibft' function to
    + * in its own file and added text attributes for every struct field. (v0.4)
    + *
    + * 21 Nov 2007 - Konrad Rzeszutek
    + * Added text attributes emulating OpenFirmware /proc/device-tree naming.
    + * Removed binary /sysfs interface (v0.3)
    + *
    + * 29 Aug 2007 - Konrad Rzeszutek
    + * Added functionality in setup.c to reserve iBFT region. (v0.2)
    + *
    + * 27 Aug 2007 - Konrad Rzeszutek
    + * First version exposing iBFT data via a binary /sysfs. (v0.1)
    + *
    + */
    +
    +
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +
    +#define IBFT_ISCSI_VERSION "0.4.5"
    +#define IBFT_ISCSI_DATE "2008-Jan-25"
    +
    +MODULE_AUTHOR("Peter Jones and \
    +Konrad Rzeszutek ");
    +MODULE_DESCRIPTION("sysfs interface to BIOS iBFT information");
    +MODULE_LICENSE("GPL");
    +MODULE_VERSION(IBFT_ISCSI_VERSION);
    +
    +
    +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__));
    +
    +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;
    + 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__));
    +
    +/*
    + * The kobject different types and its names.
    + *
    +*/
    +enum ibft_id {
    + id_reserved = 0, /* We don't support. */
    + id_control = 1, /* Should show up only once and is not exported. */
    + id_initiator = 2,
    + id_nic = 3,
    + id_target = 4,
    + id_extensions = 5, /* We don't support. */
    + id_end_marker,
    +};
    +
    +/*
    + * We do not support the other types, hence the NULL.
    + */
    +static const char *ibft_id_names[] =
    + {NULL, NULL, "initiator", "ethernet%d", "target%d", NULL, NULL};
    +
    +/*
    + * The text attributes names for each of the kobjects.
    +*/
    +enum ibft_eth_properties_enum {
    + ibft_eth_index,
    + ibft_eth_flags,
    + ibft_eth_ip_addr,
    + ibft_eth_subnet_mask,
    + ibft_eth_origin,
    + ibft_eth_gateway,
    + ibft_eth_primary_dns,
    + ibft_eth_secondary_dns,
    + ibft_eth_dhcp,
    + ibft_eth_vlan,
    + ibft_eth_mac,
    + /* ibft_eth_pci_bdf - this is replaced by link to the device itself. */
    + ibft_eth_hostname,
    + ibft_eth_end_marker,
    +};
    +
    +static const char *ibft_eth_properties[] =
    + {"index", "flags", "ip-addr", "subnet-mask", "origin", "gateway",
    + "primary-dns", "secondary-dns", "dhcp", "vlan", "mac", "hostname",
    + NULL};
    +
    +enum ibft_tgt_properties_enum {
    + ibft_tgt_index,
    + ibft_tgt_flags,
    + ibft_tgt_ip_addr,
    + ibft_tgt_port,
    + ibft_tgt_lun,
    + ibft_tgt_chap_type,
    + ibft_tgt_nic_assoc,
    + ibft_tgt_name,
    + ibft_tgt_chap_name,
    + ibft_tgt_chap_secret,
    + ibft_tgt_rev_chap_name,
    + ibft_tgt_rev_chap_secret,
    + ibft_tgt_end_marker,
    +};
    +
    +static const char *ibft_tgt_properties[] =
    + {"index", "flags", "ip-addr", "port", "lun", "chap-type", "nic-assoc",
    + "target-name", "chap-name", "chap-secret", "rev-chap-name",
    + "rev-chap-name-secret", NULL};
    +
    +enum ibft_initiator_properties_enum {
    + ibft_init_index,
    + ibft_init_flags,
    + ibft_init_isns_server,
    + ibft_init_slp_server,
    + ibft_init_pri_radius_server,
    + ibft_init_sec_radius_server,
    + ibft_init_initiator_name,
    + ibft_init_end_marker,
    +};
    +
    +static const char *ibft_initiator_properties[] =
    + {"index", "flags", "isns-server", "slp-server", "pri-radius-server",
    + "sec-radius-server", "initiator-name", NULL};
    +
    +/*
    + * The kobject and attribute structures.
    + */
    +
    +struct ibft_kobject {
    + struct ibft_table_header *header;
    + union {
    + struct ibft_initiator *initiator;
    + struct ibft_nic *nic;
    + struct ibft_tgt *tgt;
    + struct ibft_hdr *hdr;
    + };
    + struct kobject kobj;
    + struct list_head node;
    +};
    +
    +struct ibft_attribute {
    + struct attribute attr;
    + ssize_t (*show) (struct ibft_kobject *entry,
    + struct ibft_attribute *attr, char *buf);
    + union {
    + struct ibft_initiator *initiator;
    + struct ibft_nic *nic;
    + struct ibft_tgt *tgt;
    + struct ibft_hdr *hdr;
    + };
    + struct kobject *kobj;
    + int type; /* The enum of the type. This can be any value off:
    + ibft_eth_properties_enum, ibft_tgt_properties_enum,
    + or ibft_initiator_properties_enum. */
    + struct list_head node;
    +};
    +
    +static LIST_HEAD(ibft_attr_list);
    +static LIST_HEAD(ibft_kobject_list);
    +
    +static const char nulls[16];
    +static struct ibft_table_header *ibft_device;
    +
    +/*
    + * Helper functions to parse data properly.
    + */
    +static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
    +{
    + if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0 &&
    + ip[4] == 0 && ip[5] == 0 && ip[6] == 0 && ip[7] == 0 &&
    + ip[8] == 0 && ip[9] == 0 && ip[10] == 0xff && ip[11] == 0xff) {
    + /*
    + * IPV4
    + */
    + return sprintf(buf, "%d.%d.%d.%d\n", ip[12],
    + ip[13], ip[14], ip[15]);
    + } else
    + return 0;
    +}
    +
    +static ssize_t sprintf_string(char *str, int len, char *buf)
    +{
    + return sprintf(str, "%.*s\n", len, buf);
    +}
    +
    +/*
    + * Helper function to verify the IBFT header.
    + */
    +static int ibft_verify_hdr(char *t, struct ibft_hdr *hdr, int id, int length)
    +{
    +#define IBFT_VERIFY_HDR_FIELD(val, name) \
    + if (hdr->val != val) { \
    + printk(KERN_ERR \
    + "iBFT error: In structure %s field %s expected %d but" \
    + " found %d!\n", \
    + t, name, val, hdr->val); \
    + return -ENODEV; \
    + }
    + IBFT_VERIFY_HDR_FIELD(id, "id");
    + IBFT_VERIFY_HDR_FIELD(length, "length");
    + 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 = entry->initiator;
    + void *ibft_loc = entry->header;
    + char *str = buf;
    +
    + if (!initiator)
    + return 0;
    +
    + switch (attr->type) {
    + case ibft_init_index:
    + str += sprintf(str, "%d\n", initiator->hdr.index);
    + break;
    + case ibft_init_flags:
    + str += sprintf(str, "%d\n", initiator->hdr.flags);
    + break;
    + case ibft_init_isns_server:
    + str += sprintf_ipaddr(str, initiator->isns_server);
    + break;
    + case ibft_init_slp_server:
    + str += sprintf_ipaddr(str, initiator->slp_server);
    + break;
    + case ibft_init_pri_radius_server:
    + str += sprintf_ipaddr(str, initiator->pri_radius_server);
    + break;
    + case ibft_init_sec_radius_server:
    + str += sprintf_ipaddr(str, initiator->sec_radius_server);
    + break;
    + case ibft_init_initiator_name:
    + str += sprintf_string(str, initiator->initiator_name_len,
    + (char *)ibft_loc +
    + initiator->initiator_name_off);
    + break;
    + default:
    + break;
    + }
    +
    + return str - buf;
    +}
    +
    +ssize_t ibft_attr_show_nic(struct ibft_kobject *entry,
    + struct ibft_attribute *attr,
    + char *buf)
    +{
    + struct ibft_nic *nic = entry->nic;
    + void *ibft_loc = entry->header;
    + char *str = buf;
    + char *mac;
    + int val;
    +
    + if (!nic)
    + return 0;
    +
    + switch (attr->type) {
    + case ibft_eth_index:
    + str += sprintf(str, "%d\n", nic->hdr.index);
    + break;
    + case ibft_eth_flags:
    + str += sprintf(str, "%d\n", nic->hdr.flags);
    + break;
    + case ibft_eth_ip_addr:
    + str += sprintf_ipaddr(str, nic->ip_addr);
    + break;
    + case ibft_eth_subnet_mask:
    + val = ~((1 << (32-nic->subnet_mask_prefix))-1);
    + str += sprintf(str, "%d.%d.%d.%d\n",
    + (u8)(val >> 24), (u8)(val >> 16),
    + (u8)(val >> 8), (u8)(val));
    + break;
    + case ibft_eth_origin:
    + str += sprintf(str, "%d\n", nic->origin);
    + break;
    + case ibft_eth_gateway:
    + str += sprintf_ipaddr(str, nic->gateway);
    + break;
    + case ibft_eth_primary_dns:
    + str += sprintf_ipaddr(str, nic->primary_dns);
    + break;
    + case ibft_eth_secondary_dns:
    + str += sprintf_ipaddr(str, nic->secondary_dns);
    + break;
    + case ibft_eth_dhcp:
    + str += sprintf_ipaddr(str, nic->dhcp);
    + break;
    + case ibft_eth_vlan:
    + str += sprintf(str, "%d\n", nic->vlan);
    + break;
    + case ibft_eth_mac:
    + mac = nic->mac;
    + str += sprintf(str, "%02x:%02x:%02x:%02x:%02x:%02x\n",
    + (u8)mac[0], (u8)mac[1], (u8)mac[2],
    + (u8)mac[3], (u8)mac[4], (u8)mac[5]);
    + break;
    + case ibft_eth_hostname:
    + str += sprintf_string(str, nic->hostname_len,
    + (char *)ibft_loc + nic->hostname_off);
    + break;
    + default:
    + break;
    + }
    +
    + return str - buf;
    +};
    +
    +ssize_t ibft_attr_show_target(struct ibft_kobject *entry,
    + struct ibft_attribute *attr,
    + char *buf)
    +{
    + struct ibft_tgt *tgt = entry->tgt;
    + void *ibft_loc = entry->header;
    + char *str = buf;
    + int i;
    +
    + if (!tgt)
    + return 0;
    +
    + switch (attr->type) {
    + case ibft_tgt_index:
    + str += sprintf(str, "%d\n", tgt->hdr.index);
    + break;
    + case ibft_tgt_flags:
    + str += sprintf(str, "%d\n", tgt->hdr.flags);
    + break;
    + case ibft_tgt_ip_addr:
    + str += sprintf_ipaddr(str, tgt->ip_addr);
    + break;
    + case ibft_tgt_port:
    + str += sprintf(str, "%d\n", tgt->port);
    + break;
    + case ibft_tgt_lun:
    + for (i = 0; i < 8; i++)
    + str += sprintf(str, "%x", (u8)tgt->lun[i]);
    + str += sprintf(str, "\n");
    + break;
    + case ibft_tgt_nic_assoc:
    + str += sprintf(str, "%d\n", tgt->nic_assoc);
    + break;
    + case ibft_tgt_chap_type:
    + str += sprintf(str, "%d\n", tgt->chap_type);
    + break;
    + case ibft_tgt_name:
    + str += sprintf_string(str, tgt->tgt_name_len,
    + (char *)ibft_loc + tgt->tgt_name_off);
    + break;
    + case ibft_tgt_chap_name:
    + str += sprintf_string(str, tgt->chap_name_len,
    + (char *)ibft_loc + tgt->chap_name_off);
    + break;
    + case ibft_tgt_chap_secret:
    + str += sprintf_string(str, tgt->chap_secret_len,
    + (char *)ibft_loc + tgt->chap_secret_off);
    + break;
    + case ibft_tgt_rev_chap_name:
    + str += sprintf_string(str, tgt->rev_chap_name_len,
    + (char *)ibft_loc +
    + tgt->rev_chap_name_off);
    + break;
    + case ibft_tgt_rev_chap_secret:
    + str += sprintf_string(str, tgt->rev_chap_secret_len,
    + (char *)ibft_loc +
    + tgt->rev_chap_secret_off);
    + break;
    + default:
    + break;
    + }
    +
    + return str - buf;
    +}
    +
    +/*
    + * 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 ibft_ktype = {
    + .release = ibft_release,
    + .sysfs_ops = &ibft_attr_ops,
    +};
    +
    +static struct kset *ibft_kset;
    +
    +static int __init ibft_check_and_alloc_device(void *idev)
    +{
    + int len;
    + u8 *pos;
    + u8 csum = 0;
    + struct ibft_table_header *hdr;
    +
    + if (!idev)
    + return -ENOENT;
    +
    + hdr = (struct ibft_table_header *)phys_to_virt((unsigned long)
    + ibft_phys);
    + len = hdr->length;
    +
    + /* Sanity checking of iBFT. */
    + if (hdr->revision != 1) {
    + printk(KERN_ERR "iBFT module supports only revision 1, " \
    + "while this is %d.\n", hdr->revision);
    + return 1;
    + }
    + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++)
    + csum += *pos;
    +
    + if (csum) {
    + printk(KERN_ERR "iBFT has incorrect checksum (0x%x)!\n", csum);
    + return 1;
    + }
    +
    + ibft_device = kmalloc(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_register_kobjects.
    + */
    +static int __init ibft_create_kobject(struct ibft_table_header *header,
    + struct ibft_hdr *hdr,
    + struct list_head *list)
    +{
    + struct ibft_kobject *ibft_kobj = NULL;
    + struct ibft_nic *nic = (struct ibft_nic *)hdr;
    + struct pci_dev *pci_dev;
    + int rc = 0;
    +
    + ibft_kobj = kzalloc(sizeof(*ibft_kobj), GFP_KERNEL);
    + if (!ibft_kobj)
    + return -ENOMEM;
    +
    + ibft_kobj->header = header;
    + ibft_kobj->hdr = hdr;
    +
    + switch (hdr->id) {
    + case id_initiator:
    + rc = ibft_verify_hdr("initiator", hdr, id_initiator,
    + sizeof(*ibft_kobj->initiator));
    + break;
    + case id_nic:
    + rc = ibft_verify_hdr("ethernet", hdr, id_nic,
    + sizeof(*ibft_kobj->nic));
    + break;
    + case id_target:
    + rc = ibft_verify_hdr("target", hdr, id_target,
    + sizeof(*ibft_kobj->tgt));
    + break;
    + case id_reserved:
    + case id_control:
    + case id_extensions:
    + /* Fields which we don't support. Ignore them */
    + rc = 1;
    + break;
    + default:
    + printk(KERN_ERR "iBFT has unknown structure type (%d). " \
    + "Report this bug to your vendor!\n", hdr->id);
    + rc = 1;
    + break;
    + }
    +
    + if (rc) {
    + kfree(ibft_kobj);
    + goto out_invalid_struct;
    + }
    +
    + /* kobject fief. We will let the reference counter do the job
    + of deleting its name if we fail here. */
    + ibft_kobj->kobj.kset = ibft_kset;
    +
    + rc = kobject_init_and_add(&ibft_kobj->kobj, &ibft_ktype,
    + NULL, ibft_id_names[hdr->id], hdr->index);
    +
    + if (rc) {
    + kfree(ibft_kobj);
    + goto out;
    + }
    +
    + kobject_uevent(&ibft_kobj->kobj, KOBJ_ADD);
    +
    + if (hdr->id == id_nic) {
    + pci_dev = pci_get_bus_and_slot((nic->pci_bdf & 0xff00) >> 8,
    + (nic->pci_bdf & 0xff));
    + if (pci_dev) {
    + rc = sysfs_create_link(&ibft_kobj->kobj,
    + &pci_dev->dev.kobj, "device");
    + pci_dev_put(pci_dev);
    + }
    + }
    +
    + /* Nothing broke so lets add it to the list. */
    + list_add_tail(&ibft_kobj->node, list);
    +out:
    + return rc;
    +out_invalid_struct:
    + /* Unsupported structs are skipped. */
    + return 0;
    +}
    +
    +/*
    + * Scan the IBFT table structure for the NIC and Target fields. When
    + * found add them on the passed-in list.
    + */
    +static int __init ibft_register_kobjects(struct ibft_table_header *header,
    + struct list_head *list)
    +{
    + struct ibft_control *control = NULL;
    + void *ptr, *end;
    + int rc = 0;
    + u16 offset;
    + u16 eot_offset;
    +
    + control = (void *)header + sizeof(*header);
    + end = (void *)control + control->hdr.length;
    + eot_offset = (void *)header + header->length -
    + (void *)control - sizeof(*header);
    + rc = ibft_verify_hdr("control", (struct ibft_hdr *)control, id_control,
    + sizeof(*control));
    + rc |= control->hdr.index != 0;
    + if (rc)
    + return rc;
    +
    + for (ptr = &control->initiator_off; ptr < end; ptr += sizeof(u16)) {
    + offset = *(u16 *)ptr;
    + if (offset && offset < header->length && offset < eot_offset) {
    + rc = ibft_create_kobject(header,
    + (void *)header + offset,
    + list);
    + if (rc)
    + break;
    + }
    + }
    +
    + return rc;
    +}
    +
    +static void ibft_unregister(struct list_head *attr_list,
    + struct list_head *kobj_list)
    +{
    + struct ibft_kobject *data = NULL, *n;
    + struct ibft_attribute *attr = NULL, *m;
    +
    + list_for_each_entry_safe(attr, m, attr_list, node) {
    + sysfs_remove_file(attr->kobj, &attr->attr);
    + list_del(&attr->node);
    + kfree(attr);
    + };
    + list_del_init(attr_list);
    +
    + list_for_each_entry_safe(data, n, kobj_list, node) {
    + list_del(&data->node);
    + if (data->hdr->id == id_nic)
    + sysfs_remove_link(&data->kobj, "device");
    + kobject_put(&data->kobj);
    + };
    + list_del_init(kobj_list);
    +}
    +
    +static int __init ibft_create_attribute(struct ibft_kobject *kobj_data,
    + int type,
    + const char *name,
    + ssize_t (*show)(struct ibft_kobject *,
    + struct ibft_attribute*,
    + char *buf),
    + struct list_head *list)
    +{
    + struct ibft_attribute *attr = NULL;
    + struct ibft_hdr *hdr = kobj_data->hdr;
    +
    + attr = kmalloc(sizeof(*attr), GFP_KERNEL);
    + if (!attr)
    + return -ENOMEM;
    +
    + attr->attr.name = name;
    + attr->attr.mode = S_IRUSR;
    + attr->attr.owner = THIS_MODULE;
    +
    + attr->hdr = hdr;
    + attr->show = show;
    + attr->kobj = &kobj_data->kobj;
    + attr->type = type;
    +
    + list_add_tail(&attr->node, list);
    +
    + return 0;
    +}
    +
    +static int __init ibft_check_nic_for(struct ibft_nic *nic, int entry)
    +{
    + int rc = 0;
    +
    + switch (entry) {
    + case ibft_eth_index:
    + case ibft_eth_flags:
    + rc = 1;
    + break;
    + case ibft_eth_ip_addr:
    + if (!memcmp(nic->dhcp, nulls, sizeof(nic->dhcp)))
    + rc = 1;
    + break;
    + case ibft_eth_subnet_mask:
    + if (!memcmp(nic->dhcp, nulls, sizeof(nic->dhcp)))
    + rc = 1;
    + break;
    + case ibft_eth_origin:
    + rc = 1;
    + break;
    + case ibft_eth_gateway:
    + if (memcmp(nic->gateway, nulls, sizeof(nic->gateway)))
    + rc = 1;
    + break;
    + case ibft_eth_primary_dns:
    + if (memcmp(nic->primary_dns, nulls,
    + sizeof(nic->primary_dns)))
    + rc = 1;
    + break;
    + case ibft_eth_secondary_dns:
    + if (memcmp(nic->secondary_dns, nulls,
    + sizeof(nic->secondary_dns)))
    + rc = 1;
    + break;
    + case ibft_eth_dhcp:
    + if (memcmp(nic->dhcp, nulls, sizeof(nic->dhcp)))
    + rc = 1;
    + break;
    + case ibft_eth_vlan:
    + case ibft_eth_mac:
    + rc = 1;
    + break;
    + case ibft_eth_hostname:
    + if (nic->hostname_off)
    + rc = 1;
    + break;
    + default:
    + break;
    + }
    +
    + return rc;
    +}
    +
    +static int __init ibft_check_tgt_for(struct ibft_tgt *tgt, int entry)
    +{
    + int rc = 0;
    +
    + switch (entry) {
    + case ibft_tgt_index:
    + case ibft_tgt_flags:
    + case ibft_tgt_ip_addr:
    + case ibft_tgt_port:
    + case ibft_tgt_lun:
    + case ibft_tgt_nic_assoc:
    + case ibft_tgt_chap_type:
    + rc = 1;
    + case ibft_tgt_name:
    + if (tgt->tgt_name_len)
    + rc = 1;
    + break;
    + case ibft_tgt_chap_name:
    + case ibft_tgt_chap_secret:
    + if (tgt->chap_name_len)
    + rc = 1;
    + break;
    + case ibft_tgt_rev_chap_name:
    + case ibft_tgt_rev_chap_secret:
    + if (tgt->rev_chap_name_len)
    + rc = 1;
    + break;
    + default:
    + break;
    + }
    +
    + return rc;
    +}
    +
    +static int __init ibft_check_initiator_for(struct ibft_initiator *init,
    + int entry)
    +{
    + int rc = 0;
    +
    + switch (entry) {
    + case ibft_init_index:
    + case ibft_init_flags:
    + rc = 1;
    + break;
    + case ibft_init_isns_server:
    + if (memcmp(init->isns_server, nulls,
    + sizeof(init->isns_server)))
    + rc = 1;
    + break;
    + case ibft_init_slp_server:
    + if (memcmp(init->slp_server, nulls,
    + sizeof(init->slp_server)))
    + rc = 1;
    + break;
    + case ibft_init_pri_radius_server:
    + if (memcmp(init->pri_radius_server, nulls,
    + sizeof(init->pri_radius_server)))
    + rc = 1;
    + break;
    + case ibft_init_sec_radius_server:
    + if (memcmp(init->sec_radius_server, nulls,
    + sizeof(init->sec_radius_server)))
    + rc = 1;
    + break;
    + case ibft_init_initiator_name:
    + if (init->initiator_name_len)
    + rc = 1;
    + break;
    + default:
    + break;
    + }
    +
    + return rc;
    +}
    +
    +/*
    + * Register the attributes for all of the kobjects.
    + */
    +static int __init ibft_register_attributes(struct list_head *kobject_list,
    + struct list_head *attr_list)
    +{
    + int rc = 0, i = 0;
    + struct ibft_kobject *data = NULL;
    + struct ibft_attribute *attr = NULL, *m;
    +
    + list_for_each_entry(data, kobject_list, node) {
    + switch (data->hdr->id) {
    + case id_nic:
    + for (i = 0; i < ibft_eth_end_marker && !rc; i++)
    + if (ibft_check_nic_for(data->nic, i))
    + rc = ibft_create_attribute(data, i,
    + ibft_eth_properties[i],
    + ibft_attr_show_nic, attr_list);
    + break;
    + case id_target:
    + for (i = 0; i < ibft_tgt_end_marker && !rc; i++)
    + if (ibft_check_tgt_for(data->tgt, i))
    + rc = ibft_create_attribute(data, i,
    + ibft_tgt_properties[i],
    + ibft_attr_show_target,
    + attr_list);
    + break;
    + case id_initiator:
    + for (i = 0; i < ibft_init_end_marker && !rc; i++)
    + if (ibft_check_initiator_for(
    + data->initiator, i))
    + rc = ibft_create_attribute(data, i,
    + ibft_initiator_properties[i],
    + ibft_attr_show_initiator,
    + 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;
    +}
    +
    +/*
    + * ibft_init() - creates sysfs tree entries for iBFT data.
    + */
    +static int __init ibft_init(void)
    +{
    + int rc = 0;
    +
    + if (!ibft_phys)
    + find_ibft();
    +
    + ibft_kset = kset_create_and_add("ibft", NULL, firmware_kobj);
    + if (!ibft_kset)
    + return -ENOMEM;
    +
    + if (ibft_phys) {
    + printk(KERN_INFO "iBFT detected at 0x%p.\n", ibft_phys);
    +
    + rc = ibft_check_and_alloc_device(ibft_phys);
    + if (rc)
    + goto out_firmware_unregister;
    +
    + /* Scan the IBFT for data and register kobjects. */
    + rc = ibft_register_kobjects(ibft_device, &ibft_kobject_list);
    + if (rc)
    + goto out_free;
    +
    + /* Register the attributes */
    + rc = ibft_register_attributes(&ibft_kobject_list,
    + &ibft_attr_list);
    + if (rc)
    + goto out_free;
    + } else
    + printk(KERN_INFO "No iBFT detected.\n");
    +
    + return 0;
    +
    +out_free:
    + ibft_unregister(&ibft_attr_list, &ibft_kobject_list);
    + ibft_free_device(ibft_device);
    +out_firmware_unregister:
    + kset_unregister(ibft_kset);
    + return rc;
    +}
    +
    +static void __exit ibft_exit(void)
    +{
    + ibft_unregister(&ibft_attr_list, &ibft_kobject_list);
    + ibft_free_device(ibft_device);
    + kset_unregister(ibft_kset);
    +}
    +
    +module_init(ibft_init);
    +module_exit(ibft_exit);
    diff --git a/drivers/firmware/iscsi_ibft_find.c b/drivers/firmware/iscsi_ibft_find.c
    new file mode 100644
    index 0000000..d40cc25
    --- /dev/null
    +++ b/drivers/firmware/iscsi_ibft_find.c
    @@ -0,0 +1,77 @@
    +/*
    + * Copyright 2007 Red Hat, Inc.
    + * by Peter Jones
    + * Copyright 2007 IBM, Inc.
    + * by Konrad Rzeszutek
    + *
    + * This code finds the iSCSI Boot Format Table.
    + *
    + * This program is free software; you can redistribute it and/or modify
    + * it under the terms of the GNU General Public License v2.0 as published by
    + * the Free Software Foundation
    + *
    + * This program is distributed in the hope that it will be useful,
    + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    + * GNU General Public License for more details.
    + */
    +
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +#include
    +
    +
    +/*
    + * Physical location of iSCSI Boot Format Table.
    + */
    +void *ibft_phys;
    +EXPORT_SYMBOL(ibft_phys);
    +
    +#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 /* 128kB */
    +
    +
    +/*
    + * Routine used to find the iSCSI Boot Format Table. the physical
    + * location is set in the ibft_phys variable. The return value is
    + * the size of IBFT.
    + */
    +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;
    +}
    +EXPORT_SYMBOL(find_ibft);
    diff --git a/include/linux/iscsi_ibft.h b/include/linux/iscsi_ibft.h
    new file mode 100644
    index 0000000..d36ca16
    --- /dev/null
    +++ b/include/linux/iscsi_ibft.h
    @@ -0,0 +1,38 @@
    +/*
    + * Copyright 2007 Red Hat, Inc.
    + * by Peter Jones
    + * Copyright 2007 IBM, Inc.
    + * by Konrad Rzeszutek
    + *
    + * This code exposes the iSCSI Boot Format Table to userland via sysfs.
    + *
    + * This program is free software; you can redistribute it and/or modify
    + * it under the terms of the GNU General Public License v2.0 as published by
    + * the Free Software Foundation
    + *
    + * This program is distributed in the hope that it will be useful,
    + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    + * GNU General Public License for more details.
    + */
    +
    +#ifndef ISCSI_IBFT_H
    +#define ISCSI_IBFT_H
    +
    +#if defined(CONFIG_ISCSI_IBFT_FIND)
    +
    +/*
    + * Physical location of iSCSI Boot Format Table.
    + */
    +extern void *ibft_phys;
    +
    +/*
    + * Routine used to find the iSCSI Boot Format Table. The physical
    + * location is set in the ibft_phys variable. The return value is the
    + * size of IBFT.
    + */
    +extern ssize_t find_ibft(void);
    +
    +#endif
    +
    +#endif /* ISCSI_IBFT_H */
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    > On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek wrote:
    > Hey Andrew,
    >
    > Please add this patch along with Greg KH's kobject fixes.


    erm, OK. But I don't think I'm the appropriate conduit for iscsi paches.

    By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as
    maintainer...

    Oh well, at least I get to read some code.

    > This module
    > is dependent on the fixes that Greg KH has in his patches git tree.
    >
    > This patch (v0.4.5) adds /sysfs/firmware/ibft/[initiator|targetX|ethernetX]
    > directories along with text properties which export the the iSCSI
    > Boot Firmware Table (iBFT) structure.
    >
    > 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 the initrd.
    >
    > For full details of the IBFT structure please take a look at:
    > ftp://ftp.software.ibm.com/systems/s...able_v1.02.pdf


    When adding new sysfs things (especially things as complex as this) please
    fully describe the user-visible interface in the changelog so that we can
    review your interface design.

    Does this code follow the one-value-per-sysfs-file convention?

    > +#if defined(CONFIG_ISCSI_IBFT_FIND)
    > +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) { }
    > +#endif


    Usually we'd make the CONFIG_ISCSI_IBFT_FIND=n version of this function
    static inline in a header. As it stands this code will add a pointless
    empty function to the vmlinux image.

    > int __initdata user_defined_memmap = 0;


    checkpatch should have told you that this "= 0" shouldn't be there. But it
    doesn't.

    > + struct kobject *kobj;
    > + int type; /* The enum of the type. This can be any value off:
    > + ibft_eth_properties_enum, ibft_tgt_properties_enum,
    > + or ibft_initiator_properties_enum. */
    > + struct list_head node;
    > +};
    > +
    > +static LIST_HEAD(ibft_attr_list);
    > +static LIST_HEAD(ibft_kobject_list);


    A brief search for the locking which protects these lists was unsuccessful.
    What's up?

    > +/*
    > + * Helper functions to parse data properly.
    > + */
    > +static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
    > +{
    > + if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0 &&
    > + ip[4] == 0 && ip[5] == 0 && ip[6] == 0 && ip[7] == 0 &&
    > + ip[8] == 0 && ip[9] == 0 && ip[10] == 0xff && ip[11] == 0xff) {
    > + /*
    > + * IPV4
    > + */
    > + return sprintf(buf, "%d.%d.%d.%d\n", ip[12],
    > + ip[13], ip[14], ip[15]);
    > + } else
    > + return 0;
    > +}


    I'm seeing an awful lot of sprintf()s in here which look like they should
    be snprintf(). By what means is this code bulletproof against overflows?

    > +static ssize_t sprintf_string(char *str, int len, char *buf)
    > +{
    > + return sprintf(str, "%.*s\n", len, buf);
    > +}
    > +
    > +/*
    > + * Helper function to verify the IBFT header.
    > + */
    > +static int ibft_verify_hdr(char *t, struct ibft_hdr *hdr, int id, int length)
    > +{
    > +#define IBFT_VERIFY_HDR_FIELD(val, name) \
    > + if (hdr->val != val) { \
    > + printk(KERN_ERR \
    > + "iBFT error: In structure %s field %s expected %d but" \
    > + " found %d!\n", \
    > + t, name, val, hdr->val); \
    > + return -ENODEV; \
    > + }
    > + IBFT_VERIFY_HDR_FIELD(id, "id");
    > + IBFT_VERIFY_HDR_FIELD(length, "length");
    > + return 0;
    > +}


    I'm not sure that two usages really justifies IBFT_VERIFY_HDR_FIELD's
    existence. If you're really attched to it then I'd suggest that it be
    undefed immediately after having been read, for readability reasons.

    > +static void ibft_release(struct kobject *kobj)
    > +{
    > + struct ibft_kobject *ibft =
    > + container_of(kobj, struct ibft_kobject, kobj);
    > + kfree(ibft);
    > +}
    >
    > ...
    >
    > + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++)
    >


    checkpatch should have caught the " ++" but didn't. I think it used to.
    It seems to be going backwards?

    > + csum += *pos;
    > +
    > + if (csum) {
    > + printk(KERN_ERR "iBFT has incorrect checksum (0x%x)!\n", csum);
    > + return 1;
    > + }
    > +
    > + ibft_device = kmalloc(len, GFP_KERNEL);
    > + if (!ibft_device)
    > + return -ENOMEM;
    > +
    > + memcpy(ibft_device, hdr, len);
    > +
    > + return 0;
    > +}
    > +
    >
    > ...
    >
    > +
    > + /* kobject fief. We will let the reference counter do the job
    > + of deleting its name if we fail here. */


    what's a fief?

    > +/*
    > + * Physical location of iSCSI Boot Format Table.
    > + */
    > +void *ibft_phys;
    > +EXPORT_SYMBOL(ibft_phys);
    > +
    > +#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 /* 128kB */
    > +
    > +
    > +/*
    > + * Routine used to find the iSCSI Boot Format Table. the physical
    > + * location is set in the ibft_phys variable. The return value is
    > + * the size of IBFT.
    > + */
    > +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;
    > +}
    > +EXPORT_SYMBOL(find_ibft);


    Is this x86-specific? Are suitable Kconfig dependencies in place?

    > + * Copyright 2007 Red Hat, Inc.
    > + * by Peter Jones
    > + * Copyright 2007 IBM, Inc.
    > + * by Konrad Rzeszutek
    > + *
    > + * This code exposes the iSCSI Boot Format Table to userland via sysfs.
    > + *
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License v2.0 as published by
    > + * the Free Software Foundation
    > + *
    > + * This program is distributed in the hope that it will be useful,
    > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    > + * GNU General Public License for more details.
    > + */
    > +
    > +#ifndef ISCSI_IBFT_H
    > +#define ISCSI_IBFT_H
    > +
    > +#if defined(CONFIG_ISCSI_IBFT_FIND)


    We'd more usually use #ifdef here. Whatever.

    > +/*
    > + * Physical location of iSCSI Boot Format Table.
    > + */
    > +extern void *ibft_phys;
    > +
    > +/*
    > + * Routine used to find the iSCSI Boot Format Table. The physical
    > + * location is set in the ibft_phys variable. The return value is the
    > + * size of IBFT.
    > + */
    > +extern ssize_t find_ibft(void);
    > +
    > +#endif


    Often we don't bother putting declarations like this inside the ifdef.
    Upside: cleaner code. Downside: error-detection happens at link-time
    rather tha compile-time.

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

  3. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)


    Please always include a diffstat with non-trivial patches.

    > On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek wrote:
    > --- a/arch/x86/kernel/setup_32.c
    > +++ b/arch/x86/kernel/setup_32.c


    lol. You touched x86 code.


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

  4. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    Andrew Morton wrote:

    >> int __initdata user_defined_memmap = 0;


    > checkpatch should have told you that this "= 0" shouldn't be there. But it
    > doesn't.



    checkpatch checks for static initializers, not non-static ones.
    Should that be changed?



    >> + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++)
    >>

    >
    > checkpatch should have caught the " ++" but didn't. I think it used to.
    > It seems to be going backwards?



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

  5. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    On Sat, Jan 26, 2008 at 10:01:23PM -0800, Andrew Morton wrote:

    > > int __initdata user_defined_memmap = 0;

    >
    > checkpatch should have told you that this "= 0" shouldn't be there. But it
    > doesn't.


    Ok, this line would be correctly picked up if it was being added by this
    author, but this line is in the context only. We do not blame the current
    author for those.

    ERROR: do not initialise externals to 0 or NULL
    #1: FILE: Z57.c:1:
    +int __initdata user_defined_memmap = 0;

    > > + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++)

    >
    > checkpatch should have caught the " ++" but didn't. I think it used to.
    > It seems to be going backwards?


    Somehow this variant was not covered. Added to the tests and to the
    next version:

    ERROR: no space before that '++' (ctx:WxB)
    #3: FILE: Z57.c:3:
    + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++)

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

  6. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    On Sunday 27 January 2008 01:01:23 you wrote:
    > > On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek
    > > wrote: Hey Andrew,
    > >
    > > Please add this patch along with Greg KH's kobject fixes.

    >
    > erm, OK. But I don't think I'm the appropriate conduit for iscsi paches.
    >
    > By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as
    > maintainer...


    This is a bit tricky b/c this goes to the drivers/firmware and also depends on
    the kobject changes in Greg KH tree. But I should have included Mike on the
    CC which I keep on forgetting .

    >
    > When adding new sysfs things (especially things as complex as this) please
    > fully describe the user-visible interface in the changelog so that we can
    > review your interface design.


    Done. This repost:
    http://lkml.org/lkml/2008/1/28/304

    has the details.

    >
    > Does this code follow the one-value-per-sysfs-file convention?


    Yes.
    >
    > > +#if defined(CONFIG_ISCSI_IBFT_FIND)
    > > +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) { }
    > > +#endif

    >
    > Usually we'd make the CONFIG_ISCSI_IBFT_FIND=n version of this function
    > static inline in a header. As it stands this code will add a pointless
    > empty function to the vmlinux image.


    Fixed. The function now is in include/linux/iscsi_ibft.h

    >
    > > int __initdata user_defined_memmap = 0;

    >
    > checkpatch should have told you that this "= 0" shouldn't be there. But it
    > doesn't.


    Uhh, I didn't add that.

    >
    > > + struct kobject *kobj;
    > > + int type; /* The enum of the type. This can be any value off:
    > > + ibft_eth_properties_enum, ibft_tgt_properties_enum,
    > > + or ibft_initiator_properties_enum. */
    > > + struct list_head node;
    > > +};
    > > +
    > > +static LIST_HEAD(ibft_attr_list);
    > > +static LIST_HEAD(ibft_kobject_list);

    >
    > A brief search for the locking which protects these lists was unsuccessful.
    > What's up?


    The data structure does not change when the machine has booted up. The iBFT is
    a read-only structure and there are no known mechanism to write to it via the
    iBFT structure (or even BIOS up-calls). You have to use custom-vendor
    applications to flash the BIOS with the new iBFT structure. Hence no need for
    locking at this stage - when the specs becomes more advance I will add them
    if they are required.

    >
    > > +/*
    > > + * Helper functions to parse data properly.
    > > + */
    > > +static ssize_t sprintf_ipaddr(char *buf, u8 *ip)
    > > +{
    > > + if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0 &&
    > > + ip[4] == 0 && ip[5] == 0 && ip[6] == 0 && ip[7] == 0 &&
    > > + ip[8] == 0 && ip[9] == 0 && ip[10] == 0xff && ip[11] == 0xff) {
    > > + /*
    > > + * IPV4
    > > + */
    > > + return sprintf(buf, "%d.%d.%d.%d\n", ip[12],
    > > + ip[13], ip[14], ip[15]);
    > > + } else
    > > + return 0;
    > > +}

    >
    > I'm seeing an awful lot of sprintf()s in here which look like they should
    > be snprintf(). By what means is this code bulletproof against overflows?


    There is no reading of data from the user-land. All of it is just outputing to
    the /sysfs entries from reading the data in the iBFT structure which the BIOS
    created.
    >
    > > +static ssize_t sprintf_string(char *str, int len, char *buf)
    > > +{
    > > + return sprintf(str, "%.*s\n", len, buf);
    > > +}
    > > +
    > > +/*
    > > + * Helper function to verify the IBFT header.
    > > + */
    > > +static int ibft_verify_hdr(char *t, struct ibft_hdr *hdr, int id, int
    > > length) +{
    > > +#define IBFT_VERIFY_HDR_FIELD(val, name) \
    > > + if (hdr->val != val) { \
    > > + printk(KERN_ERR \
    > > + "iBFT error: In structure %s field %s expected %d but" \
    > > + " found %d!\n", \
    > > + t, name, val, hdr->val); \
    > > + return -ENODEV; \
    > > + }
    > > + IBFT_VERIFY_HDR_FIELD(id, "id");
    > > + IBFT_VERIFY_HDR_FIELD(length, "length");
    > > + return 0;
    > > +}

    >
    > I'm not sure that two usages really justifies IBFT_VERIFY_HDR_FIELD's
    > existence. If you're really attched to it then I'd suggest that it be
    > undefed immediately after having been read, for readability reasons.


    Done. Took out the #define.

    >
    > > +static void ibft_release(struct kobject *kobj)
    > > +{
    > > + struct ibft_kobject *ibft =
    > > + container_of(kobj, struct ibft_kobject, kobj);
    > > + kfree(ibft);
    > > +}
    > >
    > > ...
    > >
    > > + for (pos = (u8 *)hdr; pos < (u8 *)hdr + len; pos ++)

    >
    > checkpatch should have caught the " ++" but didn't. I think it used to.
    > It seems to be going backwards?


    Fixed.
    > > ...
    > >
    > > +
    > > + /* kobject fief. We will let the reference counter do the job
    > > + of deleting its name if we fail here. */

    >
    > what's a fief?


    FIEF, or FEUD. In its origin, a fief was a district of country allotted to
    one of the chiefs who invaded the Roman empire, as a stipend or reward.
    >
    > > +/*
    > > + * Physical location of iSCSI Boot Format Table.
    > > + */
    > > +void *ibft_phys;
    > > +EXPORT_SYMBOL(ibft_phys);
    > > +
    > > +#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 /* 128kB */
    > > +
    > > +
    > > +/*
    > > + * Routine used to find the iSCSI Boot Format Table. the physical
    > > + * location is set in the ibft_phys variable. The return value is
    > > + * the size of IBFT.
    > > + */
    > > +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;
    > > +}
    > > +EXPORT_SYMBOL(find_ibft);

    >
    > Is this x86-specific? Are suitable Kconfig dependencies in place?


    Originally I had it to be x86-specific but was told that I should make it all
    platforms since the IBFT is platform independent. Somebody can very well
    insert a NIC with IBFT on a IA64 machine or PPC.

    > > +#define ISCSI_IBFT_H
    > > +
    > > +#if defined(CONFIG_ISCSI_IBFT_FIND)

    >
    > We'd more usually use #ifdef here. Whatever.


    Fixed.

    >
    > > +/*
    > > + * Physical location of iSCSI Boot Format Table.
    > > + */
    > > +extern void *ibft_phys;
    > > +
    > > +/*
    > > + * Routine used to find the iSCSI Boot Format Table. The physical
    > > + * location is set in the ibft_phys variable. The return value is the
    > > + * size of IBFT.
    > > + */
    > > +extern ssize_t find_ibft(void);
    > > +
    > > +#endif

    >
    > Often we don't bother putting declarations like this inside the ifdef.
    > Upside: cleaner code. Downside: error-detection happens at link-time
    > rather tha compile-time.


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

  7. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)


    On Mon, 28 Jan 2008 14:04:51 EST, Konrad Rzeszutek wrote:
    > > > +EXPORT_SYMBOL(find_ibft);

    > >
    > > Is this x86-specific? Are suitable Kconfig dependencies in place?

    >
    > Originally I had it to be x86-specific but was told that I should make it all
    > platforms since the IBFT is platform independent. Somebody can very well
    > insert a NIC with IBFT on a IA64 machine or PPC.
    >


    I would beg to differ regarding the powerpc. On powerpc, the bios is
    invisible and ignored. We have our own "special" way, via the device-tree
    in procfs.

    ++doug

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

  8. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    Doug Maxey wrote:
    > On Mon, 28 Jan 2008 14:04:51 EST, Konrad Rzeszutek wrote:
    >>>> +EXPORT_SYMBOL(find_ibft);
    >>> Is this x86-specific? Are suitable Kconfig dependencies in place?

    >> Originally I had it to be x86-specific but was told that I should make it all
    >> platforms since the IBFT is platform independent. Somebody can very well
    >> insert a NIC with IBFT on a IA64 machine or PPC.

    >
    > I would beg to differ regarding the powerpc. On powerpc, the bios is
    > invisible and ignored. We have our own "special" way, via the device-tree
    > in procfs.
    >


    iBFT is not platform-independent; it only makes sense on platforms with
    ACPI (and even then, just barely; ACPI is a poor fit for it and it was
    probably "integrated" with ACPI for political reasons.)

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

  9. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    > iBFT is not platform-independent; it only makes sense on platforms with
    > ACPI (and even then, just barely; ACPI is a poor fit for it and it was
    > probably "integrated" with ACPI for political reasons.)


    The spec just mentions that iBFT table has to be "compatible with an ACPI
    table format" and nothing else. In reality I've only tested this on x86_64
    and i386. We can limit it to be X86 || IA64 but I wouldn't make it dependent
    on ACPI - as this data does not necessarily have to be exported via ACPI.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    Konrad Rzeszutek wrote:
    >> iBFT is not platform-independent; it only makes sense on platforms with
    >> ACPI (and even then, just barely; ACPI is a poor fit for it and it was
    >> probably "integrated" with ACPI for political reasons.)

    >
    > The spec just mentions that iBFT table has to be "compatible with an ACPI
    > table format" and nothing else.


    Well, that's not quite accurate. It also mentions that OEM IDs for
    vendors come from the ACPI SIG, and they also reserved the "IBFT"
    signature with the ACPI SIG (it's in ACPI 3.0). It's also pretty clear
    that the "Locating the iBFT" section in the spec used to be a list with
    more than one entry and has been edited down to one.

    That being said, I don't think there's any reason to expect the table to
    show up on anything but i386 and x86_64, and maybe ia64.

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

  11. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    Konrad Rzeszutek wrote:
    > On Sunday 27 January 2008 01:01:23 you wrote:
    >>> On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek
    >>> wrote: Hey Andrew,
    >>>
    >>> Please add this patch along with Greg KH's kobject fixes.

    >> erm, OK. But I don't think I'm the appropriate conduit for iscsi paches.
    >>
    >> By what path _does_ iscsi ode get into the tree, anyway? Mike is listed as
    >> maintainer...

    >
    > This is a bit tricky b/c this goes to the drivers/firmware and also depends on
    > the kobject changes in Greg KH tree. But I should have included Mike on the
    > CC which I keep on forgetting .
    >


    It is probably better if it goes through Greg or Andrew. It will not
    conflict with any iscsi patches. It looks like it is heavier on kobject
    and sysfs and has some acpi digging magic, and almost no iscsi stuff in
    there.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  12. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)

    On Tue, Jan 29, 2008 at 01:13:28PM -0600, Mike Christie wrote:
    > Konrad Rzeszutek wrote:
    >> On Sunday 27 January 2008 01:01:23 you wrote:
    >>>> On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek
    >>>> wrote: Hey Andrew,
    >>>>
    >>>> Please add this patch along with Greg KH's kobject fixes.
    >>> erm, OK. But I don't think I'm the appropriate conduit for iscsi paches.
    >>>
    >>> By what path _does_ iscsi ode get into the tree, anyway? Mike is listed
    >>> as
    >>> maintainer...

    >> This is a bit tricky b/c this goes to the drivers/firmware and also
    >> depends on the kobject changes in Greg KH tree. But I should have included
    >> Mike on the CC which I keep on forgetting .

    >
    > It is probably better if it goes through Greg or Andrew. It will not
    > conflict with any iscsi patches. It looks like it is heavier on kobject and
    > sysfs and has some acpi digging magic, and almost no iscsi stuff in there.


    It does not need to go through me as all of the kobject changes it
    depended on are now in Linus's tree.

    thanks,

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

  13. Re: [PATCH] Add iSCSI iBFT support (v0.4.5)


    > That being said, I don't think there's any reason to expect the table to
    > show up on anything but i386 and x86_64, and maybe ia64.


    I've posted a new patch (http://lkml.org/lkml/2008/1/30/531) that includes
    that dependency in the Kconfig (i386, x86_64, ia64)



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

+ Reply to Thread