On Thu, 9 Jul 2020 15:41:56 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
> On 2020-07-09 15:30, Cornelia Huck wrote:
> > On Thu, 9 Jul 2020 15:12:05 +0200
> > Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
> >
> >> On 2020-07-09 13:40, Cornelia Huck wrote:
> >>> On Thu, 9 Jul 2020 10:07:47 +0200
> >>> Pierre Morel <pmorel@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >>>> Acked-by: Thomas Huth <thuth@xxxxxxxxxx>
> >>>> Reviewed-by: Cornelia Huck <cohuck@xxxxxxxxxx>
> >>>> ---
> >>>> 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.)
