| Message ID | 1614599225-17734-3-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CSS Mesurement Block | expand |
On 3/1/21 12:47 PM, Pierre Morel wrote: > In order to ease the writing of tests based on: > - interrupt > - enabling a subchannel > - using multiple I/O on a channel without disabling it > > We do the following simplifications: > - the I/O interrupt handler is registered on CSS initialization > - We do not enable again a subchannel in senseid if it is already > enabled > - we add a css_enabled() function to test if a subchannel is enabled > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > --- > lib/s390x/css.h | 1 + > lib/s390x/css_lib.c | 37 ++++++++++++++++++++++++++----------- > s390x/css.c | 44 ++++++++++++++++++++++++-------------------- > 3 files changed, 51 insertions(+), 31 deletions(-) > > diff --git a/lib/s390x/css.h b/lib/s390x/css.h > index 4210472..fbfa034 100644 > --- a/lib/s390x/css.h > +++ b/lib/s390x/css.h > @@ -278,6 +278,7 @@ int css_enumerate(void); > > #define IO_SCH_ISC 3 > int css_enable(int schid, int isc); > +bool css_enabled(int schid); > > /* Library functions */ > int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw); > diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c > index f46e871..41134dc 100644 > --- a/lib/s390x/css_lib.c > +++ b/lib/s390x/css_lib.c > @@ -161,6 +161,31 @@ out: > return schid; > } > > +/* > + * css_enabled: report if the subchannel is enabled > + * @schid: Subchannel Identifier > + * Return value: > + * true if the subchannel is enabled > + * false otherwise > + */ > +bool css_enabled(int schid) > +{ > + struct pmcw *pmcw = &schib.pmcw; > + int cc; > + > + cc = stsch(schid, &schib); > + if (cc) { > + report_info("stsch: updating sch %08x failed with cc=%d", > + schid, cc); > + return false; > + } > + > + if (!(pmcw->flags & PMCW_ENABLE)) { > + report_info("stsch: sch %08x not enabled", schid); > + return false; > + } > + return true; > +} > /* > * css_enable: enable the subchannel with the specified ISC > * @schid: Subchannel Identifier > @@ -210,18 +235,8 @@ retry: > /* > * Read the SCHIB again to verify the enablement > */ > - cc = stsch(schid, &schib); > - if (cc) { > - report_info("stsch: updating sch %08x failed with cc=%d", > - schid, cc); > - return cc; > - } > - > - if ((pmcw->flags & flags) == flags) { > - report_info("stsch: sch %08x successfully modified after %d retries", > - schid, retry_count); > + if (css_enabled(schid)) > return 0; > - } > > if (retry_count++ < MAX_ENABLE_RETRIES) { > mdelay(10); /* the hardware was not ready, give it some time */ > diff --git a/s390x/css.c b/s390x/css.c > index 09703c1..c9e4903 100644 > --- a/s390x/css.c > +++ b/s390x/css.c > @@ -56,36 +56,27 @@ static void test_enable(void) > * - We need the test device as the first recognized > * device by the enumeration. > */ > -static void test_sense(void) > +static bool do_test_sense(void) > { > struct ccw1 *ccw; > + bool success = false; That is a very counter-intuitive name, something like "retval" might be better. You're free to use the normal int returns but unfortunately you can't use the E* error constants like ENOMEM. > int ret; > int len; > > if (!test_device_sid) { > report_skip("No device"); > - return; > + return success; > } > > - ret = css_enable(test_device_sid, IO_SCH_ISC); > - if (ret) { > - report(0, "Could not enable the subchannel: %08x", > - test_device_sid); > - return; > + if (!css_enabled(test_device_sid)) { > + report(0, "enabling subchannel %08x", test_device_sid); > + return success; > } > > - ret = register_io_int_func(css_irq_io); > - if (ret) { > - report(0, "Could not register IRQ handler"); > - return; > - } > - > - lowcore_ptr->io_int_param = 0; > - > senseid = alloc_io_mem(sizeof(*senseid), 0); > if (!senseid) { > report(0, "Allocation of senseid"); > - goto error_senseid; > + return success; > } > > ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI); > @@ -129,21 +120,34 @@ static void test_sense(void) > report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x", > senseid->reserved, senseid->cu_type, senseid->cu_model, > senseid->dev_type, senseid->dev_model); > + report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type, > + senseid->cu_type); > > - report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x", > - (uint16_t)cu_type, senseid->cu_type); > + success = senseid->cu_type == cu_type; > > error: > free_io_mem(ccw, sizeof(*ccw)); > error_ccw: > free_io_mem(senseid, sizeof(*senseid)); > -error_senseid: > - unregister_io_int_func(css_irq_io); > + return success; > +} > + > +static void test_sense(void) > +{ > + report(do_test_sense(), "Got CU type expected"); > } > > static void css_init(void) > { > report(!!get_chsc_scsc(), "Store Channel Characteristics"); > + > + if (register_io_int_func(css_irq_io)) { > + report(0, "Could not register IRQ handler"); > + return; assert() please > + } > + lowcore_ptr->io_int_param = 0> + > + report(1, "CSS initialized"); > } > > static struct { >
On 3/1/21 4:00 PM, Janosch Frank wrote: > On 3/1/21 12:47 PM, Pierre Morel wrote: >> In order to ease the writing of tests based on: ...snip... >> -static void test_sense(void) >> +static bool do_test_sense(void) >> { >> struct ccw1 *ccw; >> + bool success = false; > > That is a very counter-intuitive name, something like "retval" might be > better. > You're free to use the normal int returns but unfortunately you can't > use the E* error constants like ENOMEM. hum, I had retval and changed it to success on a proposition of Thomas... I find it more intuitive as a bool since this function succeed or fail, no half way and is used for the reporting. other opinion? > >> int ret; >> int len; >> >> if (!test_device_sid) { >> report_skip("No device"); >> - return; >> + return success; >> } >> >> - ret = css_enable(test_device_sid, IO_SCH_ISC); >> - if (ret) { >> - report(0, "Could not enable the subchannel: %08x", >> - test_device_sid); >> - return; >> + if (!css_enabled(test_device_sid)) { >> + report(0, "enabling subchannel %08x", test_device_sid); >> + return success; >> } >> >> - ret = register_io_int_func(css_irq_io); >> - if (ret) { >> - report(0, "Could not register IRQ handler"); >> - return; >> - } >> - >> - lowcore_ptr->io_int_param = 0; >> - >> senseid = alloc_io_mem(sizeof(*senseid), 0); >> if (!senseid) { >> report(0, "Allocation of senseid"); >> - goto error_senseid; >> + return success; >> } >> >> ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI); >> @@ -129,21 +120,34 @@ static void test_sense(void) >> report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x", >> senseid->reserved, senseid->cu_type, senseid->cu_model, >> senseid->dev_type, senseid->dev_model); >> + report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type, >> + senseid->cu_type); >> >> - report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x", >> - (uint16_t)cu_type, senseid->cu_type); >> + success = senseid->cu_type == cu_type; >> >> error: >> free_io_mem(ccw, sizeof(*ccw)); >> error_ccw: >> free_io_mem(senseid, sizeof(*senseid)); >> -error_senseid: >> - unregister_io_int_func(css_irq_io); >> + return success; >> +} >> + >> +static void test_sense(void) >> +{ >> + report(do_test_sense(), "Got CU type expected"); >> } >> >> static void css_init(void) >> { >> report(!!get_chsc_scsc(), "Store Channel Characteristics"); >> + >> + if (register_io_int_func(css_irq_io)) { >> + report(0, "Could not register IRQ handler"); >> + return; > > assert() please Yes. Thanks, Pierre
On 3/8/21 3:13 PM, Pierre Morel wrote: > > > On 3/1/21 4:00 PM, Janosch Frank wrote: >> On 3/1/21 12:47 PM, Pierre Morel wrote: >>> In order to ease the writing of tests based on: > > ...snip... > >>> -static void test_sense(void) >>> +static bool do_test_sense(void) >>> { >>> struct ccw1 *ccw; >>> + bool success = false; >> >> That is a very counter-intuitive name, something like "retval" might be >> better. >> You're free to use the normal int returns but unfortunately you can't >> use the E* error constants like ENOMEM. > > hum, I had retval and changed it to success on a proposition of Thomas... > I find it more intuitive as a bool since this function succeed or fail, > no half way and is used for the reporting. > > other opinion? Alright, it's 2:1 for "success", so keep it if you want. > > >> >>> int ret; >>> int len; >>> >>> if (!test_device_sid) { >>> report_skip("No device"); >>> - return; >>> + return success; >>> } >>> >>> - ret = css_enable(test_device_sid, IO_SCH_ISC); >>> - if (ret) { >>> - report(0, "Could not enable the subchannel: %08x", >>> - test_device_sid); >>> - return; >>> + if (!css_enabled(test_device_sid)) { >>> + report(0, "enabling subchannel %08x", test_device_sid); >>> + return success; >>> } >>> >>> - ret = register_io_int_func(css_irq_io); >>> - if (ret) { >>> - report(0, "Could not register IRQ handler"); >>> - return; >>> - } >>> - >>> - lowcore_ptr->io_int_param = 0; >>> - >>> senseid = alloc_io_mem(sizeof(*senseid), 0); >>> if (!senseid) { >>> report(0, "Allocation of senseid"); >>> - goto error_senseid; >>> + return success; >>> } >>> >>> ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI); >>> @@ -129,21 +120,34 @@ static void test_sense(void) >>> report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x", >>> senseid->reserved, senseid->cu_type, senseid->cu_model, >>> senseid->dev_type, senseid->dev_model); >>> + report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type, >>> + senseid->cu_type); >>> >>> - report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x", >>> - (uint16_t)cu_type, senseid->cu_type); >>> + success = senseid->cu_type == cu_type; >>> >>> error: >>> free_io_mem(ccw, sizeof(*ccw)); >>> error_ccw: >>> free_io_mem(senseid, sizeof(*senseid)); >>> -error_senseid: >>> - unregister_io_int_func(css_irq_io); >>> + return success; >>> +} >>> + >>> +static void test_sense(void) >>> +{ >>> + report(do_test_sense(), "Got CU type expected"); >>> } >>> >>> static void css_init(void) >>> { >>> report(!!get_chsc_scsc(), "Store Channel Characteristics"); >>> + >>> + if (register_io_int_func(css_irq_io)) { >>> + report(0, "Could not register IRQ handler"); >>> + return; >> >> assert() please > > Yes. > > Thanks, > Pierre >
On 3/8/21 3:36 PM, Janosch Frank wrote: > On 3/8/21 3:13 PM, Pierre Morel wrote: >> >> >> On 3/1/21 4:00 PM, Janosch Frank wrote: >>> On 3/1/21 12:47 PM, Pierre Morel wrote: >>>> In order to ease the writing of tests based on: >> >> ...snip... >> >>>> -static void test_sense(void) >>>> +static bool do_test_sense(void) >>>> { >>>> struct ccw1 *ccw; >>>> + bool success = false; >>> >>> That is a very counter-intuitive name, something like "retval" might be >>> better. >>> You're free to use the normal int returns but unfortunately you can't >>> use the E* error constants like ENOMEM. >> >> hum, I had retval and changed it to success on a proposition of Thomas... >> I find it more intuitive as a bool since this function succeed or fail, >> no half way and is used for the reporting. >> >> other opinion? > > Alright, it's 2:1 for "success", so keep it if you want. :) OK thanks
On 08/03/2021 15.13, Pierre Morel wrote: > > > On 3/1/21 4:00 PM, Janosch Frank wrote: >> On 3/1/21 12:47 PM, Pierre Morel wrote: >>> In order to ease the writing of tests based on: > > ...snip... > >>> -static void test_sense(void) >>> +static bool do_test_sense(void) >>> { >>> struct ccw1 *ccw; >>> + bool success = false; >> >> That is a very counter-intuitive name, something like "retval" might be >> better. >> You're free to use the normal int returns but unfortunately you can't >> use the E* error constants like ENOMEM. > > hum, I had retval and changed it to success on a proposition of Thomas... > I find it more intuitive as a bool since this function succeed or fail, no > half way and is used for the reporting. > > other opinion? I'd say either "static int ..." + retval (with 0 for success), or "static bool ..." and "success" (with true for success) ... but "bool" + "retval" sounds confusing to me. Thomas
On 3/8/21 3:41 PM, Thomas Huth wrote: > On 08/03/2021 15.13, Pierre Morel wrote: >> >> >> On 3/1/21 4:00 PM, Janosch Frank wrote: >>> On 3/1/21 12:47 PM, Pierre Morel wrote: >>>> In order to ease the writing of tests based on: >> >> ...snip... >> >>>> -static void test_sense(void) >>>> +static bool do_test_sense(void) >>>> { >>>> struct ccw1 *ccw; >>>> + bool success = false; >>> >>> That is a very counter-intuitive name, something like "retval" might be >>> better. >>> You're free to use the normal int returns but unfortunately you can't >>> use the E* error constants like ENOMEM. >> >> hum, I had retval and changed it to success on a proposition of Thomas... >> I find it more intuitive as a bool since this function succeed or >> fail, no half way and is used for the reporting. >> >> other opinion? > > I'd say either "static int ..." + retval (with 0 for success), or > "static bool ..." and "success" (with true for success) ... but "bool" + > "retval" sounds confusing to me. > > Thomas > OK, thanks, then I keep bool success :) Thanks Pierre
On 3/8/21 3:41 PM, Thomas Huth wrote: > On 08/03/2021 15.13, Pierre Morel wrote: >> >> >> On 3/1/21 4:00 PM, Janosch Frank wrote: >>> On 3/1/21 12:47 PM, Pierre Morel wrote: >>>> In order to ease the writing of tests based on: >> >> ...snip... >> >>>> -static void test_sense(void) >>>> +static bool do_test_sense(void) >>>> { >>>> struct ccw1 *ccw; >>>> + bool success = false; >>> >>> That is a very counter-intuitive name, something like "retval" might be >>> better. >>> You're free to use the normal int returns but unfortunately you can't >>> use the E* error constants like ENOMEM. >> >> hum, I had retval and changed it to success on a proposition of Thomas... >> I find it more intuitive as a bool since this function succeed or >> fail, no half way and is used for the reporting. >> >> other opinion? > > I'd say either "static int ..." + retval (with 0 for success), or > "static bool ..." and "success" (with true for success) ... but "bool" + > "retval" sounds confusing to me. > > Thomas > Hum, OK, I think I see were the unsatisfation about this function comes from. (I do not like it either) Slowly understanding the benefit of assert() and report_abort() in the tests cases I will rework this part and do not change the test_senseid() test. I will introduce a sense_id() function when needing to do I/O in the fmt0 test, asserting in this function that all parts already checked in the preceding tests are functional. This makes all much shorter and cleaner. Regards, Pierre
diff --git a/lib/s390x/css.h b/lib/s390x/css.h index 4210472..fbfa034 100644 --- a/lib/s390x/css.h +++ b/lib/s390x/css.h @@ -278,6 +278,7 @@ int css_enumerate(void); #define IO_SCH_ISC 3 int css_enable(int schid, int isc); +bool css_enabled(int schid); /* Library functions */ int start_ccw1_chain(unsigned int sid, struct ccw1 *ccw); diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c index f46e871..41134dc 100644 --- a/lib/s390x/css_lib.c +++ b/lib/s390x/css_lib.c @@ -161,6 +161,31 @@ out: return schid; } +/* + * css_enabled: report if the subchannel is enabled + * @schid: Subchannel Identifier + * Return value: + * true if the subchannel is enabled + * false otherwise + */ +bool css_enabled(int schid) +{ + struct pmcw *pmcw = &schib.pmcw; + int cc; + + cc = stsch(schid, &schib); + if (cc) { + report_info("stsch: updating sch %08x failed with cc=%d", + schid, cc); + return false; + } + + if (!(pmcw->flags & PMCW_ENABLE)) { + report_info("stsch: sch %08x not enabled", schid); + return false; + } + return true; +} /* * css_enable: enable the subchannel with the specified ISC * @schid: Subchannel Identifier @@ -210,18 +235,8 @@ retry: /* * Read the SCHIB again to verify the enablement */ - cc = stsch(schid, &schib); - if (cc) { - report_info("stsch: updating sch %08x failed with cc=%d", - schid, cc); - return cc; - } - - if ((pmcw->flags & flags) == flags) { - report_info("stsch: sch %08x successfully modified after %d retries", - schid, retry_count); + if (css_enabled(schid)) return 0; - } if (retry_count++ < MAX_ENABLE_RETRIES) { mdelay(10); /* the hardware was not ready, give it some time */ diff --git a/s390x/css.c b/s390x/css.c index 09703c1..c9e4903 100644 --- a/s390x/css.c +++ b/s390x/css.c @@ -56,36 +56,27 @@ static void test_enable(void) * - We need the test device as the first recognized * device by the enumeration. */ -static void test_sense(void) +static bool do_test_sense(void) { struct ccw1 *ccw; + bool success = false; int ret; int len; if (!test_device_sid) { report_skip("No device"); - return; + return success; } - ret = css_enable(test_device_sid, IO_SCH_ISC); - if (ret) { - report(0, "Could not enable the subchannel: %08x", - test_device_sid); - return; + if (!css_enabled(test_device_sid)) { + report(0, "enabling subchannel %08x", test_device_sid); + return success; } - ret = register_io_int_func(css_irq_io); - if (ret) { - report(0, "Could not register IRQ handler"); - return; - } - - lowcore_ptr->io_int_param = 0; - senseid = alloc_io_mem(sizeof(*senseid), 0); if (!senseid) { report(0, "Allocation of senseid"); - goto error_senseid; + return success; } ccw = ccw_alloc(CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), CCW_F_SLI); @@ -129,21 +120,34 @@ static void test_sense(void) report_info("reserved 0x%02x cu_type 0x%04x cu_model 0x%02x dev_type 0x%04x dev_model 0x%02x", senseid->reserved, senseid->cu_type, senseid->cu_model, senseid->dev_type, senseid->dev_model); + report_info("cu_type expected 0x%04x got 0x%04x", (uint16_t)cu_type, + senseid->cu_type); - report(senseid->cu_type == cu_type, "cu_type expected 0x%04x got 0x%04x", - (uint16_t)cu_type, senseid->cu_type); + success = senseid->cu_type == cu_type; error: free_io_mem(ccw, sizeof(*ccw)); error_ccw: free_io_mem(senseid, sizeof(*senseid)); -error_senseid: - unregister_io_int_func(css_irq_io); + return success; +} + +static void test_sense(void) +{ + report(do_test_sense(), "Got CU type expected"); } static void css_init(void) { report(!!get_chsc_scsc(), "Store Channel Characteristics"); + + if (register_io_int_func(css_irq_io)) { + report(0, "Could not register IRQ handler"); + return; + } + lowcore_ptr->io_int_param = 0; + + report(1, "CSS initialized"); } static struct {