[PATCH] SCSI core: better initialization for sdev->scsi_level

[PATCH] SCSI core: better initialization for sdev->scsi_level

[Alan Stern]

Both scsi_device and scsi_target include a scsi_level field, and the
SCSI core makes a half-hearted effort to keep the values equal.
Ultimately this effort may be doomed, since as far as I know there is
no reason why all LUNs in a target must report the same "ANSI-approved
version" number.  But for the most part it should work okay.

This patch (as834) changes the SCSI core so that after the first LUN
has been probed and the target's scsi_level is known, further LUNs
default to the target's scsi_level and not to SCSI_2.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

This patch will affect the CDB in INQUIRY commands sent to LUNs above 0 
when LUN-0 reports a scsi_level of 0; the LUN bits will no longer be set 
in the second byte of the CDB.  This is as it should be.  Nevertheless, 
it's possible that some wacky device might be adversely affected.  I doubt 
anyone will complain...

Alan Stern


Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -382,6 +382,7 @@ static struct scsi_target *scsi_alloc_ta
 	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
 	starget->state = STARGET_RUNNING;
+	starget->scsi_level = SCSI_2;
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
Index: usb-2.6/drivers/scsi/scsi_sysfs.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_sysfs.c
+++ usb-2.6/drivers/scsi/scsi_sysfs.c
@@ -922,7 +922,7 @@ void scsi_sysfs_device_initialize(struct
 	snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
 		 "%d:%d:%d:%d", sdev->host->host_no,
 		 sdev->channel, sdev->id, sdev->lun);
-	sdev->scsi_level = SCSI_2;
+	sdev->scsi_level = starget->scsi_level;
 	transport_setup_device(&sdev->sdev_gendev);
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_add_tail(&sdev->same_target_siblings, &starget->devices);

Re: [PATCH] SCSI core: better initialization for sdev->scsi_level

[Douglas Gilbert]

Alan Stern wrote:
> Both scsi_device and scsi_target include a scsi_level field, and the
> SCSI core makes a half-hearted effort to keep the values equal.
> Ultimately this effort may be doomed, since as far as I know there is
> no reason why all LUNs in a target must report the same "ANSI-approved
> version" number.  But for the most part it should work okay.
> 
> This patch (as834) changes the SCSI core so that after the first LUN
> has been probed and the target's scsi_level is known, further LUNs
> default to the target's scsi_level and not to SCSI_2.

Alan,
Umm, scsi_level is a mangled value derived from the
"version" field in an INQUIRY standard response. As
such it is per logical unit ***. There is nothing to stop
a single target (especially if it is a bridge that
maps targets at the remote end to luns) having a wide
variety of lus with different "version" values (and
different peripheral device types).

IMO scsi_level should only be per lu which means it
should only exist in the scsi_device structure.
If the scsi mid level was really advanced it could
track the "version" value in the INQUIRY response to
"well known" logical units (see spc4r08.pdf section 8)
because these really are per target. I don't expect
that to happen any time soon (and there wouldn't be
much benefit).

So the existing code seems broken but I'm not sure
your patch advances things.


*** this statement assumes the "peripheral qualifier"
    field is 0, otherwise there is no real lu at the
    given lun

Doug Gilbert


> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> This patch will affect the CDB in INQUIRY commands sent to LUNs above 0 
> when LUN-0 reports a scsi_level of 0; the LUN bits will no longer be set 
> in the second byte of the CDB.  This is as it should be.  Nevertheless, 
> it's possible that some wacky device might be adversely affected.  I doubt 
> anyone will complain...
> 
> Alan Stern
> 
> 
> Index: usb-2.6/drivers/scsi/scsi_scan.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi_scan.c
> +++ usb-2.6/drivers/scsi/scsi_scan.c
> @@ -382,6 +382,7 @@ static struct scsi_target *scsi_alloc_ta
>  	INIT_LIST_HEAD(&starget->siblings);
>  	INIT_LIST_HEAD(&starget->devices);
>  	starget->state = STARGET_RUNNING;
> +	starget->scsi_level = SCSI_2;
>   retry:
>  	spin_lock_irqsave(shost->host_lock, flags);
>  
> Index: usb-2.6/drivers/scsi/scsi_sysfs.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/scsi_sysfs.c
> +++ usb-2.6/drivers/scsi/scsi_sysfs.c
> @@ -922,7 +922,7 @@ void scsi_sysfs_device_initialize(struct
>  	snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
>  		 "%d:%d:%d:%d", sdev->host->host_no,
>  		 sdev->channel, sdev->id, sdev->lun);
> -	sdev->scsi_level = SCSI_2;
> +	sdev->scsi_level = starget->scsi_level;
>  	transport_setup_device(&sdev->sdev_gendev);
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	list_add_tail(&sdev->same_target_siblings, &starget->devices);
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] SCSI core: better initialization for sdev->scsi_level

[Alan Stern]

On Mon, 8 Jan 2007, Douglas Gilbert wrote:

> Alan Stern wrote:
> > Both scsi_device and scsi_target include a scsi_level field, and the
> > SCSI core makes a half-hearted effort to keep the values equal.
> > Ultimately this effort may be doomed, since as far as I know there is
> > no reason why all LUNs in a target must report the same "ANSI-approved
> > version" number.  But for the most part it should work okay.
> > 
> > This patch (as834) changes the SCSI core so that after the first LUN
> > has been probed and the target's scsi_level is known, further LUNs
> > default to the target's scsi_level and not to SCSI_2.
> 
> Alan,
> Umm, scsi_level is a mangled value derived from the
> "version" field in an INQUIRY standard response. As
> such it is per logical unit ***. There is nothing to stop
> a single target (especially if it is a bridge that
> maps targets at the remote end to luns) having a wide
> variety of lus with different "version" values (and
> different peripheral device types).

Yes, I know.  In fact I said as much in the patch description.

> IMO scsi_level should only be per lu which means it
> should only exist in the scsi_device structure.

That would make sense.  But the current codebase has it in the scsi_target 
structure as well.  I don't know why.  The only place it seems to be used 
is in scsi_report_lun_scan().

> If the scsi mid level was really advanced it could
> track the "version" value in the INQUIRY response to
> "well known" logical units (see spc4r08.pdf section 8)
> because these really are per target. I don't expect
> that to happen any time soon (and there wouldn't be
> much benefit).

That would be the logical thing to do when scsi_level is removed from 
scsi_target.

> So the existing code seems broken but I'm not sure
> your patch advances things.

It does, because it prevents the core from sending an INQUIRY to higher
LUs with the LUN bits in CDB[1] set if LU-0 has already reported
scsi_level==0.

Alan Stern

[PATCH] SCSI core: better initialization for sdev->scsi_level

[Alan Stern]

Both scsi_device and scsi_target include a scsi_level field, and the
SCSI core makes a half-hearted effort to keep the values equal.
Ultimately this effort may be doomed, since as far as I know there is
no reason why all LUNs in a target must report the same "ANSI-approved
version" number.  But for the most part it should work okay.

This patch (as834) changes the SCSI core so that after the first LUN
has been probed and the target's scsi_level is known, further LUNs
default to the target's scsi_level and not to SCSI_2.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

James, this is another resubmission.  Regarding the original submission,
Doug commented that it didn't seem to make things any better than the
current situation.  I pointed out that it did, even though it may not be a
perfect solution.  He didn't reply, which I assume means he agrees.

Alan Stern


Index: usb-2.6/drivers/scsi/scsi_scan.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_scan.c
+++ usb-2.6/drivers/scsi/scsi_scan.c
@@ -382,6 +382,7 @@ static struct scsi_target *scsi_alloc_ta
 	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
 	starget->state = STARGET_RUNNING;
+	starget->scsi_level = SCSI_2;
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
Index: usb-2.6/drivers/scsi/scsi_sysfs.c
===================================================================
--- usb-2.6.orig/drivers/scsi/scsi_sysfs.c
+++ usb-2.6/drivers/scsi/scsi_sysfs.c
@@ -922,7 +922,7 @@ void scsi_sysfs_device_initialize(struct
 	snprintf(sdev->sdev_classdev.class_id, BUS_ID_SIZE,
 		 "%d:%d:%d:%d", sdev->host->host_no,
 		 sdev->channel, sdev->id, sdev->lun);
-	sdev->scsi_level = SCSI_2;
+	sdev->scsi_level = starget->scsi_level;
 	transport_setup_device(&sdev->sdev_gendev);
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_add_tail(&sdev->same_target_siblings, &starget->devices);