On Jan 6, 2012, at 7:29 AM, Alan Stern wrote:
> On Thu, 5 Jan 2012, Alan Stern wrote:
>
>> Sure, that ought to work. I'll update the patch.
>>
>>> The flip side is to go forward with your patch as planned, and if the
>>> VPD issue becomes a problem, then add in a separate "don't get VPD
>>> pages" flag later. Unless this VPD data is *really* important, I
>>> would be inclined to not issue the command. A userspace tool could
>>> always issue the command if the user wanted the info, and thus they
>>> accept the responsibility for killing their device (until reset).
>>
>> I don't know how important that information is. It might matter for
>> things like releasing regions on SSDs. If necessary, we can always add
>> a new flag like you suggest.
>
> Here's the updated patch. I decided to add a separate flag for the VPD
> pages now; that way we can easily change the logic for setting that
> flag in usb-storage. Together the two new flags should cover all the
> important things that forcing the level to SCSI_2 did.
>
> (Except for one thing: User programs that look at the ANSI SCSI
> revision field in /proc/scsi/scsi will now see the correct value. I
> doubt that any programs rely on that, however -- it's not easy to do --
> they are much more likely to issue their own INQUIRY command to get the
> value directly.)
>
> When Perry verifies that this still works, I'll split it up into about
> three separate patches and submit it via the SCSI tree, without marking
> it for -stable.
When do you think you will be able to split it into three pieces?
One of my problems (my last!) is that I'm wanting to backport it to 2.6.31 for
an embedded ARM NAS device, and the USB_SC_UFI definition (and all its
semantics) isn't defined way back then. I'm not sure what to include and what
not to in the below patch. This is your third proposed patch, the second seems
to work fine, but this one doesn't compile due to the missing definition from 2010.
Unfortunately, I'm stuck back in 2008 with this custom-optimized 2.6.31. Sigh.
What do I need to figure out? Seems like I need to excise the part that worries
about the floppy (USB_SC_UFI), but maybe I should leave in the VPD stuff?
What's going to bite me, most likely?
> Alan Stern
Thanks!
Perry Wagle
>
>
>
> Index: usb-3.2/include/scsi/scsi_device.h
> ===================================================================
> --- usb-3.2.orig/include/scsi/scsi_device.h
> +++ usb-3.2/include/scsi/scsi_device.h
> @@ -136,6 +136,7 @@ struct scsi_device {
> unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
> unsigned skip_ms_page_8:1; /* do not use MODE SENSE page 0x08 */
> unsigned skip_ms_page_3f:1; /* do not use MODE SENSE page 0x3f */
> + unsigned skip_vpd_pages:1; /* do not read VPD pages */
> unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
> unsigned no_start_on_add:1; /* do not issue start on add */
> unsigned allow_restart:1; /* issue START_UNIT in error handler */
> @@ -247,8 +248,10 @@ struct scsi_target {
> unsigned int single_lun:1; /* Indicates we should only
> * allow I/O to one of the luns
> * for the device at a time. */
> - unsigned int pdt_1f_for_no_lun; /* PDT = 0x1f */
> - /* means no lun present */
> + unsigned int pdt_1f_for_no_lun:1; /* PDT = 0x1f
> + * means no lun present. */
> + unsigned int no_report_luns:1; /* Don't use
> + * REPORT LUNS for scanning. */
> /* commands actually active on LLD. protected by host lock. */
> unsigned int target_busy;
> /*
> Index: usb-3.2/drivers/scsi/scsi_scan.c
> ===================================================================
> --- usb-3.2.orig/drivers/scsi/scsi_scan.c
> +++ usb-3.2/drivers/scsi/scsi_scan.c
> @@ -1295,6 +1295,7 @@ EXPORT_SYMBOL(int_to_scsilun);
> * LUNs even if it's older than SCSI-3.
> * If BLIST_NOREPORTLUN is set, return 1 always.
> * If BLIST_NOLUN is set, return 0 always.
> + * If starget->no_report_luns is set, return 1 always.
> *
> * Return:
> * 0: scan completed (or no memory, so further scanning is futile)
> @@ -1321,6 +1322,7 @@ static int scsi_report_lun_scan(struct s
> * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
> * Also allow SCSI-2 if BLIST_REPORTLUN2 is set and host adapter does
> * support more than 8 LUNs.
> + * Don't attempt if the target doesn't support REPORT LUNS.
> */
> if (bflags & BLIST_NOREPORTLUN)
> return 1;
> @@ -1332,6 +1334,8 @@ static int scsi_report_lun_scan(struct s
> return 1;
> if (bflags & BLIST_NOLUN)
> return 0;
> + if (starget->no_report_luns)
> + return 1;
>
> if (!(sdev = scsi_device_lookup_by_target(starget, 0))) {
> sdev = scsi_alloc_sdev(starget, 0, NULL);
> Index: usb-3.2/drivers/scsi/sd.c
> ===================================================================
> --- usb-3.2.orig/drivers/scsi/sd.c
> +++ usb-3.2/drivers/scsi/sd.c
> @@ -2341,7 +2341,7 @@ static int sd_try_extended_inquiry(struc
> * some USB ones crash on receiving them, and the pages
> * we currently ask for are for SPC-3 and beyond
> */
> - if (sdp->scsi_level > SCSI_SPC_2)
> + if (sdp->scsi_level > SCSI_SPC_2 && !sdp->skip_vpd_pages)
> return 1;
> return 0;
> }
> Index: usb-3.2/drivers/usb/storage/scsiglue.c
> ===================================================================
> --- usb-3.2.orig/drivers/usb/storage/scsiglue.c
> +++ usb-3.2/drivers/usb/storage/scsiglue.c
> @@ -78,8 +78,6 @@ static const char* host_info(struct Scsi
>
> static int slave_alloc (struct scsi_device *sdev)
> {
> - struct us_data *us = host_to_us(sdev->host);
> -
> /*
> * Set the INQUIRY transfer length to 36. We don't use any of
> * the extra data and many devices choke if asked for more or
> @@ -104,18 +102,6 @@ static int slave_alloc (struct scsi_devi
> */
> blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
>
> - /*
> - * The UFI spec treates the Peripheral Qualifier bits in an
> - * INQUIRY result as reserved and requires devices to set them
> - * to 0. However the SCSI spec requires these bits to be set
> - * to 3 to indicate when a LUN is not present.
> - *
> - * Let the scanning code know if this target merely sets
> - * Peripheral Device Type to 0x1f to indicate no LUN.
> - */
> - if (us->subclass == USB_SC_UFI)
> - sdev->sdev_target->pdt_1f_for_no_lun = 1;
> -
> return 0;
> }
>
> @@ -197,6 +183,9 @@ static int slave_configure(struct scsi_d
> * page x08, so we will skip it. */
> sdev->skip_ms_page_8 = 1;
>
> + /* Some devices don't handle VPD pages correctly */
> + sdev->skip_vpd_pages = 1;
> +
> /* Some disks return the total number of blocks in response
> * to READ CAPACITY rather than the highest block number.
> * If this device makes that mistake, tell the sd driver. */
> @@ -217,16 +206,6 @@ static int slave_configure(struct scsi_d
> if (sdev->scsi_level > SCSI_SPC_2)
> us->fflags |= US_FL_SANE_SENSE;
>
> - /* Some devices report a SCSI revision level above 2 but are
> - * unable to handle the REPORT LUNS command (for which
> - * support is mandatory at level 3). Since we already have
> - * a Get-Max-LUN request, we won't lose much by setting the
> - * revision level down to 2. The only devices that would be
> - * affected are those with sparse LUNs. */
> - if (sdev->scsi_level > SCSI_2)
> - sdev->sdev_target->scsi_level =
> - sdev->scsi_level = SCSI_2;
> -
> /* USB-IDE bridges tend to report SK = 0x04 (Non-recoverable
> * Hardware Error) when any low-level error occurs,
> * recoverable or not. Setting this flag tells the SCSI
> @@ -283,6 +262,33 @@ static int slave_configure(struct scsi_d
> return 0;
> }
>
> +static int target_alloc(struct scsi_target *starget)
> +{
> + struct us_data *us = host_to_us(dev_to_shost(starget->dev.parent));
> +
> + /*
> + * Some USB drives don't support REPORT LUNS, even though they
> + * report a SCSI revision level above 2. Tell the SCSI layer
> + * not to issue that command; it will perform a normal sequential
> + * scan instead.
> + */
> + starget->no_report_luns = 1;
> +
> + /*
> + * The UFI spec treats the Peripheral Qualifier bits in an
> + * INQUIRY result as reserved and requires devices to set them
> + * to 0. However the SCSI spec requires these bits to be set
> + * to 3 to indicate when a LUN is not present.
> + *
> + * Let the scanning code know if this target merely sets
> + * Peripheral Device Type to 0x1f to indicate no LUN.
> + */
> + if (us->subclass == USB_SC_UFI)
> + starget->pdt_1f_for_no_lun = 1;
> +
> + return 0;
> +}
> +
> /* queue a command */
> /* This is always called with scsi_lock(host) held */
> static int queuecommand_lck(struct scsi_cmnd *srb,
> @@ -546,6 +552,7 @@ struct scsi_host_template usb_stor_host_
>
> .slave_alloc = slave_alloc,
> .slave_configure = slave_configure,
> + .target_alloc = target_alloc,
>
> /* lots of sg segments can be handled */
> .sg_tablesize = SCSI_MAX_SG_CHAIN_SEGMENTS,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
