| Message ID | 2170ea9881efc68cd2265b9df956a30eb689b8cc.1302608369.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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)
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(-)