Matthew Dharm wrote: > On Mon, Aug 04, 2008 at 11:51:57AM -0400, Alan Stern wrote: >> I tend to agree with Matt that it would be best to have the higher >> layers tell usb-storage exactly how much sense data they want to get. >> The problem is that these higher layers would not know about the >> 18-byte restriction on many earlier USB devices, so usb-storage would >> probably end up needing to make its own dynamic decisions anyway. > > Why not handle this like other fields in the struct scsi_device? Let the > core initialize it with a value, then override it in the slave_configure() > routine via unusual_devs.h flag OR via userspace command to the SCSI core? > > Matt > There was a correct and simple patch proposed that fixes this problem the right way: http://marc.info/?l=linux-usb&m=121762869915609&w=2 Doug could you please test this patch to see if it fixes your device? scsi-core already gives drivers complete control on what sense_size to fetch. It is a parameter to the scsi_eh_prep_cmnd() call. So no need for slave_configure(), default value, and all that loop. Matthieu castet <castet.matthieu@xxxxxxx> wrote ... > we have got report from 2 users that scsi sat (to do ata passthrough) was not fully \ > working with some usb bridge (maxtor and western digital). Everything works except \ > the sense result was incomplete [1]. > > > After some investigation, they need a sense size different of 18 (at least 22). [2] > > Because some devices can crash if the sense size is different of 18, in order to not \ > break these devices a flag that is set either by unusual entries, either for spc3 or \ > latter devices is used. > > > Signed-off-by: Matthieu CASTET <castet.matthieu@xxxxxxx> Reviewed-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx> > > > > [1] > http://thread.gmane.org/gmane.linux.utilities.smartmontools/5613 > > [2] > http://thread.gmane.org/gmane.linux.utilities.smartmontools/5721 > Index: linux-2.6/drivers/usb/storage/scsiglue.c > =================================================================== > --- linux-2.6.orig/drivers/usb/storage/scsiglue.c 2008-08-02 00:07:24.000000000 +0200 > +++ linux-2.6/drivers/usb/storage/scsiglue.c 2008-08-02 00:07:37.000000000 +0200 > @@ -170,6 +170,10 @@ > if (us->fflags & US_FL_CAPACITY_HEURISTICS) > sdev->guess_capacity = 1; > > + /* assume SPC3 or latter device support sense size different of 18 */ > + 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 > Index: linux-2.6/drivers/usb/storage/transport.c > =================================================================== > --- linux-2.6.orig/drivers/usb/storage/transport.c 2008-08-02 00:07:24.000000000 \ > +0200 > +++ linux-2.6/drivers/usb/storage/transport.c 2008-08-02 00:07:37.000000000 +0200 > @@ -595,10 +595,15 @@ > if (need_auto_sense) { > int temp_result; > struct scsi_eh_save ses; > + int sense_size = US_SENSE_SIZE; > + > + /* device support and need bigger sense buffer */ > + if (us->fflags & US_FL_SANE_SENSE) > + sense_size = ~0; > > US_DEBUGP("Issuing auto-REQUEST_SENSE\n"); > > - scsi_eh_prep_cmnd(srb, &ses, NULL, 0, US_SENSE_SIZE); > + scsi_eh_prep_cmnd(srb, &ses, NULL, 0, sense_size); > > /* FIXME: we must do the protocol translation here */ > if (us->subclass == US_SC_RBC || us->subclass == US_SC_SCSI || > Index: linux-2.6/drivers/usb/storage/unusual_devs.h > =================================================================== > --- linux-2.6.orig/drivers/usb/storage/unusual_devs.h 2008-08-02 00:07:24.000000000 \ > +0200 > +++ linux-2.6/drivers/usb/storage/unusual_devs.h 2008-08-02 00:07:37.000000000 +0200 > @@ -1791,6 +1791,18 @@ > US_SC_DEVICE, US_PR_DEVICE, NULL, > US_FL_CAPACITY_HEURISTICS), > > +UNUSUAL_DEV( 0x0d49, 0x7310, 0x0000, 0x9999, > + "Maxtor", > + "usb sata bridge", > + US_SC_DEVICE, US_PR_DEVICE, NULL, > + US_FL_SANE_SENSE ), > + > +UNUSUAL_DEV( 0x1058, 0x0704, 0x0000, 0x9999, > + "Western Digital", > + "External HDD", > + US_SC_DEVICE, US_PR_DEVICE, NULL, > + US_FL_SANE_SENSE ), > + > /* Control/Bulk transport for all SubClass values */ > USUAL_DEV(US_SC_RBC, US_PR_CB, USB_US_TYPE_STOR), > USUAL_DEV(US_SC_8020, US_PR_CB, USB_US_TYPE_STOR), > Index: linux-2.6/include/linux/usb_usual.h > =================================================================== > --- linux-2.6.orig/include/linux/usb_usual.h 2008-08-02 00:07:24.000000000 +0200 > +++ linux-2.6/include/linux/usb_usual.h 2008-08-02 00:07:37.000000000 +0200 > @@ -52,7 +52,9 @@ > US_FLAG(MAX_SECTORS_MIN,0x00002000) \ > /* Sets max_sectors to arch min */ \ > US_FLAG(BULK_IGNORE_TAG,0x00004000) \ > - /* Ignore tag mismatch in bulk operations */ > + /* Ignore tag mismatch in bulk operations */ \ > + US_FLAG(SANE_SENSE,0x00008000) \ > + /* Need a sense size different of 18 for some cmd (SAT) */ > > > #define US_FLAG(name, value) US_FL_##name = value , > > I recommend this patch. It does exactly what's needed with minimum risk it is even maybe over protective, with the white list. Perhaps it should be turned to a black list instead. The old broken devices been an extincting exception. Boaz -- 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
