| Message ID | 1616665147-32084-6-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Testing SSCH, CSCH and HSCH for errors | expand |
On Thu, 25 Mar 2021 10:39:04 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > When checking for an I/O completion may need to check the cause of > the interrupt depending on the test case. > > Let's provide the tests the possibility to check if the last > valid IRQ received is for the function expected after executing > an instruction or sequence of instructions and if all ctrl flags > of the SCSW are set as expected. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > lib/s390x/css.h | 4 ++-- > lib/s390x/css_lib.c | 21 ++++++++++++++++----- > s390x/css.c | 4 ++-- > 3 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/lib/s390x/css.h b/lib/s390x/css.h > index 5d1e1f0..1603781 100644 > --- a/lib/s390x/css.h > +++ b/lib/s390x/css.h > @@ -316,8 +316,8 @@ void css_irq_io(void); > int css_residual_count(unsigned int schid); > > void enable_io_isc(uint8_t isc); > -int wait_and_check_io_completion(int schid); > -int check_io_completion(int schid); > +int wait_and_check_io_completion(int schid, uint32_t ctrl); > +int check_io_completion(int schid, uint32_t ctrl); > > /* > * CHSC definitions > diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c > index 1e5c409..55e70e6 100644 > --- a/lib/s390x/css_lib.c > +++ b/lib/s390x/css_lib.c > @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, > int count, unsigned char flags) > /* wait_and_check_io_completion: > * @schid: the subchannel ID > + * @ctrl : expected SCSW control flags > */ > -int wait_and_check_io_completion(int schid) > +int wait_and_check_io_completion(int schid, uint32_t ctrl) > { > wait_for_interrupt(PSW_MASK_IO); > - return check_io_completion(schid); > + return check_io_completion(schid, ctrl); > } > > /* check_io_completion: > * @schid: the subchannel ID > + * @ctrl : expected SCSW control flags > * > - * Makes the most common check to validate a successful I/O > - * completion. > + * If the ctrl parameter is not null check the IRB SCSW ctrl I would say "not zero" instead of null, since it's an integer and not a pointer. with that fixed: Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > + * against the ctrl parameter. > + * Otherwise, makes the most common check to validate a successful > + * I/O completion. > * Only report failures. > */ > -int check_io_completion(int schid) > +int check_io_completion(int schid, uint32_t ctrl) > { > int ret = 0; > > @@ -515,6 +519,13 @@ int check_io_completion(int schid) > goto end; > } > > + if (ctrl && (ctrl ^ irb.scsw.ctrl)) { > + report_info("%08x : %s", schid, > dump_scsw_flags(irb.scsw.ctrl)); > + report_info("expected : %s", dump_scsw_flags(ctrl)); > + ret = -1; > + goto end; > + } > + > /* Verify that device status is valid */ > if (!(irb.scsw.ctrl & SCSW_SC_PENDING)) { > report(0, "No status pending after interrupt. Subch > Ctrl: %08x", diff --git a/s390x/css.c b/s390x/css.c > index 16723f6..57dc340 100644 > --- a/s390x/css.c > +++ b/s390x/css.c > @@ -95,7 +95,7 @@ static void test_sense(void) > goto error; > } > > - if (wait_and_check_io_completion(test_device_sid) < 0) > + if (wait_and_check_io_completion(test_device_sid, 0) < 0) > goto error; > > /* Test transfer completion */ > @@ -138,7 +138,7 @@ static void sense_id(void) > { > assert(!start_ccw1_chain(test_device_sid, ccw)); > > - assert(wait_and_check_io_completion(test_device_sid) >= 0); > + assert(wait_and_check_io_completion(test_device_sid, 0) >= > 0); } > > static void css_init(void)
On 25/03/2021 10.39, Pierre Morel wrote: > When checking for an I/O completion may need to check the cause of > the interrupt depending on the test case. > > Let's provide the tests the possibility to check if the last > valid IRQ received is for the function expected after executing > an instruction or sequence of instructions and if all ctrl flags > of the SCSW are set as expected. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > lib/s390x/css.h | 4 ++-- > lib/s390x/css_lib.c | 21 ++++++++++++++++----- > s390x/css.c | 4 ++-- > 3 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/lib/s390x/css.h b/lib/s390x/css.h > index 5d1e1f0..1603781 100644 > --- a/lib/s390x/css.h > +++ b/lib/s390x/css.h > @@ -316,8 +316,8 @@ void css_irq_io(void); > int css_residual_count(unsigned int schid); > > void enable_io_isc(uint8_t isc); > -int wait_and_check_io_completion(int schid); > -int check_io_completion(int schid); > +int wait_and_check_io_completion(int schid, uint32_t ctrl); > +int check_io_completion(int schid, uint32_t ctrl); > > /* > * CHSC definitions > diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c > index 1e5c409..55e70e6 100644 > --- a/lib/s390x/css_lib.c > +++ b/lib/s390x/css_lib.c > @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags) > > /* wait_and_check_io_completion: > * @schid: the subchannel ID > + * @ctrl : expected SCSW control flags > */ > -int wait_and_check_io_completion(int schid) > +int wait_and_check_io_completion(int schid, uint32_t ctrl) > { > wait_for_interrupt(PSW_MASK_IO); > - return check_io_completion(schid); > + return check_io_completion(schid, ctrl); > } > > /* check_io_completion: > * @schid: the subchannel ID > + * @ctrl : expected SCSW control flags > * > - * Makes the most common check to validate a successful I/O > - * completion. > + * If the ctrl parameter is not null check the IRB SCSW ctrl > + * against the ctrl parameter. > + * Otherwise, makes the most common check to validate a successful > + * I/O completion. > * Only report failures. > */ > -int check_io_completion(int schid) > +int check_io_completion(int schid, uint32_t ctrl) > { > int ret = 0; > > @@ -515,6 +519,13 @@ int check_io_completion(int schid) > goto end; > } > > + if (ctrl && (ctrl ^ irb.scsw.ctrl)) { With the xor, you can only check for enabled bits ... do we also want to check for disabled bits, or is this always out of scope? Acked-by: Thomas Huth <thuth@redhat.com>
On 29/03/2021 10.27, Thomas Huth wrote: > On 25/03/2021 10.39, Pierre Morel wrote: >> When checking for an I/O completion may need to check the cause of >> the interrupt depending on the test case. >> >> Let's provide the tests the possibility to check if the last >> valid IRQ received is for the function expected after executing >> an instruction or sequence of instructions and if all ctrl flags >> of the SCSW are set as expected. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> lib/s390x/css.h | 4 ++-- >> lib/s390x/css_lib.c | 21 ++++++++++++++++----- >> s390x/css.c | 4 ++-- >> 3 files changed, 20 insertions(+), 9 deletions(-) >> >> diff --git a/lib/s390x/css.h b/lib/s390x/css.h >> index 5d1e1f0..1603781 100644 >> --- a/lib/s390x/css.h >> +++ b/lib/s390x/css.h >> @@ -316,8 +316,8 @@ void css_irq_io(void); >> int css_residual_count(unsigned int schid); >> void enable_io_isc(uint8_t isc); >> -int wait_and_check_io_completion(int schid); >> -int check_io_completion(int schid); >> +int wait_and_check_io_completion(int schid, uint32_t ctrl); >> +int check_io_completion(int schid, uint32_t ctrl); >> /* >> * CHSC definitions >> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c >> index 1e5c409..55e70e6 100644 >> --- a/lib/s390x/css_lib.c >> +++ b/lib/s390x/css_lib.c >> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int >> count, unsigned char flags) >> /* wait_and_check_io_completion: >> * @schid: the subchannel ID >> + * @ctrl : expected SCSW control flags >> */ >> -int wait_and_check_io_completion(int schid) >> +int wait_and_check_io_completion(int schid, uint32_t ctrl) >> { >> wait_for_interrupt(PSW_MASK_IO); >> - return check_io_completion(schid); >> + return check_io_completion(schid, ctrl); >> } >> /* check_io_completion: >> * @schid: the subchannel ID >> + * @ctrl : expected SCSW control flags >> * >> - * Makes the most common check to validate a successful I/O >> - * completion. >> + * If the ctrl parameter is not null check the IRB SCSW ctrl >> + * against the ctrl parameter. >> + * Otherwise, makes the most common check to validate a successful >> + * I/O completion. >> * Only report failures. >> */ >> -int check_io_completion(int schid) >> +int check_io_completion(int schid, uint32_t ctrl) >> { >> int ret = 0; >> @@ -515,6 +519,13 @@ int check_io_completion(int schid) >> goto end; >> } >> + if (ctrl && (ctrl ^ irb.scsw.ctrl)) { > > With the xor, you can only check for enabled bits ... do we also want to > check for disabled bits, or is this always out of scope? Never mind, I think I just did not have enough coffee yet, the check should be fine. But couldn't you simply use "!=" instead of "^" here? Thomas
On 3/29/21 10:32 AM, Thomas Huth wrote: > On 29/03/2021 10.27, Thomas Huth wrote: >> On 25/03/2021 10.39, Pierre Morel wrote: >>> When checking for an I/O completion may need to check the cause of >>> the interrupt depending on the test case. >>> >>> Let's provide the tests the possibility to check if the last >>> valid IRQ received is for the function expected after executing >>> an instruction or sequence of instructions and if all ctrl flags >>> of the SCSW are set as expected. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> --- >>> lib/s390x/css.h | 4 ++-- >>> lib/s390x/css_lib.c | 21 ++++++++++++++++----- >>> s390x/css.c | 4 ++-- >>> 3 files changed, 20 insertions(+), 9 deletions(-) >>> >>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h >>> index 5d1e1f0..1603781 100644 >>> --- a/lib/s390x/css.h >>> +++ b/lib/s390x/css.h >>> @@ -316,8 +316,8 @@ void css_irq_io(void); >>> int css_residual_count(unsigned int schid); >>> void enable_io_isc(uint8_t isc); >>> -int wait_and_check_io_completion(int schid); >>> -int check_io_completion(int schid); >>> +int wait_and_check_io_completion(int schid, uint32_t ctrl); >>> +int check_io_completion(int schid, uint32_t ctrl); >>> /* >>> * CHSC definitions >>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c >>> index 1e5c409..55e70e6 100644 >>> --- a/lib/s390x/css_lib.c >>> +++ b/lib/s390x/css_lib.c >>> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, >>> int count, unsigned char flags) >>> /* wait_and_check_io_completion: >>> * @schid: the subchannel ID >>> + * @ctrl : expected SCSW control flags >>> */ >>> -int wait_and_check_io_completion(int schid) >>> +int wait_and_check_io_completion(int schid, uint32_t ctrl) >>> { >>> wait_for_interrupt(PSW_MASK_IO); >>> - return check_io_completion(schid); >>> + return check_io_completion(schid, ctrl); >>> } >>> /* check_io_completion: >>> * @schid: the subchannel ID >>> + * @ctrl : expected SCSW control flags >>> * >>> - * Makes the most common check to validate a successful I/O >>> - * completion. >>> + * If the ctrl parameter is not null check the IRB SCSW ctrl >>> + * against the ctrl parameter. >>> + * Otherwise, makes the most common check to validate a successful >>> + * I/O completion. >>> * Only report failures. >>> */ >>> -int check_io_completion(int schid) >>> +int check_io_completion(int schid, uint32_t ctrl) >>> { >>> int ret = 0; >>> @@ -515,6 +519,13 @@ int check_io_completion(int schid) >>> goto end; >>> } >>> + if (ctrl && (ctrl ^ irb.scsw.ctrl)) { >> >> With the xor, you can only check for enabled bits ... do we also want >> to check for disabled bits, or is this always out of scope? > > Never mind, I think I just did not have enough coffee yet, the check > should be fine. But couldn't you simply use "!=" instead of "^" here? > > Thomas > OK, yes I can
On 3/29/21 10:27 AM, Thomas Huth wrote: > On 25/03/2021 10.39, Pierre Morel wrote: >> When checking for an I/O completion may need to check the cause of >> the interrupt depending on the test case. >> >> Let's provide the tests the possibility to check if the last >> valid IRQ received is for the function expected after executing >> an instruction or sequence of instructions and if all ctrl flags >> of the SCSW are set as expected. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> lib/s390x/css.h | 4 ++-- >> lib/s390x/css_lib.c | 21 ++++++++++++++++----- >> s390x/css.c | 4 ++-- >> 3 files changed, 20 insertions(+), 9 deletions(-) >> >> diff --git a/lib/s390x/css.h b/lib/s390x/css.h >> index 5d1e1f0..1603781 100644 >> --- a/lib/s390x/css.h >> +++ b/lib/s390x/css.h >> @@ -316,8 +316,8 @@ void css_irq_io(void); >> int css_residual_count(unsigned int schid); >> void enable_io_isc(uint8_t isc); >> -int wait_and_check_io_completion(int schid); >> -int check_io_completion(int schid); >> +int wait_and_check_io_completion(int schid, uint32_t ctrl); >> +int check_io_completion(int schid, uint32_t ctrl); >> /* >> * CHSC definitions >> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c >> index 1e5c409..55e70e6 100644 >> --- a/lib/s390x/css_lib.c >> +++ b/lib/s390x/css_lib.c >> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int >> count, unsigned char flags) >> /* wait_and_check_io_completion: >> * @schid: the subchannel ID >> + * @ctrl : expected SCSW control flags >> */ >> -int wait_and_check_io_completion(int schid) >> +int wait_and_check_io_completion(int schid, uint32_t ctrl) >> { >> wait_for_interrupt(PSW_MASK_IO); >> - return check_io_completion(schid); >> + return check_io_completion(schid, ctrl); >> } >> /* check_io_completion: >> * @schid: the subchannel ID >> + * @ctrl : expected SCSW control flags >> * >> - * Makes the most common check to validate a successful I/O >> - * completion. >> + * If the ctrl parameter is not null check the IRB SCSW ctrl >> + * against the ctrl parameter. >> + * Otherwise, makes the most common check to validate a successful >> + * I/O completion. >> * Only report failures. >> */ >> -int check_io_completion(int schid) >> +int check_io_completion(int schid, uint32_t ctrl) >> { >> int ret = 0; >> @@ -515,6 +519,13 @@ int check_io_completion(int schid) >> goto end; >> } >> + if (ctrl && (ctrl ^ irb.scsw.ctrl)) { > > With the xor, you can only check for enabled bits ... do we also want to > check for disabled bits, or is this always out of scope? > > Acked-by: Thomas Huth <thuth@redhat.com> > Thanks, Pierre
On Thu, 25 Mar 2021 10:39:04 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > When checking for an I/O completion may need to check the cause of > the interrupt depending on the test case. "When we check for the completion of an I/O, we may need to check..." ? > > Let's provide the tests the possibility to check if the last > valid IRQ received is for the function expected after executing "Let's make it possible for the tests to check whether the last valid IRB received indicates the expected functions..." ? > an instruction or sequence of instructions and if all ctrl flags > of the SCSW are set as expected. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > lib/s390x/css.h | 4 ++-- > lib/s390x/css_lib.c | 21 ++++++++++++++++----- > s390x/css.c | 4 ++-- > 3 files changed, 20 insertions(+), 9 deletions(-) > (...) > diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c > index 1e5c409..55e70e6 100644 > --- a/lib/s390x/css_lib.c > +++ b/lib/s390x/css_lib.c > @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags) > > /* wait_and_check_io_completion: > * @schid: the subchannel ID > + * @ctrl : expected SCSW control flags > */ > -int wait_and_check_io_completion(int schid) > +int wait_and_check_io_completion(int schid, uint32_t ctrl) > { > wait_for_interrupt(PSW_MASK_IO); > - return check_io_completion(schid); > + return check_io_completion(schid, ctrl); > } > > /* check_io_completion: > * @schid: the subchannel ID > + * @ctrl : expected SCSW control flags > * > - * Makes the most common check to validate a successful I/O > - * completion. > + * If the ctrl parameter is not null check the IRB SCSW ctrl > + * against the ctrl parameter. > + * Otherwise, makes the most common check to validate a successful > + * I/O completion. What about: "Perform some standard checks to validate a successful I/O completion. If the ctrl parameter is not zero, additionally verify that the specified bits are indicated in the IRB SCSW ctrl flags." > * Only report failures. > */ > -int check_io_completion(int schid) > +int check_io_completion(int schid, uint32_t ctrl) > { > int ret = 0; > With Thomas' suggested change, Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 3/30/21 2:10 PM, Cornelia Huck wrote: > On Thu, 25 Mar 2021 10:39:04 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> When checking for an I/O completion may need to check the cause of >> the interrupt depending on the test case. > > "When we check for the completion of an I/O, we may need to check..." ? > yes, thanks >> >> Let's provide the tests the possibility to check if the last >> valid IRQ received is for the function expected after executing > > "Let's make it possible for the tests to check whether the last valid > IRB received indicates the expected functions..." ? better too :) > >> an instruction or sequence of instructions and if all ctrl flags >> of the SCSW are set as expected. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> lib/s390x/css.h | 4 ++-- >> lib/s390x/css_lib.c | 21 ++++++++++++++++----- >> s390x/css.c | 4 ++-- >> 3 files changed, 20 insertions(+), 9 deletions(-) >> > > (...) > >> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c >> index 1e5c409..55e70e6 100644 >> --- a/lib/s390x/css_lib.c >> +++ b/lib/s390x/css_lib.c >> @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags) >> >> /* wait_and_check_io_completion: >> * @schid: the subchannel ID >> + * @ctrl : expected SCSW control flags >> */ >> -int wait_and_check_io_completion(int schid) >> +int wait_and_check_io_completion(int schid, uint32_t ctrl) >> { >> wait_for_interrupt(PSW_MASK_IO); >> - return check_io_completion(schid); >> + return check_io_completion(schid, ctrl); >> } >> >> /* check_io_completion: >> * @schid: the subchannel ID >> + * @ctrl : expected SCSW control flags >> * >> - * Makes the most common check to validate a successful I/O >> - * completion. >> + * If the ctrl parameter is not null check the IRB SCSW ctrl >> + * against the ctrl parameter. >> + * Otherwise, makes the most common check to validate a successful >> + * I/O completion. > > What about: > > "Perform some standard checks to validate a successful I/O completion. > If the ctrl parameter is not zero, additionally verify that the > specified bits are indicated in the IRB SCSW ctrl flags." Yes, looks better, thanks > >> * Only report failures. >> */ >> -int check_io_completion(int schid) >> +int check_io_completion(int schid, uint32_t ctrl) >> { >> int ret = 0; >> > > With Thomas' suggested change, > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Thanks, Pierre
diff --git a/lib/s390x/css.h b/lib/s390x/css.h index 5d1e1f0..1603781 100644 --- a/lib/s390x/css.h +++ b/lib/s390x/css.h @@ -316,8 +316,8 @@ void css_irq_io(void); int css_residual_count(unsigned int schid); void enable_io_isc(uint8_t isc); -int wait_and_check_io_completion(int schid); -int check_io_completion(int schid); +int wait_and_check_io_completion(int schid, uint32_t ctrl); +int check_io_completion(int schid, uint32_t ctrl); /* * CHSC definitions diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c index 1e5c409..55e70e6 100644 --- a/lib/s390x/css_lib.c +++ b/lib/s390x/css_lib.c @@ -488,21 +488,25 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags) /* wait_and_check_io_completion: * @schid: the subchannel ID + * @ctrl : expected SCSW control flags */ -int wait_and_check_io_completion(int schid) +int wait_and_check_io_completion(int schid, uint32_t ctrl) { wait_for_interrupt(PSW_MASK_IO); - return check_io_completion(schid); + return check_io_completion(schid, ctrl); } /* check_io_completion: * @schid: the subchannel ID + * @ctrl : expected SCSW control flags * - * Makes the most common check to validate a successful I/O - * completion. + * If the ctrl parameter is not null check the IRB SCSW ctrl + * against the ctrl parameter. + * Otherwise, makes the most common check to validate a successful + * I/O completion. * Only report failures. */ -int check_io_completion(int schid) +int check_io_completion(int schid, uint32_t ctrl) { int ret = 0; @@ -515,6 +519,13 @@ int check_io_completion(int schid) goto end; } + if (ctrl && (ctrl ^ irb.scsw.ctrl)) { + report_info("%08x : %s", schid, dump_scsw_flags(irb.scsw.ctrl)); + report_info("expected : %s", dump_scsw_flags(ctrl)); + ret = -1; + goto end; + } + /* Verify that device status is valid */ if (!(irb.scsw.ctrl & SCSW_SC_PENDING)) { report(0, "No status pending after interrupt. Subch Ctrl: %08x", diff --git a/s390x/css.c b/s390x/css.c index 16723f6..57dc340 100644 --- a/s390x/css.c +++ b/s390x/css.c @@ -95,7 +95,7 @@ static void test_sense(void) goto error; } - if (wait_and_check_io_completion(test_device_sid) < 0) + if (wait_and_check_io_completion(test_device_sid, 0) < 0) goto error; /* Test transfer completion */ @@ -138,7 +138,7 @@ static void sense_id(void) { assert(!start_ccw1_chain(test_device_sid, ccw)); - assert(wait_and_check_io_completion(test_device_sid) >= 0); + assert(wait_and_check_io_completion(test_device_sid, 0) >= 0); } static void css_init(void)
When checking for an I/O completion may need to check the cause of the interrupt depending on the test case. Let's provide the tests the possibility to check if the last valid IRQ received is for the function expected after executing an instruction or sequence of instructions and if all ctrl flags of the SCSW are set as expected. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- lib/s390x/css.h | 4 ++-- lib/s390x/css_lib.c | 21 ++++++++++++++++----- s390x/css.c | 4 ++-- 3 files changed, 20 insertions(+), 9 deletions(-)