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

From: Konrad Rzeszutek
Date: Mon Jan 28 2008 - 14:31:19 EST


On Sunday 27 January 2008 01:01:23 you wrote:
> > On Fri, 25 Jan 2008 18:06:29 -0400 Konrad Rzeszutek <konrad@xxxxxxxxxx>
> > 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 <sigh>.

>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/