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
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
