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
