| Message ID | 20200527074920.43669-2-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/9] pc-bios: s390x: cio.c cleanup and compile fix | expand |
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>
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 --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. */