* [patch] cciss: Fix cciss SCSI rescan code to better notice device changes.
@ 2008-09-18 20:07 scameron
2008-09-20 0:19 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: scameron @ 2008-09-18 20:07 UTC (permalink / raw)
To: linux-kernel, mike.miller, axboe; +Cc: scameron
Fix cciss SCSI rescan code to better notice device changes.
If you hot-unplug a tape drive, then hot-plug a different
tape drive into the same slot in a storage enclosure,
the cciss driver wouldn't notice anything had changed, as
it was only looking at the LUN address and device type.
Now it looks at the inquiry page 0x83 device identifier,
and vendor and model strings as well.
Signed-off-by: Stephen M. Cameron <scameron@beardog.cca.cpqcorp.net>
---
drivers/block/cciss_scsi.c | 149 +++++++++++++++++++++++++++++----------------
drivers/block/cciss_scsi.h | 4 +
2 files changed, 101 insertions(+), 52 deletions(-)
diff -puN drivers/block/cciss_scsi.c~cciss_fix_hotplug_tape_bug drivers/block/cciss_scsi.c
--- linux-2.6.27-rc6/drivers/block/cciss_scsi.c~cciss_fix_hotplug_tape_bug 2008-09-18 07:34:58.000000000 -0500
+++ linux-2.6.27-rc6-scameron/drivers/block/cciss_scsi.c 2008-09-18 07:35:13.000000000 -0500
@@ -365,7 +365,7 @@ struct scsi2map {
static int
cciss_scsi_add_entry(int ctlr, int hostno,
- unsigned char *scsi3addr, int devtype,
+ struct cciss_scsi_dev_t *device,
struct scsi2map *added, int *nadded)
{
/* assumes hba[ctlr]->scsi_ctlr->lock is held */
@@ -384,12 +384,12 @@ cciss_scsi_add_entry(int ctlr, int hostn
lun = 0;
/* Is this device a non-zero lun of a multi-lun device */
/* byte 4 of the 8-byte LUN addr will contain the logical unit no. */
- if (scsi3addr[4] != 0) {
+ if (device->scsi3addr[4] != 0) {
/* Search through our list and find the device which */
/* has the same 8 byte LUN address, excepting byte 4. */
/* Assign the same bus and target for this new LUN. */
/* Use the logical unit number from the firmware. */
- memcpy(addr1, scsi3addr, 8);
+ memcpy(addr1, device->scsi3addr, 8);
addr1[4] = 0;
for (i = 0; i < n; i++) {
sd = &ccissscsi[ctlr].dev[i];
@@ -399,7 +399,7 @@ cciss_scsi_add_entry(int ctlr, int hostn
if (memcmp(addr1, addr2, 8) == 0) {
bus = sd->bus;
target = sd->target;
- lun = scsi3addr[4];
+ lun = device->scsi3addr[4];
break;
}
}
@@ -420,8 +420,12 @@ cciss_scsi_add_entry(int ctlr, int hostn
added[*nadded].lun = sd->lun;
(*nadded)++;
- memcpy(&sd->scsi3addr[0], scsi3addr, 8);
- sd->devtype = devtype;
+ memcpy(sd->scsi3addr, device->scsi3addr, 8);
+ memcpy(sd->vendor, device->vendor, sizeof(sd->vendor));
+ memcpy(sd->revision, device->revision, sizeof(sd->revision));
+ memcpy(sd->device_id, device->device_id, sizeof(sd->device_id));
+ sd->devtype = device->devtype;
+
ccissscsi[ctlr].ndevices++;
/* initially, (before registering with scsi layer) we don't
@@ -487,6 +491,22 @@ static void fixup_botched_add(int ctlr,
CPQ_TAPE_UNLOCK(ctlr, flags);
}
+static int device_is_the_same(struct cciss_scsi_dev_t *dev1,
+ struct cciss_scsi_dev_t *dev2)
+{
+ return dev1->devtype == dev2->devtype &&
+ memcmp(dev1->scsi3addr, dev2->scsi3addr,
+ sizeof(dev1->scsi3addr)) == 0 &&
+ memcmp(dev1->device_id, dev2->device_id,
+ sizeof(dev1->device_id)) == 0 &&
+ memcmp(dev1->vendor, dev2->vendor,
+ sizeof(dev1->vendor)) == 0 &&
+ memcmp(dev1->model, dev2->model,
+ sizeof(dev1->model)) == 0 &&
+ memcmp(dev1->revision, dev2->revision,
+ sizeof(dev1->revision)) == 0;
+}
+
static int
adjust_cciss_scsi_table(int ctlr, int hostno,
struct cciss_scsi_dev_t sd[], int nsds)
@@ -532,7 +552,7 @@ adjust_cciss_scsi_table(int ctlr, int ho
for (j=0;j<nsds;j++) {
if (SCSI3ADDR_EQ(sd[j].scsi3addr,
csd->scsi3addr)) {
- if (sd[j].devtype == csd->devtype)
+ if (device_is_the_same(&sd[j], csd))
found=2;
else
found=1;
@@ -548,22 +568,26 @@ adjust_cciss_scsi_table(int ctlr, int ho
cciss_scsi_remove_entry(ctlr, hostno, i,
removed, &nremoved);
/* remove ^^^, hence i not incremented */
- }
- else if (found == 1) { /* device is different kind */
+ } else if (found == 1) { /* device is different in some way */
changes++;
- printk("cciss%d: device c%db%dt%dl%d type changed "
- "(device type now %s).\n",
- ctlr, hostno, csd->bus, csd->target, csd->lun,
- scsi_device_type(csd->devtype));
+ printk("cciss%d: device c%db%dt%dl%d has changed.\n",
+ ctlr, hostno, csd->bus, csd->target, csd->lun);
cciss_scsi_remove_entry(ctlr, hostno, i,
removed, &nremoved);
/* remove ^^^, hence i not incremented */
- if (cciss_scsi_add_entry(ctlr, hostno,
- &sd[j].scsi3addr[0], sd[j].devtype,
+ if (cciss_scsi_add_entry(ctlr, hostno, &sd[j],
added, &nadded) != 0)
/* we just removed one, so add can't fail. */
BUG();
csd->devtype = sd[j].devtype;
+ memcpy(csd->device_id, sd[j].device_id,
+ sizeof(csd->device_id));
+ memcpy(csd->vendor, sd[j].vendor,
+ sizeof(csd->vendor));
+ memcpy(csd->model, sd[j].model,
+ sizeof(csd->model));
+ memcpy(csd->revision, sd[j].revision,
+ sizeof(csd->revision));
} else /* device is same as it ever was, */
i++; /* so just move along. */
}
@@ -577,7 +601,7 @@ adjust_cciss_scsi_table(int ctlr, int ho
csd = &ccissscsi[ctlr].dev[j];
if (SCSI3ADDR_EQ(sd[i].scsi3addr,
csd->scsi3addr)) {
- if (sd[i].devtype == csd->devtype)
+ if (device_is_the_same(&sd[i], csd))
found=2; /* found device */
else
found=1; /* found a bug. */
@@ -586,16 +610,14 @@ adjust_cciss_scsi_table(int ctlr, int ho
}
if (!found) {
changes++;
- if (cciss_scsi_add_entry(ctlr, hostno,
-
- &sd[i].scsi3addr[0], sd[i].devtype,
+ if (cciss_scsi_add_entry(ctlr, hostno, &sd[i],
added, &nadded) != 0)
break;
} else if (found == 1) {
/* should never happen... */
changes++;
- printk("cciss%d: device unexpectedly changed type\n",
- ctlr);
+ printk(KERN_WARNING "cciss%d: device "
+ "unexpectedly changed\n", ctlr);
/* but if it does happen, we just ignore that device */
}
}
@@ -1012,7 +1034,8 @@ cciss_scsi_interpret_error(CommandList_s
static int
cciss_scsi_do_inquiry(ctlr_info_t *c, unsigned char *scsi3addr,
- unsigned char *buf, unsigned char bufsize)
+ unsigned char page, unsigned char *buf,
+ unsigned char bufsize)
{
int rc;
CommandList_struct *cp;
@@ -1032,8 +1055,8 @@ cciss_scsi_do_inquiry(ctlr_info_t *c, un
ei = cp->err_info;
cdb[0] = CISS_INQUIRY;
- cdb[1] = 0;
- cdb[2] = 0;
+ cdb[1] = (page != 0);
+ cdb[2] = page;
cdb[3] = 0;
cdb[4] = bufsize;
cdb[5] = 0;
@@ -1053,6 +1076,25 @@ cciss_scsi_do_inquiry(ctlr_info_t *c, un
return rc;
}
+/* Get the device id from inquiry page 0x83 */
+static int cciss_scsi_get_device_id(ctlr_info_t *c, unsigned char *scsi3addr,
+ unsigned char *device_id, int buflen)
+{
+ int rc;
+ unsigned char *buf;
+
+ if (buflen > 16)
+ buflen = 16;
+ buf = kzalloc(64, GFP_KERNEL);
+ if (!buf)
+ return -1;
+ rc = cciss_scsi_do_inquiry(c, scsi3addr, 0x83, buf, 64);
+ if (rc == 0)
+ memcpy(device_id, &buf[8], buflen);
+ kfree(buf);
+ return rc != 0;
+}
+
static int
cciss_scsi_do_report_phys_luns(ctlr_info_t *c,
ReportLunData_struct *buf, int bufsize)
@@ -1142,25 +1184,21 @@ cciss_update_non_disk_devices(int cntl_n
ctlr_info_t *c;
__u32 num_luns=0;
unsigned char *ch;
- /* unsigned char found[CCISS_MAX_SCSI_DEVS_PER_HBA]; */
- struct cciss_scsi_dev_t currentsd[CCISS_MAX_SCSI_DEVS_PER_HBA];
+ struct cciss_scsi_dev_t *currentsd, *this_device;
int ncurrent=0;
int reportlunsize = sizeof(*ld_buff) + CISS_MAX_PHYS_LUN * 8;
int i;
c = (ctlr_info_t *) hba[cntl_num];
ld_buff = kzalloc(reportlunsize, GFP_KERNEL);
- if (ld_buff == NULL) {
- printk(KERN_ERR "cciss: out of memory\n");
- return;
- }
inq_buff = kmalloc(OBDR_TAPE_INQ_SIZE, GFP_KERNEL);
- if (inq_buff == NULL) {
- printk(KERN_ERR "cciss: out of memory\n");
- kfree(ld_buff);
- return;
+ currentsd = kzalloc(sizeof(*currentsd) *
+ (CCISS_MAX_SCSI_DEVS_PER_HBA+1), GFP_KERNEL);
+ if (ld_buff == NULL || inq_buff == NULL || currentsd == NULL) {
+ printk(KERN_ERR "cciss: out of memory\n");
+ goto out;
}
-
+ this_device = ¤tsd[CCISS_MAX_SCSI_DEVS_PER_HBA];
if (cciss_scsi_do_report_phys_luns(c, ld_buff, reportlunsize) == 0) {
ch = &ld_buff->LUNListLength[0];
num_luns = ((ch[0]<<24) | (ch[1]<<16) | (ch[2]<<8) | ch[3]) / 8;
@@ -1179,23 +1217,34 @@ cciss_update_non_disk_devices(int cntl_n
/* adjust our table of devices */
- for(i=0; i<num_luns; i++)
- {
- int devtype;
-
+ for (i = 0; i < num_luns; i++) {
/* for each physical lun, do an inquiry */
if (ld_buff->LUN[i][3] & 0xC0) continue;
memset(inq_buff, 0, OBDR_TAPE_INQ_SIZE);
memcpy(&scsi3addr[0], &ld_buff->LUN[i][0], 8);
- if (cciss_scsi_do_inquiry(hba[cntl_num], scsi3addr, inq_buff,
- (unsigned char) OBDR_TAPE_INQ_SIZE) != 0) {
+ if (cciss_scsi_do_inquiry(hba[cntl_num], scsi3addr, 0, inq_buff,
+ (unsigned char) OBDR_TAPE_INQ_SIZE) != 0)
/* Inquiry failed (msg printed already) */
- devtype = 0; /* so we will skip this device. */
- } else /* what kind of device is this? */
- devtype = (inq_buff[0] & 0x1f);
+ continue; /* so we will skip this device. */
+
+ this_device->devtype = (inq_buff[0] & 0x1f);
+ this_device->bus = -1;
+ this_device->target = -1;
+ this_device->lun = -1;
+ memcpy(this_device->scsi3addr, scsi3addr, 8);
+ memcpy(this_device->vendor, &inq_buff[8],
+ sizeof(this_device->vendor));
+ memcpy(this_device->model, &inq_buff[16],
+ sizeof(this_device->model));
+ memcpy(this_device->revision, &inq_buff[32],
+ sizeof(this_device->revision));
+ memset(this_device->device_id, 0,
+ sizeof(this_device->device_id));
+ cciss_scsi_get_device_id(hba[cntl_num], scsi3addr,
+ this_device->device_id, sizeof(this_device->device_id));
- switch (devtype)
+ switch (this_device->devtype)
{
case 0x05: /* CD-ROM */ {
@@ -1220,15 +1269,10 @@ cciss_update_non_disk_devices(int cntl_n
if (ncurrent >= CCISS_MAX_SCSI_DEVS_PER_HBA) {
printk(KERN_INFO "cciss%d: %s ignored, "
"too many devices.\n", cntl_num,
- scsi_device_type(devtype));
+ scsi_device_type(this_device->devtype));
break;
}
- memcpy(¤tsd[ncurrent].scsi3addr[0],
- &scsi3addr[0], 8);
- currentsd[ncurrent].devtype = devtype;
- currentsd[ncurrent].bus = -1;
- currentsd[ncurrent].target = -1;
- currentsd[ncurrent].lun = -1;
+ currentsd[ncurrent] = *this_device;
ncurrent++;
break;
default:
@@ -1240,6 +1284,7 @@ cciss_update_non_disk_devices(int cntl_n
out:
kfree(inq_buff);
kfree(ld_buff);
+ kfree(currentsd);
return;
}
diff -puN drivers/block/cciss_scsi.h~cciss_fix_hotplug_tape_bug drivers/block/cciss_scsi.h
--- linux-2.6.27-rc6/drivers/block/cciss_scsi.h~cciss_fix_hotplug_tape_bug 2008-09-18 07:35:00.000000000 -0500
+++ linux-2.6.27-rc6-scameron/drivers/block/cciss_scsi.h 2008-09-18 07:35:13.000000000 -0500
@@ -66,6 +66,10 @@ struct cciss_scsi_dev_t {
int devtype;
int bus, target, lun; /* as presented to the OS */
unsigned char scsi3addr[8]; /* as presented to the HW */
+ unsigned char device_id[16]; /* from inquiry pg. 0x83 */
+ unsigned char vendor[8]; /* bytes 8-15 of inquiry data */
+ unsigned char model[16]; /* bytes 16-31 of inquiry data */
+ unsigned char revision[4]; /* bytes 32-35 of inquiry data */
};
struct cciss_scsi_hba_t {
_
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [patch] cciss: Fix cciss SCSI rescan code to better notice device changes.
2008-09-18 20:07 [patch] cciss: Fix cciss SCSI rescan code to better notice device changes scameron
@ 2008-09-20 0:19 ` Andrew Morton
2008-10-01 15:12 ` scameron
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2008-09-20 0:19 UTC (permalink / raw)
To: scameron; +Cc: linux-kernel, mike.miller, axboe, scameron
On Thu, 18 Sep 2008 15:07:44 -0500
scameron@beardog.cca.cpqcorp.net wrote:
>
> Fix cciss SCSI rescan code to better notice device changes.
> If you hot-unplug a tape drive, then hot-plug a different
> tape drive into the same slot in a storage enclosure,
> the cciss driver wouldn't notice anything had changed, as
> it was only looking at the LUN address and device type.
> Now it looks at the inquiry page 0x83 device identifier,
> and vendor and model strings as well.
>
> ...
>
> diff -puN drivers/block/cciss_scsi.c~cciss_fix_hotplug_tape_bug drivers/block/cciss_scsi.c
> --- linux-2.6.27-rc6/drivers/block/cciss_scsi.c~cciss_fix_hotplug_tape_bug 2008-09-18 07:34:58.000000000 -0500
> +++ linux-2.6.27-rc6-scameron/drivers/block/cciss_scsi.c 2008-09-18 07:35:13.000000000 -0500
> @@ -365,7 +365,7 @@ struct scsi2map {
>
> static int
> cciss_scsi_add_entry(int ctlr, int hostno,
> - unsigned char *scsi3addr, int devtype,
> + struct cciss_scsi_dev_t *device,
> struct scsi2map *added, int *nadded)
> {
> /* assumes hba[ctlr]->scsi_ctlr->lock is held */
> @@ -384,12 +384,12 @@ cciss_scsi_add_entry(int ctlr, int hostn
> lun = 0;
> /* Is this device a non-zero lun of a multi-lun device */
> /* byte 4 of the 8-byte LUN addr will contain the logical unit no. */
> - if (scsi3addr[4] != 0) {
> + if (device->scsi3addr[4] != 0) {
> /* Search through our list and find the device which */
> /* has the same 8 byte LUN address, excepting byte 4. */
> /* Assign the same bus and target for this new LUN. */
> /* Use the logical unit number from the firmware. */
> - memcpy(addr1, scsi3addr, 8);
> + memcpy(addr1, device->scsi3addr, 8);
> addr1[4] = 0;
> for (i = 0; i < n; i++) {
> sd = &ccissscsi[ctlr].dev[i];
> @@ -399,7 +399,7 @@ cciss_scsi_add_entry(int ctlr, int hostn
> if (memcmp(addr1, addr2, 8) == 0) {
> bus = sd->bus;
> target = sd->target;
> - lun = scsi3addr[4];
> + lun = device->scsi3addr[4];
> break;
> }
> }
> @@ -420,8 +420,12 @@ cciss_scsi_add_entry(int ctlr, int hostn
> added[*nadded].lun = sd->lun;
> (*nadded)++;
>
> - memcpy(&sd->scsi3addr[0], scsi3addr, 8);
> - sd->devtype = devtype;
> + memcpy(sd->scsi3addr, device->scsi3addr, 8);
> + memcpy(sd->vendor, device->vendor, sizeof(sd->vendor));
> + memcpy(sd->revision, device->revision, sizeof(sd->revision));
> + memcpy(sd->device_id, device->device_id, sizeof(sd->device_id));
> + sd->devtype = device->devtype;
y'know, it would be a lot simpler to just do
*sd = *device;
a few lines up, before assigning sd->bus/target/lun.
Putting these:
@@ -66,6 +66,10 @@ struct cciss_scsi_dev_t {
int devtype;
int bus, target, lun; /* as presented to the OS */
unsigned char scsi3addr[8]; /* as presented to the HW */
+ unsigned char device_id[16]; /* from inquiry pg. 0x83 */
+ unsigned char vendor[8]; /* bytes 8-15 of inquiry data */
+ unsigned char model[16]; /* bytes 16-31 of inquiry data */
+ unsigned char revision[4]; /* bytes 32-35 of inquiry data */
};
into a separate nested structure might make that neater still.
> ccissscsi[ctlr].ndevices++;
>
> /* initially, (before registering with scsi layer) we don't
> @@ -487,6 +491,22 @@ static void fixup_botched_add(int ctlr,
> CPQ_TAPE_UNLOCK(ctlr, flags);
> }
>
> +static int device_is_the_same(struct cciss_scsi_dev_t *dev1,
> + struct cciss_scsi_dev_t *dev2)
`struct cciss_scsi_dev_t' should have been named `struct cciss_scsi_dev',
but I guess I'm a bit late to that party.
> +{
> + return dev1->devtype == dev2->devtype &&
> + memcmp(dev1->scsi3addr, dev2->scsi3addr,
> + sizeof(dev1->scsi3addr)) == 0 &&
> + memcmp(dev1->device_id, dev2->device_id,
> + sizeof(dev1->device_id)) == 0 &&
> + memcmp(dev1->vendor, dev2->vendor,
> + sizeof(dev1->vendor)) == 0 &&
> + memcmp(dev1->model, dev2->model,
> + sizeof(dev1->model)) == 0 &&
> + memcmp(dev1->revision, dev2->revision,
> + sizeof(dev1->revision)) == 0;
> +}
> +
>
> ...
>
> + } else if (found == 1) { /* device is different in some way */
> changes++;
> - printk("cciss%d: device c%db%dt%dl%d type changed "
> - "(device type now %s).\n",
> - ctlr, hostno, csd->bus, csd->target, csd->lun,
> - scsi_device_type(csd->devtype));
> + printk("cciss%d: device c%db%dt%dl%d has changed.\n",
> + ctlr, hostno, csd->bus, csd->target, csd->lun);
> cciss_scsi_remove_entry(ctlr, hostno, i,
> removed, &nremoved);
> /* remove ^^^, hence i not incremented */
> - if (cciss_scsi_add_entry(ctlr, hostno,
> - &sd[j].scsi3addr[0], sd[j].devtype,
> + if (cciss_scsi_add_entry(ctlr, hostno, &sd[j],
> added, &nadded) != 0)
> /* we just removed one, so add can't fail. */
> BUG();
> csd->devtype = sd[j].devtype;
> + memcpy(csd->device_id, sd[j].device_id,
> + sizeof(csd->device_id));
> + memcpy(csd->vendor, sd[j].vendor,
> + sizeof(csd->vendor));
> + memcpy(csd->model, sd[j].model,
> + sizeof(csd->model));
> + memcpy(csd->revision, sd[j].revision,
> + sizeof(csd->revision));
didn't cciss_scsi_add_entry() just do that? I didn't look too closely,
so maybe it didn't, and we're taking two copies for some reason.
Please double-check?
> } else /* device is same as it ever was, */
> i++; /* so just move along. */
> }
>
> ...
>
> +/* Get the device id from inquiry page 0x83 */
> +static int cciss_scsi_get_device_id(ctlr_info_t *c, unsigned char *scsi3addr,
> + unsigned char *device_id, int buflen)
> +{
> + int rc;
> + unsigned char *buf;
> +
> + if (buflen > 16)
> + buflen = 16;
> + buf = kzalloc(64, GFP_KERNEL);
> + if (!buf)
> + return -1;
> + rc = cciss_scsi_do_inquiry(c, scsi3addr, 0x83, buf, 64);
> + if (rc == 0)
> + memcpy(device_id, &buf[8], buflen);
> + kfree(buf);
> + return rc != 0;
> +}
The caller ignores the error return from this function. I guess that's
reasonable, and kzalloc() will have spewed a bit warning anyway..
> static int
> cciss_scsi_do_report_phys_luns(ctlr_info_t *c,
> ReportLunData_struct *buf, int bufsize)
> @@ -1142,25 +1184,21 @@ cciss_update_non_disk_devices(int cntl_n
> ctlr_info_t *c;
> __u32 num_luns=0;
> unsigned char *ch;
> - /* unsigned char found[CCISS_MAX_SCSI_DEVS_PER_HBA]; */
> - struct cciss_scsi_dev_t currentsd[CCISS_MAX_SCSI_DEVS_PER_HBA];
> + struct cciss_scsi_dev_t *currentsd, *this_device;
> int ncurrent=0;
> int reportlunsize = sizeof(*ld_buff) + CISS_MAX_PHYS_LUN * 8;
> int i;
>
> c = (ctlr_info_t *) hba[cntl_num];
> ld_buff = kzalloc(reportlunsize, GFP_KERNEL);
> - if (ld_buff == NULL) {
> - printk(KERN_ERR "cciss: out of memory\n");
> - return;
> - }
> inq_buff = kmalloc(OBDR_TAPE_INQ_SIZE, GFP_KERNEL);
> - if (inq_buff == NULL) {
> - printk(KERN_ERR "cciss: out of memory\n");
> - kfree(ld_buff);
> - return;
> + currentsd = kzalloc(sizeof(*currentsd) *
> + (CCISS_MAX_SCSI_DEVS_PER_HBA+1), GFP_KERNEL);
That's kcalloc().
> + if (ld_buff == NULL || inq_buff == NULL || currentsd == NULL) {
> + printk(KERN_ERR "cciss: out of memory\n");
> + goto out;
> }
> -
> + this_device = ¤tsd[CCISS_MAX_SCSI_DEVS_PER_HBA];
> if (cciss_scsi_do_report_phys_luns(c, ld_buff, reportlunsize) == 0) {
> ch = &ld_buff->LUNListLength[0];
> num_luns = ((ch[0]<<24) | (ch[1]<<16) | (ch[2]<<8) | ch[3]) / 8;
> @@ -1179,23 +1217,34 @@ cciss_update_non_disk_devices(int cntl_n
>
> ...
>
> + for (i = 0; i < num_luns; i++) {
> /* for each physical lun, do an inquiry */
> if (ld_buff->LUN[i][3] & 0xC0) continue;
> memset(inq_buff, 0, OBDR_TAPE_INQ_SIZE);
> memcpy(&scsi3addr[0], &ld_buff->LUN[i][0], 8);
>
> - if (cciss_scsi_do_inquiry(hba[cntl_num], scsi3addr, inq_buff,
> - (unsigned char) OBDR_TAPE_INQ_SIZE) != 0) {
> + if (cciss_scsi_do_inquiry(hba[cntl_num], scsi3addr, 0, inq_buff,
> + (unsigned char) OBDR_TAPE_INQ_SIZE) != 0)
> /* Inquiry failed (msg printed already) */
> - devtype = 0; /* so we will skip this device. */
> - } else /* what kind of device is this? */
> - devtype = (inq_buff[0] & 0x1f);
> + continue; /* so we will skip this device. */
> +
> + this_device->devtype = (inq_buff[0] & 0x1f);
> + this_device->bus = -1;
> + this_device->target = -1;
> + this_device->lun = -1;
> + memcpy(this_device->scsi3addr, scsi3addr, 8);
> + memcpy(this_device->vendor, &inq_buff[8],
> + sizeof(this_device->vendor));
> + memcpy(this_device->model, &inq_buff[16],
> + sizeof(this_device->model));
> + memcpy(this_device->revision, &inq_buff[32],
> + sizeof(this_device->revision));
> + memset(this_device->device_id, 0,
> + sizeof(this_device->device_id));
> + cciss_scsi_get_device_id(hba[cntl_num], scsi3addr,
> + this_device->device_id, sizeof(this_device->device_id));
Quite a bit of duplication. Perhaps a helper function is needed somewhere.
>
> - switch (devtype)
> + switch (this_device->devtype)
> {
> case 0x05: /* CD-ROM */ {
>
>
> ...
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [patch] cciss: Fix cciss SCSI rescan code to better notice device changes.
2008-09-20 0:19 ` Andrew Morton
@ 2008-10-01 15:12 ` scameron
0 siblings, 0 replies; 3+ messages in thread
From: scameron @ 2008-10-01 15:12 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, mike.miller, axboe
On Fri, Sep 19, 2008 at 05:19:31PM -0700, Andrew Morton wrote:
> On Thu, 18 Sep 2008 15:07:44 -0500
[...]
> y'know, it would be a lot simpler to just do
>
> *sd = *device;
>
> a few lines up, before assigning sd->bus/target/lun.
>
> Putting these:
>
> @@ -66,6 +66,10 @@ struct cciss_scsi_dev_t {
> int devtype;
> int bus, target, lun; /* as presented to the OS */
> unsigned char scsi3addr[8]; /* as presented to the HW */
> + unsigned char device_id[16]; /* from inquiry pg. 0x83 */
> + unsigned char vendor[8]; /* bytes 8-15 of inquiry data */
> + unsigned char model[16]; /* bytes 16-31 of inquiry data */
> + unsigned char revision[4]; /* bytes 32-35 of inquiry data */
> };
>
> into a separate nested structure might make that neater still.
>
[... and a bunch of other helpful comments. ]
Sorry for the slow reply, I've been on vacation.
Thanks for looking over my patch. I'll try to make a
patch to incorporate your suggestions.
> didn't cciss_scsi_add_entry() just do that? I didn't look too closely,
> so maybe it didn't, and we're taking two copies for some reason.
> Please double-chec
I think you're right about that, good catch.
-- steve
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-10-01 15:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-09-18 20:07 [patch] cciss: Fix cciss SCSI rescan code to better notice device changes scameron
2008-09-20 0:19 ` Andrew Morton
2008-10-01 15:12 ` scameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®