Patchwork [v2,1/2] cdrom: Allow the TEST_UNIT_READY command after a cdrom change

login
register
mail settings
Submitter Amit Shah
Date April 7, 2011, 5:05 a.m.
Message ID <f6b374c65b0b3c95edc73d6b39abae1fdbcd0750.1302152648.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/90130/
State New
Headers show

Comments

Amit Shah - April 7, 2011, 5:05 a.m.
We restrict the commands that a guest can send us after a cdrom change
event.  The current list includes REQUEST_SENSE and INQUIRY commands.
Guests can also issue TEST_UNIT_READY to inquire for the status, so
allow this command as well.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/ide/core.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
Paolo Bonzini - April 7, 2011, 7:22 a.m.
On 04/07/2011 07:05 AM, Amit Shah wrote:
> We restrict the commands that a guest can send us after a cdrom change
> event.  The current list includes REQUEST_SENSE and INQUIRY commands.
> Guests can also issue TEST_UNIT_READY to inquire for the status, so
> allow this command as well.
>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
>   hw/ide/core.c |    9 +++++----
>   1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 007a4ee..d55d804 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)
>       /* If there's a UNIT_ATTENTION condition pending, only
>          REQUEST_SENSE and INQUIRY commands are allowed to complete. */
>       if (s->sense_key == SENSE_UNIT_ATTENTION&&
> -	s->io_buffer[0] != GPCMD_REQUEST_SENSE&&
> -	s->io_buffer[0] != GPCMD_INQUIRY) {
> -	ide_atapi_cmd_check_status(s);
> -	return;
> +        s->io_buffer[0] != GPCMD_REQUEST_SENSE&&
> +        s->io_buffer[0] != GPCMD_INQUIRY&&
> +        s->io_buffer[0] != GPCMD_TEST_UNIT_READY) {
> +        ide_atapi_cmd_check_status(s);
> +        return;
>       }
>       switch(s->io_buffer[0]) {
>       case GPCMD_TEST_UNIT_READY:

ACK

Paolo
Stefan Hajnoczi - April 7, 2011, 8:42 a.m.
On Thu, Apr 07, 2011 at 10:35:18AM +0530, Amit Shah wrote:
> @@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)
>      /* If there's a UNIT_ATTENTION condition pending, only
>         REQUEST_SENSE and INQUIRY commands are allowed to complete. */
>      if (s->sense_key == SENSE_UNIT_ATTENTION &&
> -	s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
> -	s->io_buffer[0] != GPCMD_INQUIRY) {
> -	ide_atapi_cmd_check_status(s);
> -	return;
> +        s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
> +        s->io_buffer[0] != GPCMD_INQUIRY &&
> +        s->io_buffer[0] != GPCMD_TEST_UNIT_READY) {
> +        ide_atapi_cmd_check_status(s);
> +        return;

Looks good but please update the comment.

Stefan
Amit Shah - April 7, 2011, 8:51 a.m.
On (Thu) 07 Apr 2011 [09:42:30], Stefan Hajnoczi wrote:
> On Thu, Apr 07, 2011 at 10:35:18AM +0530, Amit Shah wrote:
> > @@ -1105,10 +1105,11 @@ static void ide_atapi_cmd(IDEState *s)
> >      /* If there's a UNIT_ATTENTION condition pending, only
> >         REQUEST_SENSE and INQUIRY commands are allowed to complete. */
> >      if (s->sense_key == SENSE_UNIT_ATTENTION &&
> > -	s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
> > -	s->io_buffer[0] != GPCMD_INQUIRY) {
> > -	ide_atapi_cmd_check_status(s);
> > -	return;
> > +        s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
> > +        s->io_buffer[0] != GPCMD_INQUIRY &&
> > +        s->io_buffer[0] != GPCMD_TEST_UNIT_READY) {
> > +        ide_atapi_cmd_check_status(s);
> > +        return;
> 
> Looks good but please update the comment.

Argh!  I'll update to a generic one as there are more commands that we
need to allow.

		Amit
Kevin Wolf - April 7, 2011, 8:59 a.m.
Am 07.04.2011 07:05, schrieb Amit Shah:
> We restrict the commands that a guest can send us after a cdrom change
> event.  The current list includes REQUEST_SENSE and INQUIRY commands.
> Guests can also issue TEST_UNIT_READY to inquire for the status, so
> allow this command as well.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Hm... MMC-5, section 4.1.6.1 seems to conflict with this:

"If a Host issues a command other than GET CONFIGURATION, GET EVENT
STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention
condition exists for that Host, the Drive shall not perform the command
and shall report CHECK CONDITION status unless a higher priority status
as defined by the Drive is also pending."

So while you're right that our list is incomplete, TEST UNIT READY
doesn't seem to be among the missing commands.

Kevin
Amit Shah - April 7, 2011, 9:11 a.m.
On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote:
> Am 07.04.2011 07:05, schrieb Amit Shah:
> > We restrict the commands that a guest can send us after a cdrom change
> > event.  The current list includes REQUEST_SENSE and INQUIRY commands.
> > Guests can also issue TEST_UNIT_READY to inquire for the status, so
> > allow this command as well.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> Hm... MMC-5, section 4.1.6.1 seems to conflict with this:
> 
> "If a Host issues a command other than GET CONFIGURATION, GET EVENT
> STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention
> condition exists for that Host, the Drive shall not perform the command
> and shall report CHECK CONDITION status unless a higher priority status
> as defined by the Drive is also pending."
> 
> So while you're right that our list is incomplete, TEST UNIT READY
> doesn't seem to be among the missing commands.

Hm - older Linux guests (pre 2.6.38) and Windows guests, as Gleb's
commit mentioned, rely on this command to get CD change notifications:

/* identical to scsi_test_unit_ready except that it doesn't
 * eat the NOT_READY returns for removable media */
int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
{
        int retries = MAX_RETRIES;
        int the_result;
        u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 };

        /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION
         * conditions are gone, or a timeout happens
         */
        do {
                the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL,
                                              0, sshdr, SR_TIMEOUT,
                                              retries--, NULL);
                if (scsi_sense_valid(sshdr) &&
                    sshdr->sense_key == UNIT_ATTENTION)
                        sdev->changed = 1;

        } while (retries > 0 &&
                 (!scsi_status_is_good(the_result) ||
                  (scsi_sense_valid(sshdr) &&
                   sshdr->sense_key == UNIT_ATTENTION)));
        return the_result;
}


There could be something else brewing as well.  We never report a
'tray open' condition; maybe the guest will be happy with such a thing
as well.  Will have to check.

		Amit
Kevin Wolf - April 7, 2011, 10:20 a.m.
Am 07.04.2011 11:11, schrieb Amit Shah:
> On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote:
>> Am 07.04.2011 07:05, schrieb Amit Shah:
>>> We restrict the commands that a guest can send us after a cdrom change
>>> event.  The current list includes REQUEST_SENSE and INQUIRY commands.
>>> Guests can also issue TEST_UNIT_READY to inquire for the status, so
>>> allow this command as well.
>>>
>>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>>
>> Hm... MMC-5, section 4.1.6.1 seems to conflict with this:
>>
>> "If a Host issues a command other than GET CONFIGURATION, GET EVENT
>> STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention
>> condition exists for that Host, the Drive shall not perform the command
>> and shall report CHECK CONDITION status unless a higher priority status
>> as defined by the Drive is also pending."
>>
>> So while you're right that our list is incomplete, TEST UNIT READY
>> doesn't seem to be among the missing commands.
> 
> Hm - older Linux guests (pre 2.6.38) and Windows guests, as Gleb's
> commit mentioned, rely on this command to get CD change notifications:
> 
> /* identical to scsi_test_unit_ready except that it doesn't
>  * eat the NOT_READY returns for removable media */
> int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
> {
>         int retries = MAX_RETRIES;
>         int the_result;
>         u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 };
> 
>         /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION
>          * conditions are gone, or a timeout happens
>          */
>         do {
>                 the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL,
>                                               0, sshdr, SR_TIMEOUT,
>                                               retries--, NULL);
>                 if (scsi_sense_valid(sshdr) &&
>                     sshdr->sense_key == UNIT_ATTENTION)
>                         sdev->changed = 1;
> 
>         } while (retries > 0 &&
>                  (!scsi_status_is_good(the_result) ||
>                   (scsi_sense_valid(sshdr) &&
>                    sshdr->sense_key == UNIT_ATTENTION)));
>         return the_result;
> }

I think the scsi_execute_req() call might issue a REQUEST SENSE
internally and therefore clear the unit attention condition. Tried to
check that in the source, but I'm hopelessly lost in the kernel...

Kevin
Amit Shah - April 7, 2011, 11:15 a.m.
On (Thu) 07 Apr 2011 [12:20:16], Kevin Wolf wrote:
> Am 07.04.2011 11:11, schrieb Amit Shah:
> > On (Thu) 07 Apr 2011 [10:59:20], Kevin Wolf wrote:
> >> Am 07.04.2011 07:05, schrieb Amit Shah:
> >>> We restrict the commands that a guest can send us after a cdrom change
> >>> event.  The current list includes REQUEST_SENSE and INQUIRY commands.
> >>> Guests can also issue TEST_UNIT_READY to inquire for the status, so
> >>> allow this command as well.
> >>>
> >>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >>
> >> Hm... MMC-5, section 4.1.6.1 seems to conflict with this:
> >>
> >> "If a Host issues a command other than GET CONFIGURATION, GET EVENT
> >> STATUS NOTIFICATION, INQUIRY or REQUEST SENSE while a unit attention
> >> condition exists for that Host, the Drive shall not perform the command
> >> and shall report CHECK CONDITION status unless a higher priority status
> >> as defined by the Drive is also pending."
> >>
> >> So while you're right that our list is incomplete, TEST UNIT READY
> >> doesn't seem to be among the missing commands.
> > 
> > Hm - older Linux guests (pre 2.6.38) and Windows guests, as Gleb's
> > commit mentioned, rely on this command to get CD change notifications:
> > 
> > /* identical to scsi_test_unit_ready except that it doesn't
> >  * eat the NOT_READY returns for removable media */
> > int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
> > {
> >         int retries = MAX_RETRIES;
> >         int the_result;
> >         u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 };
> > 
> >         /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION
> >          * conditions are gone, or a timeout happens
> >          */
> >         do {
> >                 the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL,
> >                                               0, sshdr, SR_TIMEOUT,
> >                                               retries--, NULL);
> >                 if (scsi_sense_valid(sshdr) &&
> >                     sshdr->sense_key == UNIT_ATTENTION)
> >                         sdev->changed = 1;
> > 
> >         } while (retries > 0 &&
> >                  (!scsi_status_is_good(the_result) ||
> >                   (scsi_sense_valid(sshdr) &&
> >                    sshdr->sense_key == UNIT_ATTENTION)));
> >         return the_result;
> > }
> 
> I think the scsi_execute_req() call might issue a REQUEST SENSE
> internally and therefore clear the unit attention condition. Tried to
> check that in the source, but I'm hopelessly lost in the kernel...

No, it doesn't.

It's clear the 2nd patch is needed; the 1st one must be papering over
something else.  Right now I'm inclined to think that it's because we
don't report tray open and no media as separate events to the guest,
as Markus has found out.

		Amit

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 007a4ee..d55d804 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1105,10 +1105,11 @@  static void ide_atapi_cmd(IDEState *s)
     /* If there's a UNIT_ATTENTION condition pending, only
        REQUEST_SENSE and INQUIRY commands are allowed to complete. */
     if (s->sense_key == SENSE_UNIT_ATTENTION &&
-	s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
-	s->io_buffer[0] != GPCMD_INQUIRY) {
-	ide_atapi_cmd_check_status(s);
-	return;
+        s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
+        s->io_buffer[0] != GPCMD_INQUIRY &&
+        s->io_buffer[0] != GPCMD_TEST_UNIT_READY) {
+        ide_atapi_cmd_check_status(s);
+        return;
     }
     switch(s->io_buffer[0]) {
     case GPCMD_TEST_UNIT_READY: