2.4.20: possibly wrong handling of removeable scsi disks
[Please CC me on replies, I'm not subscribed to the list]
There is possibly a wrong handling of removable disks with no medium inserted in the sd driver. From 2.4.20 drivers/scsi/sd.c (...=snip):
static int sd_init_onedisk(int i)
{
...
do {
retries = 0;
while (retries < 3) {
cmd[0] = TEST_UNIT_READY;
...
if (the_result == 0
|| SRpnt->sr_sense_buffer[2] != UNIT_ATTENTION)
break;
}
/*
* If the drive has indicated to us that it doesn't have
* any media in it, don't bother with any of the rest of
* this crap.
*/
if( the_result != 0
&& ((driver_byte(the_result) & DRIVER_SENSE) != 0)
===> && SRpnt->sr_sense_buffer[2] == UNIT_ATTENTION
&& SRpnt->sr_sense_buffer[12] == 0x3A ) {
rscsi_disks[i].capacity = 0x1fffff;
sector_size = 512;
rscsi_disks[i].device->changed = 1;
rscsi_disks[i].ready = 0;
===> break;
}
...
} while (the_result && spintime &&
time_after(spintime_value + 100 * HZ, jiffies));...
Now look at the marked (===>) lines above. I dont believe the test for UNIT_ATTENTION is correct. As far as I could find out the sense information from TEST_UNIT_READY should be either NO_SENSE, ILLEGAL_REQUEST or NOT_READY. As there is a check for 'medium not present' (0x3A) the test should be for NOT_READY instead of UNIT_ATTENTION. Furthermore the 'break;' statement seems wrong to me as the function lateron does e.g. things like READ_CAPACITY which doesn't make any sense if no medium is present. The correct statement seems to me 'return i;'.
As a sidenote sd_init_onedisk() should be changed to return void. It is static and the return value is nowhere used.
If my above assumptions are right please let me know, I'll submit patches in this case.
Quoted 10 lines · On 2003.02.23 07:17 Andreas Steinmetz wrote:
if( the_result != 0
&& ((driver_byte(the_result) & DRIVER_SENSE) != 0)
===> && SRpnt->sr_sense_buffer[2] == UNIT_ATTENTION
&& SRpnt->sr_sense_buffer[12] == 0x3A ) {
rscsi_disks[i].capacity = 0x1fffff;
sector_size = 512;
rscsi_disks[i].device->changed = 1;
rscsi_disks[i].ready = 0;
...Quoted 6 lines
Now look at the marked (===>) lines above. I dont believe the test for
UNIT_ATTENTIONis correct. As far as I could find out the sense information fromTEST_UNIT_READYshould be eitherNO_SENSE,ILLEGAL_REQUESTorNOT_READY. As there is a check for 'medium not present' (0x3A) the test should be forNOT_READYinstead ofUNIT_ATTENTION. ger.kernel.org/majordomo-info.html
You are right about this one. It should be NOT_READY, value 0x02. As a matter of fact, ASQ 0x3A is unique, so you could just delete the test of sr_sense_buffer[2] and it would work as expected.
Success, Willem Riede.
Quoted 12 lines · Willem Riede
Quoted 10 lines · On 2003.02.23 07:17 Andreas Steinmetz wrote:
if( the_result != 0 && ((driver_byte(the_result) & DRIVER_SENSE) != 0) ===> && SRpnt->sr_sense_buffer[2] == UNIT_ATTENTION && SRpnt->sr_sense_buffer[12] == 0x3A ) { rscsi_disks[i].capacity = 0x1fffff; sector_size = 512; rscsi_disks[i].device->changed = 1; rscsi_disks[i].ready = 0;...
Quoted 7 lines
Quoted 1 line
Now look at the marked (===>) lines above. I dont believe the test for
UNIT_ATTENTIONis correct. As far as I could find out the sense information fromTEST_UNIT_READYshould be eitherNO_SENSE,ILLEGAL_REQUESTorNOT_READY. As there is a check for 'medium not present' (0x3A) the test should be forNOT_READYinstead ofUNIT_ATTENTION. ger.kernel.org/majordomo-info.htmlYou are right about this one. It should be
NOT_READY, value0x02. As a matter of fact, ASQ0x3Ais unique, so you could just delete the test of sr_sense_buffer[2] and it would work as expected.
OK,
this still leaves the question why, if a device reports 'no medium found', the code still tries to do a READ_CAPACITY (multiple times). Capacity reporting depends on available medium, isn't possible when there's no medium found and causes unneeded communication and cpu overhead as well as syslog spamming.
Quoted 26 lines · On Sun, 23 Feb 2003 14:41:00 +0100 Andreas Steinmetz <[email protected]> wrote:
Quoted 20 lines · Willem Riede
Quoted 8 lines · On 2003.02.23 07:17 Andreas Steinmetz wrote:
if( the_result != 0 && ((driver_byte(the_result) & DRIVER_SENSE) != 0) ===> && SRpnt->sr_sense_buffer[2] == UNIT_ATTENTION && SRpnt->sr_sense_buffer[12] == 0x3A ) { rscsi_disks[i].capacity = 0x1fffff; sector_size = 512; rscsi_disks[i].device->changed = 1; rscsi_disks[i].ready = 0;Quoted 8 lines
...
Now look at the marked (===>) lines above. I dont believe the test for
UNIT_ATTENTIONis correct. As far as I could find out the sense information fromTEST_UNIT_READYshould be eitherNO_SENSE,ILLEGAL_REQUESTorNOT_READY. As there is a check for 'medium not present' (0x3A) the test should be forNOT_READYinstead ofUNIT_ATTENTION. ger.kernel.org/majordomo-info.htmlQuoted 4 lines
You are right about this one. It should be
NOT_READY, value0x02. As a matter of fact, ASQ0x3Ais unique, so you could just delete the test of sr_sense_buffer[2] and it would work as expected.
Quoted 7 lines
OK,
this still leaves the question why, if a device reports 'no medium found', the code still tries to do aREAD_CAPACITY(multiple times). Capacity reporting depends on available medium, isn't possible when there's no medium found and causes unneeded communication and cpu overhead as well as syslog spamming.
Andreas,
Yes, I was/am also annoyed by the syslog spamming. In my case, it was with a "permanently" connected USB compact flash reader that was typically empty while booting. I posted a patch (available on request) for this in 2.4.19, but it did not get "resolved" into a form suitable for incorporation. I would have to go back and refresh my memory, but as I recall this test needs to be for "NOT READY" and "UNIT ATTENTION", as the later will occur if the media was removed just prior to the test being performed.
After later reflection, I realized that my patch might have caused problems with some seriously "non-conforming" disk drives and thus I never "pushed" it. To truly solve this problem, I believe we would need to add a status flag to the scsi structure indicating that the last examination of the device determined that there was no media present. This status flag would then be used to bypass the subsequent Read Capacity (and partition table reading) commands.
Regards, Scott.
Quoted 21 lines · Andreas Steinmetz
There is possibly a wrong handling of removable disks with no medium inserted in the sd driver. From 2.4.20
drivers/scsi/sd.c(...=snip):/* * If the drive has indicated to us that it doesn't have * any media in it, don't bother with any of the rest of * this crap. */ if( the_result != 0 && ((driver_byte(the_result) & DRIVER_SENSE) != 0) ===> && SRpnt->sr_sense_buffer[2] == UNIT_ATTENTION && SRpnt->sr_sense_buffer[12] == 0x3A ) { rscsi_disks[i].capacity = 0x1fffff; sector_size = 512; rscsi_disks[i].device->changed = 1; rscsi_disks[i].ready = 0; ===> break; } ... } while (the_result && spintime && time_after(spintime_value + 100 * HZ, jiffies));
Quoted 8 lines
...
Now look at the marked (===>) lines above. I dont believe the test for
UNIT_ATTENTIONis correct. As far as I could find out the sense information fromTEST_UNIT_READYshould be eitherNO_SENSE,ILLEGAL_REQUESTorNOT_READY. As there is a check for 'medium not present' (0x3A) the test should be forNOT_READYinstead ofUNIT_ATTENTION.
I wrote in 2.5:
static int media_not_present(struct scsi_disk *sdkp, struct scsi_request *srp)
{
if (!srp->sr_result)
return 0;
if (!(driver_byte(srp->sr_result) & DRIVER_SENSE))
return 0;
if (srp->sr_sense_buffer[2] != NOT_READY &&
srp->sr_sense_buffer[2] != UNIT_ATTENTION)
return 0;
if (srp->sr_sense_buffer[12] != 0x3A) /* medium not present */
return 0;
set_media_not_present(sdkp);
return 1;
}Quoted 3 lines
Furthermore the 'break;' statement seems wrong to me as
the function lateron does e.g. things likeREAD_CAPACITYwhich doesn't make any sense if no medium is present.
I wrote in 2.5:
if (sdkp->media_present)
sd_read_capacity(sdkp, disk->disk_name, SRpnt, buffer);That code can be backported.
Andries
Quoted 9 lines · On Tue, 25 Feb 2003 00:10:55 +0100 Andries Brouwer <[email protected]> wrote:
On Sun, Feb 23, 2003 at 01:17:07PM +0100, Andreas Steinmetz wrote:
I wrote in 2.5:
if (sdkp->media_present) sd_read_capacity(sdkp, disk->disk_name, SRpnt, buffer);That code can be backported.
The code you posted looks great for the problem that I've been having with empty compact flash card readers.
I would certainly vote for backporting these changes to the 2.4 chain :) In fact, I'd even offer to attempt the backport myself if we can't find a more qualified volunteer.
Thanks, Scott.