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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux