Patchwork ATAPI: Add support for ASCQ in sense codes

login
register
mail settings
Submitter Paolo Bonzini
Date July 31, 2012, 7:14 a.m.
Message ID <501785F0.8050607@redhat.com>
Download mbox | patch
Permalink /patch/174166/
State New
Headers show

Comments

Paolo Bonzini - July 31, 2012, 7:14 a.m.
Il 31/07/2012 04:07, Ronnie Sahlberg ha scritto:
> Add support for setting the ASCQ for SCSI sense codes in the ATAPI driver.
> Use this to set ASCQ==2 for the medium removal prevention that is recommended in MMC for this condition.
> 
> asc:0x53 ascq:0x02 is the recommended error for MEDIUM_REMOVAL_PREVENTED and is listed in Annex F in MMC

You also need to cover migration.

You could either add a subsection, or something like this:



In fact, I wonder if it is simpler to "make up" the ascq directly in
cmd_request_sense, instead of changing all invocations of
ide_atapi_cmd_error.  Kevin, any preferences?

Paolo
Kevin Wolf - Aug. 27, 2012, 10:37 a.m.
Am 31.07.2012 09:14, schrieb Paolo Bonzini:
> Il 31/07/2012 04:07, Ronnie Sahlberg ha scritto:
>> Add support for setting the ASCQ for SCSI sense codes in the ATAPI driver.
>> Use this to set ASCQ==2 for the medium removal prevention that is recommended in MMC for this condition.
>>
>> asc:0x53 ascq:0x02 is the recommended error for MEDIUM_REMOVAL_PREVENTED and is listed in Annex F in MMC
> 
> You also need to cover migration.
> 
> You could either add a subsection, or something like this:
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index f7f714c..89c0157 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -1143,3 +1143,20 @@ void ide_atapi_cmd(IDEState *s)
>  
>      ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
>  }
> +
> +void ide_atapi_post_load(IDEState *s, int version_id)
> +{
> +    if (version_id < 3) {
> +        if (s->sense_key == UNIT_ATTENTION &&
> +            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
> +            s->cdrom_changed = 1;
> +        }
> +    }
> +
> +    /* This is simpler than adding a subsection just for the ascq.  */
> +    if (s->asc == ASC_MEDIA_REMOVAL_PREVENTED) {
> +        s->ascq = 2;
> +    } else {
> +        s->ascq = 0;
> +    }
> +}
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index cb5ca4b..959ac48 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2154,12 +2154,7 @@ static int ide_drive_post_load(void *opaque, int version_id)
>  {
>      IDEState *s = opaque;
>  
> -    if (version_id < 3) {
> -        if (s->sense_key == UNIT_ATTENTION &&
> -            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
> -            s->cdrom_changed = 1;
> -        }
> -    }
> +    ide_atapi_post_load(s, version_id);
>      if (s->identify_set) {
>          bdrv_set_enable_write_cache(s->bs, !!(s->identify_data[85] & (1 << 5)));
>      }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 7170bd9..2572461 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -572,6 +572,7 @@ BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
>  /* hw/ide/atapi.c */
>  void ide_atapi_cmd(IDEState *s);
>  void ide_atapi_cmd_reply_end(IDEState *s);
> +void ide_atapi_post_load(IDEState *s, int version_id);
>  
>  /* hw/ide/qdev.c */
>  void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id);
> 
> 
> In fact, I wonder if it is simpler to "make up" the ascq directly in
> cmd_request_sense, instead of changing all invocations of
> ide_atapi_cmd_error.  Kevin, any preferences?

Oh, I missed this question and wondered why there was no v2... I'd
prefer this patch with migration support added.

Kevin

Patch

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index f7f714c..89c0157 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -1143,3 +1143,20 @@  void ide_atapi_cmd(IDEState *s)
 
     ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_ILLEGAL_OPCODE);
 }
+
+void ide_atapi_post_load(IDEState *s, int version_id)
+{
+    if (version_id < 3) {
+        if (s->sense_key == UNIT_ATTENTION &&
+            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
+            s->cdrom_changed = 1;
+        }
+    }
+
+    /* This is simpler than adding a subsection just for the ascq.  */
+    if (s->asc == ASC_MEDIA_REMOVAL_PREVENTED) {
+        s->ascq = 2;
+    } else {
+        s->ascq = 0;
+    }
+}
diff --git a/hw/ide/core.c b/hw/ide/core.c
index cb5ca4b..959ac48 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2154,12 +2154,7 @@  static int ide_drive_post_load(void *opaque, int version_id)
 {
     IDEState *s = opaque;
 
-    if (version_id < 3) {
-        if (s->sense_key == UNIT_ATTENTION &&
-            s->asc == ASC_MEDIUM_MAY_HAVE_CHANGED) {
-            s->cdrom_changed = 1;
-        }
-    }
+    ide_atapi_post_load(s, version_id);
     if (s->identify_set) {
         bdrv_set_enable_write_cache(s->bs, !!(s->identify_data[85] & (1 << 5)));
     }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 7170bd9..2572461 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -572,6 +572,7 @@  BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
 /* hw/ide/atapi.c */
 void ide_atapi_cmd(IDEState *s);
 void ide_atapi_cmd_reply_end(IDEState *s);
+void ide_atapi_post_load(IDEState *s, int version_id);
 
 /* hw/ide/qdev.c */
 void ide_bus_new(IDEBus *idebus, DeviceState *dev, int bus_id);