Patchwork [03/35] atapi: move GESN definitions to scsi-defs.h

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 13, 2011, 11:03 a.m.
Message ID <1318503845-11473-4-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/119423/
State New
Headers show

Comments

Paolo Bonzini - Oct. 13, 2011, 11:03 a.m.
As a complement to the previous patch, move definitions for GET EVENT
STATUS NOTIFICATION from the two functions to scsi-defs.h.

The NCR_* constants are just bit values corresponding to the ENC_*
values, with no offsets even, so keep just one copy.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/atapi.c |   43 ++++++-------------------------------------
 hw/scsi-defs.h |   21 +++++++++++++++++++++
 2 files changed, 27 insertions(+), 37 deletions(-)
Kevin Wolf - Oct. 17, 2011, 1:41 p.m.
Am 13.10.2011 13:03, schrieb Paolo Bonzini:
> As a complement to the previous patch, move definitions for GET EVENT
> STATUS NOTIFICATION from the two functions to scsi-defs.h.
> 
> The NCR_* constants are just bit values corresponding to the ENC_*
> values, with no offsets even, so keep just one copy.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I'm not sure about the NCR_* constants. They happen to be shifted
values, but the spec doesn't define them as such but has two separate
tables for them.

> ---
>  hw/ide/atapi.c |   43 ++++++-------------------------------------
>  hw/scsi-defs.h |   21 +++++++++++++++++++++
>  2 files changed, 27 insertions(+), 37 deletions(-)

> diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
> index dfa3095..8094698 100644
> --- a/hw/scsi-defs.h
> +++ b/hw/scsi-defs.h
> @@ -203,6 +203,27 @@
>   * of MODE_PAGE_SENSE_POWER */
>  #define MODE_PAGE_CDROM			0x0d
>  
> +/* Event notification classes for GET EVENT STATUS NOTIFICATION */
> +#define GESN_NO_EVENTS			0
> +#define GESN_OPERATIONAL_CHANGE		1
> +#define GESN_POWER_MANAGEMENT		2
> +#define GESN_EXTERNAL_REQUEST		3
> +#define GESN_MEDIA			4
> +#define GESN_MULTIPLE_HOSTS		5
> +#define GESN_DEVICE_BUSY		6
> +
> +/* Event codes for MEDIA event status notification */
> +#define MEC_NO_CHANGE		0
> +#define MEC_EJECT_REQUESTED	1
> +#define MEC_NEW_MEDIA		2
> +#define MEC_MEDIA_REMOVAL	3 /* only for media changers */
> +#define MEC_MEDIA_CHANGED	4 /* only for media changers */
> +#define MEC_BG_FORMAT_COMPLETED	5 /* MRW or DVD+RW b/g format completed */
> +#define MEC_BG_FORMAT_RESTARTED	6 /* MRW or DVD+RW b/g format restarted */
> +
> +#define MS_TRAY_OPEN		1
> +#define MS_MEDIA_PRESENT	2

This should be spaces instead of tabs, the alignment is completely
broken for me.

Now that I looked for them I see that patch 2 has some tabs as well,
even though they are just moved. You could fix them anyway while
touching the code.

Kevin
Paolo Bonzini - Oct. 17, 2011, 1:53 p.m.
On 10/17/2011 03:41 PM, Kevin Wolf wrote:
> I'm not sure about the NCR_* constants. They happen to be shifted
> values, but the spec doesn't define them as such but has two separate
> tables for them.

Yeah, on the other hand the spec has hints that they are really the same 
thing:

1) one is called notification class request, the other is called 
notification class.  Using the same name is surprising since the SCSI 
spec usually cannot praised for its consistency.  For example the 
returned header includes a field called "supported event classes" 
(rather than for example "supported notification classes"), but the 
description refers explicitly to the same table used for the 
"notification class request" field.

2) "bit 0 is perpetually reserved" in the notification class request 
field, and the notification class field value "000" means "no requested 
event classes are supported".

Paolo

Patch

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index be3b728..347c38d 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -505,19 +505,6 @@  static int ide_dvd_read_structure(IDEState *s, int format,
 static unsigned int event_status_media(IDEState *s,
                                        uint8_t *buf)
 {
-    enum media_event_code {
-        MEC_NO_CHANGE = 0,       /* Status unchanged */
-        MEC_EJECT_REQUESTED,     /* received a request from user to eject */
-        MEC_NEW_MEDIA,           /* new media inserted and ready for access */
-        MEC_MEDIA_REMOVAL,       /* only for media changers */
-        MEC_MEDIA_CHANGED,       /* only for media changers */
-        MEC_BG_FORMAT_COMPLETED, /* MRW or DVD+RW b/g format completed */
-        MEC_BG_FORMAT_RESTARTED, /* MRW or DVD+RW b/g format restarted */
-    };
-    enum media_status {
-        MS_TRAY_OPEN = 1,
-        MS_MEDIA_PRESENT = 2,
-    };
     uint8_t event_code, media_status;
 
     media_status = 0;
@@ -564,27 +551,6 @@  static void cmd_get_event_status_notification(IDEState *s,
         uint8_t notification_class;
         uint8_t supported_events;
     } QEMU_PACKED *gesn_event_header;
-
-    enum notification_class_request_type {
-        NCR_RESERVED1 = 1 << 0,
-        NCR_OPERATIONAL_CHANGE = 1 << 1,
-        NCR_POWER_MANAGEMENT = 1 << 2,
-        NCR_EXTERNAL_REQUEST = 1 << 3,
-        NCR_MEDIA = 1 << 4,
-        NCR_MULTI_HOST = 1 << 5,
-        NCR_DEVICE_BUSY = 1 << 6,
-        NCR_RESERVED2 = 1 << 7,
-    };
-    enum event_notification_class_field {
-        ENC_NO_EVENTS = 0,
-        ENC_OPERATIONAL_CHANGE,
-        ENC_POWER_MANAGEMENT,
-        ENC_EXTERNAL_REQUEST,
-        ENC_MEDIA,
-        ENC_MULTIPLE_HOSTS,
-        ENC_DEVICE_BUSY,
-        ENC_RESERVED,
-    };
     unsigned int max_len, used_len;
 
     gesn_cdb = (void *)packet;
@@ -606,8 +572,11 @@  static void cmd_get_event_status_notification(IDEState *s,
      * These are the supported events.
      *
      * We currently only support requests of the 'media' type.
+     * Notification class requests and supported event classes are bitmasks,
+     * but they are build from the same values as the "notification class"
+     * field.
      */
-    gesn_event_header->supported_events = NCR_MEDIA;
+    gesn_event_header->supported_events = 1 << GESN_MEDIA;
 
     /*
      * We use |= below to set the class field; other bits in this byte
@@ -621,8 +590,8 @@  static void cmd_get_event_status_notification(IDEState *s,
      * notification_class_request_type enum above specifies the
      * priority: upper elements are higher prio than lower ones.
      */
-    if (gesn_cdb->class & NCR_MEDIA) {
-        gesn_event_header->notification_class |= ENC_MEDIA;
+    if (gesn_cdb->class & (1 << GESN_MEDIA)) {
+        gesn_event_header->notification_class |= GESN_MEDIA;
         used_len = event_status_media(s, buf);
     } else {
         gesn_event_header->notification_class = 0x80; /* No event available */
diff --git a/hw/scsi-defs.h b/hw/scsi-defs.h
index dfa3095..8094698 100644
--- a/hw/scsi-defs.h
+++ b/hw/scsi-defs.h
@@ -203,6 +203,27 @@ 
  * of MODE_PAGE_SENSE_POWER */
 #define MODE_PAGE_CDROM			0x0d
 
+/* Event notification classes for GET EVENT STATUS NOTIFICATION */
+#define GESN_NO_EVENTS			0
+#define GESN_OPERATIONAL_CHANGE		1
+#define GESN_POWER_MANAGEMENT		2
+#define GESN_EXTERNAL_REQUEST		3
+#define GESN_MEDIA			4
+#define GESN_MULTIPLE_HOSTS		5
+#define GESN_DEVICE_BUSY		6
+
+/* Event codes for MEDIA event status notification */
+#define MEC_NO_CHANGE		0
+#define MEC_EJECT_REQUESTED	1
+#define MEC_NEW_MEDIA		2
+#define MEC_MEDIA_REMOVAL	3 /* only for media changers */
+#define MEC_MEDIA_CHANGED	4 /* only for media changers */
+#define MEC_BG_FORMAT_COMPLETED	5 /* MRW or DVD+RW b/g format completed */
+#define MEC_BG_FORMAT_RESTARTED	6 /* MRW or DVD+RW b/g format restarted */
+
+#define MS_TRAY_OPEN		1
+#define MS_MEDIA_PRESENT	2
+
 /*
  * Based on values from <linux/cdrom.h> but extending CD_MINS
  * to the maximum common size allowed by the Orange's Book ATIP