Skip to contentSkip to conversation

scsi scan bug when peripheral qualifier of 3 is returned

DISCUSSION Mike Christie 2 participants 7 messages Jan 4, 2006 – Jan 5, 2006 SCSI

lore
Share & tools
lore

Mike Christie

author
#0 lore raw

It looks like in previous kernels scsi_scan_target would pass scsi_sequential_lun_scan BLIST_SPARSELUN.

} else if (res == SCSI_SCAN_TARGET_PRESENT) {
/*
* There's a target here, but lun 0 is offline so we
* can't use the report_lun scan. Fall back to a
* sequential lun scan with a bflags of SPARSELUN and
* a default scsi level of SCSI_2
*/
scsi_sequential_lun_scan(starget, BLIST_SPARSELUN,

And in the current kernel we pass it bflags.

/*
* The REPORT LUN did not scan the target,
* do a sequential scan.
*/
scsi_sequential_lun_scan(starget, bflags,

But for SCSI_SCAN_TARGET_PRESENT bflags is not set. Is the correct fix to move where bflagsp gets set in scsi_probe_and_add_lun so that it gets set for the SCSI_SCAN_TARGET_PRESENT case, or should __scsi_scan_target be passing scsi_sequential_lun_scan and possibly scsi_report_lun_scan some default bflags values?

#1 lore raw
Quoted 5 lines · Mike Christie

But for SCSI_SCAN_TARGET_PRESENT bflags is not set. Is the correct fix to move where bflagsp gets set in scsi_probe_and_add_lun so that it gets set for the SCSI_SCAN_TARGET_PRESENT case, or should __scsi_scan_target be passing scsi_sequential_lun_scan and possibly scsi_report_lun_scan some default bflags values?

It looks OK to me as-is, since bflags is also passed to and set in scsi_probe_lun(), right?

-- Patrick Mansfield

author
#2 lore raw
Quoted 9 lines · Patrick Mansfield
Quoted 1 line · Mike Christie

But for SCSI_SCAN_TARGET_PRESENT bflags is not set. Is the correct fix to move where bflagsp gets set in scsi_probe_and_add_lun so that it gets set for the SCSI_SCAN_TARGET_PRESENT case, or should __scsi_scan_target be passing scsi_sequential_lun_scan and possibly scsi_report_lun_scan some default bflags values?

It looks OK to me as-is, since bflags is also passed to and set in scsi_probe_lun(), right?

A blagfs variable gets set but it is not the same one passed into scsi_probe_and_add_lun.

static int scsi_probe_and_add_lun(struct scsi_target *starget,
uint lun, int *bflagsp,

{
        struct scsi_device *sdev;
        unsigned char *result;
        int bflags,

scsi_probe_and_add_lun gets a *bflagsp passed to it as a function arg, but then also decalres a bflags variable itself. It then passes scsi_probe_lun() the bflags it declared and does this

        res = scsi_add_lun(sdev, result, &bflags);
        if (res == SCSI_SCAN_LUN_PRESENT) {
                if (bflags & BLIST_KEY) {
                        sdev->lockable = 0;
                        scsi_unlock_floptical(sdev, result);
                }
                if (bflagsp)
                        *bflagsp = bflags;
        }

so *bflagsp pointer only gets set if SCSI_SCAN_LUN_PRESENT was returned by scsi_add_lun. For SCSI_SCAN_TARGET_PRESENT we do not even get to scsi_add_lun, so for this case *bflagsp never gets set and __scsi_scan_target gets zero. Previously, __scsi_scan_target would just pass scsi_sequential_lun_scan the sparse blist flag, but now it passes zero.

#3 lore raw
Quoted 11 lines · Mike Christie
Quoted 10 lines · Patrick Mansfield
Quoted 5 lines · Mike Christie

But for SCSI_SCAN_TARGET_PRESENT bflags is not set. Is the correct fix to move where bflagsp gets set in scsi_probe_and_add_lun so that it gets set for the SCSI_SCAN_TARGET_PRESENT case, or should __scsi_scan_target be passing scsi_sequential_lun_scan and possibly scsi_report_lun_scan some default bflags values?

Quoted 6 lines
Quoted 3 lines

It looks OK to me as-is, since bflags is also passed to and set in scsi_probe_lun(), right?

A blagfs variable gets set but it is not the same one passed into scsi_probe_and_add_lun.

Oh ... good catch there.

Quoted 24 lines

static int scsi_probe_and_add_lun(struct scsi_target *starget,
uint lun, int *bflagsp,

{
        struct scsi_device *sdev;
        unsigned char *result;
        int bflags,

scsi_probe_and_add_lun gets a *bflagsp passed to it as a function arg, but then also decalres a bflags variable itself. It then passes scsi_probe_lun() the bflags it declared and does this

        res = scsi_add_lun(sdev, result, &bflags);
        if (res == SCSI_SCAN_LUN_PRESENT) {
                if (bflags & BLIST_KEY) {
                        sdev->lockable = 0;
                        scsi_unlock_floptical(sdev, result);
                }
                if (bflagsp)
                        *bflagsp = bflags;
        }
Quoted 5 lines

so *bflagsp pointer only gets set if SCSI_SCAN_LUN_PRESENT was returned by scsi_add_lun. For SCSI_SCAN_TARGET_PRESENT we do not even get to scsi_add_lun, so for this case *bflagsp never gets set and __scsi_scan_target gets zero. Previously, __scsi_scan_target would just pass scsi_sequential_lun_scan the sparse blist flag, but now it passes zero.

So yes we should always set *bflagsp. i.e.:

drivers/scsi/scsi_scan.c renamed from drivers/scsi/orig-scsi_scan.c +2 -2
@@ -891,13 +891,13 @@ static int scsi_probe_and_add_lun(struct
 	}
 
 	res = scsi_add_lun(sdev, result, &bflags);
+	if (bflagsp)
+		*bflagsp = bflags;
 	if (res == SCSI_SCAN_LUN_PRESENT) {
 		if (bflags & BLIST_KEY) {
 			sdev->lockable = 0;
 			scsi_unlock_floptical(sdev, result);
 		}
-		if (bflagsp)
-			*bflagsp = bflags;
 	}
 
  out_free_result:

Also ... it looks like we should just pass bflags not &bflags to scsi_add_lun(), since scsi_add_lun() does not modifies bflags.

-- Patrick Mansfield

author
#4 lore raw

drivers/scsi/scsi_scan.c static int scsi_probe_and_add_lun(struct · line 895

Quoted 10 lines · Patrick Mansfield

So yes we should always set *bflagsp. i.e.:

drivers/scsi/scsi_scan.c renamed from drivers/scsi/orig-scsi_scan.c +2 -0
@@ -891,13 +891,13 @@ static int scsi_probe_and_add_lun(struct
 	}
 
 	res = scsi_add_lun(sdev, result, &bflags);
+	if (bflagsp)
+		*bflagsp = bflags;

For SCSI_SCAN_TARGET_PRESENT, we never get this far. We bail out a couple of lines above in the check for PQ=3. Also, for some boxes like Clarions we have this in devinfo:

        {"DGC", "RAID", NULL, BLIST_SPARSELUN}, /* Dell PV 650F, 
storage on LUN 0 */
        {"DGC", "DISK", NULL, BLIST_SPARSELUN},

But LUN0 shows up as DGC/LUNZ. When we used to hardcode BLIST_SPARSELUN as a argument to scsi_sequential_lun_scan, we could find a least 8 devices but now we hit this in scsi_sequential_lun_scan:

       /*
         * If not sparse lun and no device attached at LUN 0 do not scan
         * any further.
         */
        if (!sparse_lun && (lun0_res != SCSI_SCAN_LUN_PRESENT))
                return;

Does IBM's fastt/DS or other boxes have this problem too? For Clarions do I just have to add a DGC/LUNZ entry to the devinfo list?

#5 lore raw
Quoted 10 lines · Mike Christie
Quoted 9 lines · Patrick Mansfield

So yes we should always set *bflagsp. i.e.:

--- linux-2.6.15/drivers/scsi/orig-scsi_scan.c 2006-01-02
21:52:12.000000000 -0800
+++ linux-2.6.15/drivers/scsi/scsi_scan.c 2006-01-04
10:58:36.000000000 -0800

(quoted diff) +0 -0
@@ -891,13 +891,13 @@ static int scsi_probe_and_add_lun(struct
 	}
Quoted 7 lines

Review static int scsi_probe_and_add_lun(struct · line 895

Quoted 3 lines
(quoted diff) +2 -0
 	res = scsi_add_lun(sdev, result, &bflags);
+	if (bflagsp)
+		*bflagsp = bflags;

For SCSI_SCAN_TARGET_PRESENT, we never get this far. We bail out a couple of lines above in the check for PQ=3. Also, for some boxes like

OK :-/

Then like this?

drivers/scsi/scsi_scan.c renamed from drivers/scsi/orig-scsi_scan.c +2 -2
@@ -868,6 +868,8 @@ static int scsi_probe_and_add_lun(struct
 
 	if (scsi_probe_lun(sdev, result, result_len, &bflags))
 		goto out_free_result;
+	if (bflagsp)
+		*bflagsp = bflags;
 
 	/*
 	 * result contains valid SCSI INQUIRY data.

@@ -896,8 +898,6 @@ static int scsi_probe_and_add_lun(struct
 			sdev->lockable = 0;
 			scsi_unlock_floptical(sdev, result);
 		}
-		if (bflagsp)
-			*bflagsp = bflags;
 	}
 
  out_free_result:
Quoted 19 lines

Clarions we have this in devinfo:

        {"DGC", "RAID", NULL, BLIST_SPARSELUN}, /* Dell PV 650F, 
storage on LUN 0 */
        {"DGC", "DISK", NULL, BLIST_SPARSELUN},

But LUN0 shows up as DGC/LUNZ. When we used to hardcode BLIST_SPARSELUN as a argument to scsi_sequential_lun_scan, we could find a least 8 devices but now we hit this in scsi_sequential_lun_scan:

       /*
         * If not sparse lun and no device attached at LUN 0 do not scan
         * any further.
         */
        if (!sparse_lun && (lun0_res != SCSI_SCAN_LUN_PRESENT))
                return;
Quoted 1 line

Does IBM's fastt/DS or other boxes have this problem too?

For fastt, DS and other newer storage, we (likely) always use REPORT LUN and so would never hit that code.

Quoted 2 lines

For Clarions
do I just have to add a DGC/LUNZ entry to the devinfo list?

If they do not support REPORT LUN, yes (AFAIR/CT, my brain is slow ...).

Also, on some storage boxes, you can conditionaly set the scsi level too. You could try changing that and/or also add a blist BLIST_REPORTLUN2 for them to devinfo, or just use the sg report lun to test it.

Don't forget you can dynamically add devinfo values via modparam and /proc/scsi.

-- Patrick Mansfield

author
#6 lore raw
Quoted 1 line · Patrick Mansfield

Then like this?

Yep, this works for our setup. Thanks.

Quoted 22 lines
drivers/scsi/scsi_scan.c renamed from drivers/scsi/orig-scsi_scan.c +2 -2
@@ -868,6 +868,8 @@ static int scsi_probe_and_add_lun(struct
 
 	if (scsi_probe_lun(sdev, result, result_len, &bflags))
 		goto out_free_result;
+	if (bflagsp)
+		*bflagsp = bflags;
 
 	/*
 	 * result contains valid SCSI INQUIRY data.

@@ -896,8 +898,6 @@ static int scsi_probe_and_add_lun(struct
 			sdev->lockable = 0;
 			scsi_unlock_floptical(sdev, result);
 		}
-		if (bflagsp)
-			*bflagsp = bflags;
 	}
 
  out_free_result:

.....

Quoted 4 lines

Don't forget you can dynamically add devinfo values via modparam and /proc/scsi.

I did not know this. I was looking for a writable proc file for some reason. Thanks.