2.4.20: possibly wrong handling of removeable scsi disks

2.4.20: possibly wrong handling of removeable scsi disks

[Andreas Steinmetz]

[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.

Re: 2.4.20: possibly wrong handling of removeable scsi disks

[Willem Riede]

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.

Re: 2.4.20: possibly wrong handling of removeable scsi disks

[Andreas Steinmetz]

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.

Re: 2.4.20: possibly wrong handling of removeable scsi disks

[Scott Merritt]

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.

Re: 2.4.20: possibly wrong handling of removeable scsi disks

[Andries Brouwer]

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

Re: 2.4.20: possibly wrong handling of removeable scsi disks

[Scott Merritt]

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.