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

2008-01-11 Thread Hans de Goede

Boaz Harrosh wrote:

On Thu, Jan 10 2008 at 12:52 +0200, Hans de Goede [EMAIL PROTECTED] 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


-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2008-01-11 Thread 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.



-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2008-01-10 Thread Boaz Harrosh
- Original Message -
*From:* Hans de Goede [EMAIL PROTECTED]
*To:* USB Storage list [EMAIL PROTECTED]
*CC:* [EMAIL PROTECTED], USB development list
[EMAIL PROTECTED], David Brown
[EMAIL PROTECTED], Guillaume Bedot [EMAIL PROTECTED],
linux-scsi@vger.kernel.org, [EMAIL PROTECTED]
*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

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2008-01-10 Thread Hans de Goede

Boaz Harrosh wrote:

- Original Message -
*From:* Hans de Goede [EMAIL PROTECTED]
*To:* USB Storage list [EMAIL PROTECTED]
*CC:* [EMAIL PROTECTED], USB development list
[EMAIL PROTECTED], David Brown
[EMAIL PROTECTED], Guillaume Bedot [EMAIL PROTECTED],
linux-scsi@vger.kernel.org, [EMAIL PROTECTED]
*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

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2008-01-10 Thread Boaz Harrosh
On Thu, Jan 10 2008 at 12:52 +0200, Hans de Goede [EMAIL PROTECTED] wrote:
 Boaz Harrosh wrote:
 - Original Message -
 *From:* Hans de Goede [EMAIL PROTECTED]
 *To:* USB Storage list [EMAIL PROTECTED]
 *CC:* [EMAIL PROTECTED], USB development list
 [EMAIL PROTECTED], David Brown
 [EMAIL PROTECTED], Guillaume Bedot [EMAIL PROTECTED],
 linux-scsi@vger.kernel.org, [EMAIL PROTECTED]
 *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
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html