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

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).

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.

----- 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]>, [email protected], [email protected]
*Sent:* Wed, Jan 09 2008 at 23:44 +0200
*Subject:* Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

Quoted 57 lines

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.

drivers/usb/storage/protocol.c void usb_stor_ufi_command(struct scsi_cm · line 161

Quoted 7 lines

drivers/usb/storage/protocol.c renamed from drivers/usb/storage/protocol.c.psc1350 +7 -0

+		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?

Quoted 78 lines · 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]>, [email protected], [email protected]
*Sent:* Wed, Jan 09 2008 at 23:44 +0200
*Subject:* Linux scsi / usb-mass-storage and HP printer cardreader bug + fix

Quoted 57 lines

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.

drivers/usb/storage/protocol.c void usb_stor_ufi_command(struct scsi_cm · line 161

Quoted 7 lines

drivers/usb/storage/protocol.c renamed from drivers/usb/storage/protocol.c.psc1350 +7 -0

+		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.

Quoted 2 lines

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

Quoted 8 lines · Hans de Goede

Quoted 2 lines

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,

Quoted 1 line

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.

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

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.

Quoted 7 lines · Boaz Harrosh

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.

Quoted 7 lines · Boaz Harrosh

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 !

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 ?

Quoted 5 lines · Guillaume Bedot

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

Quoted 3 lines

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.

Quoted 10 lines · Hans de Goede

Quoted 8 lines · Guillaume Bedot

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 ?

Quoted 2 lines

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.

+ 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;

Quoted 10 lines · Matthew Dharm

Quoted 9 lines · Hans de Goede

Quoted 8 lines · Guillaume Bedot

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 ?

Quoted 5 lines

Quoted 1 line

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).

drivers/scsi/sd.c static int sd_prep_fn(struct request_que · line 399

Quoted 2 lines

drivers/scsi/sd.c renamed from drivers/scsi/sd.c.psc1350 +2 -0

+       /* 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
*/

Quoted 1 line

+ 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).

Quoted 1 line

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

rq->nr_sectors should be this_count

Quoted 1 line

+ 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;

Quoted 13 lines · James Bottomley

Quoted 12 lines · Matthew Dharm

Quoted 11 lines · Hans de Goede

Quoted 8 lines · Guillaume Bedot

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 9 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+       /* 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,

Quoted 4 lines

Quoted 1 line

+ 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.

Quoted 5 lines

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.

Quoted 4 lines

Quoted 1 line

+ 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?

Quoted 5 lines

Quoted 1 line

+ 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.

Quoted 8 lines

Quoted 5 lines

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

Quoted 1 line

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.

Quoted 15 lines · James Bottomley

Quoted 8 lines

Quoted 5 lines

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

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 16 lines · Hans de Goede

Quoted 15 lines · James Bottomley

Quoted 9 lines

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 10 lines · Matthew Dharm

Quoted 6 lines · Hans de Goede

Quoted 2 lines · Guillaume Bedot

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.

Quoted 2 lines

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 ...

Quoted 2 lines · Matthew Dharm

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 ...

Quoted 5 lines · James Bottomley

Quoted 2 lines · Matthew Dharm

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.