diff mbox

[5/5] ide/atapi: Introduce NEED_DISK flag for commands

Message ID 1303216603-11658-6-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf April 19, 2011, 12:36 p.m. UTC 
Some commands require a medium to be present in order to execute successfully.
Instead of duplicating the check in each command implementation, let's add a
flag and check it before calling the command.

This patch only converts existing checks, it does not introduce new checks for
any of the other commands that can/should report a Not Ready Condition.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/atapi.c |   46 +++++++++++++++++++++-------------------------
 1 files changed, 21 insertions(+), 25 deletions(-)

Comments

Amit Shah April 19, 2011, 1:18 p.m. UTC | #1 
On (Tue) 19 Apr 2011 [14:36:43], Kevin Wolf wrote:

> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 032d1b0..88e7791 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -813,11 +813,7 @@ error_cmd:
>  
>  static void cmd_test_unit_ready(IDEState *s, uint8_t *buf)
>  {
> -    if (bdrv_is_inserted(s->bs)) {
> -        ide_atapi_cmd_ok(s);
> -    } else {
> -        ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
> -    }
> +    ide_atapi_cmd_ok(s);
>  }

It'll look really weird to have test_unit_ready() to always return
GOOD status (outside the context of this patch).  A comment will help.

		Amit
Kevin Wolf April 19, 2011, 1:45 p.m. UTC | #2 
Am 19.04.2011 15:18, schrieb Amit Shah:
> On (Tue) 19 Apr 2011 [14:36:43], Kevin Wolf wrote:
> 
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 032d1b0..88e7791 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -813,11 +813,7 @@ error_cmd:
>>  
>>  static void cmd_test_unit_ready(IDEState *s, uint8_t *buf)
>>  {
>> -    if (bdrv_is_inserted(s->bs)) {
>> -        ide_atapi_cmd_ok(s);
>> -    } else {
>> -        ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
>> -    }
>> +    ide_atapi_cmd_ok(s);
>>  }
> 
> It'll look really weird to have test_unit_ready() to always return
> GOOD status (outside the context of this patch).  A comment will help.

Ok, I'll add a comment for v2.

Kevin
diff mbox

Patch

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 032d1b0..88e7791 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -813,11 +813,7 @@  error_cmd:
 
 static void cmd_test_unit_ready(IDEState *s, uint8_t *buf)
 {
-    if (bdrv_is_inserted(s->bs)) {
-        ide_atapi_cmd_ok(s);
-    } else {
-        ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
-    }
+    ide_atapi_cmd_ok(s);
 }
 
 static void cmd_prevent_allow_medium_removal(IDEState *s, uint8_t* buf)
@@ -883,11 +879,6 @@  static void cmd_seek(IDEState *s, uint8_t* buf)
     unsigned int lba;
     uint64_t total_sectors = s->nb_sectors >> 2;
 
-    if (total_sectors == 0) {
-        ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
-        return;
-    }
-
     lba = ube32_to_cpu(buf + 2);
     if (lba >= total_sectors) {
         ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR);
@@ -941,13 +932,8 @@  static void cmd_mechanism_status(IDEState *s, uint8_t* buf)
 static void cmd_read_toc_pma_atip(IDEState *s, uint8_t* buf)
 {
     int format, msf, start_track, len;
-    uint64_t total_sectors = s->nb_sectors >> 2;
     int max_len;
-
-    if (total_sectors == 0) {
-        ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
-        return;
-    }
+    uint64_t total_sectors = s->nb_sectors >> 2;
 
     max_len = ube16_to_cpu(buf + 7);
     format = buf[9] >> 6;
@@ -986,11 +972,6 @@  static void cmd_read_cdvd_capacity(IDEState *s, uint8_t* buf)
 {
     uint64_t total_sectors = s->nb_sectors >> 2;
 
-    if (total_sectors == 0) {
-        ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
-        return;
-    }
-
     /* NOTE: it is really the number of sectors minus 1 */
     cpu_to_ube32(buf, total_sectors - 1);
     cpu_to_ube32(buf + 4, 2048);
@@ -1062,22 +1043,29 @@  enum {
      * unit attention condition. (See MMC-5, section 4.1.6.1)
      */
     ALLOW_UA = 0x01,
+
+    /*
+     * Commands flagged with CHECK_READY can only execute if a medium is present.
+     * Otherwise they report the Not Ready Condition. (See MMC-5, section
+     * 4.1.8)
+     */
+    CHECK_READY = 0x02,
 };
 
 struct {
     void (*handler)(IDEState *s, uint8_t *buf);
     int flags;
 } atapi_cmd_table[0x100] = {
-    [ 0x00 ] = { cmd_test_unit_ready,               0 },
+    [ 0x00 ] = { cmd_test_unit_ready,               CHECK_READY },
     [ 0x03 ] = { cmd_request_sense,                 ALLOW_UA },
     [ 0x12 ] = { cmd_inquiry,                       ALLOW_UA },
     [ 0x1a ] = { cmd_mode_sense, /* (6) */          0 },
     [ 0x1b ] = { cmd_start_stop_unit,               0 },
     [ 0x1e ] = { cmd_prevent_allow_medium_removal,  0 },
-    [ 0x25 ] = { cmd_read_cdvd_capacity,            0 },
+    [ 0x25 ] = { cmd_read_cdvd_capacity,            CHECK_READY },
     [ 0x28 ] = { cmd_read, /* (10) */               0 },
-    [ 0x2b ] = { cmd_seek,                          0 },
-    [ 0x43 ] = { cmd_read_toc_pma_atip,             0 },
+    [ 0x2b ] = { cmd_seek,                          CHECK_READY },
+    [ 0x43 ] = { cmd_read_toc_pma_atip,             CHECK_READY },
     [ 0x46 ] = { cmd_get_configuration,             ALLOW_UA },
     [ 0x4a ] = { cmd_get_event_status_notification, ALLOW_UA },
     [ 0x5a ] = { cmd_mode_sense, /* (10) */         0 },
@@ -1126,6 +1114,14 @@  void ide_atapi_cmd(IDEState *s)
         return;
     }
 
+    /* Report a Not Ready condition if appropriate for the command */
+    if ((atapi_cmd_table[s->io_buffer[0]].flags & CHECK_READY) &&
+        (!media_present(s) || !bdrv_is_inserted(s->bs)))
+    {
+        ide_atapi_cmd_error(s, SENSE_NOT_READY, ASC_MEDIUM_NOT_PRESENT);
+        return;
+    }
+
     /* Execute the command */
     if (atapi_cmd_table[s->io_buffer[0]].handler) {
         atapi_cmd_table[s->io_buffer[0]].handler(s, buf);