On Fri, 14 Dec 2007 16:17:44 -0600
Mike Miller <mike.miller@xxxxxx> wrote:
> Patch 1 of 3
>
> Sorry to take so long to repost.
>
> This patch exports more attributes to /sys so we can work work better with
> udev. Some distros use unique_id among other attributes. This patch attempts
> to provide that and other attributes to reveal more information about cciss
> devices in /sys. It's also an effort to be more sysfs friendly.
> Please consider this for inclusion.
>
I'm getting some deja vu here. I'm sure I already commented on some of
these things?
>
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index 7d70496..54080e6 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -229,20 +229,485 @@ static inline CommandList_struct *removeQ(CommandList_struct **Qptr,
> return c;
> }
>
> +static inline int find_drv_index(int ctlr, drive_info_struct *drv){
> + int i;
> + for (i=0; i < CISS_MAX_LUN; i++) {
> + if (hba[ctlr]->drv[i].LunID == drv->LunID)
> + return i;
> + }
> + return i;
> +}
pleeeeeeeeeeeze always feed all diffs through scripts/checkpatch.pl. Twice.
This function has multiple coding-style mistakes.
It is also far too large to be inlined.
> #include "cciss_scsi.c" /* For SCSI tape support */
>
> +#define ENG_GIG 1000000000
> +#define ENG_GIG_FACTOR (ENG_GIG/512)
> #define RAID_UNKNOWN 6
> +static const char *raid_label[] = { "0", "4", "1(1+0)", "5", "5+1", "ADG",
> + "UNKNOWN"};
> +
> +
> +static spinlock_t sysfs_lock = SPIN_LOCK_UNLOCKED;
checkpatch would have informed you about this mistake as well.
> +static void cciss_sysfs_stat_inquiry(int ctlr, int logvol,
> + int withirq, drive_info_struct *drv)
> +{
> + int return_code;
> + InquiryData_struct *inq_buff;
> +
> + /* If there are no heads then this is the controller disk and
> + * not a valid logical drive so don't query it.
> + */
> + if (!drv->heads)
> + return;
> +
> + inq_buff = kzalloc(sizeof(InquiryData_struct), GFP_KERNEL);
> + if (!inq_buff) {
> + printk(KERN_ERR "cciss: out of memory\n");
This failure gets dropped on the floor. Is there really no need to report
it? Will the driver still correctly function even thoug this function
didn't do anything?
> + goto err;
> + }
> +
> + if (withirq)
> + return_code = sendcmd_withirq(CISS_INQUIRY, ctlr,
> + inq_buff, sizeof(*inq_buff), 1, logvol ,0, TYPE_CMD);
> + else
> + return_code = sendcmd(CISS_INQUIRY, ctlr, inq_buff,
> + sizeof(*inq_buff), 1, logvol , 0, NULL, TYPE_CMD);
> + if (return_code == IO_OK) {
> + memcpy(drv->vendor, &inq_buff->data_byte[8], 8);
> + drv->vendor[8]='\0';
> + memcpy(drv->model, &inq_buff->data_byte[16], 16);
> + drv->model[16] = '\0';
> + memcpy(drv->rev, &inq_buff->data_byte[32], 4);
> + drv->rev[4] = '\0';
> + } else { /* Get geometry failed */
> + printk(KERN_WARNING "cciss: inquiry for VPD page 0 failed\n");
> + }
> +
> + if (withirq)
> + return_code = sendcmd_withirq(CISS_INQUIRY, ctlr,
> + inq_buff, sizeof(*inq_buff), 1, logvol ,0x83, TYPE_CMD);
> + else
> + return_code = sendcmd(CISS_INQUIRY, ctlr, inq_buff,
> + sizeof(*inq_buff), 1, logvol , 0x83, NULL, TYPE_CMD);
> +
> + if (return_code == IO_OK) {
> + memcpy(drv->uid, &inq_buff->data_byte[8], 16);
> + } else { /* Get geometry failed */
> + printk(KERN_WARNING "cciss: inquiry for VPD page 83 failed\n");
> + }
> +
> + kfree(inq_buff);
> +err:
> + drv->vendor[8] = '\0';
> + drv->model[16] = '\0';
> + drv->rev[4] = '\0';
> +
> +}
> +
> +static ssize_t cciss_show_raid_level(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct drv_dynamic *d;
> + drive_info_struct *drv;
> + ctlr_info_t *h;
> + unsigned long flags;
> + int raid;
> +
> + d = container_of(dev, struct drv_dynamic, dev);
> + spin_lock(&sysfs_lock);
> + if (!d->disk) {
> + spin_unlock(&sysfs_lock);
> + return -ENOENT;
> + }
> +
> + h = get_host(d->disk);
> +
> + spin_lock_irqsave(CCISS_LOCK(h->ctlr), flags);
> + if (h->busy_configuring) {
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return snprintf(buf, 30, "Device busy configuring\n");
> + }
The above code snippet gets repeated again and again and again. As I
suggested last time: can this be fixed?
> + drv = d->disk->private_data;
> + if ((drv->raid_level < 0) || (drv->raid_level) > 5)
> + raid = RAID_UNKNOWN;
> + else
> + raid = drv->raid_level;
> +
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return snprintf(buf, 20, "RAID %s\n", raid_label[raid]);
> +}
>
> ...
>
> +static ssize_t cciss_show_disk_size(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct drv_dynamic *d;
> + drive_info_struct *drv;
> + ctlr_info_t *h;
> + unsigned long flags;
> + sector_t vol_sz, vol_sz_frac;
> +
> + d = container_of(dev, struct drv_dynamic, dev);
> + spin_lock(&sysfs_lock);
> + if (!d->disk) {
> + spin_unlock(&sysfs_lock);
> + return -ENOENT;
> + }
> + h = get_host(d->disk);
> +
> + spin_lock_irqsave(CCISS_LOCK(h->ctlr), flags);
> + if (h->busy_configuring) {
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return snprintf(buf, 30, "Device busy configuring\n");
> + }
> +
> + drv = d->disk->private_data;
> + vol_sz = drv->nr_blocks;
> + vol_sz_frac = sector_div(vol_sz, ENG_GIG_FACTOR);
> + vol_sz_frac *= 100;
> + sector_div(vol_sz_frac, ENG_GIG_FACTOR);
> +
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return snprintf(buf, 30, "%4u.%02uGB\n", (int)vol_sz, (int)vol_sz_frac);
Strange to print it as an unsigned quantity, yet cast it to a signed one.
Are you sure that vol_sz cannot overflow a 32-bit integer?
The patch feeds magical 20's and 30's into snprintf in lots of places. Can
these magic numbers be cleaned up?
> +static ssize_t cciss_show_vendor(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct drv_dynamic *d;
> + drive_info_struct *drv;
> + ctlr_info_t *h;
> + unsigned long flags;
> + int drv_index;
> +
> + d = container_of(dev, struct drv_dynamic, dev);
> + spin_lock(&sysfs_lock);
> + if (!d->disk) {
> + spin_unlock(&sysfs_lock);
> + return -ENOENT;
> + }
> +
> + h = get_host(d->disk);
> +
> + spin_lock_irqsave(CCISS_LOCK(h->ctlr), flags);
> + if (h->busy_configuring) {
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return -EBUSY;
> + }
> +
> + drv = d->disk->private_data;
> +
> + if (!drv->heads) {
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return -ENOTTY;
> + }
> +
> + drv_index = find_drv_index(h->ctlr, drv);
> + if (drv_index != CISS_MAX_LUN) {
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return snprintf(buf, 20, "%s\n", (char *)drv->vendor);
> + }
> +
> + printk(KERN_ERR "cciss: logical drive not found\n");
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return -ENOTTY;
> +}
Multiple return points per function.
There are several reasons why we prefer the `goto out' approach to
unwinding:
- Probably generates less code
- Easier to verify that all resources adn locks got released.
- Much easier to modify in the future: if you later add an allocation of
a lock-taking then there is only one site where it needs to be undone.
> +static ssize_t cciss_show_model(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct drv_dynamic *d;
> + drive_info_struct *drv;
> + ctlr_info_t *h;
> + unsigned long flags;
> + int drv_index;
> +
> + d = container_of(dev, struct drv_dynamic, dev);
> + spin_lock(&sysfs_lock);
> + if (!d->disk) {
> + spin_unlock(&sysfs_lock);
> + return -ENOENT;
> + }
> +
> + h = get_host(d->disk);
> +
> + spin_lock_irqsave(CCISS_LOCK(h->ctlr), flags);
> + if (h->busy_configuring) {
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return -EBUSY;
> + }
> +
> + drv = d->disk->private_data;
> +
> + if (!drv->heads) {
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return -ENOTTY;
> + }
> +
> + drv_index = find_drv_index(h->ctlr, drv);
> + if (drv_index != CISS_MAX_LUN) {
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return snprintf(buf, 20, "%s\n", (char *)drv->model);
> + }
> + printk(KERN_ERR "cciss: logical drive not found\n");
> + spin_unlock_irqrestore(CCISS_LOCK(h->ctlr), flags);
> + spin_unlock(&sysfs_lock);
> + return -ENOTTY;
> +}
>
Much duplication...
> +static void cciss_add_blk_sysfs_dev(drive_info_struct *drv,
> + struct gendisk* disk,
> + struct pci_dev *pdev, int disk_num)
> +{
> + int r;
> + struct drv_dynamic *d = kzalloc(sizeof(struct drv_dynamic), GFP_KERNEL);
> + if (!d)
> + return;
What are the consequences of this failure?
> + disk->driverfs_dev = &d->dev;
> + d->dev.parent = &pdev->dev;
> + d->dev.release = (void (*)(struct device *))kfree;
> + sprintf(d->dev.bus_id, "disk%d", disk_num);
> + d->dev.driver_data = "cciss";
> + if (device_register(&d->dev)) {
> + put_device(&d->dev);
> + return;
> + }
> + r = sysfs_create_group(&d->dev.kobj, &cciss_attrs);
> + if (r)
> + return;
> + d->disk = disk;
> + drv->dev_info = &d->dev;
> +}
> +
> +static void cciss_remove_blk_sysfs_dev(struct gendisk *disk)
> +{
> + drive_info_struct *drv = get_drv(disk);
> + struct drv_dynamic *d;
> +
> + if (!drv->dev_info)
> + return;
> +
> + d = container_of(drv->dev_info, struct drv_dynamic, dev);
> + spin_lock(&sysfs_lock);
> + sysfs_remove_group(&d->dev.kobj, &cciss_attrs);
> + d->disk = NULL;
> + spin_unlock(&sysfs_lock);
Mike, what's going on? I definitely told you last time I looked at this
patch that it is a bug to call sysfs_remove_group() under spinlock, because
sysfs_remove_group() takes sleeping locks. Yet here it is again.
Maybe you lost my email. It is here: http://lkml.org/lkml/2007/11/20/77
Also please see http://lkml.org/lkml/2007/11/20/79
-
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
