Patchwork [v3,4/5] atapi: GESN: Standardise event response handling for future additions

login
register
mail settings
Submitter Amit Shah
Date April 12, 2011, 11:43 a.m.
Message ID <2170ea9881efc68cd2265b9df956a30eb689b8cc.1302608369.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/90771/
State New
Headers show

Comments

Amit Shah - April 12, 2011, 11:43 a.m.
Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available response in a
generic way so that future additions to the code to handle other
response types is easier.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/ide/core.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)
Kevin Wolf - April 12, 2011, 1:09 p.m.
Am 12.04.2011 13:43, schrieb Amit Shah:
> Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available response in a
> generic way so that future additions to the code to handle other
> response types is easier.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/ide/core.c |   24 +++++++++++++++++++-----
>  1 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index e838990..a9bc1e3 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1098,9 +1098,17 @@ static void handle_get_event_status_notification(IDEState *s,
>          uint8_t control;
>      } __attribute__((packed)) *gesn_cdb;
>  
> +    struct {
> +        uint16_t len;
> +        uint8_t notification_class;
> +        uint8_t supported_events;
> +    } __attribute((packed)) *gesn_event_header;
> +
>      unsigned int max_len, used_len;
>  
>      gesn_cdb = (void *)packet;
> +    gesn_event_header = (void *)packet;

I think this should be buf, not packet.

> +
>      max_len = be16_to_cpu(gesn_cdb->len);
>  
>      /* It is fine by the MMC spec to not support async mode operations */
> @@ -1111,12 +1119,18 @@ static void handle_get_event_status_notification(IDEState *s,
>          return;
>      }
>  
> -    /* polling */
> +    /* polling mode operation */
> +
>      /* We don't support any event class (yet). */
> -    cpu_to_ube16(buf, 0x00); /* No event descriptor returned */
> -    buf[2] = 0x80;           /* No Event Available (NEA) */
> -    buf[3] = 0x00;           /* Empty supported event classes */
> -    ide_atapi_cmd_reply(s, 4, max_len);
> +    gesn_event_header->supported_events = 0;
> +    gesn_event_header->notification_class = 0;

This line has no effect, even after the next patch.

Kevin
Amit Shah - April 12, 2011, 1:59 p.m.
On (Tue) 12 Apr 2011 [15:09:33], Kevin Wolf wrote:
> Am 12.04.2011 13:43, schrieb Amit Shah:
> > Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available response in a
> > generic way so that future additions to the code to handle other
> > response types is easier.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/ide/core.c |   24 +++++++++++++++++++-----
> >  1 files changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ide/core.c b/hw/ide/core.c
> > index e838990..a9bc1e3 100644
> > --- a/hw/ide/core.c
> > +++ b/hw/ide/core.c
> > @@ -1098,9 +1098,17 @@ static void handle_get_event_status_notification(IDEState *s,
> >          uint8_t control;
> >      } __attribute__((packed)) *gesn_cdb;
> >  
> > +    struct {
> > +        uint16_t len;
> > +        uint8_t notification_class;
> > +        uint8_t supported_events;
> > +    } __attribute((packed)) *gesn_event_header;
> > +
> >      unsigned int max_len, used_len;
> >  
> >      gesn_cdb = (void *)packet;
> > +    gesn_event_header = (void *)packet;
> 
> I think this should be buf, not packet.

Ah, right.  (Though they're the same.)

> >      max_len = be16_to_cpu(gesn_cdb->len);
> >  
> >      /* It is fine by the MMC spec to not support async mode operations */
> > @@ -1111,12 +1119,18 @@ static void handle_get_event_status_notification(IDEState *s,
> >          return;
> >      }
> >  
> > -    /* polling */
> > +    /* polling mode operation */
> > +
> >      /* We don't support any event class (yet). */
> > -    cpu_to_ube16(buf, 0x00); /* No event descriptor returned */
> > -    buf[2] = 0x80;           /* No Event Available (NEA) */
> > -    buf[3] = 0x00;           /* Empty supported event classes */
> > -    ide_atapi_cmd_reply(s, 4, max_len);
> > +    gesn_event_header->supported_events = 0;
> > +    gesn_event_header->notification_class = 0;
> 
> This line has no effect, even after the next patch.

Yep, will remove.

		Amit
Kevin Wolf - April 12, 2011, 2:07 p.m.
Am 12.04.2011 15:59, schrieb Amit Shah:
> On (Tue) 12 Apr 2011 [15:09:33], Kevin Wolf wrote:
>> Am 12.04.2011 13:43, schrieb Amit Shah:
>>> Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available response in a
>>> generic way so that future additions to the code to handle other
>>> response types is easier.
>>>
>>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>>> ---
>>>  hw/ide/core.c |   24 +++++++++++++++++++-----
>>>  1 files changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index e838990..a9bc1e3 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -1098,9 +1098,17 @@ static void handle_get_event_status_notification(IDEState *s,
>>>          uint8_t control;
>>>      } __attribute__((packed)) *gesn_cdb;
>>>  
>>> +    struct {
>>> +        uint16_t len;
>>> +        uint8_t notification_class;
>>> +        uint8_t supported_events;
>>> +    } __attribute((packed)) *gesn_event_header;
>>> +
>>>      unsigned int max_len, used_len;
>>>  
>>>      gesn_cdb = (void *)packet;
>>> +    gesn_event_header = (void *)packet;
>>
>> I think this should be buf, not packet.
> 
> Ah, right.  (Though they're the same.)

Oh, you're right. I wasn't even aware of that. At some point we should
clean this up, it's an invitation for bugs...

Kevin
Amit Shah - April 12, 2011, 2:12 p.m.
On (Tue) 12 Apr 2011 [16:07:23], Kevin Wolf wrote:
> Am 12.04.2011 15:59, schrieb Amit Shah:
> > On (Tue) 12 Apr 2011 [15:09:33], Kevin Wolf wrote:
> >> Am 12.04.2011 13:43, schrieb Amit Shah:
> >>> Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available response in a
> >>> generic way so that future additions to the code to handle other
> >>> response types is easier.
> >>>
> >>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> >>> ---
> >>>  hw/ide/core.c |   24 +++++++++++++++++++-----
> >>>  1 files changed, 19 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >>> index e838990..a9bc1e3 100644
> >>> --- a/hw/ide/core.c
> >>> +++ b/hw/ide/core.c
> >>> @@ -1098,9 +1098,17 @@ static void handle_get_event_status_notification(IDEState *s,
> >>>          uint8_t control;
> >>>      } __attribute__((packed)) *gesn_cdb;
> >>>  
> >>> +    struct {
> >>> +        uint16_t len;
> >>> +        uint8_t notification_class;
> >>> +        uint8_t supported_events;
> >>> +    } __attribute((packed)) *gesn_event_header;
> >>> +
> >>>      unsigned int max_len, used_len;
> >>>  
> >>>      gesn_cdb = (void *)packet;
> >>> +    gesn_event_header = (void *)packet;
> >>
> >> I think this should be buf, not packet.
> > 
> > Ah, right.  (Though they're the same.)
> 
> Oh, you're right. I wasn't even aware of that. At some point we should
> clean this up, it's an invitation for bugs...

It is.  Actually it's surprising gcc didn't tell me that casting from
const uint8_t * to void * and then writing to the pointer is not a
good idea.

		Amit
Paolo Bonzini - April 12, 2011, 2:40 p.m.
On 04/12/2011 04:12 PM, Amit Shah wrote:
>>>>> >  >>>        gesn_cdb = (void *)packet;
>>>>> >  >>>  +    gesn_event_header = (void *)packet;
>>>> >  >>
>>>> >  >>  I think this should be buf, not packet.
>>> >  >
>>> >  >  Ah, right.  (Though they're the same.)
>> >
>> >  Oh, you're right. I wasn't even aware of that. At some point we should
>> >  clean this up, it's an invitation for bugs...
> It is.  Actually it's surprising gcc didn't tell me that casting from
> const uint8_t * to void * and then writing to the pointer is not a
> good idea.

Perhaps those structs should be made global and added to an atapi.h 
header?  This way you can avoid (void *) casts.

(In fact, there's a scsi.h file in the Win32 free header files waiting 
to be lifted in QEMU... unfortunately, the similar atapi header file has 
not been written yet though I think it is in the Windows DDK).

Paolo

Patch

diff --git a/hw/ide/core.c b/hw/ide/core.c
index e838990..a9bc1e3 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1098,9 +1098,17 @@  static void handle_get_event_status_notification(IDEState *s,
         uint8_t control;
     } __attribute__((packed)) *gesn_cdb;
 
+    struct {
+        uint16_t len;
+        uint8_t notification_class;
+        uint8_t supported_events;
+    } __attribute((packed)) *gesn_event_header;
+
     unsigned int max_len, used_len;
 
     gesn_cdb = (void *)packet;
+    gesn_event_header = (void *)packet;
+
     max_len = be16_to_cpu(gesn_cdb->len);
 
     /* It is fine by the MMC spec to not support async mode operations */
@@ -1111,12 +1119,18 @@  static void handle_get_event_status_notification(IDEState *s,
         return;
     }
 
-    /* polling */
+    /* polling mode operation */
+
     /* We don't support any event class (yet). */
-    cpu_to_ube16(buf, 0x00); /* No event descriptor returned */
-    buf[2] = 0x80;           /* No Event Available (NEA) */
-    buf[3] = 0x00;           /* Empty supported event classes */
-    ide_atapi_cmd_reply(s, 4, max_len);
+    gesn_event_header->supported_events = 0;
+    gesn_event_header->notification_class = 0;
+
+    gesn_event_header->notification_class = 0x80; /* No event available */
+    used_len = sizeof(*gesn_event_header);
+
+    gesn_event_header->len = cpu_to_be16(used_len
+                                         - sizeof(*gesn_event_header));
+    ide_atapi_cmd_reply(s, used_len, max_len);
 }
 
 static void ide_atapi_cmd(IDEState *s)