| Message ID | 20170830163609.50260-2-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | | expand |
On 30.08.2017 18:36, Halil Pasic wrote: > The function ioinst_handle_xsch is presenting cc 2 when it's supposed to > present cc 1 and the other way around, because css_do_xsch has the error > codes mixed up. Fixing the error codes also fixes the priority. > > Let us fix this. (Nit: In case you respin, I'd suggest to remove the last sentence. You already used "fix" two times in the previous one) > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > Reported-by: Pierre Morel<pmorel@linux.vnet.ibm.com> Space missing -------------^ > --- > hw/s390x/css.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > index 1880b1a0ff..a50fb0727e 100644 > --- a/hw/s390x/css.c > +++ b/hw/s390x/css.c > @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch) > (!(s->ctrl & > (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) || > (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) { > - ret = -EINPROGRESS; > + ret = -EBUSY; > goto out; > } > > if (s->ctrl & SCSW_CTRL_MASK_STCTL) { > - ret = -EBUSY; > + ret = -EINPROGRESS; > goto out; > } Using both, EBUSY and EINPROGRESS as error codes sounds very confusing to me here ... what's the difference between busy and in-progress? So while you're at it, maybe you could replace the code for CC 2 ("CANCEL SUBCHANNEL not applicable") with a different error code? Thomas
On Thu, 31 Aug 2017 07:51:17 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 30.08.2017 18:36, Halil Pasic wrote: > > The function ioinst_handle_xsch is presenting cc 2 when it's supposed to > > present cc 1 and the other way around, because css_do_xsch has the error > > codes mixed up. Fixing the error codes also fixes the priority. > > > > Let us fix this. > > (Nit: In case you respin, I'd suggest to remove the last sentence. You > already used "fix" two times in the previous one) I can also remove it on applying, if Halil agrees (I have not yet reviewed it, though). > > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > > Reported-by: Pierre Morel<pmorel@linux.vnet.ibm.com> > > Space missing -------------^ And I can also add that space on applying :) > > > --- > > hw/s390x/css.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/s390x/css.c b/hw/s390x/css.c > > index 1880b1a0ff..a50fb0727e 100644 > > --- a/hw/s390x/css.c > > +++ b/hw/s390x/css.c > > @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch) > > (!(s->ctrl & > > (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) || > > (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) { > > - ret = -EINPROGRESS; > > + ret = -EBUSY; > > goto out; > > } > > > > if (s->ctrl & SCSW_CTRL_MASK_STCTL) { > > - ret = -EBUSY; > > + ret = -EINPROGRESS; > > goto out; > > } > > Using both, EBUSY and EINPROGRESS as error codes sounds very confusing > to me here ... what's the difference between busy and in-progress? So > while you're at it, maybe you could replace the code for CC 2 ("CANCEL > SUBCHANNEL not applicable") with a different error code? IIRC, I used these two as they matched my idea of what happens best (there's a difference between "subchannel is busy" and "the I/O is already in progress, too late to cancel"). "xsch not applicable" is very hard to translate to an Unix error code :/ I'll double-check with the PoP.
On 31.08.2017 08:38, Cornelia Huck wrote: > On Thu, 31 Aug 2017 07:51:17 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 30.08.2017 18:36, Halil Pasic wrote: >>> The function ioinst_handle_xsch is presenting cc 2 when it's supposed to >>> present cc 1 and the other way around, because css_do_xsch has the error >>> codes mixed up. Fixing the error codes also fixes the priority. >>> >>> Let us fix this. >> >> (Nit: In case you respin, I'd suggest to remove the last sentence. You >> already used "fix" two times in the previous one) > > I can also remove it on applying, if Halil agrees (I have not yet > reviewed it, though). > >> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> Reported-by: Pierre Morel<pmorel@linux.vnet.ibm.com> >> >> Space missing -------------^ > > And I can also add that space on applying :) > >> >>> --- >>> hw/s390x/css.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>> index 1880b1a0ff..a50fb0727e 100644 >>> --- a/hw/s390x/css.c >>> +++ b/hw/s390x/css.c >>> @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch) >>> (!(s->ctrl & >>> (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) || >>> (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) { >>> - ret = -EINPROGRESS; >>> + ret = -EBUSY; >>> goto out; >>> } >>> >>> if (s->ctrl & SCSW_CTRL_MASK_STCTL) { >>> - ret = -EBUSY; >>> + ret = -EINPROGRESS; >>> goto out; >>> } >> >> Using both, EBUSY and EINPROGRESS as error codes sounds very confusing >> to me here ... what's the difference between busy and in-progress? So >> while you're at it, maybe you could replace the code for CC 2 ("CANCEL >> SUBCHANNEL not applicable") with a different error code? > > IIRC, I used these two as they matched my idea of what happens best > (there's a difference between "subchannel is busy" and "the I/O is > already in progress, too late to cancel"). "xsch not applicable" is > very hard to translate to an Unix error code :/ OK, the codes make more sense with your description ==> Maybe simply add a proper comment for each of the return codes? Thomas
On Thu, 31 Aug 2017 09:32:49 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 31.08.2017 08:38, Cornelia Huck wrote: > > On Thu, 31 Aug 2017 07:51:17 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> On 30.08.2017 18:36, Halil Pasic wrote: > >>> The function ioinst_handle_xsch is presenting cc 2 when it's supposed to > >>> present cc 1 and the other way around, because css_do_xsch has the error > >>> codes mixed up. Fixing the error codes also fixes the priority. > >>> > >>> Let us fix this. > >> > >> (Nit: In case you respin, I'd suggest to remove the last sentence. You > >> already used "fix" two times in the previous one) > > > > I can also remove it on applying, if Halil agrees (I have not yet > > reviewed it, though). > > > >> > >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >>> Reported-by: Pierre Morel<pmorel@linux.vnet.ibm.com> > >> > >> Space missing -------------^ > > > > And I can also add that space on applying :) > > > >> > >>> --- > >>> hw/s390x/css.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c > >>> index 1880b1a0ff..a50fb0727e 100644 > >>> --- a/hw/s390x/css.c > >>> +++ b/hw/s390x/css.c > >>> @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch) > >>> (!(s->ctrl & > >>> (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) || > >>> (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) { > >>> - ret = -EINPROGRESS; > >>> + ret = -EBUSY; > >>> goto out; > >>> } > >>> > >>> if (s->ctrl & SCSW_CTRL_MASK_STCTL) { > >>> - ret = -EBUSY; > >>> + ret = -EINPROGRESS; > >>> goto out; > >>> } > >> > >> Using both, EBUSY and EINPROGRESS as error codes sounds very confusing > >> to me here ... what's the difference between busy and in-progress? So > >> while you're at it, maybe you could replace the code for CC 2 ("CANCEL > >> SUBCHANNEL not applicable") with a different error code? > > > > IIRC, I used these two as they matched my idea of what happens best > > (there's a difference between "subchannel is busy" and "the I/O is > > already in progress, too late to cancel"). "xsch not applicable" is > > very hard to translate to an Unix error code :/ > > OK, the codes make more sense with your description ==> Maybe simply add > a proper comment for each of the return codes? Taking a step back and looking at the other I/O instructions and their implementation in qemu: - For those instructions that can set it, cc 1 is set when the subchannel is status pending. That's usually the "default" branch in ioinst.c. - cc 2 is set when the subchannel is "busy" (or, in the case of xsch, "not applicable for xsch"... sigh) This is supposed to be handled via -EBUSY. So, there are actually two problems with the current implementation of xsch: - The return codes are switched around, which this patch fixes. - But "status pending" should also take precedence over "not applicable", if I read the PoP correctly, so the second if needs to be moved up. And yes, it makes sense do add some comments...
On 08/31/2017 07:51 AM, Thomas Huth wrote: > On 30.08.2017 18:36, Halil Pasic wrote: >> The function ioinst_handle_xsch is presenting cc 2 when it's supposed to >> present cc 1 and the other way around, because css_do_xsch has the error >> codes mixed up. Fixing the error codes also fixes the priority. >> >> Let us fix this. > > (Nit: In case you respin, I'd suggest to remove the last sentence. You > already used "fix" two times in the previous one) > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> Reported-by: Pierre Morel<pmorel@linux.vnet.ibm.com> > > Space missing -------------^ > copy-paste :( >> --- >> hw/s390x/css.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >> index 1880b1a0ff..a50fb0727e 100644 >> --- a/hw/s390x/css.c >> +++ b/hw/s390x/css.c >> @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch) >> (!(s->ctrl & >> (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) || >> (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) { >> - ret = -EINPROGRESS; >> + ret = -EBUSY; >> goto out; >> } >> >> if (s->ctrl & SCSW_CTRL_MASK_STCTL) { >> - ret = -EBUSY; >> + ret = -EINPROGRESS; >> goto out; >> } > > Using both, EBUSY and EINPROGRESS as error codes sounds very confusing > to me here ... what's the difference between busy and in-progress? So > while you're at it, maybe you could replace the code for CC 2 ("CANCEL > SUBCHANNEL not applicable") with a different error code? > > Thomas > Well, the idea of the series is to get rid of these artificial error codes, so your concern of using EBUSY and EINPROGRESS will be dealt with in patch 5. The idea was to first do the fixes and then do the transformation without changing behavior. Thanks for having a look! Regards, Halil
On 31.08.2017 11:09, Halil Pasic wrote: > > > On 08/31/2017 07:51 AM, Thomas Huth wrote: >> On 30.08.2017 18:36, Halil Pasic wrote: >>> The function ioinst_handle_xsch is presenting cc 2 when it's supposed to >>> present cc 1 and the other way around, because css_do_xsch has the error >>> codes mixed up. Fixing the error codes also fixes the priority. >>> >>> Let us fix this. >> >> (Nit: In case you respin, I'd suggest to remove the last sentence. You >> already used "fix" two times in the previous one) >> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> Reported-by: Pierre Morel<pmorel@linux.vnet.ibm.com> >> >> Space missing -------------^ >> > > copy-paste :( > >>> --- >>> hw/s390x/css.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>> index 1880b1a0ff..a50fb0727e 100644 >>> --- a/hw/s390x/css.c >>> +++ b/hw/s390x/css.c >>> @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch) >>> (!(s->ctrl & >>> (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) || >>> (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) { >>> - ret = -EINPROGRESS; >>> + ret = -EBUSY; >>> goto out; >>> } >>> >>> if (s->ctrl & SCSW_CTRL_MASK_STCTL) { >>> - ret = -EBUSY; >>> + ret = -EINPROGRESS; >>> goto out; >>> } >> >> Using both, EBUSY and EINPROGRESS as error codes sounds very confusing >> to me here ... what's the difference between busy and in-progress? So >> while you're at it, maybe you could replace the code for CC 2 ("CANCEL >> SUBCHANNEL not applicable") with a different error code? >> >> Thomas >> > > Well, the idea of the series is to get rid of these artificial error codes, > so your concern of using EBUSY and EINPROGRESS will be dealt with in patch > 5. > > The idea was to first do the fixes and then do the transformation without > changing behavior. Yeah, I realized that when I started to look at the later patches ... so please ignore my comment, it should be OK the way you're doing it. Thomas
On 08/31/2017 10:42 AM, Cornelia Huck wrote: > On Thu, 31 Aug 2017 09:32:49 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 31.08.2017 08:38, Cornelia Huck wrote: >>> On Thu, 31 Aug 2017 07:51:17 +0200 >>> Thomas Huth <thuth@redhat.com> wrote: >>> >>>> On 30.08.2017 18:36, Halil Pasic wrote: >>>>> The function ioinst_handle_xsch is presenting cc 2 when it's supposed to >>>>> present cc 1 and the other way around, because css_do_xsch has the error >>>>> codes mixed up. Fixing the error codes also fixes the priority. >>>>> >>>>> Let us fix this. >>>> >>>> (Nit: In case you respin, I'd suggest to remove the last sentence. You >>>> already used "fix" two times in the previous one) >>> >>> I can also remove it on applying, if Halil agrees (I have not yet >>> reviewed it, though). >>> >>>> >>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>>> Reported-by: Pierre Morel<pmorel@linux.vnet.ibm.com> >>>> >>>> Space missing -------------^ >>> >>> And I can also add that space on applying :) >>> >>>> >>>>> --- >>>>> hw/s390x/css.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c >>>>> index 1880b1a0ff..a50fb0727e 100644 >>>>> --- a/hw/s390x/css.c >>>>> +++ b/hw/s390x/css.c >>>>> @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch) >>>>> (!(s->ctrl & >>>>> (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) || >>>>> (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) { >>>>> - ret = -EINPROGRESS; >>>>> + ret = -EBUSY; >>>>> goto out; >>>>> } >>>>> >>>>> if (s->ctrl & SCSW_CTRL_MASK_STCTL) { >>>>> - ret = -EBUSY; >>>>> + ret = -EINPROGRESS; >>>>> goto out; >>>>> } >>>> >>>> Using both, EBUSY and EINPROGRESS as error codes sounds very confusing >>>> to me here ... what's the difference between busy and in-progress? So >>>> while you're at it, maybe you could replace the code for CC 2 ("CANCEL >>>> SUBCHANNEL not applicable") with a different error code? >>> >>> IIRC, I used these two as they matched my idea of what happens best >>> (there's a difference between "subchannel is busy" and "the I/O is >>> already in progress, too late to cancel"). "xsch not applicable" is >>> very hard to translate to an Unix error code :/ >> >> OK, the codes make more sense with your description ==> Maybe simply add >> a proper comment for each of the return codes? > > Taking a step back and looking at the other I/O instructions and their > implementation in qemu: > > - For those instructions that can set it, cc 1 is set when the > subchannel is status pending. That's usually the "default" branch in > ioinst.c. > - cc 2 is set when the subchannel is "busy" (or, in the case of xsch, > "not applicable for xsch"... sigh) This is supposed to be handled via > -EBUSY. > > So, there are actually two problems with the current implementation of > xsch: > > - The return codes are switched around, which this patch fixes. > - But "status pending" should also take precedence over "not > applicable", if I read the PoP correctly, so the second if needs to > be moved up. You are right and I was wrong. "Condition code 1 has precedence over condition code 2." So it's 3 > 1 > 2 (and I remembered 3 > 2 > 1). I will fix this for v2. > > And yes, it makes sense do add some comments... > If we apply the series as a whole adding comments would an overkill IMHO. We will switch this to iret.cc = ? so it should become obvious.
diff --git a/hw/s390x/css.c b/hw/s390x/css.c index 1880b1a0ff..a50fb0727e 100644 --- a/hw/s390x/css.c +++ b/hw/s390x/css.c @@ -1281,12 +1281,12 @@ int css_do_xsch(SubchDev *sch) (!(s->ctrl & (SCSW_ACTL_RESUME_PEND | SCSW_ACTL_START_PEND | SCSW_ACTL_SUSP))) || (s->ctrl & SCSW_ACTL_SUBCH_ACTIVE)) { - ret = -EINPROGRESS; + ret = -EBUSY; goto out; } if (s->ctrl & SCSW_CTRL_MASK_STCTL) { - ret = -EBUSY; + ret = -EINPROGRESS; goto out; }
The function ioinst_handle_xsch is presenting cc 2 when it's supposed to present cc 1 and the other way around, because css_do_xsch has the error codes mixed up. Fixing the error codes also fixes the priority. Let us fix this. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> Reported-by: Pierre Morel<pmorel@linux.vnet.ibm.com> --- hw/s390x/css.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)