| Message ID | 1616073988-10381-5-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 3/18/21 2:26 PM, Pierre Morel wrote: > When waiting for an interrupt we 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 expected instruction. s/instruction/command/? We're checking for some value in an IO structure, right? Instruction makes me expect an actual processor instruction. Is there another word that can be used to describe what we're checking here? If yes please also add it to the "pending" variable. "pending_fc" or "pending_scsw_fc" for example. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > lib/s390x/css.h | 2 +- > lib/s390x/css_lib.c | 11 ++++++++++- > s390x/css.c | 4 ++-- > 3 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/lib/s390x/css.h b/lib/s390x/css.h > index 65fc335..add3b4e 100644 > --- a/lib/s390x/css.h > +++ b/lib/s390x/css.h > @@ -316,7 +316,7 @@ 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 wait_and_check_io_completion(int schid, uint32_t pending); > > /* > * CHSC definitions > diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c > index 211c73c..4cdd7ad 100644 > --- a/lib/s390x/css_lib.c > +++ b/lib/s390x/css_lib.c > @@ -537,7 +537,7 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags) > * completion. > * Only report failures. > */ > -int wait_and_check_io_completion(int schid) > +int wait_and_check_io_completion(int schid, uint32_t pending) > { > int ret = 0; > struct irq_entry *irq = NULL; > @@ -569,6 +569,15 @@ int wait_and_check_io_completion(int schid) > goto end; > } > > + if (pending) { > + if (!(pending & irq->irb.scsw.ctrl)) { > + report_info("%08x : %s", schid, dump_scsw_flags(irq->irb.scsw.ctrl)); > + report_info("expect : %s", dump_scsw_flags(pending)); > + ret = -1; > + goto end; > + } > + } > + > /* clear and halt pending are valid even without secondary or primary status */ > if (irq->irb.scsw.ctrl & (SCSW_FC_CLEAR | SCSW_FC_HALT)) { > ret = 0; > diff --git a/s390x/css.c b/s390x/css.c > index c340c53..a6a9773 100644 > --- a/s390x/css.c > +++ b/s390x/css.c > @@ -94,7 +94,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, SCSW_FC_START) < 0) > goto error; > > /* Test transfer completion */ > @@ -137,7 +137,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, SCSW_FC_START) >= 0); > } > > static void css_init(void) >
On 3/19/21 10:09 AM, Janosch Frank wrote: > On 3/18/21 2:26 PM, Pierre Morel wrote: >> When waiting for an interrupt we 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 expected instruction. > > s/instruction/command/? Right, instruction may not be the optimal wording. I/O architecture description have some strange (for me) wording, the best is certainly to stick on this. Then I will use "the expected function" here. > > We're checking for some value in an IO structure, right? > Instruction makes me expect an actual processor instruction. > > Is there another word that can be used to describe what we're checking > here? If yes please also add it to the "pending" variable. "pending_fc" > or "pending_scsw_fc" for example. Pending is used to specify that the instruction has been accepted but the according function is still pending, i.e. not finished and will stay pending for a normal operation until the device active bit is set. So pending is not the right word, what we check here is the function control, indicating the function the status refers too. > >> ...snip... >> * Only report failures. >> */ >> -int wait_and_check_io_completion(int schid) >> +int wait_and_check_io_completion(int schid, uint32_t pending) Consequently I will change "pending" with "function_ctrl" Thanks for forcing clarification I hope Connie will agree with this :) Regards, Pierre
On Fri, 19 Mar 2021 10:50:09 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 3/19/21 10:09 AM, Janosch Frank wrote: > > On 3/18/21 2:26 PM, Pierre Morel wrote: > >> When waiting for an interrupt we 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 expected instruction. > > > > s/instruction/command/? > > Right, instruction may not be the optimal wording. > I/O architecture description have some strange (for me) wording, the > best is certainly to stick on this. > > Then I will use "the expected function" here. > > > > > We're checking for some value in an IO structure, right? > > Instruction makes me expect an actual processor instruction. > > > > Is there another word that can be used to describe what we're checking > > here? If yes please also add it to the "pending" variable. "pending_fc" > > or "pending_scsw_fc" for example. > > Pending is used to specify that the instruction has been accepted but > the according function is still pending, i.e. not finished and will stay > pending for a normal operation until the device active bit is set. > > So pending is not the right word, what we check here is the function > control, indicating the function the status refers too. > > > > >> > ...snip... > > >> * Only report failures. > >> */ > >> -int wait_and_check_io_completion(int schid) > >> +int wait_and_check_io_completion(int schid, uint32_t pending) > > > Consequently I will change "pending" with "function_ctrl" > > Thanks for forcing clarification > I hope Connie will agree with this :) I'm not quite sure yet :) I/O wording and operation can be complicated... we basically have: - various instructions: ssch, hsch, csch - invoking one of those instructions may initiate a function at the subchannel - if an instruction lead to a function being initiated (but not necessarily actually being performed!), the matching bit is set in the fctl - the fctl bits are accumulative (e.g. if you do a hsch on a subchannel where a start function is still in progress, you'll have both the start and the halt function indicated) and only cleared after collecting final status So, setting the function is a direct consequence of executing an I/O instruction -- but the interrupt may not be directly related to *all* of the functions that are indicated (e.g. in the example above, where we may get an interrupt for the hsch, but none directly for the ssch; you can also add a csch on top of this; fortunately, we only stack in the start->halt->clear direction.) Regarding wording: Maybe "if the last valid IRQ received is for the function expected after executing an instruction or sequence of instructions." and int wait_and_check_io_completion(int schid, uint32_t expected_fctl) ?
On 3/19/21 12:23 PM, Cornelia Huck wrote: > On Fri, 19 Mar 2021 10:50:09 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 3/19/21 10:09 AM, Janosch Frank wrote: >>> On 3/18/21 2:26 PM, Pierre Morel wrote: >>>> When waiting for an interrupt we 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 expected instruction. >>> >>> s/instruction/command/? >> >> Right, instruction may not be the optimal wording. >> I/O architecture description have some strange (for me) wording, the >> best is certainly to stick on this. >> >> Then I will use "the expected function" here. >> >>> >>> We're checking for some value in an IO structure, right? >>> Instruction makes me expect an actual processor instruction. >>> >>> Is there another word that can be used to describe what we're checking >>> here? If yes please also add it to the "pending" variable. "pending_fc" >>> or "pending_scsw_fc" for example. >> >> Pending is used to specify that the instruction has been accepted but >> the according function is still pending, i.e. not finished and will stay >> pending for a normal operation until the device active bit is set. >> >> So pending is not the right word, what we check here is the function >> control, indicating the function the status refers too. >> >>> >>>> >> ...snip... >> >>>> * Only report failures. >>>> */ >>>> -int wait_and_check_io_completion(int schid) >>>> +int wait_and_check_io_completion(int schid, uint32_t pending) >> >> >> Consequently I will change "pending" with "function_ctrl" >> >> Thanks for forcing clarification >> I hope Connie will agree with this :) > > I'm not quite sure yet :) > > I/O wording and operation can be complicated... we basically have: > > - various instructions: ssch, hsch, csch > - invoking one of those instructions may initiate a function at the > subchannel > - if an instruction lead to a function being initiated (but not > necessarily actually being performed!), the matching bit is set in > the fctl > - the fctl bits are accumulative (e.g. if you do a hsch on a subchannel > where a start function is still in progress, you'll have both the > start and the halt function indicated) and only cleared after > collecting final status > > So, setting the function is a direct consequence of executing an I/O > instruction -- but the interrupt may not be directly related to *all* > of the functions that are indicated (e.g. in the example above, where > we may get an interrupt for the hsch, but none directly for the ssch; > you can also add a csch on top of this; fortunately, we only stack in > the start->halt->clear direction.) For the real machine but QEMU serialize every I/O instruction so we never get 2 activities indicated at the same time. That is something I tried to check with the last 2 patches. > > Regarding wording: > > Maybe > > "if the last valid IRQ received is for the function expected > after executing an instruction or sequence of instructions." > > and > > int wait_and_check_io_completion(int schid, uint32_t expected_fctl) > > ? > Yes better. Thanks for the comments, Regards, Pierre
diff --git a/lib/s390x/css.h b/lib/s390x/css.h index 65fc335..add3b4e 100644 --- a/lib/s390x/css.h +++ b/lib/s390x/css.h @@ -316,7 +316,7 @@ 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 wait_and_check_io_completion(int schid, uint32_t pending); /* * CHSC definitions diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c index 211c73c..4cdd7ad 100644 --- a/lib/s390x/css_lib.c +++ b/lib/s390x/css_lib.c @@ -537,7 +537,7 @@ struct ccw1 *ccw_alloc(int code, void *data, int count, unsigned char flags) * completion. * Only report failures. */ -int wait_and_check_io_completion(int schid) +int wait_and_check_io_completion(int schid, uint32_t pending) { int ret = 0; struct irq_entry *irq = NULL; @@ -569,6 +569,15 @@ int wait_and_check_io_completion(int schid) goto end; } + if (pending) { + if (!(pending & irq->irb.scsw.ctrl)) { + report_info("%08x : %s", schid, dump_scsw_flags(irq->irb.scsw.ctrl)); + report_info("expect : %s", dump_scsw_flags(pending)); + ret = -1; + goto end; + } + } + /* clear and halt pending are valid even without secondary or primary status */ if (irq->irb.scsw.ctrl & (SCSW_FC_CLEAR | SCSW_FC_HALT)) { ret = 0; diff --git a/s390x/css.c b/s390x/css.c index c340c53..a6a9773 100644 --- a/s390x/css.c +++ b/s390x/css.c @@ -94,7 +94,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, SCSW_FC_START) < 0) goto error; /* Test transfer completion */ @@ -137,7 +137,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, SCSW_FC_START) >= 0); } static void css_init(void)
When waiting for an interrupt we 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 expected instruction. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- lib/s390x/css.h | 2 +- lib/s390x/css_lib.c | 11 ++++++++++- s390x/css.c | 4 ++-- 3 files changed, 13 insertions(+), 4 deletions(-)