| Message ID | 1589818051-20549-10-git-send-email-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Testing the Channel Subsystem I/O | expand |
On Mon, 18 May 2020 18:07:28 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > A second step when testing the channel subsystem is to prepare a channel > for use. > This includes: > - Get the current subchannel Information Block (SCHIB) using STSCH > - Update it in memory to set the ENABLE bit > - Tell the CSS that the SCHIB has been modified using MSCH > - Get the SCHIB from the CSS again to verify that the subchannel is > enabled. > - If the subchannel is not enabled retry a predefined retries count. > > This tests the MSCH instruction to enable a channel succesfuly. > This is NOT a routine to really enable the channel, no retry is done, > in case of error, a report is made. Hm... so you retry if the subchannel is not enabled after cc 0, but you don't retry if the cc indicates busy/status pending? Makes sense, as we don't expect the subchannel to be busy, but a more precise note in the patch description would be good :) > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > --- > s390x/css.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 67 insertions(+) > > diff --git a/s390x/css.c b/s390x/css.c > index d7989d8..1b60a47 100644 > --- a/s390x/css.c > +++ b/s390x/css.c > @@ -16,6 +16,7 @@ > #include <string.h> > #include <interrupt.h> > #include <asm/arch_def.h> > +#include <asm/time.h> > > #include <css.h> > > @@ -65,11 +66,77 @@ out: > scn, scn_found, dev_found); > } > > +#define MAX_ENABLE_RETRIES 5 > +static void test_enable(void) > +{ > + struct pmcw *pmcw = &schib.pmcw; > + int retry_count = 0; > + int cc; > + > + if (!test_device_sid) { > + report_skip("No device"); > + return; > + } > + > + /* Read the SCHIB for this subchannel */ > + cc = stsch(test_device_sid, &schib); > + if (cc) { > + report(0, "stsch cc=%d", cc); > + return; > + } > + > + if (pmcw->flags & PMCW_ENABLE) { > + report(1, "stsch: sch %08x already enabled", test_device_sid); > + return; > + } > + > +retry: > + /* Update the SCHIB to enable the channel */ > + pmcw->flags |= PMCW_ENABLE; > + > + /* Tell the CSS we want to modify the subchannel */ > + cc = msch(test_device_sid, &schib); > + if (cc) { > + /* > + * If the subchannel is status pending or > + * if a function is in progress, > + * we consider both cases as errors. Could also be cc 3, but that would be even more weird. Just logging the cc seems fine, though. > + */ > + report(0, "msch cc=%d", cc); > + return; > + } > + > + /* > + * Read the SCHIB again to verify the enablement > + */ > + cc = stsch(test_device_sid, &schib); > + if (cc) { > + report(0, "stsch cc=%d", cc); > + return; > + } > + > + if (pmcw->flags & PMCW_ENABLE) { > + report(1, "msch: sch %08x enabled after %d retries", > + test_device_sid, retry_count); > + return; > + } > + > + if (retry_count++ < MAX_ENABLE_RETRIES) { > + mdelay(10); /* the hardware was not ready, let it some time */ s/let/give/ > + goto retry; > + } > + > + report(0, > + "msch: enabling sch %08x failed after %d retries. pmcw: %x", > + test_device_sid, retry_count, pmcw->flags); > +} > + > static struct { > const char *name; > void (*func)(void); > } tests[] = { > { "enumerate (stsch)", test_enumerate }, > + { "enable (msch)", test_enable }, > { NULL, NULL } > }; > Otherwise, Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 2020-05-27 11:42, Cornelia Huck wrote: > On Mon, 18 May 2020 18:07:28 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> A second step when testing the channel subsystem is to prepare a channel >> for use. >> This includes: >> - Get the current subchannel Information Block (SCHIB) using STSCH >> - Update it in memory to set the ENABLE bit >> - Tell the CSS that the SCHIB has been modified using MSCH >> - Get the SCHIB from the CSS again to verify that the subchannel is >> enabled. >> - If the subchannel is not enabled retry a predefined retries count. >> >> This tests the MSCH instruction to enable a channel succesfuly. >> This is NOT a routine to really enable the channel, no retry is done, >> in case of error, a report is made. > > Hm... so you retry if the subchannel is not enabled after cc 0, but you > don't retry if the cc indicates busy/status pending? Makes sense, as we > don't expect the subchannel to be busy, but a more precise note in the > patch description would be good :) OK, I add something like " - If the command succeed but subchannel is not enabled retry a predefined retries count. - If the command fails, report the failure and do not retry, even if cc indicates a busy/status as we do not expect this. " > >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> s390x/css.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 67 insertions(+) >> >> diff --git a/s390x/css.c b/s390x/css.c >> index d7989d8..1b60a47 100644 >> --- a/s390x/css.c >> +++ b/s390x/css.c >> @@ -16,6 +16,7 @@ >> #include <string.h> >> #include <interrupt.h> >> #include <asm/arch_def.h> >> +#include <asm/time.h> >> >> #include <css.h> >> >> @@ -65,11 +66,77 @@ out: >> scn, scn_found, dev_found); >> } >> >> +#define MAX_ENABLE_RETRIES 5 >> +static void test_enable(void) >> +{ >> + struct pmcw *pmcw = &schib.pmcw; >> + int retry_count = 0; >> + int cc; >> + >> + if (!test_device_sid) { >> + report_skip("No device"); >> + return; >> + } >> + >> + /* Read the SCHIB for this subchannel */ >> + cc = stsch(test_device_sid, &schib); >> + if (cc) { >> + report(0, "stsch cc=%d", cc); >> + return; >> + } >> + >> + if (pmcw->flags & PMCW_ENABLE) { >> + report(1, "stsch: sch %08x already enabled", test_device_sid); >> + return; >> + } >> + >> +retry: >> + /* Update the SCHIB to enable the channel */ >> + pmcw->flags |= PMCW_ENABLE; >> + >> + /* Tell the CSS we want to modify the subchannel */ >> + cc = msch(test_device_sid, &schib); >> + if (cc) { >> + /* >> + * If the subchannel is status pending or >> + * if a function is in progress, >> + * we consider both cases as errors. > > Could also be cc 3, but that would be even more weird. Just logging the > cc seems fine, though. > >> + */ >> + report(0, "msch cc=%d", cc); >> + return; >> + } >> + >> + /* >> + * Read the SCHIB again to verify the enablement >> + */ >> + cc = stsch(test_device_sid, &schib); >> + if (cc) { >> + report(0, "stsch cc=%d", cc); >> + return; >> + } >> + >> + if (pmcw->flags & PMCW_ENABLE) { >> + report(1, "msch: sch %08x enabled after %d retries", >> + test_device_sid, retry_count); >> + return; >> + } >> + >> + if (retry_count++ < MAX_ENABLE_RETRIES) { >> + mdelay(10); /* the hardware was not ready, let it some time */ > > s/let/give/ > >> + goto retry; >> + } >> + >> + report(0, >> + "msch: enabling sch %08x failed after %d retries. pmcw: %x", >> + test_device_sid, retry_count, pmcw->flags); >> +} >> + >> static struct { >> const char *name; >> void (*func)(void); >> } tests[] = { >> { "enumerate (stsch)", test_enumerate }, >> + { "enable (msch)", test_enable }, >> { NULL, NULL } >> }; >> > > Otherwise, > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > Thanks a lot. I make the corrections. regards, Pierre
On Thu, 4 Jun 2020 14:46:05 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 2020-05-27 11:42, Cornelia Huck wrote: > > On Mon, 18 May 2020 18:07:28 +0200 > > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > >> A second step when testing the channel subsystem is to prepare a channel > >> for use. > >> This includes: > >> - Get the current subchannel Information Block (SCHIB) using STSCH > >> - Update it in memory to set the ENABLE bit > >> - Tell the CSS that the SCHIB has been modified using MSCH > >> - Get the SCHIB from the CSS again to verify that the subchannel is > >> enabled. > >> - If the subchannel is not enabled retry a predefined retries count. > >> > >> This tests the MSCH instruction to enable a channel succesfuly. > >> This is NOT a routine to really enable the channel, no retry is done, > >> in case of error, a report is made. > > > > Hm... so you retry if the subchannel is not enabled after cc 0, but you > > don't retry if the cc indicates busy/status pending? Makes sense, as we > > don't expect the subchannel to be busy, but a more precise note in the > > patch description would be good :) > > OK, I add something like > " > - If the command succeed but subchannel is not enabled retry a s/succeed/succeeds/ :) > predefined retries count. > - If the command fails, report the failure and do not retry, even > if cc indicates a busy/status as we do not expect this. "indicates busy/status pending" ? > "
On 2020-06-04 15:29, Cornelia Huck wrote: > On Thu, 4 Jun 2020 14:46:05 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 2020-05-27 11:42, Cornelia Huck wrote: >>> On Mon, 18 May 2020 18:07:28 +0200 >>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>> >>>> A second step when testing the channel subsystem is to prepare a channel >>>> for use. >>>> This includes: >>>> - Get the current subchannel Information Block (SCHIB) using STSCH >>>> - Update it in memory to set the ENABLE bit >>>> - Tell the CSS that the SCHIB has been modified using MSCH >>>> - Get the SCHIB from the CSS again to verify that the subchannel is >>>> enabled. >>>> - If the subchannel is not enabled retry a predefined retries count. >>>> >>>> This tests the MSCH instruction to enable a channel succesfuly. >>>> This is NOT a routine to really enable the channel, no retry is done, >>>> in case of error, a report is made. >>> >>> Hm... so you retry if the subchannel is not enabled after cc 0, but you >>> don't retry if the cc indicates busy/status pending? Makes sense, as we >>> don't expect the subchannel to be busy, but a more precise note in the >>> patch description would be good :) >> >> OK, I add something like >> " >> - If the command succeed but subchannel is not enabled retry a > > s/succeed/succeeds/ :) > >> predefined retries count. >> - If the command fails, report the failure and do not retry, even >> if cc indicates a busy/status as we do not expect this. > > "indicates busy/status pending" ? > >> " > done, thanks, Pierre
Hi Connie, for the next series, I will move more code of the css.c to the css_lib.c to be able to reuse it for other tests. One of your suggestions IIRC. So I will not be able to keep your RB until you have a look at the changes. I will keep the same algorithms but still... regards, Pierre On 2020-06-05 09:37, Pierre Morel wrote: > > > On 2020-06-04 15:29, Cornelia Huck wrote: >> On Thu, 4 Jun 2020 14:46:05 +0200 >> Pierre Morel <pmorel@linux.ibm.com> wrote: >> >>> On 2020-05-27 11:42, Cornelia Huck wrote: >>>> On Mon, 18 May 2020 18:07:28 +0200 >>>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>>>> A second step when testing the channel subsystem is to prepare a >>>>> channel >>>>> for use. >>>>> This includes: >>>>> - Get the current subchannel Information Block (SCHIB) using STSCH >>>>> - Update it in memory to set the ENABLE bit >>>>> - Tell the CSS that the SCHIB has been modified using MSCH >>>>> - Get the SCHIB from the CSS again to verify that the subchannel is >>>>> enabled. >>>>> - If the subchannel is not enabled retry a predefined retries count. >>>>> >>>>> This tests the MSCH instruction to enable a channel succesfuly. >>>>> This is NOT a routine to really enable the channel, no retry is done, >>>>> in case of error, a report is made. >>>> >>>> Hm... so you retry if the subchannel is not enabled after cc 0, but you >>>> don't retry if the cc indicates busy/status pending? Makes sense, as we >>>> don't expect the subchannel to be busy, but a more precise note in the >>>> patch description would be good :) >>> >>> OK, I add something like >>> " >>> - If the command succeed but subchannel is not enabled retry a >> >> s/succeed/succeeds/ :) >> >>> predefined retries count. >>> - If the command fails, report the failure and do not retry, even >>> if cc indicates a busy/status as we do not expect this. >> >> "indicates busy/status pending" ? >> >>> " >> > done, thanks, > > Pierre >
On Fri, 5 Jun 2020 10:24:56 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > Hi Connie, > > for the next series, I will move more code of the css.c to the css_lib.c > to be able to reuse it for other tests. > One of your suggestions IIRC. > So I will not be able to keep your RB until you have a look at the changes. > I will keep the same algorithms but still... np, I'll look at it again.
diff --git a/s390x/css.c b/s390x/css.c index d7989d8..1b60a47 100644 --- a/s390x/css.c +++ b/s390x/css.c @@ -16,6 +16,7 @@ #include <string.h> #include <interrupt.h> #include <asm/arch_def.h> +#include <asm/time.h> #include <css.h> @@ -65,11 +66,77 @@ out: scn, scn_found, dev_found); } +#define MAX_ENABLE_RETRIES 5 +static void test_enable(void) +{ + struct pmcw *pmcw = &schib.pmcw; + int retry_count = 0; + int cc; + + if (!test_device_sid) { + report_skip("No device"); + return; + } + + /* Read the SCHIB for this subchannel */ + cc = stsch(test_device_sid, &schib); + if (cc) { + report(0, "stsch cc=%d", cc); + return; + } + + if (pmcw->flags & PMCW_ENABLE) { + report(1, "stsch: sch %08x already enabled", test_device_sid); + return; + } + +retry: + /* Update the SCHIB to enable the channel */ + pmcw->flags |= PMCW_ENABLE; + + /* Tell the CSS we want to modify the subchannel */ + cc = msch(test_device_sid, &schib); + if (cc) { + /* + * If the subchannel is status pending or + * if a function is in progress, + * we consider both cases as errors. + */ + report(0, "msch cc=%d", cc); + return; + } + + /* + * Read the SCHIB again to verify the enablement + */ + cc = stsch(test_device_sid, &schib); + if (cc) { + report(0, "stsch cc=%d", cc); + return; + } + + if (pmcw->flags & PMCW_ENABLE) { + report(1, "msch: sch %08x enabled after %d retries", + test_device_sid, retry_count); + return; + } + + if (retry_count++ < MAX_ENABLE_RETRIES) { + mdelay(10); /* the hardware was not ready, let it some time */ + goto retry; + } + + report(0, + "msch: enabling sch %08x failed after %d retries. pmcw: %x", + test_device_sid, retry_count, pmcw->flags); +} + static struct { const char *name; void (*func)(void); } tests[] = { { "enumerate (stsch)", test_enumerate }, + { "enable (msch)", test_enable }, { NULL, NULL } };
A second step when testing the channel subsystem is to prepare a channel for use. This includes: - Get the current subchannel Information Block (SCHIB) using STSCH - Update it in memory to set the ENABLE bit - Tell the CSS that the SCHIB has been modified using MSCH - Get the SCHIB from the CSS again to verify that the subchannel is enabled. - If the subchannel is not enabled retry a predefined retries count. This tests the MSCH instruction to enable a channel succesfuly. This is NOT a routine to really enable the channel, no retry is done, in case of error, a report is made. Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> --- s390x/css.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)