sd: Refactor sd_read_capacity()
The sd_read_capacity() function was about 180 lines long and included a backwards goto and a tricky state variable. Splitting out read_capacity_10() and read_capacity_16() (about 50 lines each) reduces sd_read_capacity to about 100 lines and gets rid of the backwards goto and the state variable. I've tried to avoid any behaviour change with this patch.
--- drivers/scsi/sd.c | 241 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 155 insertions(+), 86 deletions(-)
drivers/scsi/sd.c +155 -86
@@ -1274,42 +1274,61 @@ disable: sdkp->capacity =0 ; } -/* -* read disk capacity -*/ -static void -sd_read_capacity (struct scsi_disk *sdkp,unsigned char *buffer) +static void read_capacity_error (struct scsi_disk *sdkp,struct scsi_device *sdp, +struct scsi_sense_hdr *sshdr,int sense_valid, +int the_result) +{ +sd_print_result (sdkp, the_result); +if (driver_byte (the_result) &DRIVER_SENSE ) +sd_print_sense_hdr (sdkp, sshdr); +else +sd_printk (KERN_NOTICE , sdkp,"Sense not available.\n" ); + +/* +* Set dirty bit for removable devices if not ready - +* sometimes drives will not report this properly. +*/ +if (sdp->removable && + sense_valid && sshdr->sense_key ==NOT_READY ) + sdp->changed =1 ; + +/* +* We used to set media_present to 0 here to indicate no media +* in the drive, but some drives fail read capacity even with +* media present, so we can't do that. +*/ + sdkp->capacity =0 ;/* unknown mapped to zero - as usual */ +} + +#define RC16_LEN 13 +#if RC16_LEN >SD_BUF_SIZE +#error RC16_LEN must not be more than SD_BUF_SIZE +#endif + +static int read_capacity_16 (struct scsi_disk *sdkp,struct scsi_device *sdp, +unsigned char *buffer) {unsigned char cmd[16 ]; -int the_result, retries; -int sector_size =0 ; -/* Force READ CAPACITY(16) when PROTECT=1 */ -int longrc =scsi_device_protection (sdkp->device) ?1 :0 ;struct scsi_sense_hdr sshdr;int sense_valid =0 ; -struct scsi_device *sdp = sdkp->device; +int the_result; +int retries =3 ; +unsigned long long lba; +unsigned sector_size; -repeat: - retries =3 ; do { - if (longrc) { - memset((void *) cmd, 0, 16); - cmd[0] = SERVICE_ACTION_IN; - cmd[1] = SAI_READ_CAPACITY_16; - cmd[13] = 13; - memset((void *) buffer, 0, 13); - } else { - cmd[0] = READ_CAPACITY; - memset((void *) &cmd[1], 0, 9); - memset((void *) buffer, 0, 8); - } - + memset(cmd, 0, 16); + cmd[0] = SERVICE_ACTION_IN; + cmd[1] = SAI_READ_CAPACITY_16; + cmd[13] = RC16_LEN; + memset(buffer, 0, RC16_LEN); + the_result = scsi_execute_req(sdp, cmd, DMA_FROM_DEVICE, - buffer, longrc ? 13 : 8, &sshdr, - SD_TIMEOUT, SD_MAX_RETRIES); + buffer, RC16_LEN, &sshdr, + SD_TIMEOUT, SD_MAX_RETRIES); if (media_not_present(sdkp, &sshdr)) - return; + return -ENODEV; if (the_result) sense_valid = scsi_sense_valid(&sshdr);
@@ -1317,72 +1336,122 @@ repeat: }while (the_result && retries); -if (the_result && !longrc) { +if (the_result) { +sd_printk (KERN_NOTICE , sdkp,"READ CAPACITY(16) failed\n" ); +read_capacity_error (sdkp, sdp, &sshdr, sense_valid, the_result); +return -EINVAL ; + } + + sector_size = (buffer[8 ] <<24 ) | (buffer[9 ] <<16 ) | + (buffer[10 ] <<8 ) | buffer[11 ]; + lba = (((u64 )buffer[0 ] <<56 ) | ((u64 )buffer[1 ] <<48 ) | + ((u64 )buffer[2 ] <<40 ) | ((u64 )buffer[3 ] <<32 ) | + ((u64 )buffer[4 ] <<24 ) | ((u64 )buffer[5 ] <<16 ) | + ((u64 )buffer[6 ] <<8 ) | (u64 )buffer[7 ]); + +sd_read_protection_type (sdkp, buffer); + +if ((sizeof (sdkp->capacity) ==4 ) && (lba >=0xffffffffULL )) { +sd_printk (KERN_ERR , sdkp,"Too big for this kernel. Use a " +"kernel compiled with support for large block " +"devices.\n" ); + sdkp->capacity =0 ; +return -EOVERFLOW ; + } + + sdkp->capacity = lba +1 ; +return sector_size; +} + +static int read_capacity_10 (struct scsi_disk *sdkp,struct scsi_device *sdp, +unsigned char *buffer) +{ +unsigned char cmd[16 ]; +struct scsi_sense_hdr sshdr; +int sense_valid =0 ; +int the_result; +int retries =3 ; +sector_t lba; +unsigned sector_size; + +do { + cmd[0 ] =READ_CAPACITY ; +memset (&cmd[1 ],0 ,9 ); +memset (buffer,0 ,8 ); + + the_result =scsi_execute_req (sdp, cmd,DMA_FROM_DEVICE , + buffer,8 , &sshdr, +SD_TIMEOUT ,SD_MAX_RETRIES ); + +if (media_not_present (sdkp, &sshdr)) +return -ENODEV ; + +if (the_result) + sense_valid =scsi_sense_valid (&sshdr); + retries--; + + }while (the_result && retries); + +if (the_result) {sd_printk (KERN_NOTICE , sdkp,"READ CAPACITY failed\n" ); -sd_print_result (sdkp, the_result); -if (driver_byte (the_result) &DRIVER_SENSE ) -sd_print_sense_hdr (sdkp, &sshdr); -else -sd_printk (KERN_NOTICE , sdkp,"Sense not available.\n" ); +read_capacity_error (sdkp, sdp, &sshdr, sense_valid, the_result); +return -EINVAL ; + } -/* Set dirty bit for removable devices if not ready - -* sometimes drives will not report this properly. */ -if (sdp->removable && - sense_valid && sshdr.sense_key ==NOT_READY ) - sdp->changed =1 ; + sector_size = (buffer[4 ] <<24 ) | (buffer[5 ] <<16 ) | + (buffer[6 ] <<8 ) | buffer[7 ]; + lba = (buffer[0 ] <<24 ) | (buffer[1 ] <<16 ) | + (buffer[2 ] <<8 ) | buffer[3 ]; -/* Either no media are present but the drive didn't tell us, -or they are present but the read capacity command fails */ -/* sdkp->media_present = 0; -- not always correct */ - sdkp->capacity =0 ;/* unknown mapped to zero - as usual */ +if ((sizeof (sdkp->capacity) ==4 ) && (lba ==0xffffffff )) { +sd_printk (KERN_ERR , sdkp,"Too big for this kernel. Use a " +"kernel compiled with support for large block " +"devices.\n" ); + sdkp->capacity =0 ; +return -EOVERFLOW ; + } -return ; - }else if (the_result && longrc) { -/* READ CAPACITY(16) has been failed */ -sd_printk (KERN_NOTICE , sdkp,"READ CAPACITY(16) failed\n" ); -sd_print_result (sdkp, the_result); -sd_printk (KERN_NOTICE , sdkp,"Use 0xffffffff as device size\n" ); + sdkp->capacity = lba +1 ; +return sector_size; +} - sdkp->capacity =1 + (sector_t )0xffffffff ; - goto got_data; - } - -if (!longrc) { - sector_size = (buffer[4 ] <<24 ) | - (buffer[5 ] <<16 ) | (buffer[6 ] <<8 ) | buffer[7 ]; -if (buffer[0 ] ==0xff && buffer[1 ] ==0xff && - buffer[2 ] ==0xff && buffer[3 ] ==0xff ) { -if (sizeof (sdkp->capacity) >4 ) { -sd_printk (KERN_NOTICE , sdkp,"Very big device. " -"Trying to use READ CAPACITY(16).\n" ); - longrc =1 ; - goto repeat; - } -sd_printk (KERN_ERR , sdkp,"Too big for this kernel. Use " -"a kernel compiled with support for large " -"block devices.\n" ); - sdkp->capacity =0 ; +/* +* read disk capacity +*/ +static void +sd_read_capacity (struct scsi_disk *sdkp,unsigned char *buffer) +{ +int sector_size; +struct scsi_device *sdp = sdkp->device; + +/* Force READ CAPACITY(16) when PROTECT=1 */ +if (scsi_device_protection (sdp)) { + sector_size =read_capacity_16 (sdkp, sdp, buffer); +if (sector_size == -EOVERFLOW ) goto got_data; - } - sdkp->capacity =1 + (((sector_t )buffer[0 ] <<24 ) | - (buffer[1 ] <<16 ) | - (buffer[2 ] <<8 ) | - buffer[3 ]); +if (sector_size <0 ) +return ; }else { - sdkp->capacity =1 + (((u64 )buffer[0 ] <<56 ) | - ((u64 )buffer[1 ] <<48 ) | - ((u64 )buffer[2 ] <<40 ) | - ((u64 )buffer[3 ] <<32 ) | - ((sector_t )buffer[4 ] <<24 ) | - ((sector_t )buffer[5 ] <<16 ) | - ((sector_t )buffer[6 ] <<8 ) | - (sector_t )buffer[7 ]); - - sector_size = (buffer[8 ] <<24 ) | - (buffer[9 ] <<16 ) | (buffer[10 ] <<8 ) | buffer[11 ]; - -sd_read_protection_type (sdkp, buffer); - } + sector_size =read_capacity_10 (sdkp, sdp, buffer); +if (sector_size == -EOVERFLOW ) + goto got_data; +if (sector_size <0 ) +return ; +if ((sizeof (sdkp->capacity) >4 ) && + (sdkp->capacity >0xffffffffULL )) { +int old_sector_size = sector_size; +sd_printk (KERN_NOTICE , sdkp,"Very big device. " +"Trying to use READ CAPACITY(16).\n" ); + sector_size =read_capacity_16 (sdkp, sdp, buffer); +if (sector_size <0 ) { +sd_printk (KERN_NOTICE , sdkp, +"Using 0xffffffff as device size\n" ); + sdkp->capacity =1 + (sector_t )0xffffffff ; + sector_size = old_sector_size; + goto got_data; + } + } + }/* Some devices return the total number of sectors, not the * highest sector number. Make the necessary adjustment. */
Signature · 4 lines
Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."
Quoted 6 lines · Matthew Wilcox
The
sd_read_capacity()function was about 180 lines long and included a backwards goto and a tricky state variable. Splitting outread_capacity_10()andread_capacity_16()(about 50 lines each) reducessd_read_capacityto about 100 lines and gets rid of the backwards goto and the state variable. I've tried to avoid any behaviour change with this patch.
I should have probably said what testing I've done (with both patches applied, not just this one).
I've tried numerous <= SCSI-2 devices -- USB keys, older SCSI drives ... even newer SCSI drives still seem to claim SCSI-2 compliance. Almost all my equipment claims ANSI rev 2.
I've tried a few libata devices (which claim ANSI rev 5). I've tried a WD My Book (attached via firewire) which claims ANSI rev 4. I've also tried an HP fibre channel array which claims ANSI rev 3.
I can't claim to have exercised all the error paths here.
Signature · 4 lines
Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step."