* 2.4.20: possibly wrong handling of removeable scsi disks
@ 2003-02-23 12:17 Andreas Steinmetz
2003-02-23 13:20 ` Willem Riede
2003-02-24 23:10 ` Andries Brouwer
0 siblings, 2 replies; 6+ messages in thread
From: Andreas Steinmetz @ 2003-02-23 12:17 UTC (permalink / raw)
To: linux-scsi
[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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.4.20: possibly wrong handling of removeable scsi disks
2003-02-23 12:17 2.4.20: possibly wrong handling of removeable scsi disks Andreas Steinmetz
@ 2003-02-23 13:20 ` Willem Riede
2003-02-23 13:41 ` Andreas Steinmetz
2003-02-24 23:10 ` Andries Brouwer
1 sibling, 1 reply; 6+ messages in thread
From: Willem Riede @ 2003-02-23 13:20 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: linux-scsi
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;
> ...
>
> 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. 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.4.20: possibly wrong handling of removeable scsi disks
2003-02-23 13:20 ` Willem Riede
@ 2003-02-23 13:41 ` Andreas Steinmetz
2003-02-23 16:14 ` Scott Merritt
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Steinmetz @ 2003-02-23 13:41 UTC (permalink / raw)
To: wrlk; +Cc: linux-scsi
Willem Riede wrote:
> 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;
>> ...
>>
>>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. 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.
>
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.4.20: possibly wrong handling of removeable scsi disks
2003-02-23 13:41 ` Andreas Steinmetz
@ 2003-02-23 16:14 ` Scott Merritt
0 siblings, 0 replies; 6+ messages in thread
From: Scott Merritt @ 2003-02-23 16:14 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: wrlk, linux-scsi
On Sun, 23 Feb 2003 14:41:00 +0100
Andreas Steinmetz <ast@domdv.de> wrote:
> Willem Riede wrote:
> > 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;
> >> ...
> >>
> >>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. 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.
> >
>
> 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.
>
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.4.20: possibly wrong handling of removeable scsi disks
2003-02-23 12:17 2.4.20: possibly wrong handling of removeable scsi disks Andreas Steinmetz
2003-02-23 13:20 ` Willem Riede
@ 2003-02-24 23:10 ` Andries Brouwer
2003-02-24 23:46 ` Scott Merritt
1 sibling, 1 reply; 6+ messages in thread
From: Andries Brouwer @ 2003-02-24 23:10 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: linux-scsi
On Sun, Feb 23, 2003 at 01:17:07PM +0100, Andreas Steinmetz wrote:
> 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));
> ...
>
> 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.
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;
}
> 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.
I wrote in 2.5:
if (sdkp->media_present)
sd_read_capacity(sdkp, disk->disk_name, SRpnt, buffer);
That code can be backported.
Andries
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: 2.4.20: possibly wrong handling of removeable scsi disks
2003-02-24 23:10 ` Andries Brouwer
@ 2003-02-24 23:46 ` Scott Merritt
0 siblings, 0 replies; 6+ messages in thread
From: Scott Merritt @ 2003-02-24 23:46 UTC (permalink / raw)
To: Andries Brouwer; +Cc: linux-scsi
On Tue, 25 Feb 2003 00:10:55 +0100
Andries Brouwer <aebr@win.tue.nl> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2003-02-24 23:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-02-23 12:17 2.4.20: possibly wrong handling of removeable scsi disks Andreas Steinmetz
2003-02-23 13:20 ` Willem Riede
2003-02-23 13:41 ` Andreas Steinmetz
2003-02-23 16:14 ` Scott Merritt
2003-02-24 23:10 ` Andries Brouwer
2003-02-24 23:46 ` Scott Merritt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox