diff mbox series

[v3,1/9] pc-bios: s390x: cio.c cleanup and compile fix

Message ID 20200527074920.43669-2-frankja@linux.ibm.com
State New
Headers show
Series pc-bios: s390x: Cleanup part 1 | expand

Commit Message

Janosch Frank May 27, 2020, 7:49 a.m. UTC 
Let's initialize the structs at the beginning to ease reading and also
zeroing all other fields. This also makes the compiler stop
complaining about sense_id_ccw.flags being ored into when it's not
initialized.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 pc-bios/s390-ccw/cio.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

Comments

Christian Borntraeger May 27, 2020, 8:24 a.m. UTC | #1 
On 27.05.20 09:49, Janosch Frank wrote:
> Let's initialize the structs at the beginning to ease reading and also
> zeroing all other fields. This also makes the compiler stop
> complaining about sense_id_ccw.flags being ored into when it's not
> initialized.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  pc-bios/s390-ccw/cio.c | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
> index 339ec5fbe7..dbae1e240e 100644
> --- a/pc-bios/s390-ccw/cio.c
> +++ b/pc-bios/s390-ccw/cio.c
> @@ -49,13 +49,13 @@ void enable_subchannel(SubChannelId schid)
>  
>  uint16_t cu_type(SubChannelId schid)
>  {
> -    Ccw1 sense_id_ccw;
>      SenseId sense_data;
> -
> -    sense_id_ccw.cmd_code = CCW_CMD_SENSE_ID;
> -    sense_id_ccw.cda = ptr2u32(&sense_data);
> -    sense_id_ccw.count = sizeof(sense_data);
> -    sense_id_ccw.flags |= CCW_FLAG_SLI;
> +    Ccw1 sense_id_ccw = {
> +        .cmd_code = CCW_CMD_SENSE_ID,
> +        .count = sizeof(sense_data),
> +        .flags = CCW_FLAG_SLI,
> +        .cda = ptr2u32(&sense_data),
> +    };

nitpicking, since you change the order anyway you _could_ swap count and flag
to match the order of struct Ccw. Only necessary when redoing.


>  
>      if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
>          panic("Failed to run SenseID CCw\n");
> @@ -67,13 +67,13 @@ uint16_t cu_type(SubChannelId schid)
>  int basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
>                   uint16_t data_size)
>  {
> -    Ccw1 senseCcw;
> +    Ccw1 senseCcw = {
> +        .cmd_code = CCW_CMD_BASIC_SENSE,
> +        .count = data_size,
> +        .cda = ptr2u32(sense_data),
> +    };
>      Irb irb;
>  
> -    senseCcw.cmd_code = CCW_CMD_BASIC_SENSE;
> -    senseCcw.cda = ptr2u32(sense_data);
> -    senseCcw.count = data_size;
> -

here it is fine, due to the lack of flags. 
Was this actually a bug before that senseCcw.flags was
not zeroed out?


[...]

Other than that
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Janosch Frank May 27, 2020, 8:34 a.m. UTC | #2 
On 5/27/20 10:24 AM, Christian Borntraeger wrote:
> 
> 
> On 27.05.20 09:49, Janosch Frank wrote:
>> Let's initialize the structs at the beginning to ease reading and also
>> zeroing all other fields. This also makes the compiler stop
>> complaining about sense_id_ccw.flags being ored into when it's not
>> initialized.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>>  pc-bios/s390-ccw/cio.c | 40 ++++++++++++++++++++++------------------
>>  1 file changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
>> index 339ec5fbe7..dbae1e240e 100644
>> --- a/pc-bios/s390-ccw/cio.c
>> +++ b/pc-bios/s390-ccw/cio.c
>> @@ -49,13 +49,13 @@ void enable_subchannel(SubChannelId schid)
>>  
>>  uint16_t cu_type(SubChannelId schid)
>>  {
>> -    Ccw1 sense_id_ccw;
>>      SenseId sense_data;
>> -
>> -    sense_id_ccw.cmd_code = CCW_CMD_SENSE_ID;
>> -    sense_id_ccw.cda = ptr2u32(&sense_data);
>> -    sense_id_ccw.count = sizeof(sense_data);
>> -    sense_id_ccw.flags |= CCW_FLAG_SLI;
>> +    Ccw1 sense_id_ccw = {
>> +        .cmd_code = CCW_CMD_SENSE_ID,
>> +        .count = sizeof(sense_data),
>> +        .flags = CCW_FLAG_SLI,
>> +        .cda = ptr2u32(&sense_data),
>> +    };
> 
> nitpicking, since you change the order anyway you _could_ swap count and flag
> to match the order of struct Ccw. Only necessary when redoing.

Sure, if it makes you happy :-)

> 
> 
>>  
>>      if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
>>          panic("Failed to run SenseID CCw\n");
>> @@ -67,13 +67,13 @@ uint16_t cu_type(SubChannelId schid)
>>  int basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
>>                   uint16_t data_size)
>>  {
>> -    Ccw1 senseCcw;
>> +    Ccw1 senseCcw = {
>> +        .cmd_code = CCW_CMD_BASIC_SENSE,
>> +        .count = data_size,
>> +        .cda = ptr2u32(sense_data),
>> +    };
>>      Irb irb;
>>  
>> -    senseCcw.cmd_code = CCW_CMD_BASIC_SENSE;
>> -    senseCcw.cda = ptr2u32(sense_data);
>> -    senseCcw.count = data_size;
>> -
> 
> here it is fine, due to the lack of flags. 
> Was this actually a bug before that senseCcw.flags was
> not zeroed out?

I'd guess so, seems like the compiler went into other code paths with my
recent changes and then started complaining.
But it makes sense to clean them up anyway.

> 
> 
> [...]
> 
> Other than that
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks!
diff mbox series

Patch

diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
index 339ec5fbe7..dbae1e240e 100644
--- a/pc-bios/s390-ccw/cio.c
+++ b/pc-bios/s390-ccw/cio.c
@@ -49,13 +49,13 @@  void enable_subchannel(SubChannelId schid)
 
 uint16_t cu_type(SubChannelId schid)
 {
-    Ccw1 sense_id_ccw;
     SenseId sense_data;
-
-    sense_id_ccw.cmd_code = CCW_CMD_SENSE_ID;
-    sense_id_ccw.cda = ptr2u32(&sense_data);
-    sense_id_ccw.count = sizeof(sense_data);
-    sense_id_ccw.flags |= CCW_FLAG_SLI;
+    Ccw1 sense_id_ccw = {
+        .cmd_code = CCW_CMD_SENSE_ID,
+        .count = sizeof(sense_data),
+        .flags = CCW_FLAG_SLI,
+        .cda = ptr2u32(&sense_data),
+    };
 
     if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
         panic("Failed to run SenseID CCw\n");
@@ -67,13 +67,13 @@  uint16_t cu_type(SubChannelId schid)
 int basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
                  uint16_t data_size)
 {
-    Ccw1 senseCcw;
+    Ccw1 senseCcw = {
+        .cmd_code = CCW_CMD_BASIC_SENSE,
+        .count = data_size,
+        .cda = ptr2u32(sense_data),
+    };
     Irb irb;
 
-    senseCcw.cmd_code = CCW_CMD_BASIC_SENSE;
-    senseCcw.cda = ptr2u32(sense_data);
-    senseCcw.count = data_size;
-
     return __do_cio(schid, ptr2u32(&senseCcw), CCW_FMT1, &irb);
 }
 
@@ -314,7 +314,17 @@  static void print_irb_err(Irb *irb)
  */
 static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
 {
-    CmdOrb orb = {};
+    /*
+     * QEMU's CIO implementation requires prefetch and 64-bit idaws. We
+     * allow all paths.
+     */
+    CmdOrb orb = {
+        .fmt = fmt,
+        .pfch = 1,
+        .c64 = 1,
+        .lpm = 0xFF,
+        .cpa = ccw_addr,
+    };
     int rc;
 
     IPL_assert(fmt == 0 || fmt == 1, "Invalid ccw format");
@@ -324,12 +334,6 @@  static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
         IPL_assert(ccw_addr <= 0xFFFFFF - 8, "Invalid ccw address");
     }
 
-    orb.fmt = fmt;
-    orb.pfch = 1;  /* QEMU's cio implementation requires prefetch */
-    orb.c64 = 1;   /* QEMU's cio implementation requires 64-bit idaws */
-    orb.lpm = 0xFF; /* All paths allowed */
-    orb.cpa = ccw_addr;
-
     rc = ssch(schid, &orb);
     if (rc == 1 || rc == 2) {
         /* Subchannel status pending or busy. Eat status and ask for retry. */