scsi: Generalise last_sector_bug; fixes regression and potential future issues on USB cardreaders
A message that is not in the archive (sent off-list or not yet received): [email protected]
The last_sector_bug flag was added to work around a bug in certain usb cardreaders, where they would crash if a multiple sector read included the last sector. The original implementation avoids this by e.g. splitting an 8 sector read which includes the last sector into a 7 sector read, and a single sector read for the last sector. The flag is enabled for all USB devices.
This revealed a second bug in other usb cardreaders, which crash when they get a multiple sector read which stops 1 sector short of the last sector. Affected hardware includes the Kingston "MobileLite" external USB cardreader and the internal USB cardreader on the Asus EeePC.
Extend the last_sector_bug workaround to ensure that any access which touches the last 8 hardware sectors of the device is a single sector long. Requests are shrunk as necessary to meet this constraint.
This gives us a safety margin against potential unknown or future bugs affecting multi-sector access to the end of the device. The two known bugs only affect the last 2 sectors. However, they suggest that these devices are prone to fencepost errors and that multi-sector access to the end of the device is not well tested. Popular OS's use multi-sector accesses, but they rarely read the last few sectors. Linux (with udev & vol_id) automatically reads sectors from the end of the device on insertion. It is assumed that single sector accesses are more thoroughly tested during development.
drivers/scsi/sd.c +16 -6
@@ -406,13 +406,23 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) }/* -* Some devices (some sdcards for one) don't like it if the -* last sector gets read in a larger then 1 sector read. +* Some sd cardreaders can't handle multisector accesses which touch +* the last one or two hardware sectors. There are likely to be even +* buggier devices, so apply a workaround to the last eight sectors. */ -if (unlikely (sdp->last_sector_bug && - rq->nr_sectors > sdp->sector_size /512 && - block + this_count ==get_capacity (disk))) - this_count -= sdp->sector_size /512 ; +if (sdp->last_sector_bug) { +unsigned threshold =get_capacity (disk) -8 * (sdp->sector_size /512 ); + +if (block + this_count <= threshold) { + ;/* Okay as is */ + }else if (block < threshold) { +/* Access up to the threshold but not beyond */ + this_count = threshold - block; + }else { +/* Access only a single hardware sector */ + this_count = sdp->sector_size /512 ; + } + }SCSI_LOG_HLQUEUE (2 ,scmd_printk (KERN_INFO , SCpnt,"block=%llu\n" , (unsigned long long )block));
include/scsi/scsi_device.h +2 -1
@@ -139,7 +139,8 @@ struct scsi_device { unsigned fix_capacity:1; /* READ_CAPACITY is too high by 1 */ unsigned guess_capacity:1; /* READ_CAPACITY might be too high by 1 */ unsigned retry_hwerror:1; /* Retry HARDWARE_ERROR */ - unsigned last_sector_bug:1; /* Always read last sector in a 1 sector read */ + unsigned last_sector_bug:1; /* do not use multisector accesses on +the last 8 hardware sectors */ DECLARE_BITMAP (supported_events,SDEV_EVT_MAXBITS );/* supported events */ struct list_head event_list;/* asserted events */
James Bottomley
maintainerQuoted 36 lines · Alan Jenkins
The
last_sector_bugflag was added to work around a bug in certain usb cardreaders, where they would crash if a multiple sector read included the last sector. The original implementation avoids this by e.g. splitting an 8 sector read which includes the last sector into a 7 sector read, and a single sector read for the last sector. The flag is enabled for all USB devices.This revealed a second bug in other usb cardreaders, which crash when they get a multiple sector read which stops 1 sector short of the last sector. Affected hardware includes the Kingston "MobileLite" external USB cardreader and the internal USB cardreader on the Asus EeePC.
Extend the
last_sector_bugworkaround to ensure that any access which touches the last 8 hardware sectors of the device is a single sector long. Requests are shrunk as necessary to meet this constraint.This gives us a safety margin against potential unknown or future bugs affecting multi-sector access to the end of the device. The two known bugs only affect the last 2 sectors. However, they suggest that these devices are prone to fencepost errors and that multi-sector access to the end of the device is not well tested. Popular OS's use multi-sector accesses, but they rarely read the last few sectors. Linux (with udev &
vol_id) automatically reads sectors from the end of the device on insertion. It is assumed that single sector accesses are more thoroughly tested during development.Signed-off-by Alan Jenkins <[email protected]>Tested-by Alan Jenkins <[email protected]>drivers/scsi/sd.c +0
-1@@ -406,13 +406,23 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) }/* -* Some devices (some sdcards for one) don't like it if the
drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 413
Quoted 10 lines
drivers/scsi/sd.c +4
-5-* last sector gets read in a larger then 1 sector read. +* Some sd cardreaders can't handle multisector accesses which touch +* the last one or two hardware sectors. There are likely to be even +* buggier devices, so apply a workaround to the last eight sectors. */ -if (unlikely (sdp->last_sector_bug && - rq->nr_sectors > sdp->sector_size /512 && - block + this_count ==get_capacity (disk))) - this_count -= sdp->sector_size /512 ; +if (sdp->last_sector_bug) {
This should be unlikely() as the previous one was
drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 414
Quoted 1 line
drivers/scsi/sd.c +1
-0+unsigned threshold =get_capacity (disk) -8 * (sdp->sector_size /512 );
This can't be an unsigned, it has to be sector_t otherwise it could overflow on large capacity drives.
I can also see that someone will find a drive that needs the last 16 or something sectors, so perhaps the bare 8 should be a nice #define in sd.h (comment above would need altering too).
drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 416
Quoted 1 line
drivers/scsi/sd.c +1
-0+if (block + this_count <= threshold) {
This should be likely() as well.
Quoted 17 lines
drivers/scsi/sd.c +9
-0+ ;/* Okay as is */ + }else if (block < threshold) { +/* Access up to the threshold but not beyond */ + this_count = threshold - block; + }else { +/* Access only a single hardware sector */ + this_count = sdp->sector_size /512 ; + } + }SCSI_LOG_HLQUEUE (2 ,scmd_printk (KERN_INFO , SCpnt,"block=%llu\n" , (unsigned long long )block));include/scsi/scsi_device.h +0
-0
include/scsi/scsi_device.h struct scsi_device { · line 146
Quoted 9 lines
include/scsi/scsi_device.h +2
-1@@ -139,7 +139,8 @@ struct scsi_device { unsigned fix_capacity:1; /* READ_CAPACITY is too high by 1 */ unsigned guess_capacity:1; /* READ_CAPACITY might be too high by 1 */ unsigned retry_hwerror:1; /* Retry HARDWARE_ERROR */ - unsigned last_sector_bug:1; /* Always read last sector in a 1 sector read */ + unsigned last_sector_bug:1; /* do not use multisector accesses on +the last 8 hardware sectors */ DECLARE_BITMAP (supported_events,SDEV_EVT_MAXBITS );/* supported events */ struct list_head event_list;/* asserted events */
Everything else looks fine ... do a quick turn around and I'll slide it under the merge window.
Thanks,
James
Quoted 36 lines · James Bottomley
Quoted 34 lines · Alan Jenkins
The
last_sector_bugflag was added to work around a bug in certain usb cardreaders, where they would crash if a multiple sector read included the last sector. The original implementation avoids this by e.g. splitting an 8 sector read which includes the last sector into a 7 sector read, and a single sector read for the last sector. The flag is enabled for all USB devices.This revealed a second bug in other usb cardreaders, which crash when they get a multiple sector read which stops 1 sector short of the last sector. Affected hardware includes the Kingston "MobileLite" external USB cardreader and the internal USB cardreader on the Asus EeePC.
Extend the
last_sector_bugworkaround to ensure that any access which touches the last 8 hardware sectors of the device is a single sector long. Requests are shrunk as necessary to meet this constraint.This gives us a safety margin against potential unknown or future bugs affecting multi-sector access to the end of the device. The two known bugs only affect the last 2 sectors. However, they suggest that these devices are prone to fencepost errors and that multi-sector access to the end of the device is not well tested. Popular OS's use multi-sector accesses, but they rarely read the last few sectors. Linux (with udev &
vol_id) automatically reads sectors from the end of the device on insertion. It is assumed that single sector accesses are more thoroughly tested during development.Signed-off-by Alan Jenkins <[email protected]>Tested-by Alan Jenkins <[email protected]>drivers/scsi/sd.c +0
-0@@ -406,13 +406,23 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) }
Quoted 16 lines
drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 426
Quoted 13 lines
drivers/scsi/sd.c +4
-6/* -* Some devices (some sdcards for one) don't like it if the -* last sector gets read in a larger then 1 sector read. +* Some sd cardreaders can't handle multisector accesses which touch +* the last one or two hardware sectors. There are likely to be even +* buggier devices, so apply a workaround to the last eight sectors. */ -if (unlikely (sdp->last_sector_bug && - rq->nr_sectors > sdp->sector_size /512 && - block + this_count ==get_capacity (disk))) - this_count -= sdp->sector_size /512 ; +if (sdp->last_sector_bug) {This should be
unlikely()as the previous one was
Fine. I'll change it back to a single combined unlikely() test.
threshold = ...
if (unlikely(sdp->last_sector_bug && block + this_count > threshold)) {
Quoted 6 lines
drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 426
Quoted 2 lines
drivers/scsi/sd.c +1
-0+unsigned threshold =get_capacity (disk) -8 * (sdp->sector_size /512 );This can't be an unsigned, it has to be
sector_totherwise it could overflow on large capacity drives.
Ok.
Quoted 5 lines
I can also see that someone will find a drive that needs the last 16 or something sectors, so perhaps the bare 8 should be a nice #define in
sd.h(comment above would need altering too).
Good idea.
Quoted 7 lines
drivers/scsi/sd.c static int sd_prep_fn(struct request_queue *q, struct request *rq) · line 426
Quoted 3 lines
drivers/scsi/sd.c +1
-0+if (block + this_count <= threshold) {This should be
likely()as well.
(see above)
Quoted 35 lines
Quoted 19 lines
drivers/scsi/sd.c +9
-0+ ;/* Okay as is */ + }else if (block < threshold) { +/* Access up to the threshold but not beyond */ + this_count = threshold - block; + }else { +/* Access only a single hardware sector */ + this_count = sdp->sector_size /512 ; + } + }SCSI_LOG_HLQUEUE (2 ,scmd_printk (KERN_INFO , SCpnt,"block=%llu\n" , (unsigned long long )block));include/scsi/scsi_device.h +0
-0include/scsi/scsi_device.h struct scsi_device { · line 146
Quoted 9 lines
include/scsi/scsi_device.h +2
-1@@ -139,7 +139,8 @@ struct scsi_device { unsigned fix_capacity:1; /* READ_CAPACITY is too high by 1 */ unsigned guess_capacity:1; /* READ_CAPACITY might be too high by 1 */ unsigned retry_hwerror:1; /* Retry HARDWARE_ERROR */ - unsigned last_sector_bug:1; /* Always read last sector in a 1 sector read */ + unsigned last_sector_bug:1; /* do not use multisector accesses on +the last 8 hardware sectors */ DECLARE_BITMAP (supported_events,SDEV_EVT_MAXBITS );/* supported events */ struct list_head event_list;/* asserted events */ Everything else looks fine ... do a quick turn around and I'll slide it under the merge window.
Thanks,
James
PATCH 1/1 scsi: Generalise last_sector_bug; fixes regression and potential future issues on USB cardreaders
The last_sector_bug flag was added to work around a bug in certain usb cardreaders, where they would crash if a multiple sector read included the last sector. The original implementation avoids this by e.g. splitting an 8 sector read which includes the last sector into a 7 sector read, and a single sector read for the last sector. The flag is enabled for all USB devices.
This revealed a second bug in other usb cardreaders, which crash when they get a multiple sector read which stops 1 sector short of the last sector. Affected hardware includes the Kingston "MobileLite" external USB cardreader and the internal USB cardreader on the Asus EeePC.
Extend the last_sector_bug workaround to ensure that any access which touches the last 8 hardware sectors of the device is a single sector long. Requests are shrunk as necessary to meet this constraint.
This gives us a safety margin against potential unknown or future bugs affecting multi-sector access to the end of the device. The two known bugs only affect the last 2 sectors. However, they suggest that these devices are prone to fencepost errors and that multi-sector access to the end of the device is not well tested. Popular OS's use multi-sector accesses, but they rarely read the last few sectors. Linux (with udev & vol_id) automatically reads sectors from the end of the device on insertion. It is assumed that single sector accesses are more thoroughly tested during development.
drivers/scsi/sd.c +15 -6
@@ -360,6 +360,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)struct scsi_device *sdp = q->queuedata;struct gendisk *disk = rq->rq_disk;sector_t block = rq->sector; +sector_t threshold;unsigned int this_count = rq->nr_sectors;unsigned int timeout = sdp->timeout;int ret;
@@ -406,13 +407,21 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq) }/* -* Some devices (some sdcards for one) don't like it if the -* last sector gets read in a larger then 1 sector read. +* Some SD card readers can't handle multi-sector accesses which touch +* the last one or two hardware sectors. Split accesses as needed. */ -if (unlikely (sdp->last_sector_bug && - rq->nr_sectors > sdp->sector_size /512 && - block + this_count ==get_capacity (disk))) - this_count -= sdp->sector_size /512 ; + threshold =get_capacity (disk) -SD_LAST_BUGGY_SECTORS * + (sdp->sector_size /512 ); + +if (unlikely (sdp->last_sector_bug && block + this_count > threshold)) { +if (block < threshold) { +/* Access up to the threshold but not beyond */ + this_count = threshold - block; + }else { +/* Access only a single hardware sector */ + this_count = sdp->sector_size /512 ; + } + }SCSI_LOG_HLQUEUE (2 ,scmd_printk (KERN_INFO , SCpnt,"block=%llu\n" , (unsigned long long )block));
include/scsi/scsi_device.h +2 -1
@@ -139,7 +139,8 @@ struct scsi_device { unsigned fix_capacity:1; /* READ_CAPACITY is too high by 1 */ unsigned guess_capacity:1; /* READ_CAPACITY might be too high by 1 */ unsigned retry_hwerror:1; /* Retry HARDWARE_ERROR */ - unsigned last_sector_bug:1; /* Always read last sector in a 1 sector read */ + unsigned last_sector_bug:1; /* do not use multisector accesses on +SD_LAST_BUGGY_SECTORS */ DECLARE_BITMAP (supported_events,SDEV_EVT_MAXBITS );/* supported events */ struct list_head event_list;/* asserted events */
include/scsi/sd.h +6 -0
@@ -31,6 +31,12 @@*/ #define SD_BUF_SIZE 512 +/* +* Number of sectors at the end of the device to avoid multi-sector accesses to +* in the case of last_sector_bug +*/ +#define SD_LAST_BUGGY_SECTORS 8 +struct scsi_disk {struct scsi_driver *driver;/* always &sd_template */ struct scsi_device *device;