| Message ID | 1594282068-11054-9-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 Thu, 9 Jul 2020 10:07:47 +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 and the specified ISC > - 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 and uses the specified ISC. > - If the command succeeds but subchannel is not enabled or the ISC > field is not as expected, retry a predefined retries count. > - If the command fails, report the failure and do not retry, even > if cc indicates a busy/status pending as we do not expect this. > > This tests the MSCH instruction to enable a channel successfully. > Retries are done and in case of error, and if the retries count > is exceeded, a report is made. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Acked-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > --- > lib/s390x/css.h | 8 +++-- > lib/s390x/css_lib.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ > s390x/css.c | 15 ++++++++++ > 3 files changed, 92 insertions(+), 3 deletions(-) (...) > +/* > + * css_msch: enable subchannel and set with specified ISC "css_enable: enable the subchannel with the specified ISC" ? > + * @schid: Subchannel Identifier > + * @isc : number of the interruption subclass to use > + * Return value: > + * On success: 0 > + * On error the CC of the faulty instruction > + * or -1 if the retry count is exceeded. > + */ > +int css_enable(int schid, int isc) > +{ > + struct pmcw *pmcw = &schib.pmcw; > + int retry_count = 0; > + uint16_t flags; > + int cc; > + > + /* Read the SCHIB for this subchannel */ > + cc = stsch(schid, &schib); > + if (cc) { > + report_info("stsch: sch %08x failed with cc=%d", schid, cc); > + return cc; > + } > + > + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); > + if ((pmcw->flags & flags) == flags) { I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this catches the case of "subchannel has been enabled before, but with a different isc". > + report_info("stsch: sch %08x already enabled", schid); > + return 0; > + } (...)
On 2020-07-09 13:40, Cornelia Huck wrote: > On Thu, 9 Jul 2020 10:07:47 +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 and the specified ISC >> - 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 and uses the specified ISC. >> - If the command succeeds but subchannel is not enabled or the ISC >> field is not as expected, retry a predefined retries count. >> - If the command fails, report the failure and do not retry, even >> if cc indicates a busy/status pending as we do not expect this. >> >> This tests the MSCH instruction to enable a channel successfully. >> Retries are done and in case of error, and if the retries count >> is exceeded, a report is made. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> Acked-by: Thomas Huth <thuth@redhat.com> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >> --- >> lib/s390x/css.h | 8 +++-- >> lib/s390x/css_lib.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ >> s390x/css.c | 15 ++++++++++ >> 3 files changed, 92 insertions(+), 3 deletions(-) > > (...) > >> +/* >> + * css_msch: enable subchannel and set with specified ISC > > "css_enable: enable the subchannel with the specified ISC" > > ? > >> + * @schid: Subchannel Identifier >> + * @isc : number of the interruption subclass to use >> + * Return value: >> + * On success: 0 >> + * On error the CC of the faulty instruction >> + * or -1 if the retry count is exceeded. >> + */ >> +int css_enable(int schid, int isc) >> +{ >> + struct pmcw *pmcw = &schib.pmcw; >> + int retry_count = 0; >> + uint16_t flags; >> + int cc; >> + >> + /* Read the SCHIB for this subchannel */ >> + cc = stsch(schid, &schib); >> + if (cc) { >> + report_info("stsch: sch %08x failed with cc=%d", schid, cc); >> + return cc; >> + } >> + >> + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); >> + if ((pmcw->flags & flags) == flags) { > > I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this > catches the case of "subchannel has been enabled before, but with a > different isc". If with a different ISC, we need to modify the ISC. Don't we ? > >> + report_info("stsch: sch %08x already enabled", schid); >> + return 0; >> + } > > (...) >
On Thu, 9 Jul 2020 15:12:05 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 2020-07-09 13:40, Cornelia Huck wrote: > > On Thu, 9 Jul 2020 10:07:47 +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 and the specified ISC > >> - 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 and uses the specified ISC. > >> - If the command succeeds but subchannel is not enabled or the ISC > >> field is not as expected, retry a predefined retries count. > >> - If the command fails, report the failure and do not retry, even > >> if cc indicates a busy/status pending as we do not expect this. > >> > >> This tests the MSCH instruction to enable a channel successfully. > >> Retries are done and in case of error, and if the retries count > >> is exceeded, a report is made. > >> > >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >> Acked-by: Thomas Huth <thuth@redhat.com> > >> Reviewed-by: Cornelia Huck <cohuck@redhat.com> > >> --- > >> lib/s390x/css.h | 8 +++-- > >> lib/s390x/css_lib.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ > >> s390x/css.c | 15 ++++++++++ > >> 3 files changed, 92 insertions(+), 3 deletions(-) > > > > (...) > > > >> +/* > >> + * css_msch: enable subchannel and set with specified ISC > > > > "css_enable: enable the subchannel with the specified ISC" > > > > ? > > > >> + * @schid: Subchannel Identifier > >> + * @isc : number of the interruption subclass to use > >> + * Return value: > >> + * On success: 0 > >> + * On error the CC of the faulty instruction > >> + * or -1 if the retry count is exceeded. > >> + */ > >> +int css_enable(int schid, int isc) > >> +{ > >> + struct pmcw *pmcw = &schib.pmcw; > >> + int retry_count = 0; > >> + uint16_t flags; > >> + int cc; > >> + > >> + /* Read the SCHIB for this subchannel */ > >> + cc = stsch(schid, &schib); > >> + if (cc) { > >> + report_info("stsch: sch %08x failed with cc=%d", schid, cc); > >> + return cc; > >> + } > >> + > >> + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); > >> + if ((pmcw->flags & flags) == flags) { > > > > I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this > > catches the case of "subchannel has been enabled before, but with a > > different isc". > > If with a different ISC, we need to modify the ISC. > Don't we ? I think that's a policy decision (I would probably fail and require a disable before setting another isc, but that's a matter of taste). Regardless, I think the current check doesn't even catch the 'different isc' case? > > > > >> + report_info("stsch: sch %08x already enabled", schid); > >> + return 0; > >> + } > > > > > > > (...) > > > >
On 2020-07-09 15:30, Cornelia Huck wrote: > On Thu, 9 Jul 2020 15:12:05 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 2020-07-09 13:40, Cornelia Huck wrote: >>> On Thu, 9 Jul 2020 10:07:47 +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 and the specified ISC >>>> - 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 and uses the specified ISC. >>>> - If the command succeeds but subchannel is not enabled or the ISC >>>> field is not as expected, retry a predefined retries count. >>>> - If the command fails, report the failure and do not retry, even >>>> if cc indicates a busy/status pending as we do not expect this. >>>> >>>> This tests the MSCH instruction to enable a channel successfully. >>>> Retries are done and in case of error, and if the retries count >>>> is exceeded, a report is made. >>>> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>> Acked-by: Thomas Huth <thuth@redhat.com> >>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>> --- >>>> lib/s390x/css.h | 8 +++-- >>>> lib/s390x/css_lib.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ >>>> s390x/css.c | 15 ++++++++++ >>>> 3 files changed, 92 insertions(+), 3 deletions(-) >>> >>> (...) >>> >>>> +/* >>>> + * css_msch: enable subchannel and set with specified ISC >>> >>> "css_enable: enable the subchannel with the specified ISC" >>> >>> ? >>> >>>> + * @schid: Subchannel Identifier >>>> + * @isc : number of the interruption subclass to use >>>> + * Return value: >>>> + * On success: 0 >>>> + * On error the CC of the faulty instruction >>>> + * or -1 if the retry count is exceeded. >>>> + */ >>>> +int css_enable(int schid, int isc) >>>> +{ >>>> + struct pmcw *pmcw = &schib.pmcw; >>>> + int retry_count = 0; >>>> + uint16_t flags; >>>> + int cc; >>>> + >>>> + /* Read the SCHIB for this subchannel */ >>>> + cc = stsch(schid, &schib); >>>> + if (cc) { >>>> + report_info("stsch: sch %08x failed with cc=%d", schid, cc); >>>> + return cc; >>>> + } >>>> + >>>> + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); >>>> + if ((pmcw->flags & flags) == flags) { >>> >>> I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this >>> catches the case of "subchannel has been enabled before, but with a >>> different isc". >> >> If with a different ISC, we need to modify the ISC. >> Don't we ? > > I think that's a policy decision (I would probably fail and require a > disable before setting another isc, but that's a matter of taste). > > Regardless, I think the current check doesn't even catch the 'different > isc' case? hum, right. If it is OK I remove this one. And I must rework the same test I do later in this patch. Thanks, Pierre
On Thu, 9 Jul 2020 15:41:56 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 2020-07-09 15:30, Cornelia Huck wrote: > > On Thu, 9 Jul 2020 15:12:05 +0200 > > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > >> On 2020-07-09 13:40, Cornelia Huck wrote: > >>> On Thu, 9 Jul 2020 10:07:47 +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 and the specified ISC > >>>> - 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 and uses the specified ISC. > >>>> - If the command succeeds but subchannel is not enabled or the ISC > >>>> field is not as expected, retry a predefined retries count. > >>>> - If the command fails, report the failure and do not retry, even > >>>> if cc indicates a busy/status pending as we do not expect this. > >>>> > >>>> This tests the MSCH instruction to enable a channel successfully. > >>>> Retries are done and in case of error, and if the retries count > >>>> is exceeded, a report is made. > >>>> > >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >>>> Acked-by: Thomas Huth <thuth@redhat.com> > >>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> > >>>> --- > >>>> lib/s390x/css.h | 8 +++-- > >>>> lib/s390x/css_lib.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ > >>>> s390x/css.c | 15 ++++++++++ > >>>> 3 files changed, 92 insertions(+), 3 deletions(-) > >>> > >>> (...) > >>> > >>>> +/* > >>>> + * css_msch: enable subchannel and set with specified ISC > >>> > >>> "css_enable: enable the subchannel with the specified ISC" > >>> > >>> ? > >>> > >>>> + * @schid: Subchannel Identifier > >>>> + * @isc : number of the interruption subclass to use > >>>> + * Return value: > >>>> + * On success: 0 > >>>> + * On error the CC of the faulty instruction > >>>> + * or -1 if the retry count is exceeded. > >>>> + */ > >>>> +int css_enable(int schid, int isc) > >>>> +{ > >>>> + struct pmcw *pmcw = &schib.pmcw; > >>>> + int retry_count = 0; > >>>> + uint16_t flags; > >>>> + int cc; > >>>> + > >>>> + /* Read the SCHIB for this subchannel */ > >>>> + cc = stsch(schid, &schib); > >>>> + if (cc) { > >>>> + report_info("stsch: sch %08x failed with cc=%d", schid, cc); > >>>> + return cc; > >>>> + } > >>>> + > >>>> + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); > >>>> + if ((pmcw->flags & flags) == flags) { > >>> > >>> I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this > >>> catches the case of "subchannel has been enabled before, but with a > >>> different isc". > >> > >> If with a different ISC, we need to modify the ISC. > >> Don't we ? > > > > I think that's a policy decision (I would probably fail and require a > > disable before setting another isc, but that's a matter of taste). > > > > Regardless, I think the current check doesn't even catch the 'different > > isc' case? > > hum, right. > If it is OK I remove this one. > And I must rework the same test I do later > in this patch. So, you mean checking for PMCW_ENABLE? Or not at all? (I'd check for PMCW_ENABLE.)
On 2020-07-09 15:52, Cornelia Huck wrote: > On Thu, 9 Jul 2020 15:41:56 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 2020-07-09 15:30, Cornelia Huck wrote: >>> On Thu, 9 Jul 2020 15:12:05 +0200 >>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>> >>>> On 2020-07-09 13:40, Cornelia Huck wrote: >>>>> On Thu, 9 Jul 2020 10:07:47 +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 and the specified ISC >>>>>> - 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 and uses the specified ISC. >>>>>> - If the command succeeds but subchannel is not enabled or the ISC >>>>>> field is not as expected, retry a predefined retries count. >>>>>> - If the command fails, report the failure and do not retry, even >>>>>> if cc indicates a busy/status pending as we do not expect this. >>>>>> >>>>>> This tests the MSCH instruction to enable a channel successfully. >>>>>> Retries are done and in case of error, and if the retries count >>>>>> is exceeded, a report is made. >>>>>> >>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>>>>> Acked-by: Thomas Huth <thuth@redhat.com> >>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>>>> --- >>>>>> lib/s390x/css.h | 8 +++-- >>>>>> lib/s390x/css_lib.c | 72 +++++++++++++++++++++++++++++++++++++++++++++ >>>>>> s390x/css.c | 15 ++++++++++ >>>>>> 3 files changed, 92 insertions(+), 3 deletions(-) >>>>> >>>>> (...) >>>>> >>>>>> +/* >>>>>> + * css_msch: enable subchannel and set with specified ISC >>>>> >>>>> "css_enable: enable the subchannel with the specified ISC" >>>>> >>>>> ? >>>>> >>>>>> + * @schid: Subchannel Identifier >>>>>> + * @isc : number of the interruption subclass to use >>>>>> + * Return value: >>>>>> + * On success: 0 >>>>>> + * On error the CC of the faulty instruction >>>>>> + * or -1 if the retry count is exceeded. >>>>>> + */ >>>>>> +int css_enable(int schid, int isc) >>>>>> +{ >>>>>> + struct pmcw *pmcw = &schib.pmcw; >>>>>> + int retry_count = 0; >>>>>> + uint16_t flags; >>>>>> + int cc; >>>>>> + >>>>>> + /* Read the SCHIB for this subchannel */ >>>>>> + cc = stsch(schid, &schib); >>>>>> + if (cc) { >>>>>> + report_info("stsch: sch %08x failed with cc=%d", schid, cc); >>>>>> + return cc; >>>>>> + } >>>>>> + >>>>>> + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); >>>>>> + if ((pmcw->flags & flags) == flags) { >>>>> >>>>> I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this >>>>> catches the case of "subchannel has been enabled before, but with a >>>>> different isc". >>>> >>>> If with a different ISC, we need to modify the ISC. >>>> Don't we ? >>> >>> I think that's a policy decision (I would probably fail and require a >>> disable before setting another isc, but that's a matter of taste). >>> >>> Regardless, I think the current check doesn't even catch the 'different >>> isc' case? >> >> hum, right. >> If it is OK I remove this one. >> And I must rework the same test I do later >> in this patch. > > So, you mean checking for PMCW_ENABLE? Or not at all? > > (I'd check for PMCW_ENABLE.) > - if ((pmcw->flags & flags) == flags) { + if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) { report_info("stsch: sch %08x already enabled", schid); return 0; } I keep both, otherwise I return 0 without setting the ISC. then I have another error: retry: /* Update the SCHIB to enable the channel and set the ISC */ + pmcw->flags &= ~(PMCW_ISC_MASK | PMCW_ENABLE); pmcw->flags |= flags; and finaly the same as the first later... - if ((pmcw->flags & flags) == flags) { + if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) { report_info("stsch: sch %08x successfully modified after %d retries", schid, retry_count); is better I think. What do you think?
On Thu, 9 Jul 2020 15:58:07 +0200 Pierre Morel <pmorel@linux.ibm.com> wrote: > On 2020-07-09 15:52, Cornelia Huck wrote: > > On Thu, 9 Jul 2020 15:41:56 +0200 > > Pierre Morel <pmorel@linux.ibm.com> wrote: > > > >> On 2020-07-09 15:30, Cornelia Huck wrote: > >>> On Thu, 9 Jul 2020 15:12:05 +0200 > >>> Pierre Morel <pmorel@linux.ibm.com> wrote: > >>> > >>>> On 2020-07-09 13:40, Cornelia Huck wrote: > >>>>> On Thu, 9 Jul 2020 10:07:47 +0200 > >>>>> Pierre Morel <pmorel@linux.ibm.com> wrote: > >>>>> (...) > >>>>> > >>>>>> +/* > >>>>>> + * css_msch: enable subchannel and set with specified ISC > >>>>> > >>>>> "css_enable: enable the subchannel with the specified ISC" > >>>>> > >>>>> ? > >>>>> > >>>>>> + * @schid: Subchannel Identifier > >>>>>> + * @isc : number of the interruption subclass to use > >>>>>> + * Return value: > >>>>>> + * On success: 0 > >>>>>> + * On error the CC of the faulty instruction > >>>>>> + * or -1 if the retry count is exceeded. > >>>>>> + */ > >>>>>> +int css_enable(int schid, int isc) > >>>>>> +{ > >>>>>> + struct pmcw *pmcw = &schib.pmcw; > >>>>>> + int retry_count = 0; > >>>>>> + uint16_t flags; > >>>>>> + int cc; > >>>>>> + > >>>>>> + /* Read the SCHIB for this subchannel */ > >>>>>> + cc = stsch(schid, &schib); > >>>>>> + if (cc) { > >>>>>> + report_info("stsch: sch %08x failed with cc=%d", schid, cc); > >>>>>> + return cc; > >>>>>> + } > >>>>>> + > >>>>>> + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); > >>>>>> + if ((pmcw->flags & flags) == flags) { > >>>>> > >>>>> I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this > >>>>> catches the case of "subchannel has been enabled before, but with a > >>>>> different isc". > >>>> > >>>> If with a different ISC, we need to modify the ISC. > >>>> Don't we ? > >>> > >>> I think that's a policy decision (I would probably fail and require a > >>> disable before setting another isc, but that's a matter of taste). > >>> > >>> Regardless, I think the current check doesn't even catch the 'different > >>> isc' case? > >> > >> hum, right. > >> If it is OK I remove this one. > >> And I must rework the same test I do later > >> in this patch. > > > > So, you mean checking for PMCW_ENABLE? Or not at all? > > > > (I'd check for PMCW_ENABLE.) > > > > - if ((pmcw->flags & flags) == flags) { > + if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) { > report_info("stsch: sch %08x already enabled", schid); > return 0; > } > > I keep both, otherwise I return 0 without setting the ISC. Ah, I missed the 'return 0'. > then I have another error: > > retry: > /* Update the SCHIB to enable the channel and set the ISC */ > + pmcw->flags &= ~(PMCW_ISC_MASK | PMCW_ENABLE); Maybe ~PMCW_ISC_MASK is enough? > pmcw->flags |= flags; > > and finaly the same as the first later... > > - if ((pmcw->flags & flags) == flags) { > + if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) { I think you can keep that as-is. > report_info("stsch: sch %08x successfully modified > after %d retries", > schid, retry_count); > > > is better I think. > What do you think? It's probably the right direction.
On 2020-07-09 16:22, Cornelia Huck wrote: > On Thu, 9 Jul 2020 15:58:07 +0200 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >> On 2020-07-09 15:52, Cornelia Huck wrote: >>> On Thu, 9 Jul 2020 15:41:56 +0200 >>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>> >>>> On 2020-07-09 15:30, Cornelia Huck wrote: >>>>> On Thu, 9 Jul 2020 15:12:05 +0200 >>>>> Pierre Morel <pmorel@linux.ibm.com> wrote: >>>>> >>>>>> On 2020-07-09 13:40, Cornelia Huck wrote: >>>>>>> On Thu, 9 Jul 2020 10:07:47 +0200 >>>>>>> Pierre Morel <pmorel@linux.ibm.com> wrote: > >>>>>>> (...) >>>>>>> >>>>>>>> +/* >>>>>>>> + * css_msch: enable subchannel and set with specified ISC >>>>>>> >>>>>>> "css_enable: enable the subchannel with the specified ISC" >>>>>>> >>>>>>> ? >>>>>>> >>>>>>>> + * @schid: Subchannel Identifier >>>>>>>> + * @isc : number of the interruption subclass to use >>>>>>>> + * Return value: >>>>>>>> + * On success: 0 >>>>>>>> + * On error the CC of the faulty instruction >>>>>>>> + * or -1 if the retry count is exceeded. >>>>>>>> + */ >>>>>>>> +int css_enable(int schid, int isc) >>>>>>>> +{ >>>>>>>> + struct pmcw *pmcw = &schib.pmcw; >>>>>>>> + int retry_count = 0; >>>>>>>> + uint16_t flags; >>>>>>>> + int cc; >>>>>>>> + >>>>>>>> + /* Read the SCHIB for this subchannel */ >>>>>>>> + cc = stsch(schid, &schib); >>>>>>>> + if (cc) { >>>>>>>> + report_info("stsch: sch %08x failed with cc=%d", schid, cc); >>>>>>>> + return cc; >>>>>>>> + } >>>>>>>> + >>>>>>>> + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); >>>>>>>> + if ((pmcw->flags & flags) == flags) { >>>>>>> >>>>>>> I think you want (pmcw->flags & PMCW_ENABLE) == PMCW_ENABLE -- this >>>>>>> catches the case of "subchannel has been enabled before, but with a >>>>>>> different isc". >>>>>> >>>>>> If with a different ISC, we need to modify the ISC. >>>>>> Don't we ? >>>>> >>>>> I think that's a policy decision (I would probably fail and require a >>>>> disable before setting another isc, but that's a matter of taste). >>>>> >>>>> Regardless, I think the current check doesn't even catch the 'different >>>>> isc' case? >>>> >>>> hum, right. >>>> If it is OK I remove this one. >>>> And I must rework the same test I do later >>>> in this patch. >>> >>> So, you mean checking for PMCW_ENABLE? Or not at all? >>> >>> (I'd check for PMCW_ENABLE.) >>> >> >> - if ((pmcw->flags & flags) == flags) { >> + if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) { >> report_info("stsch: sch %08x already enabled", schid); >> return 0; >> } >> >> I keep both, otherwise I return 0 without setting the ISC. > > Ah, I missed the 'return 0'. > >> then I have another error: >> >> retry: >> /* Update the SCHIB to enable the channel and set the ISC */ >> + pmcw->flags &= ~(PMCW_ISC_MASK | PMCW_ENABLE); > > Maybe ~PMCW_ISC_MASK is enough? yes > >> pmcw->flags |= flags; >> >> and finaly the same as the first later... >> >> - if ((pmcw->flags & flags) == flags) { >> + if ((pmcw->flags & (PMCW_ISC_MASK | PMCW_ENABLE)) == flags) { > > I think you can keep that as-is. I don't thing so, I just stored the pmcw. if ISC is stored as 3 and I want 1 it is a false positive. Same error as you showed me before. ? > >> report_info("stsch: sch %08x successfully modified >> after %d retries", >> schid, retry_count); >> >> >> is better I think. >> What do you think? > > It's probably the right direction. > Thanks, Pierre
diff --git a/lib/s390x/css.h b/lib/s390x/css.h index 0ddceb1..106479d 100644 --- a/lib/s390x/css.h +++ b/lib/s390x/css.h @@ -71,8 +71,9 @@ struct scsw { struct pmcw { uint32_t intparm; -#define PMCW_DNV 0x0001 -#define PMCW_ENABLE 0x0080 +#define PMCW_DNV 0x0001 +#define PMCW_ENABLE 0x0080 +#define PMCW_ISC_SHIFT 11 uint16_t flags; uint16_t devnum; uint8_t lpm; @@ -251,6 +252,7 @@ void dump_orb(struct orb *op); int css_enumerate(void); #define MAX_ENABLE_RETRIES 5 -int css_enable(int schid); +#define IO_SCH_ISC 3 +int css_enable(int schid, int isc); #endif diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c index fd087ce..eda68a4 100644 --- a/lib/s390x/css_lib.c +++ b/lib/s390x/css_lib.c @@ -15,6 +15,7 @@ #include <string.h> #include <interrupt.h> #include <asm/arch_def.h> +#include <asm/time.h> #include <css.h> @@ -68,3 +69,74 @@ out: scn, scn_found, dev_found); return schid; } + +/* + * css_msch: enable subchannel and set with specified ISC + * @schid: Subchannel Identifier + * @isc : number of the interruption subclass to use + * Return value: + * On success: 0 + * On error the CC of the faulty instruction + * or -1 if the retry count is exceeded. + */ +int css_enable(int schid, int isc) +{ + struct pmcw *pmcw = &schib.pmcw; + int retry_count = 0; + uint16_t flags; + int cc; + + /* Read the SCHIB for this subchannel */ + cc = stsch(schid, &schib); + if (cc) { + report_info("stsch: sch %08x failed with cc=%d", schid, cc); + return cc; + } + + flags = PMCW_ENABLE | (isc << PMCW_ISC_SHIFT); + if ((pmcw->flags & flags) == flags) { + report_info("stsch: sch %08x already enabled", schid); + return 0; + } + +retry: + /* Update the SCHIB to enable the channel and set the ISC */ + pmcw->flags |= flags; + + /* Tell the CSS we want to modify the subchannel */ + cc = msch(schid, &schib); + if (cc) { + /* + * If the subchannel is status pending or + * if a function is in progress, + * we consider both cases as errors. + */ + report_info("msch: sch %08x failed with cc=%d", schid, cc); + return cc; + } + + /* + * 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); + return 0; + } + + if (retry_count++ < MAX_ENABLE_RETRIES) { + mdelay(10); /* the hardware was not ready, give it some time */ + goto retry; + } + + report_info("msch: modifying sch %08x failed after %d retries. pmcw flags: %04x", + schid, retry_count, pmcw->flags); + return -1; +} diff --git a/s390x/css.c b/s390x/css.c index e19ffc8..f314a0c 100644 --- a/s390x/css.c +++ b/s390x/css.c @@ -31,11 +31,26 @@ static void test_enumerate(void) report(0, "No I/O device found"); } +static void test_enable(void) +{ + int cc; + + if (!test_device_sid) { + report_skip("No device"); + return; + } + + cc = css_enable(test_device_sid, IO_SCH_ISC); + + report(cc == 0, "Enable subchannel %08x", test_device_sid); +} + static struct { const char *name; void (*func)(void); } tests[] = { { "enumerate (stsch)", test_enumerate }, + { "enable (msch)", test_enable }, { NULL, NULL } };