Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]

Hi All,

First of all sorry for the somewhat massive cross-posting, I've spend a 
significant amount of time hunting down this bug, and so far the response has 
been less the overwhelming.

The problem is with the HP PSC 1350 (my printer and confirmed by 2 others) and 
atleast also the HP PSC 1610 (confirmed by Guillaume Bedot, in the CC).

The cardreader of the multi function printers will "crash" and from that moment 
on no longer communicate in any sane way, if you try to read the last sector of 
an sdcard* in a read that is more then 1 sector, so trying to read 8 sectors 
starting at sector capicity-8 will crash it, as will reading 2 sectors starting 
at sector capicity-2, however reading the last sector in a one 1 sector read 
will succeed! (* xdcards seem to be fine).

I haven't tried if it will crash on larger then 1 sector writes which include 
the last sector too, I immediately added code to not do that in both the read 
and write paths. I have tested reading and writing the end of the disk with 
this kludge in and it works.

I currently have a somewhat ugly proof of concept patch for this, which adds 
another type of usb-massstorage quirk. When this quirk flag is set, the 
usb-massstorage driver modifies READ_10 and WRITE_10 commands of more then 1 
sector which includes the last sector to become one sector less. I've been told 
by scsi subsystem developers that doing a shorter read / write then requested 
is not a problem, the scsi subsystem is designed to handle getting less then it 
asked for and will send a seperate request for the last sector.

I and 3 others (2 on a PSC 1350 too, one on a PSC1610) have tested this patch 
with success. I'm not asking for this patch to be included to the kernel as is, 
I'm asking for the now known workaround for this to be added to the kernel in 
someway!

Perhaps its an idea to add the posibility to have a scsi command filter 
function / callback to the scsi or usb-massstorage subsystem, and then add a 
mechanism to set this filter depending on usb id's and if added to the scsi 
layer, a mechanism to set it based on scsi device and manufacturer 
identification strings. Such a mechanism might be usefull in the future to work 
around other broken hardware too, and has the added advantage of not having 
todo much changes to the normal code path, keep that readable.

I'm willing to come up with a patch for such a filter mechanism, provided I get 
some pointers where this is best added.

Thanks & Regards,

Hans


p.s.

I've also included the fedora-kernel list in the addressee's because I was 
hoping that maybe someone can take one of these printers to the kernel hackfest 
   in the weekend's fudcon and take a look at this.

[-- Attachment #2: usb-storage-psc1350-v3.patch --]
[-- Type: text/x-patch, Size: 8631 bytes --]

This patch makes the cardreader on the HP PSC 1350 multifunction printer work,
it adds a new US_FL_ flag + handling, and a new UNUSUAL_DEV_MULTI macro to
support multifunction devices.

The difference between this version and the previous v2 version is that this
version also adds an unusual_devs entry for the psc1610, which also needs this
patch for its cardreader to function.

Signed-off-by: Hans de Goede <j.w.r.degoede@hhs.nl>
diff -up linux-2.6.23.9/include/scsi/sd.h.psc1350 linux-2.6.23.9/include/scsi/sd.h
--- linux-2.6.23.9/include/scsi/sd.h.psc1350	2008-01-09 21:58:52.000000000 +0100
+++ linux-2.6.23.9/include/scsi/sd.h	2008-01-09 21:59:06.000000000 +0100
@@ -47,6 +47,8 @@ struct scsi_disk {
 };
 #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,cdev)
 
+#ifdef __SD_C__
+
 static int  sd_revalidate_disk(struct gendisk *disk);
 static void sd_rw_intr(struct scsi_cmnd * SCpnt);
 static int  sd_probe(struct device *);
@@ -61,6 +63,8 @@ static void scsi_disk_release(struct cla
 static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);
 static void sd_print_result(struct scsi_disk *, int);
 
+#endif
+
 #define sd_printk(prefix, sdsk, fmt, a...)				\
         (sdsk)->disk ?							\
 	sdev_printk(prefix, (sdsk)->device, "[%s] " fmt,		\
diff -up linux-2.6.23.9/include/linux/usb_usual.h.psc1350 linux-2.6.23.9/include/linux/usb_usual.h
--- linux-2.6.23.9/include/linux/usb_usual.h.psc1350	2008-01-09 21:58:52.000000000 +0100
+++ linux-2.6.23.9/include/linux/usb_usual.h	2008-01-09 21:59:06.000000000 +0100
@@ -48,7 +48,22 @@
 	US_FLAG(IGNORE_DEVICE,	0x00000800)			\
 		/* Don't claim device */			\
 	US_FLAG(CAPACITY_HEURISTICS,	0x00001000)		\
-		/* sometimes sizes is too big */
+		/* sometimes sizes is too big */		\
+	US_FLAG(LAST_SECTOR_BUG,	0x00002000)		\
+		/* The cardreader of the HP PSC 1350 all-in-one \
+		   printer / scanner / cardreader. Has a nasty	\
+		   bug where the cardreader part will return an	\
+		   error when the last sector of a SD card gets \
+		   read in a read command of more then 1 sector \
+		   once this has happened the cardreader will	\
+		   return nothing but errors. This bug gets	\
+		   triggered on every sd-card insertion with	\
+		   newer kernels, because the partition code	\
+		   now tries to read the last sector. When this \
+		   flag is set a read request of more then 1	\
+		   sector which incompases the last sector will \
+		   be split into 2 requests avoiding the 	\
+		   triggering of this cardreader bug. */
 
 #define US_FLAG(name, value)	US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
diff -up linux-2.6.23.9/drivers/scsi/sd.c.psc1350 linux-2.6.23.9/drivers/scsi/sd.c
--- linux-2.6.23.9/drivers/scsi/sd.c.psc1350	2008-01-09 21:58:52.000000000 +0100
+++ linux-2.6.23.9/drivers/scsi/sd.c	2008-01-09 21:59:06.000000000 +0100
@@ -49,6 +49,8 @@
 #include <linux/mutex.h>
 #include <asm/uaccess.h>
 
+#define __SD_C__
+
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
diff -up linux-2.6.23.9/drivers/usb/storage/libusual.c.psc1350 linux-2.6.23.9/drivers/usb/storage/libusual.c
--- linux-2.6.23.9/drivers/usb/storage/libusual.c.psc1350	2008-01-09 21:58:52.000000000 +0100
+++ linux-2.6.23.9/drivers/usb/storage/libusual.c	2008-01-09 21:59:06.000000000 +0100
@@ -45,6 +45,18 @@ static int usu_probe_thread(void *arg);
 { USB_DEVICE_VER(id_vendor, id_product, bcdDeviceMin,bcdDeviceMax), \
   .driver_info = (flags)|(USB_US_TYPE_STOR<<24) }
 
+/* for multiple interface / function devices */
+#define UNUSUAL_DEV_MULTI(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
+		interfaceClass, vendorName, productName, useProtocol, \
+		useTransport, initFunction, flags) \
+{	.match_flags = USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION| \
+		USB_DEVICE_ID_MATCH_INT_CLASS, \
+	.idVendor = (id_vendor), .idProduct = (id_product), \
+	.bcdDevice_lo = (bcdDeviceMin), .bcdDevice_hi = (bcdDeviceMax), \
+	.bInterfaceClass = (interfaceClass), \
+	.driver_info = (flags) | (USB_US_TYPE_STOR << 24) \
+}
+
 #define USUAL_DEV(useProto, useTrans, useType) \
 { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans), \
   .driver_info = ((useType)<<24) }
@@ -56,6 +68,7 @@ struct usb_device_id storage_usb_ids [] 
 
 #undef USUAL_DEV
 #undef UNUSUAL_DEV
+#undef UNUSUAL_DEV_MULTI
 
 MODULE_DEVICE_TABLE(usb, storage_usb_ids);
 EXPORT_SYMBOL_GPL(storage_usb_ids);
diff -up linux-2.6.23.9/drivers/usb/storage/usb.c.psc1350 linux-2.6.23.9/drivers/usb/storage/usb.c
--- linux-2.6.23.9/drivers/usb/storage/usb.c.psc1350	2008-01-09 21:58:52.000000000 +0100
+++ linux-2.6.23.9/drivers/usb/storage/usb.c	2008-01-09 21:59:06.000000000 +0100
@@ -124,6 +124,18 @@ MODULE_PARM_DESC(delay_use, "seconds to 
 { USB_DEVICE_VER(id_vendor, id_product, bcdDeviceMin,bcdDeviceMax), \
   .driver_info = (flags)|(USB_US_TYPE_STOR<<24) }
 
+/* for multiple interface / function devices */
+#define UNUSUAL_DEV_MULTI(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
+		interfaceClass, vendorName, productName, useProtocol, \
+		useTransport, initFunction, flags) \
+{	.match_flags = USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION| \
+		USB_DEVICE_ID_MATCH_INT_CLASS, \
+	.idVendor = (id_vendor), .idProduct = (id_product), \
+	.bcdDevice_lo = (bcdDeviceMin), .bcdDevice_hi = (bcdDeviceMax), \
+	.bInterfaceClass = (interfaceClass), \
+	.driver_info = (flags) | (USB_US_TYPE_STOR << 24) \
+}
+
 #define USUAL_DEV(useProto, useTrans, useType) \
 { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans), \
   .driver_info = (USB_US_TYPE_STOR<<24) }
@@ -132,6 +144,7 @@ static struct usb_device_id storage_usb_
 
 #	include "unusual_devs.h"
 #undef UNUSUAL_DEV
+#undef UNUSUAL_DEV_MULTI
 #undef USUAL_DEV
 	/* Terminating entry */
 	{ }
@@ -162,6 +175,13 @@ MODULE_DEVICE_TABLE (usb, storage_usb_id
 	.initFunction = init_function,	\
 }
 
+#define UNUSUAL_DEV_MULTI(idVendor, idProduct, bcdDeviceMin, bcdDeviceMax, \
+		interfaceClass, vendorName, productName, useProtocol, \
+		useTransport, initFunction, flags) \
+	UNUSUAL_DEV(idVendor, idProduct, bcdDeviceMin, bcdDeviceMax, \
+		vendorName, productName, useProtocol, useTransport, \
+		initFunction, flags)
+
 #define USUAL_DEV(use_protocol, use_transport, use_type) \
 { \
 	.useProtocol = use_protocol,	\
diff -up linux-2.6.23.9/drivers/usb/storage/unusual_devs.h.psc1350 linux-2.6.23.9/drivers/usb/storage/unusual_devs.h
--- linux-2.6.23.9/drivers/usb/storage/unusual_devs.h.psc1350	2008-01-09 21:58:52.000000000 +0100
+++ linux-2.6.23.9/drivers/usb/storage/unusual_devs.h	2008-01-09 22:05:28.000000000 +0100
@@ -1542,6 +1542,20 @@ UNUSUAL_DEV(  0xed06, 0x4500, 0x0001, 0x
 		US_SC_DEVICE, US_PR_DEVICE, NULL,
 		US_FL_CAPACITY_HEURISTICS),
 
+/* Reported by Hans de Goede <j.w.r.degoede@hhs.nl> */
+UNUSUAL_DEV_MULTI( 0x03F0, 0x3B11, 0x100, 0x100, USB_CLASS_MASS_STORAGE,
+		"hp",
+		"psc 1300 series",
+		US_SC_DEVICE, US_PR_DEVICE, NULL,
+		US_FL_LAST_SECTOR_BUG),
+
+/* Reported by Guillaume Bedot <littletux@zarb.org> */
+UNUSUAL_DEV_MULTI( 0x03F0, 0x4811, 0x100, 0x100, USB_CLASS_MASS_STORAGE,
+		"hp",
+		"psc 1600 series",
+		US_SC_DEVICE, US_PR_DEVICE, NULL,
+		US_FL_LAST_SECTOR_BUG),
+
 /* 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),
diff -up linux-2.6.23.9/drivers/usb/storage/protocol.c.psc1350 linux-2.6.23.9/drivers/usb/storage/protocol.c
--- linux-2.6.23.9/drivers/usb/storage/protocol.c.psc1350	2008-01-09 21:58:52.000000000 +0100
+++ linux-2.6.23.9/drivers/usb/storage/protocol.c	2008-01-09 21:59:06.000000000 +0100
@@ -47,6 +47,8 @@
 #include <linux/highmem.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
+#include <scsi/sd.h>
 
 #include "usb.h"
 #include "protocol.h"
@@ -140,6 +142,24 @@ void usb_stor_ufi_command(struct scsi_cm
 void usb_stor_transparent_scsi_command(struct scsi_cmnd *srb,
 				       struct us_data *us)
 {
+	if ((us->flags & US_FL_LAST_SECTOR_BUG) &&
+			(srb->cmnd[0] == READ_10 ||
+			 srb->cmnd[0] == WRITE_10) &&
+			srb->device->type == TYPE_DISK) {
+		struct scsi_disk *sdkp = dev_get_drvdata(
+						&srb->device->sdev_gendev);
+		sector_t offset = srb->cmnd[2] << 24 | srb->cmnd[3] << 16 |
+			srb->cmnd[4] << 8 | srb->cmnd[5];
+		sector_t num = srb->cmnd[7] << 8 | srb->cmnd[8];
+		
+		if ((offset + num) == sdkp->capacity && num > 1) {
+			if (srb->cmnd[8] == 0)
+				srb->cmnd[7]--;
+			srb->cmnd[8]--;
+			srb->request_bufflen -= 512;
+			srb->underflow -= 512;
+		}
+	}
 	/* send the command to the transport layer */
 	usb_stor_invoke_transport(srb, us);
 }

[-- Attachment #3: Type: text/plain, Size: 278 bytes --]

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[-- Attachment #4: Type: text/plain, Size: 191 bytes --]

_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

[Matthew Dharm]

[-- Attachment #1.1: Type: text/plain, Size: 1608 bytes --]

On Wed, Jan 09, 2008 at 10:44:49PM +0100, Hans de Goede wrote:
> First of all sorry for the somewhat massive cross-posting, I've spend a 
> significant amount of time hunting down this bug, and so far the response 
> has been less the overwhelming.
 
> The cardreader of the multi function printers will "crash" and from that 
> moment on no longer communicate in any sane way, if you try to read the 
> last sector of an sdcard* in a read that is more then 1 sector, so trying 
> to read 8 sectors starting at sector capicity-8 will crash it, as will 
> reading 2 sectors starting at sector capicity-2, however reading the last 
> sector in a one 1 sector read will succeed! (* xdcards seem to be fine).

To continue the history on this.... we over in usb-storage land looked at
this and think it belongs in the SCSI layer.  We don't like changing
commands in-flight; it has, historically, caused us all sorts of issues in
the past.

Furthermore, this seems like the likely sort of off-by-one bug that can
affect many types of devices, not just USB.

I'd really like to see this in sd_mod -- I have no objection to requiring
an HCD to set a flag to indicate that it should be used, if really desired.
But, it seems to me to be a much easier change to make where the command
originated rather than in mid-flight.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

P:  Nine more messages in admin.policy.
M: I know, I'm typing as fast as I can!
					-- Pitr and Mike
User Friendly, 11/27/97

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 278 bytes --]

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[-- Attachment #3: Type: text/plain, Size: 191 bytes --]

_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

[Boaz Harrosh]

----- Original Message -----
*From:* Hans de Goede <j.w.r.degoede@hhs.nl>
*To:* USB Storage list <usb-storage@lists.one-eyed-alien.net>
*CC:* fedora-kernel-list@redhat.com, USB development list
<linux-usb-devel@lists.sourceforge.net>, David Brown
<usb-storage2@davidb.org>, Guillaume Bedot <littletux@zarb.org>,
linux-scsi@vger.kernel.org, linux-usb@vger.kernel.org
*Sent:* Wed, Jan 09 2008 at 23:44 +0200
*Subject:* Linux scsi / usb-mass-storage and HP printer cardreader bug + fix


> Hi All,
>
> First of all sorry for the somewhat massive cross-posting, I've spend a 
> significant amount of time hunting down this bug, and so far the response has 
> been less the overwhelming.
>
> The problem is with the HP PSC 1350 (my printer and confirmed by 2 others) and 
> atleast also the HP PSC 1610 (confirmed by Guillaume Bedot, in the CC).
>
> The cardreader of the multi function printers will "crash" and from that moment 
> on no longer communicate in any sane way, if you try to read the last sector of 
> an sdcard* in a read that is more then 1 sector, so trying to read 8 sectors 
> starting at sector capicity-8 will crash it, as will reading 2 sectors starting 
> at sector capicity-2, however reading the last sector in a one 1 sector read 
> will succeed! (* xdcards seem to be fine).
>
> I haven't tried if it will crash on larger then 1 sector writes which include 
> the last sector too, I immediately added code to not do that in both the read 
> and write paths. I have tested reading and writing the end of the disk with 
> this kludge in and it works.
>
> I currently have a somewhat ugly proof of concept patch for this, which adds 
> another type of usb-massstorage quirk. When this quirk flag is set, the 
> usb-massstorage driver modifies READ_10 and WRITE_10 commands of more then 1 
> sector which includes the last sector to become one sector less. I've been told 
> by scsi subsystem developers that doing a shorter read / write then requested 
> is not a problem, the scsi subsystem is designed to handle getting less then it 
> asked for and will send a seperate request for the last sector.
>
> I and 3 others (2 on a PSC 1350 too, one on a PSC1610) have tested this patch 
> with success. I'm not asking for this patch to be included to the kernel as is, 
> I'm asking for the now known workaround for this to be added to the kernel in 
> someway!
>
> Perhaps its an idea to add the posibility to have a scsi command filter 
> function / callback to the scsi or usb-massstorage subsystem, and then add a 
> mechanism to set this filter depending on usb id's and if added to the scsi 
> layer, a mechanism to set it based on scsi device and manufacturer 
> identification strings. Such a mechanism might be usefull in the future to work 
> around other broken hardware too, and has the added advantage of not having 
> todo much changes to the normal code path, keep that readable.
>
> I'm willing to come up with a patch for such a filter mechanism, provided I get 
> some pointers where this is best added.
>
> Thanks & Regards,
>
> Hans
>
>
> p.s.
>
> I've also included the fedora-kernel list in the addressee's because I was 
> hoping that maybe someone can take one of these printers to the kernel hackfest 
>    in the weekend's fudcon and take a look at this.
>   
> +		if ((offset + num) == sdkp->capacity && num > 1) {
> +			if (srb->cmnd[8] == 0)
> +				srb->cmnd[7]--;
> +			srb->cmnd[8]--;
> +			srb->request_bufflen -= 512;
> +			srb->underflow -= 512;
> +		}
>   
This will no longer compile on top of latest scsi-misc, and
LLDs are not suppose to modify request_bufflen anymore.

I'm not sure what the proper solution should be?

Boaz


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

[Hans de Goede]

Boaz Harrosh wrote:
> ----- Original Message -----
> *From:* Hans de Goede <j.w.r.degoede@hhs.nl>
> *To:* USB Storage list <usb-storage@lists.one-eyed-alien.net>
> *CC:* fedora-kernel-list@redhat.com, USB development list
> <linux-usb-devel@lists.sourceforge.net>, David Brown
> <usb-storage2@davidb.org>, Guillaume Bedot <littletux@zarb.org>,
> linux-scsi@vger.kernel.org, linux-usb@vger.kernel.org
> *Sent:* Wed, Jan 09 2008 at 23:44 +0200
> *Subject:* Linux scsi / usb-mass-storage and HP printer cardreader bug + fix
> 
> 
>> Hi All,
>>
>> First of all sorry for the somewhat massive cross-posting, I've spend a 
>> significant amount of time hunting down this bug, and so far the response has 
>> been less the overwhelming.
>>
>> The problem is with the HP PSC 1350 (my printer and confirmed by 2 others) and 
>> atleast also the HP PSC 1610 (confirmed by Guillaume Bedot, in the CC).
>>
>> The cardreader of the multi function printers will "crash" and from that moment 
>> on no longer communicate in any sane way, if you try to read the last sector of 
>> an sdcard* in a read that is more then 1 sector, so trying to read 8 sectors 
>> starting at sector capicity-8 will crash it, as will reading 2 sectors starting 
>> at sector capicity-2, however reading the last sector in a one 1 sector read 
>> will succeed! (* xdcards seem to be fine).
>>
>> I haven't tried if it will crash on larger then 1 sector writes which include 
>> the last sector too, I immediately added code to not do that in both the read 
>> and write paths. I have tested reading and writing the end of the disk with 
>> this kludge in and it works.
>>
>> I currently have a somewhat ugly proof of concept patch for this, which adds 
>> another type of usb-massstorage quirk. When this quirk flag is set, the 
>> usb-massstorage driver modifies READ_10 and WRITE_10 commands of more then 1 
>> sector which includes the last sector to become one sector less. I've been told 
>> by scsi subsystem developers that doing a shorter read / write then requested 
>> is not a problem, the scsi subsystem is designed to handle getting less then it 
>> asked for and will send a seperate request for the last sector.
>>
>> I and 3 others (2 on a PSC 1350 too, one on a PSC1610) have tested this patch 
>> with success. I'm not asking for this patch to be included to the kernel as is, 
>> I'm asking for the now known workaround for this to be added to the kernel in 
>> someway!
>>
>> Perhaps its an idea to add the posibility to have a scsi command filter 
>> function / callback to the scsi or usb-massstorage subsystem, and then add a 
>> mechanism to set this filter depending on usb id's and if added to the scsi 
>> layer, a mechanism to set it based on scsi device and manufacturer 
>> identification strings. Such a mechanism might be usefull in the future to work 
>> around other broken hardware too, and has the added advantage of not having 
>> todo much changes to the normal code path, keep that readable.
>>
>> I'm willing to come up with a patch for such a filter mechanism, provided I get 
>> some pointers where this is best added.
>>
>> Thanks & Regards,
>>
>> Hans
>>
>>
>> p.s.
>>
>> I've also included the fedora-kernel list in the addressee's because I was 
>> hoping that maybe someone can take one of these printers to the kernel hackfest 
>>    in the weekend's fudcon and take a look at this.
>>   
>> +		if ((offset + num) == sdkp->capacity && num > 1) {
>> +			if (srb->cmnd[8] == 0)
>> +				srb->cmnd[7]--;
>> +			srb->cmnd[8]--;
>> +			srb->request_bufflen -= 512;
>> +			srb->underflow -= 512;
>> +		}
>>   
> This will no longer compile on top of latest scsi-misc, and
> LLDs are not suppose to modify request_bufflen anymore.
> 
> I'm not sure what the proper solution should be?
> 

I guess the proper solution would be to add a special case to the scsi layer 
where the read10 / write10 command is issued, and split the request in 2 there 
when it involves the last sector.

There was another reply in this thread stating that problems reading the last 
sector with sd / mmc cards happen quite often, and that this is most likely not 
an isolated case.

Regards,

Hans


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

[Boaz Harrosh]

On Thu, Jan 10 2008 at 12:52 +0200, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> Boaz Harrosh wrote:
>> ----- Original Message -----
>> *From:* Hans de Goede <j.w.r.degoede@hhs.nl>
>> *To:* USB Storage list <usb-storage@lists.one-eyed-alien.net>
>> *CC:* fedora-kernel-list@redhat.com, USB development list
>> <linux-usb-devel@lists.sourceforge.net>, David Brown
>> <usb-storage2@davidb.org>, Guillaume Bedot <littletux@zarb.org>,
>> linux-scsi@vger.kernel.org, linux-usb@vger.kernel.org
>> *Sent:* Wed, Jan 09 2008 at 23:44 +0200
>> *Subject:* Linux scsi / usb-mass-storage and HP printer cardreader bug + fix
>>
>>
>>> Hi All,
>>>
>>> First of all sorry for the somewhat massive cross-posting, I've spend a 
>>> significant amount of time hunting down this bug, and so far the response has 
>>> been less the overwhelming.
>>>
>>> The problem is with the HP PSC 1350 (my printer and confirmed by 2 others) and 
>>> atleast also the HP PSC 1610 (confirmed by Guillaume Bedot, in the CC).
>>>
>>> The cardreader of the multi function printers will "crash" and from that moment 
>>> on no longer communicate in any sane way, if you try to read the last sector of 
>>> an sdcard* in a read that is more then 1 sector, so trying to read 8 sectors 
>>> starting at sector capicity-8 will crash it, as will reading 2 sectors starting 
>>> at sector capicity-2, however reading the last sector in a one 1 sector read 
>>> will succeed! (* xdcards seem to be fine).
>>>
>>> I haven't tried if it will crash on larger then 1 sector writes which include 
>>> the last sector too, I immediately added code to not do that in both the read 
>>> and write paths. I have tested reading and writing the end of the disk with 
>>> this kludge in and it works.
>>>
>>> I currently have a somewhat ugly proof of concept patch for this, which adds 
>>> another type of usb-massstorage quirk. When this quirk flag is set, the 
>>> usb-massstorage driver modifies READ_10 and WRITE_10 commands of more then 1 
>>> sector which includes the last sector to become one sector less. I've been told 
>>> by scsi subsystem developers that doing a shorter read / write then requested 
>>> is not a problem, the scsi subsystem is designed to handle getting less then it 
>>> asked for and will send a seperate request for the last sector.
>>>
>>> I and 3 others (2 on a PSC 1350 too, one on a PSC1610) have tested this patch 
>>> with success. I'm not asking for this patch to be included to the kernel as is, 
>>> I'm asking for the now known workaround for this to be added to the kernel in 
>>> someway!
>>>
>>> Perhaps its an idea to add the posibility to have a scsi command filter 
>>> function / callback to the scsi or usb-massstorage subsystem, and then add a 
>>> mechanism to set this filter depending on usb id's and if added to the scsi 
>>> layer, a mechanism to set it based on scsi device and manufacturer 
>>> identification strings. Such a mechanism might be usefull in the future to work 
>>> around other broken hardware too, and has the added advantage of not having 
>>> todo much changes to the normal code path, keep that readable.
>>>
>>> I'm willing to come up with a patch for such a filter mechanism, provided I get 
>>> some pointers where this is best added.
>>>
>>> Thanks & Regards,
>>>
>>> Hans
>>>
>>>
>>> p.s.
>>>
>>> I've also included the fedora-kernel list in the addressee's because I was 
>>> hoping that maybe someone can take one of these printers to the kernel hackfest 
>>>    in the weekend's fudcon and take a look at this.
>>>   
>>> +		if ((offset + num) == sdkp->capacity && num > 1) {
>>> +			if (srb->cmnd[8] == 0)
>>> +				srb->cmnd[7]--;
>>> +			srb->cmnd[8]--;
>>> +			srb->request_bufflen -= 512;
>>> +			srb->underflow -= 512;
>>> +		}
>>>   
>> This will no longer compile on top of latest scsi-misc, and
>> LLDs are not suppose to modify request_bufflen anymore.
>>
>> I'm not sure what the proper solution should be?
>>
> 
> I guess the proper solution would be to add a special case to the scsi layer 
> where the read10 / write10 command is issued, and split the request in 2 there 
> when it involves the last sector.
> 
> There was another reply in this thread stating that problems reading the last 
> sector with sd / mmc cards happen quite often, and that this is most likely not 
> an isolated case.
> 
> Regards,
> 
> Hans

Yes, you're right. in ULDs it is a much proper way to do this.

So I guess you'll have to do that special host flag or device
flag, and add a check for it in sd.c. You'll see that sd.c is 
already doing bufflen truncation at sd_prep_fn(), just add one
more case.

Boaz

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

[Hans de Goede]

Boaz Harrosh wrote:
> On Thu, Jan 10 2008 at 12:52 +0200, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
>>> I'm not sure what the proper solution should be?
>>>
>> I guess the proper solution would be to add a special case to the scsi layer 
>> where the read10 / write10 command is issued, and split the request in 2 there 
>> when it involves the last sector.
>>
>> There was another reply in this thread stating that problems reading the last 
>> sector with sd / mmc cards happen quite often, and that this is most likely not 
>> an isolated case.
>>
>> Regards,
>>
>> Hans
> 
> Yes, you're right. in ULDs it is a much proper way to do this.
> 
> So I guess you'll have to do that special host flag or device
> flag, and add a check for it in sd.c. You'll see that sd.c is 
> already doing bufflen truncation at sd_prep_fn(), just add one
> more case.
> 

Yes,

That will work nicely, I'll write an updated patch this evening (when I have 
access to the printer to test again).

Regards,

Hans

Re: Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

[Guillaume Bedot]

Hello,

Le vendredi 11 janvier 2008 à 13:48 +0100, Hans de Goede a écrit :
> That will work nicely, I'll write an updated patch this evening (when I have 
> access to the printer to test again).
> 
Great news, i am impatient to test this new patch.

I may face an other bug with the Transcend 1GB SD card, would be
possible that the patch would be available for latests kernels ?

Thanks in advance,

Best regards,

Guillaume B.




-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

Boaz Harrosh wrote:
> Yes, you're right. in ULDs it is a much proper way to do this.
> 
> So I guess you'll have to do that special host flag or device
> flag, and add a check for it in sd.c. You'll see that sd.c is 
> already doing bufflen truncation at sd_prep_fn(), just add one
> more case.
> 

Done, thanks for the hint. Patch implementing my fix this way attached, please 
apply.

Thanks & Regards,

Hans


[-- Attachment #2: usb-storage-psc1350-v4.patch --]
[-- Type: text/x-patch, Size: 7789 bytes --]

This patch makes the cardreader on the HP PSC 1350 multifunction printer work,
it adds a new scsi_device and US_FL_ flag + handling, and a new
UNUSUAL_DEV_MULTI macro to support multifunction devices.

The difference between this version and the previous v3 version is that this
version has been rewritten to lower the number of sectors requested in the
scsi command when the command is generated instead of later modifying the
command in flight. This patch is against 2.6.24rc7 .

Signed-off-by: Hans de Goede <j.w.r.degoede-fbo2DhPpy/Q@public.gmane.org>
diff -up vanilla-2.6.24-rc7/include/scsi/scsi_device.h.psc1350 vanilla-2.6.24-rc7/include/scsi/scsi_device.h
--- vanilla-2.6.24-rc7/include/scsi/scsi_device.h.psc1350	2008-01-11 19:40:31.000000000 +0100
+++ vanilla-2.6.24-rc7/include/scsi/scsi_device.h	2008-01-11 19:40:48.000000000 +0100
@@ -142,6 +142,7 @@ struct scsi_device {
 	unsigned fix_capacity:1;	/* READ_CAPACITY is too high by 1 */
 	unsigned guess_capacity:1;	/* READ_CAPACITY might be too high by 1 */
 	unsigned retry_hwerror:1;	/* Retry HARDWARE_ERROR */
+	unsigned last_sector_bug:1;	/* Always read last sector in a 1 sector read */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
diff -up vanilla-2.6.24-rc7/include/linux/usb_usual.h.psc1350 vanilla-2.6.24-rc7/include/linux/usb_usual.h
--- vanilla-2.6.24-rc7/include/linux/usb_usual.h.psc1350	2008-01-11 19:40:31.000000000 +0100
+++ vanilla-2.6.24-rc7/include/linux/usb_usual.h	2008-01-11 19:42:14.000000000 +0100
@@ -50,7 +50,9 @@
 	US_FLAG(CAPACITY_HEURISTICS,	0x00001000)		\
 		/* sometimes sizes is too big */		\
 	US_FLAG(MAX_SECTORS_MIN,0x00002000)			\
-		/* Sets max_sectors to arch min */
+		/* Sets max_sectors to arch min */		\
+	US_FLAG(LAST_SECTOR_BUG,0x00004000)			\
+		/* Always read last sector in a 1 sector read */
 
 
 #define US_FLAG(name, value)	US_FL_##name = value ,
diff -up vanilla-2.6.24-rc7/drivers/scsi/sd.c.psc1350 vanilla-2.6.24-rc7/drivers/scsi/sd.c
--- vanilla-2.6.24-rc7/drivers/scsi/sd.c.psc1350	2008-01-11 19:55:43.000000000 +0100
+++ vanilla-2.6.24-rc7/drivers/scsi/sd.c	2008-01-11 20:48:54.000000000 +0100
@@ -395,6 +395,13 @@ static int sd_prep_fn(struct request_que
 		goto out;
 	}
 
+	/* Some devices (some sdcards for one) don't like it if the last sector
+	   gets read in a larger then 1 sector read */
+	if (sdp->last_sector_bug && rq->nr_sectors > sdp->sector_size / 512 &&
+ 	    block + rq->nr_sectors == get_capacity(disk)) {
+		this_count -= sdp->sector_size / 512;
+	}
+
 	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt, "block=%llu\n",
 					(unsigned long long)block));
 
diff -up vanilla-2.6.24-rc7/drivers/usb/storage/libusual.c.psc1350 vanilla-2.6.24-rc7/drivers/usb/storage/libusual.c
--- vanilla-2.6.24-rc7/drivers/usb/storage/libusual.c.psc1350	2008-01-11 19:40:31.000000000 +0100
+++ vanilla-2.6.24-rc7/drivers/usb/storage/libusual.c	2008-01-11 19:40:48.000000000 +0100
@@ -45,6 +45,18 @@ static int usu_probe_thread(void *arg);
 { USB_DEVICE_VER(id_vendor, id_product, bcdDeviceMin,bcdDeviceMax), \
   .driver_info = (flags)|(USB_US_TYPE_STOR<<24) }
 
+/* for multiple interface / function devices */
+#define UNUSUAL_DEV_MULTI(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
+		interfaceClass, vendorName, productName, useProtocol, \
+		useTransport, initFunction, flags) \
+{	.match_flags = USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION| \
+		USB_DEVICE_ID_MATCH_INT_CLASS, \
+	.idVendor = (id_vendor), .idProduct = (id_product), \
+	.bcdDevice_lo = (bcdDeviceMin), .bcdDevice_hi = (bcdDeviceMax), \
+	.bInterfaceClass = (interfaceClass), \
+	.driver_info = (flags) | (USB_US_TYPE_STOR << 24) \
+}
+
 #define USUAL_DEV(useProto, useTrans, useType) \
 { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans), \
   .driver_info = ((useType)<<24) }
@@ -56,6 +68,7 @@ struct usb_device_id storage_usb_ids [] 
 
 #undef USUAL_DEV
 #undef UNUSUAL_DEV
+#undef UNUSUAL_DEV_MULTI
 
 MODULE_DEVICE_TABLE(usb, storage_usb_ids);
 EXPORT_SYMBOL_GPL(storage_usb_ids);
diff -up vanilla-2.6.24-rc7/drivers/usb/storage/scsiglue.c.psc1350 vanilla-2.6.24-rc7/drivers/usb/storage/scsiglue.c
--- vanilla-2.6.24-rc7/drivers/usb/storage/scsiglue.c.psc1350	2008-01-11 19:40:31.000000000 +0100
+++ vanilla-2.6.24-rc7/drivers/usb/storage/scsiglue.c	2008-01-11 19:40:48.000000000 +0100
@@ -165,6 +165,9 @@ static int slave_configure(struct scsi_d
 		if (us->flags & US_FL_CAPACITY_HEURISTICS)
 			sdev->guess_capacity = 1;
 
+		if (us->flags & US_FL_LAST_SECTOR_BUG)
+			sdev->last_sector_bug = 1;
+
 		/* 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
diff -up vanilla-2.6.24-rc7/drivers/usb/storage/usb.c.psc1350 vanilla-2.6.24-rc7/drivers/usb/storage/usb.c
--- vanilla-2.6.24-rc7/drivers/usb/storage/usb.c.psc1350	2008-01-11 19:40:31.000000000 +0100
+++ vanilla-2.6.24-rc7/drivers/usb/storage/usb.c	2008-01-11 19:40:48.000000000 +0100
@@ -124,6 +124,18 @@ MODULE_PARM_DESC(delay_use, "seconds to 
 { USB_DEVICE_VER(id_vendor, id_product, bcdDeviceMin,bcdDeviceMax), \
   .driver_info = (flags)|(USB_US_TYPE_STOR<<24) }
 
+/* for multiple interface / function devices */
+#define UNUSUAL_DEV_MULTI(id_vendor, id_product, bcdDeviceMin, bcdDeviceMax, \
+		interfaceClass, vendorName, productName, useProtocol, \
+		useTransport, initFunction, flags) \
+{	.match_flags = USB_DEVICE_ID_MATCH_DEVICE_AND_VERSION| \
+		USB_DEVICE_ID_MATCH_INT_CLASS, \
+	.idVendor = (id_vendor), .idProduct = (id_product), \
+	.bcdDevice_lo = (bcdDeviceMin), .bcdDevice_hi = (bcdDeviceMax), \
+	.bInterfaceClass = (interfaceClass), \
+	.driver_info = (flags) | (USB_US_TYPE_STOR << 24) \
+}
+
 #define USUAL_DEV(useProto, useTrans, useType) \
 { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans), \
   .driver_info = (USB_US_TYPE_STOR<<24) }
@@ -132,6 +144,7 @@ static struct usb_device_id storage_usb_
 
 #	include "unusual_devs.h"
 #undef UNUSUAL_DEV
+#undef UNUSUAL_DEV_MULTI
 #undef USUAL_DEV
 	/* Terminating entry */
 	{ }
@@ -162,6 +175,13 @@ MODULE_DEVICE_TABLE (usb, storage_usb_id
 	.initFunction = init_function,	\
 }
 
+#define UNUSUAL_DEV_MULTI(idVendor, idProduct, bcdDeviceMin, bcdDeviceMax, \
+		interfaceClass, vendorName, productName, useProtocol, \
+		useTransport, initFunction, flags) \
+	UNUSUAL_DEV(idVendor, idProduct, bcdDeviceMin, bcdDeviceMax, \
+		vendorName, productName, useProtocol, useTransport, \
+		initFunction, flags)
+
 #define USUAL_DEV(use_protocol, use_transport, use_type) \
 { \
 	.useProtocol = use_protocol,	\
diff -up vanilla-2.6.24-rc7/drivers/usb/storage/unusual_devs.h.psc1350 vanilla-2.6.24-rc7/drivers/usb/storage/unusual_devs.h
--- vanilla-2.6.24-rc7/drivers/usb/storage/unusual_devs.h.psc1350	2008-01-11 19:40:31.000000000 +0100
+++ vanilla-2.6.24-rc7/drivers/usb/storage/unusual_devs.h	2008-01-11 19:40:48.000000000 +0100
@@ -1580,6 +1580,20 @@ UNUSUAL_DEV(  0xed06, 0x4500, 0x0001, 0x
 		US_SC_DEVICE, US_PR_DEVICE, NULL,
 		US_FL_CAPACITY_HEURISTICS),
 
+/* Reported by Hans de Goede <j.w.r.degoede-fbo2DhPpy/Q@public.gmane.org> */
+UNUSUAL_DEV_MULTI( 0x03F0, 0x3B11, 0x100, 0x100, USB_CLASS_MASS_STORAGE,
+		"hp",
+		"psc 1300 series",
+		US_SC_DEVICE, US_PR_DEVICE, NULL,
+		US_FL_LAST_SECTOR_BUG),
+
+/* Reported by Guillaume Bedot <littletux-QFiJtcdG+fw@public.gmane.org> */
+UNUSUAL_DEV_MULTI( 0x03F0, 0x4811, 0x100, 0x100, USB_CLASS_MASS_STORAGE,
+		"hp",
+		"psc 1600 series",
+		US_SC_DEVICE, US_PR_DEVICE, NULL,
+		US_FL_LAST_SECTOR_BUG),
+
 /* 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),

Re: [usb-storage] PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Matthew Dharm]

[-- Attachment #1.1: Type: text/plain, Size: 1015 bytes --]

On Fri, Jan 11, 2008 at 09:14:00PM +0100, Hans de Goede wrote:
> Boaz Harrosh wrote:
> >Yes, you're right. in ULDs it is a much proper way to do this.
> >
> >So I guess you'll have to do that special host flag or device
> >flag, and add a check for it in sd.c. You'll see that sd.c is 
> >already doing bufflen truncation at sd_prep_fn(), just add one
> >more case.
> >
> 
> Done, thanks for the hint. Patch implementing my fix this way attached, 
> please apply.

Well, I can't say that I'm really a big fan of matching a multi-interface
device based on the interface class code, but I don't see a better solution
that isn't a major coding project.

This approach looks pretty reasonable to me.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

SP: I sell software for Microsoft.  Can you set me free?
DP: Natural Selection says I shouldn't.
					-- MS Salesman and Dust Puppy
User Friendly, 4/2/1998

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 278 bytes --]

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[-- Attachment #3: Type: text/plain, Size: 191 bytes --]

_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Guillaume Bedot]

Hello,

On ven, 2008-01-11 at 21:14 +0100, Hans de Goede wrote:
> Boaz Harrosh wrote:
> > Yes, you're right. in ULDs it is a much proper way to do this.
> > 
> > So I guess you'll have to do that special host flag or device
> > flag, and add a check for it in sd.c. You'll see that sd.c is 
> > already doing bufflen truncation at sd_prep_fn(), just add one
> > more case.
> > 
> 
> Done, thanks for the hint. Patch implementing my fix this way attached, please 
> apply.
> 
> Thanks & Regards,
> 
> Hans
> 
I have tested this time with two PSC 1610 printers, and two SD cards,
the same bug occured without the patch.
And is fixed with your new patch. Good work !

The bug however did not occur with a microSD card in a SD adaptator ?!

But it fixes only two models.
Do you think other devices (hp or not) can be impacted ?
There are hundreds of models with card readers only for hp :
http://hplip.sourceforge.net/supported_devices/combined.html

Will this be possible to use "LAST_SECTOR_BUG" quirk for testing without
recompiling a kernel ?

Best regards,

Guillaume B.

-
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: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Hans de Goede]

Guillaume Bedot wrote:
> I have tested this time with two PSC 1610 printers, and two SD cards,
> the same bug occured without the patch.
> And is fixed with your new patch. Good work !
> 

Hi,

Thanks for testing!

> But it fixes only two models.
> Do you think other devices (hp or not) can be impacted ?
> There are hundreds of models with card readers only for hp :
> http://hplip.sourceforge.net/supported_devices/combined.html
> 
> Will this be possible to use "LAST_SECTOR_BUG" quirk for testing without
> recompiling a kernel ?
> 

This is not possible AFAIK, I've already wrote a blog post about this asking 
for people to test this, but got no responses.

I think it would be good if the people from the hplip project would take some 
time to test all the printer models they have got access too.

Regards,

Hans


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Matthew Dharm]

[-- Attachment #1.1: Type: text/plain, Size: 1133 bytes --]

On Mon, Jan 14, 2008 at 10:46:56AM +0100, Hans de Goede wrote:
> Guillaume Bedot wrote:
> >But it fixes only two models.
> >Do you think other devices (hp or not) can be impacted ?
> >There are hundreds of models with card readers only for hp :
> >http://hplip.sourceforge.net/supported_devices/combined.html
> >
> >Will this be possible to use "LAST_SECTOR_BUG" quirk for testing without
> >recompiling a kernel ?
> >
> 
> This is not possible AFAIK, I've already wrote a blog post about this 
> asking for people to test this, but got no responses.

Once the patches are accepted by the SCSI people, one of the things we can
consider doing is enabling this quirk for all USB devices.  It should be
pretty harmless to all properly working devices, and the performance hit
should be pretty minimal.

We may be able to convince the SCSI people to enable it for all devices,
regardless of HCD.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What the hell are you?
					-- Pitr to Dust Puppy 
User Friendly, 12/3/1997

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 278 bytes --]

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[-- Attachment #3: Type: text/plain, Size: 191 bytes --]

_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[James Bottomley]

On Mon, 2008-01-14 at 08:03 -0800, Matthew Dharm wrote:
> On Mon, Jan 14, 2008 at 10:46:56AM +0100, Hans de Goede wrote:
> > Guillaume Bedot wrote:
> > >But it fixes only two models.
> > >Do you think other devices (hp or not) can be impacted ?
> > >There are hundreds of models with card readers only for hp :
> > >http://hplip.sourceforge.net/supported_devices/combined.html
> > >
> > >Will this be possible to use "LAST_SECTOR_BUG" quirk for testing without
> > >recompiling a kernel ?
> > >
> > 
> > This is not possible AFAIK, I've already wrote a blog post about this 
> > asking for people to test this, but got no responses.
> 
> Once the patches are accepted by the SCSI people, one of the things we can
> consider doing is enabling this quirk for all USB devices.  It should be
> pretty harmless to all properly working devices, and the performance hit
> should be pretty minimal.

The SCSI patches look OK, with the stylistic points fixed below ...
we'll need two separate patches as well (one for SCSI, one for USB).

> We may be able to convince the SCSI people to enable it for all devices,
> regardless of HCD.

No ... I'm not particularly keen to have enterprise vendors after my
blood ...


> +       /* Some devices (some sdcards for one) don't like it if the last sector
> +          gets read in a larger then 1 sector read */

The comment style in sd is

/*
 * comment
 */

> +       if (sdp->last_sector_bug && rq->nr_sectors > sdp->sector_size / 512 &&

An unlikely() here, please to force the compiler to optimise for the
non-buggy case.  Plus what is the rq->nr_sectors > sdp->sector_size /
512 test supposed to be doing?  that being true is supposed to be a
guarantee of the block layer (and if something goes wrong there's a
check for this lower down).

> +           block + rq->nr_sectors == get_capacity(disk)) {

rq->nr_sectors should be this_count

> +               this_count -= sdp->sector_size / 512;

If you relocate this code to after the sector_size/this_count adjustment
code (i.e. about line 442) you can just do --this_count;

James

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Hans de Goede]

James Bottomley wrote:
> On Mon, 2008-01-14 at 08:03 -0800, Matthew Dharm wrote:
>> On Mon, Jan 14, 2008 at 10:46:56AM +0100, Hans de Goede wrote:
>>> Guillaume Bedot wrote:
>>>> But it fixes only two models.
>>>> Do you think other devices (hp or not) can be impacted ?
>>>> There are hundreds of models with card readers only for hp :
>>>> http://hplip.sourceforge.net/supported_devices/combined.html
>>>>
>>>> Will this be possible to use "LAST_SECTOR_BUG" quirk for testing without
>>>> recompiling a kernel ?
>>>>
>>> This is not possible AFAIK, I've already wrote a blog post about this 
>>> asking for people to test this, but got no responses.
>> Once the patches are accepted by the SCSI people, one of the things we can
>> consider doing is enabling this quirk for all USB devices.  It should be
>> pretty harmless to all properly working devices, and the performance hit
>> should be pretty minimal.
> 
> The SCSI patches look OK, with the stylistic points fixed below ...
> we'll need two separate patches as well (one for SCSI, one for USB).
> 

Okay,

Thanks for the review! I'll do a new scsi changes only patch once I get an 
answer to some questions regarding your review.

>> +       /* Some devices (some sdcards for one) don't like it if the last sector
>> +          gets read in a larger then 1 sector read */
> 
> The comment style in sd is
> 
> /*
>  * comment
>  */
> 

Will fix,

>> +       if (sdp->last_sector_bug && rq->nr_sectors > sdp->sector_size / 512 &&
> 
> An unlikely() here, please to force the compiler to optimise for the
> non-buggy case.

Will do.

> Plus what is the rq->nr_sectors > sdp->sector_size /
> 512 test supposed to be doing?  that being true is supposed to be a
> guarantee of the block layer (and if something goes wrong there's a
> check for this lower down).
> 

It first is was just:
rq->nr_sectors > 1

Then I changed it to also do the right thing for 1024 and larger sector disks. 
The whole purpose is to not read the last sector in a larger then one sector 
read, so the amount of sectors gets reduced by one if (block + rq->nr_sectors 
== get_capacity(disk)) but we do want still want to be able to read the last 
sector by itself, so we must not reduce the no sectors to be read by one if it 
is already one.

>> +           block + rq->nr_sectors == get_capacity(disk)) {
> 
> rq->nr_sectors should be this_count
> 

Fine by me, but in this place in the code they are the same value, and the 
check for a read beyond the end of disk a few lines above also uses 
rq->nr_sectors, which one should be used when?

>> +               this_count -= sdp->sector_size / 512;
> 
> If you relocate this code to after the sector_size/this_count adjustment
> code (i.e. about line 442) you can just do --this_count;
> 

I cannot move the check down as then block has been adjusted for the sector 
size, so if I move the check down it becomes:
if (block * (sdp->sector_size / 512) + rq->nr_sectors == get_capacity(disk))

I would rather not have the sdp->sector_size / 512 code in the check (slow) but 
rather in the not often entered if block.

Regards,

Hans

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Stefan Richter]

James Bottomley wrote:
> On Mon, 2008-01-14 at 08:03 -0800, Matthew Dharm wrote:
>> On Mon, Jan 14, 2008 at 10:46:56AM +0100, Hans de Goede wrote:
>> > Guillaume Bedot wrote:
>> > >Will this be possible to use "LAST_SECTOR_BUG" quirk for testing without
>> > >recompiling a kernel ?
>> > 
>> > This is not possible AFAIK, I've already wrote a blog post about this 
>> > asking for people to test this, but got no responses.
>> 
>> Once the patches are accepted by the SCSI people, one of the things we can
>> consider doing is enabling this quirk for all USB devices.
...
>> We may be able to convince the SCSI people to enable it for all devices,
>> regardless of HCD.
> 
> No ... I'm not particularly keen to have enterprise vendors after my
> blood ...

Guillaume, you can tell the SCSI core driver at boot time (or module
insertion time) and/or at runtime to
  - switch on default quirk flags,
  - add quirk flags for selected devices per name matching.

Alas I don't know of a good documention how to do either of this, and I
am not familiar enough with the procedure to explain it here.
-- 
Stefan Richter
-=====-==--- ---= -===-
http://arcgraph.de/sr/

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Matthew Dharm]

[-- Attachment #1.1: Type: text/plain, Size: 740 bytes --]

On Mon, Jan 14, 2008 at 10:33:08AM -0600, James Bottomley wrote:
> 
> On Mon, 2008-01-14 at 08:03 -0800, Matthew Dharm wrote:
> > We may be able to convince the SCSI people to enable it for all devices,
> > regardless of HCD.
> 
> No ... I'm not particularly keen to have enterprise vendors after my
> blood ...

Fair enough.  Tho, we should probably just enable this blindly for all
usb-storage devices.  Otherwise, this is just going to become an
unusual_devs.h nightmare.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

What, are you one of those Microsoft-bashing Linux freaks?
					-- Customer to Greg
User Friendly, 2/10/1999

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 278 bytes --]

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[-- Attachment #3: Type: text/plain, Size: 191 bytes --]

_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[James Bottomley]

On Mon, 2008-01-14 at 19:37 +0100, Hans de Goede wrote:
> James Bottomley wrote:
> > On Mon, 2008-01-14 at 08:03 -0800, Matthew Dharm wrote:
> >> On Mon, Jan 14, 2008 at 10:46:56AM +0100, Hans de Goede wrote:
> >>> Guillaume Bedot wrote:
> >>>> But it fixes only two models.
> >>>> Do you think other devices (hp or not) can be impacted ?
> >>>> There are hundreds of models with card readers only for hp :
> >>>> http://hplip.sourceforge.net/supported_devices/combined.html
> >>>>
> >>>> Will this be possible to use "LAST_SECTOR_BUG" quirk for testing without
> >>>> recompiling a kernel ?
> >>>>
> >>> This is not possible AFAIK, I've already wrote a blog post about this 
> >>> asking for people to test this, but got no responses.
> >> Once the patches are accepted by the SCSI people, one of the things we can
> >> consider doing is enabling this quirk for all USB devices.  It should be
> >> pretty harmless to all properly working devices, and the performance hit
> >> should be pretty minimal.
> > 
> > The SCSI patches look OK, with the stylistic points fixed below ...
> > we'll need two separate patches as well (one for SCSI, one for USB).
> > 
> 
> Okay,
> 
> Thanks for the review! I'll do a new scsi changes only patch once I get an 
> answer to some questions regarding your review.
> 
> >> +       /* Some devices (some sdcards for one) don't like it if the last sector
> >> +          gets read in a larger then 1 sector read */
> > 
> > The comment style in sd is
> > 
> > /*
> >  * comment
> >  */
> > 
> 
> Will fix,
> 
> >> +       if (sdp->last_sector_bug && rq->nr_sectors > sdp->sector_size / 512 &&
> > 
> > An unlikely() here, please to force the compiler to optimise for the
> > non-buggy case.
> 
> Will do.
> 
> > Plus what is the rq->nr_sectors > sdp->sector_size /
> > 512 test supposed to be doing?  that being true is supposed to be a
> > guarantee of the block layer (and if something goes wrong there's a
> > check for this lower down).
> > 
> 
> It first is was just:
> rq->nr_sectors > 1
> 
> Then I changed it to also do the right thing for 1024 and larger sector disks. 
> The whole purpose is to not read the last sector in a larger then one sector 
> read, so the amount of sectors gets reduced by one if (block + rq->nr_sectors 
> == get_capacity(disk)) but we do want still want to be able to read the last 
> sector by itself, so we must not reduce the no sectors to be read by one if it 
> is already one.

Yes, I know that.  What I mean is the block subsystem sends reads and
writes down in increments of the sector size.  Checking if the current
number of pending sectors is greater than the current block size is
checking that guarantee.  The current code already has a check in it to
see if this guarantee is observed ... you don't need to check it again.

> >> +           block + rq->nr_sectors == get_capacity(disk)) {
> > 
> > rq->nr_sectors should be this_count
> > 
> 
> Fine by me, but in this place in the code they are the same value, and the 
> check for a read beyond the end of disk a few lines above also uses 
> rq->nr_sectors, which one should be used when?

this_count is the adjusted sector size.  At the moment, there's no size
transformation in the code between your check and the top (where
this_count is set to rq->nr_sectors).  But if there were (and someone
might add one one day) you'd be wanting to check the adjusted size (i.e.
this_count).

> >> +               this_count -= sdp->sector_size / 512;
> > 
> > If you relocate this code to after the sector_size/this_count adjustment
> > code (i.e. about line 442) you can just do --this_count;
> > 
> 
> I cannot move the check down as then block has been adjusted for the sector 
> size, so if I move the check down it becomes:
> if (block * (sdp->sector_size / 512) + rq->nr_sectors == get_capacity(disk))
> 
> I would rather not have the sdp->sector_size / 512 code in the check (slow) but 
> rather in the not often entered if block.

OK, point taken, it would be worse.  I think today's compilers are happy
to translate x/512 to x>>9, so it shouldn't be that slow.

James



-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Hans de Goede]

Matthew Dharm wrote:
> On Mon, Jan 14, 2008 at 10:33:08AM -0600, James Bottomley wrote:
>> On Mon, 2008-01-14 at 08:03 -0800, Matthew Dharm wrote:
>>> We may be able to convince the SCSI people to enable it for all devices,
>>> regardless of HCD.
>> No ... I'm not particularly keen to have enterprise vendors after my
>> blood ...
> 
> Fair enough.  Tho, we should probably just enable this blindly for all
> usb-storage devices.  Otherwise, this is just going to become an
> unusual_devs.h nightmare.
> 

Since this will make all devices with this bug "just work" (tm), I'm all for it!

As soon as I get an answer from the scsi people on my questions resulting from 
there review I'll do a new patch for the scsi layer for this.

Regards,

Hans

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Hans de Goede]

James Bottomley wrote:
>>> Plus what is the rq->nr_sectors > sdp->sector_size /
>>> 512 test supposed to be doing?  that being true is supposed to be a
>>> guarantee of the block layer (and if something goes wrong there's a
>>> check for this lower down).
>>>
>> It first is was just:
>> rq->nr_sectors > 1
>>
>> Then I changed it to also do the right thing for 1024 and larger sector disks. 
>> The whole purpose is to not read the last sector in a larger then one sector 
>> read, so the amount of sectors gets reduced by one if (block + rq->nr_sectors 
>> == get_capacity(disk)) but we do want still want to be able to read the last 
>> sector by itself, so we must not reduce the no sectors to be read by one if it 
>> is already one.
> 
> Yes, I know that.  What I mean is the block subsystem sends reads and
> writes down in increments of the sector size.  Checking if the current
> number of pending sectors is greater than the current block size is
> checking that guarantee.  The current code already has a check in it to
> see if this guarantee is observed ... you don't need to check it again.
> 

I'm not checking for the guarantee, I'm checking that the read > 1 
realsize_sector, before decrementing the number of _512_bytes_sectors by 
realsize_sector, this check must be there, as after reading all but the last 
sector, another request will be send with 1 realsize_sector size, for which
(block + rq->nr_sectors) == get_capacity(disk) will still hold true, in this 
case this_count must not be lowered, otherwise this_count will become 0 in this 
case and the last sector will never get read.

I hope that clarifies why that code is there, if not can you tell how you would 
want the code to look by just giving the full if statement as it should be, I 
think we are somehow misunderstanding each other.

Regards,

Hans

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[James Bottomley]

On Mon, 2008-01-14 at 20:27 +0100, Hans de Goede wrote:
> James Bottomley wrote:
> >>> Plus what is the rq->nr_sectors > sdp->sector_size /
> >>> 512 test supposed to be doing?  that being true is supposed to be a
> >>> guarantee of the block layer (and if something goes wrong there's a
> >>> check for this lower down).
> >>>
> >> It first is was just:
> >> rq->nr_sectors > 1
> >>
> >> Then I changed it to also do the right thing for 1024 and larger sector disks. 
> >> The whole purpose is to not read the last sector in a larger then one sector 
> >> read, so the amount of sectors gets reduced by one if (block + rq->nr_sectors 
> >> == get_capacity(disk)) but we do want still want to be able to read the last 
> >> sector by itself, so we must not reduce the no sectors to be read by one if it 
> >> is already one.
> > 
> > Yes, I know that.  What I mean is the block subsystem sends reads and
> > writes down in increments of the sector size.  Checking if the current
> > number of pending sectors is greater than the current block size is
> > checking that guarantee.  The current code already has a check in it to
> > see if this guarantee is observed ... you don't need to check it again.
> > 
> 
> I'm not checking for the guarantee, I'm checking that the read > 1 
> realsize_sector, before decrementing the number of _512_bytes_sectors by 
> realsize_sector, this check must be there, as after reading all but the last 
> sector, another request will be send with 1 realsize_sector size, for which
> (block + rq->nr_sectors) == get_capacity(disk) will still hold true, in this 
> case this_count must not be lowered, otherwise this_count will become 0 in this 
> case and the last sector will never get read.
> 
> I hope that clarifies why that code is there, if not can you tell how you would 
> want the code to look by just giving the full if statement as it should be, I 
> think we are somehow misunderstanding each other.

Oh, right; it's > rather than >= ... sorry, yes that's fine.

James

Re: PATCH: usb-storage-psc1350-v4.patch (was Linux scsi / usb-mass-storage and HP printer cardreader bug + fix)

[Hans de Goede]

James Bottomley wrote:
> On Mon, 2008-01-14 at 20:27 +0100, Hans de Goede wrote:
>> James Bottomley wrote:
>>>>> Plus what is the rq->nr_sectors > sdp->sector_size /
>>>>> 512 test supposed to be doing?  that being true is supposed to be a
>>>>> guarantee of the block layer (and if something goes wrong there's a
>>>>> check for this lower down).
>>>>>
>>>> It first is was just:
>>>> rq->nr_sectors > 1
>>>>
>>>> Then I changed it to also do the right thing for 1024 and larger sector disks. 
>>>> The whole purpose is to not read the last sector in a larger then one sector 
>>>> read, so the amount of sectors gets reduced by one if (block + rq->nr_sectors 
>>>> == get_capacity(disk)) but we do want still want to be able to read the last 
>>>> sector by itself, so we must not reduce the no sectors to be read by one if it 
>>>> is already one.
>>> Yes, I know that.  What I mean is the block subsystem sends reads and
>>> writes down in increments of the sector size.  Checking if the current
>>> number of pending sectors is greater than the current block size is
>>> checking that guarantee.  The current code already has a check in it to
>>> see if this guarantee is observed ... you don't need to check it again.
>>>
>> I'm not checking for the guarantee, I'm checking that the read > 1 
>> realsize_sector, before decrementing the number of _512_bytes_sectors by 
>> realsize_sector, this check must be there, as after reading all but the last 
>> sector, another request will be send with 1 realsize_sector size, for which
>> (block + rq->nr_sectors) == get_capacity(disk) will still hold true, in this 
>> case this_count must not be lowered, otherwise this_count will become 0 in this 
>> case and the last sector will never get read.
>>
>> I hope that clarifies why that code is there, if not can you tell how you would 
>> want the code to look by just giving the full if statement as it should be, I 
>> think we are somehow misunderstanding each other.
> 
> Oh, right; it's > rather than >= ... sorry, yes that's fine.
> 

Ok,

I got swamped @work this week so it took a while, but I've made a new seperate 
(scsi-sd only) patch with the cleanups as discussed.

I'm sending this (to the full recipient list) in a new top level post titled:
PATCH: scsi-sd-last-sector-bug-flag.patch

Regards,

Hans

(unknown),

[wangzhilei-pcEzf3JNfARxfCqBhyfcug]

subscribe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html