On Fri, 2016-04-01 at 20:53 -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 23, 2016 at 09:49:26PM -0400, David Bond wrote:
> >
> > Some ethernet adapter vendors are supplying products which support optional
> > (payed license) features. On some adapters this includes a hardware iscsi
> > initiator. The same adapters in a normal (no extra licenses) mode of
> > operation can be used as a software iscsi initiator. In addition, software
> > iscsi boot initiators are becoming a standard part of many vendors uefi
> > implementations. This is creating difficulties during early boot/install
> > determining the proper configuration method for these adapters when they
> > are used as a boot device.
> >
> > The attached patch creates sysfs entries to expose information from the
> > acpi header of the ibft table. This information allows for a method to
> > easily determining if an ibft table was created by a ethernet card's
> > firmware or the system uefi/bios. In the case of a hardware initiator this
> > information in combination with the pci vendor and device id can be used
> > to ascertain any vendor specific behaviors that need to be accommodated.
>
> Heya!
Hello, and thanks for the review.
>
> Is there an patch in iscsiadm for this as well? Or is this
I had not planned a iscsiadm change. This patch is simply to expose the
information for use by early install/boot scripts. If a change to iscsiadm
is desired, this information would (IMHO) fit with the "-m fw" output.
> >
> > Reviewed-by: Lee Duncan <lduncan@xxxxxxxx>
> > Signed-off-by: David Bond <dbond@xxxxxxxx>
> > ---
> > Documentation/ABI/testing/sysfs-ibft | 10 ++++++
> > drivers/firmware/iscsi_ibft.c | 64 +++++++++++++++++++++++++++++++++++-
> > drivers/scsi/iscsi_boot_sysfs.c | 62 ++++++++++++++++++++++++++++++++++
> > include/linux/iscsi_boot_sysfs.h | 13 ++++++++
> > 4 files changed, 148 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-ibft b/Documentation/ABI/testing/sysfs-ibft
> > index cac3930..7d6725f 100644
> > --- a/Documentation/ABI/testing/sysfs-ibft
> > +++ b/Documentation/ABI/testing/sysfs-ibft
> > @@ -21,3 +21,13 @@ Contact: Konrad Rzeszutek <ketuzsezr@xxxxxxxxxx>
> > Description: The /sys/firmware/ibft/ethernetX directory will contain
> > files that expose the iSCSI Boot Firmware Table NIC data.
> > Usually this contains the IP address, MAC, and gateway of the NIC.
> > +
> > +What: /sys/firmware/ibft/acpi_header
> > +Date: March 2016
> > +Contact: David Bond <dbond@xxxxxxxx>
> > +Description: The /sys/firmware/ibft/acpi_header directory will contain files
> > + that expose the SIGNATURE, OEM_ID, and OEM_TABLE_ID fields of the
> > + acpi table header of the iBFT structure. This will allow for
> > + identification of the creator of the table which is useful in
> > + determining quirks associated with some adapters when used in
> > + hardware vs software iscsi initiator mode.
> > diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c
> > index 81037e5..2b1900b 100644
> > --- a/drivers/firmware/iscsi_ibft.c
> > +++ b/drivers/firmware/iscsi_ibft.c
> > @@ -418,6 +418,31 @@ static ssize_t ibft_attr_show_target(void *data, int type, char *buf)
> > return str - buf;
> > }
> >
> > +static ssize_t ibft_attr_show_acpitbl(void *data, int type, char *buf)
> > +{
> > + struct ibft_kobject *entry = data;
> > + char *str = buf;
> > +
> > + switch (type) {
> > + case ISCSI_BOOT_ACPITBL_SIGNATURE:
> > + str += sprintf_string(str, ACPI_NAME_SIZE,
> > + entry->header->header.signature);
> > + break;
> > + case ISCSI_BOOT_ACPITBL_OEM_ID:
> > + str += sprintf_string(str, ACPI_OEM_ID_SIZE,
> > + entry->header->header.oem_id);
> > + break;
> > + case ISCSI_BOOT_ACPITBL_OEM_TABLE_ID:
> > + str += sprintf_string(str, ACPI_OEM_TABLE_ID_SIZE,
> > + entry->header->header.oem_table_id);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return str - buf;
> > +}
> > +
> > static int __init ibft_check_device(void)
> > {
> > int len;
> > @@ -576,6 +601,24 @@ static umode_t __init ibft_check_initiator_for(void *data, int type)
> > return rc;
> > }
> >
> > +static umode_t __init ibft_check_acpitbl_for(void *data, int type)
> > +{
> > +
> > + umode_t rc = 0;
> > +
> > + switch (type) {
> > + case ISCSI_BOOT_ACPITBL_SIGNATURE:
> > + case ISCSI_BOOT_ACPITBL_OEM_ID:
> > + case ISCSI_BOOT_ACPITBL_OEM_TABLE_ID:
> > + rc = S_IRUGO;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > static void ibft_kobj_release(void *data)
> > {
> > kfree(data);
> > @@ -699,6 +742,8 @@ free_ibft_obj:
> > static int __init ibft_register_kobjects(struct acpi_table_ibft *header)
> > {
> > struct ibft_control *control = NULL;
> > + struct iscsi_boot_kobj *boot_kobj;
> > + struct ibft_kobject *ibft_kobj;
> > void *ptr, *end;
> > int rc = 0;
> > u16 offset;
> > @@ -727,6 +772,23 @@ static int __init ibft_register_kobjects(struct acpi_table_ibft *header)
> > }
> > }
> >
>
> What about the rc earlier (from the loop)? You end up ignoring it.
> Should we have:
>
> if (rc)
> return rc;
>
> ?
Yes, this is most certainly required.
> > + ibft_kobj = kzalloc(sizeof(*ibft_kobj), GFP_KERNEL);
> > + if (!ibft_kobj)
> > + return -ENOMEM;
> > +
> > + ibft_kobj->header = header;
> > + ibft_kobj->hdr = NULL; /*for ibft_unregister*/
> > +
> > + boot_kobj = iscsi_boot_create_acpitbl(boot_kset, 0,
> > + ibft_kobj,
> > + ibft_attr_show_acpitbl,
> > + ibft_check_acpitbl_for,
> > + ibft_kobj_release);
>
> There something off with the offset here, but I presume it is just the
> tabs and them not showing up properly in my editor.
>
More likely a disconnect between my brain and fingers.
> > + if (!boot_kobj) {
> > + kfree(ibft_kobj);
> > + rc = -ENOMEM;
> > + }
> > +
> > return rc;
> > }
> >
> > @@ -738,7 +800,7 @@ static void ibft_unregister(void)
> > list_for_each_entry_safe(boot_kobj, tmp_kobj,
> > &boot_kset->kobj_list, list) {
> > ibft_kobj = boot_kobj->data;
> > - if (ibft_kobj->hdr->id == id_nic)
> > + if (ibft_kobj->hdr && ibft_kobj->hdr->id == id_nic)
> > sysfs_remove_link(&boot_kobj->kobj, "device");
> > };
> > }
> > diff --git a/drivers/scsi/iscsi_boot_sysfs.c b/drivers/scsi/iscsi_boot_sysfs.c
> > index 8f0ea97..b472f56 100644
> > --- a/drivers/scsi/iscsi_boot_sysfs.c
> > +++ b/drivers/scsi/iscsi_boot_sysfs.c
> > @@ -306,6 +306,42 @@ static struct attribute_group iscsi_boot_initiator_attr_group = {
> > .is_visible = iscsi_boot_ini_attr_is_visible,
> > };
> >
> > +/* iBFT ACPI Table attributes */
> > +iscsi_boot_rd_attr(acpitbl_signature, signature, ISCSI_BOOT_ACPITBL_SIGNATURE);
> > +iscsi_boot_rd_attr(acpitbl_oem_id, oem_id, ISCSI_BOOT_ACPITBL_OEM_ID);
> > +iscsi_boot_rd_attr(acpitbl_oem_table_id, oem_table_id,
> > + ISCSI_BOOT_ACPITBL_OEM_TABLE_ID);
> > +
> > +static struct attribute *acpitbl_attrs[] = {
> > + &iscsi_boot_attr_acpitbl_signature.attr,
> > + &iscsi_boot_attr_acpitbl_oem_id.attr,
> > + &iscsi_boot_attr_acpitbl_oem_table_id.attr,
> > + NULL
> > +};
> > +
> > +static umode_t iscsi_boot_acpitbl_attr_is_visible(struct kobject *kobj,
> > + struct attribute *attr, int i)
> > +{
> > + struct iscsi_boot_kobj *boot_kobj =
> > + container_of(kobj, struct iscsi_boot_kobj, kobj);
> > +
> > + if (attr == &iscsi_boot_attr_acpitbl_signature.attr)
> > + return boot_kobj->is_visible(boot_kobj->data,
> > + ISCSI_BOOT_ACPITBL_SIGNATURE);
> > + if (attr == &iscsi_boot_attr_acpitbl_oem_id.attr)
> > + return boot_kobj->is_visible(boot_kobj->data,
> > + ISCSI_BOOT_ACPITBL_OEM_ID);
> > + if (attr == &iscsi_boot_attr_acpitbl_oem_table_id.attr)
> > + return boot_kobj->is_visible(boot_kobj->data,
> > + ISCSI_BOOT_ACPITBL_OEM_TABLE_ID);
> > + return 0;
> > +}
> > +
> > +static struct attribute_group iscsi_boot_acpitbl_attr_group = {
> > + .attrs = acpitbl_attrs,
> > + .is_visible = iscsi_boot_acpitbl_attr_is_visible,
> > +};
> > +
> > static struct iscsi_boot_kobj *
> > iscsi_boot_create_kobj(struct iscsi_boot_kset *boot_kset,
> > struct attribute_group *attr_group,
> > @@ -436,6 +472,32 @@ iscsi_boot_create_ethernet(struct iscsi_boot_kset *boot_kset, int index,
> > EXPORT_SYMBOL_GPL(iscsi_boot_create_ethernet);
> >
> > /**
> > + * iscsi_boot_create_acpitbl() - create boot acpi table sysfs dir
> > + * @boot_kset: boot kset
> > + * @index: not used
> > + * @data: driver specific data
> > + * @show: attr show function
> > + * @is_visible: attr visibility function
> > + * @release: release function
> > + *
> > + * Note: The boot sysfs lib will free the data passed in for the caller
> > + * when all refs to the acpitbl kobject have been released.
> > + */
> > +struct iscsi_boot_kobj *
> > +iscsi_boot_create_acpitbl(struct iscsi_boot_kset *boot_kset, int index,
> > + void *data,
> > + ssize_t (*show)(void *data, int type, char *buf),
> > + umode_t (*is_visible)(void *data, int type),
> > + void (*release)(void *data))
> > +{
> > + return iscsi_boot_create_kobj(boot_kset,
> > + &iscsi_boot_acpitbl_attr_group,
> > + "acpi_header", index, data, show,
> > + is_visible, release);
> > +}
> > +EXPORT_SYMBOL_GPL(iscsi_boot_create_acpitbl);
> > +
> > +/**
> > * iscsi_boot_create_kset() - creates root sysfs tree
> > * @set_name: name of root dir
> > */
> > diff --git a/include/linux/iscsi_boot_sysfs.h b/include/linux/iscsi_boot_sysfs.h
> > index 548d553..10923d7 100644
> > --- a/include/linux/iscsi_boot_sysfs.h
> > +++ b/include/linux/iscsi_boot_sysfs.h
> > @@ -64,6 +64,12 @@ enum iscsi_boot_initiator_properties_enum {
> > ISCSI_BOOT_INI_END_MARKER,
> > };
> >
> > +enum iscsi_boot_acpitbl_properties_enum {
> > + ISCSI_BOOT_ACPITBL_SIGNATURE,
> > + ISCSI_BOOT_ACPITBL_OEM_ID,
> > + ISCSI_BOOT_ACPITBL_OEM_TABLE_ID,
> > +};
> > +
> > struct attribute_group;
> >
> > struct iscsi_boot_kobj {
> > @@ -127,6 +133,13 @@ iscsi_boot_create_target(struct iscsi_boot_kset *boot_kset, int index,
> > umode_t (*is_visible) (void *data, int type),
> > void (*release) (void *data));
> >
> > +struct iscsi_boot_kobj *
> > +iscsi_boot_create_acpitbl(struct iscsi_boot_kset *boot_kset, int index,
> > + void *data,
> > + ssize_t (*show)(void *data, int type, char *buf),
> > + umode_t (*is_visible)(void *data, int type),
> > + void (*release)(void *data));
> > +
> > struct iscsi_boot_kset *iscsi_boot_create_kset(const char *set_name);
> > struct iscsi_boot_kset *iscsi_boot_create_host_kset(unsigned int hostno);
> > void iscsi_boot_destroy_kset(struct iscsi_boot_kset *boot_kset);
I'll fix these issues and post a v2.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
