On Mon, 2008-01-14 at 19:37 +0100, Hans de Goede wrote: > 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. 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. > >> + 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 is the adjusted sector size. At the moment, there's no size transformation in the code between your check and the top (where this_count is set to rq->nr_sectors). But if there were (and someone might add one one day) you'd be wanting to check the adjusted size (i.e. this_count). > >> + 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. OK, point taken, it would be worse. I think today's compilers are happy to translate x/512 to x>>9, so it shouldn't be that slow. James - 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
