bus_type->dev_attrs not properly thought out

[Russell King]

Hi,

I thought I'd look into converting the MMC sysfs code to use the new
bus_type->dev_attrs pointer.  However, it doesn't appear that enough
thought has been put into how this should work.

1. include/linux/device.h, macro for creating device attributes:

#define DEVICE_ATTR(_name,_mode,_show,_store) \
struct device_attribute dev_attr_##_name = __ATTR(_name,_mode,_show,_store)

2. include/linux/device.h, bus_type definition:

struct bus_type {
...
        struct device_attribute * dev_attrs;
...
};

3. device_add_attrs(), the code which adds the attributes to a device:

        if (bus->dev_attrs) {
                for (i = 0; attr_name(bus->dev_attrs[i]); i++) {
                        error = device_create_file(dev,&bus->dev_attrs[i]);
                        if (error)
                                goto Err;
                }
        }

The crux of the problem:
 - As can be seen from (3) and (2), we expect dev_attrs to point to an
   array of at least two struct device_attributes.  This is incompatible
   with (1).

Example of the problem:

 - MMC code can do this:

#define MMC_ATTR(name, fmt, args...)                                    \
static ssize_t mmc_dev_show_##name (struct device *dev, char *buf)      \
{                                                                       \
        struct mmc_card *card = dev_to_mmc_card(dev);                   \
        return sprintf(buf, fmt, args);                                 \
}                                                                       \
static DEVICE_ATTR(name, S_IRUGO, mmc_dev_show_##name, NULL)

MMC_ATTR(cid, "%08x %08x %08x %08x\n",
        card->raw_cid[0], card->raw_cid[1],
        card->raw_cid[2], card->raw_cid[3]);
MMC_ATTR(csd, "%08x %08x %08x %08x\n",
        card->raw_csd[0], card->raw_csd[1],
        card->raw_csd[2], card->raw_csd[3]);
MMC_ATTR(date, "%02d/%04d\n", card->cid.month, card->cid.year);
MMC_ATTR(fwrev, "0x%x\n", card->cid.fwrev);
MMC_ATTR(hwrev, "0x%x\n", card->cid.hwrev);
MMC_ATTR(manfid, "0x%06x\n", card->cid.manfid);
MMC_ATTR(name, "%s\n", card->cid.prod_name);
MMC_ATTR(oemid, "0x%02x\n", card->cid.oemid);
MMC_ATTR(serial, "0x%08x\n", card->cid.serial);

static struct device_attribute *mmc_dev_attributes[] = {
        &dev_attr_cid,
        &dev_attr_csd,
        &dev_attr_date,
        &dev_attr_fwrev,
        &dev_attr_hwrev,
        &dev_attr_manfid,
        &dev_attr_name,
        &dev_attr_oemid,
        &dev_attr_serial,
};

and handle the array of mmc_dev_attributes itself.  However, converting
this to a suitable form to allow it to be poked into bus_type->dev_attrs
makes this:

#define MMC_ATTR(name, fmt, args...)                                    \
static ssize_t mmc_dev_show_##name (struct device *dev, char *buf)      \
{                                                                       \
        struct mmc_card *card = dev_to_mmc_card(dev);                   \
        return sprintf(buf, fmt, args);                                 \
}

MMC_ATTR(cid, "%08x %08x %08x %08x\n",
        card->raw_cid[0], card->raw_cid[1],
        card->raw_cid[2], card->raw_cid[3]);
MMC_ATTR(csd, "%08x %08x %08x %08x\n",
        card->raw_csd[0], card->raw_csd[1],
        card->raw_csd[2], card->raw_csd[3]);
MMC_ATTR(date, "%02d/%04d\n", card->cid.month, card->cid.year);
MMC_ATTR(fwrev, "0x%x\n", card->cid.fwrev);
MMC_ATTR(hwrev, "0x%x\n", card->cid.hwrev);
MMC_ATTR(manfid, "0x%06x\n", card->cid.manfid);
MMC_ATTR(name, "%s\n", card->cid.prod_name);
MMC_ATTR(oemid, "0x%02x\n", card->cid.oemid);
MMC_ATTR(serial, "0x%08x\n", card->cid.serial);

static struct device_attributes mmc_dev_attrs[] = {
	{
		{
			.name = "cid",
			.owner = THIS_MODULE,
			.mode = S_IRUGO,
		},
		.show = mmc_dev_show_cid,
	}, {
		{
			.name = "csd",
			.owner = THIS_MODULE,
			.mode = S_IRUGO,
		},
		.show = mmc_dev_show_csd,
	}, {
		{
			.name = "date",
			.owner = THIS_MODULE,
			.mode = S_IRUGO,
		},
		.show = mmc_dev_show_date,
	}, ... etc ...
};

Hardly elegant, hardly clean, and hardly foolproof from silly C'n'P
errors.

Can we please convert the attribute stuff to something a little saner
so the existing macros can still be used?

I don't think anyone uses this yet, so now would be an opportune
moment to fix this.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core
-
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/