Re: [virtio] [PATCH 1/1] ccw: add CCW_CMD_READ_STATUS

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Mon, 12 Oct 2015 20:15:31 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Oct 12, 2015 at 06:53:52PM +0200, Cornelia Huck wrote:

> > Although I'm wondering: While 2.1.2 talks about "sending" a change
> > notification (which one might interpret as making the interrupt
> > pending), 3.2.3 explicitly talks about delivering/triggering an
> > interrupt. Should the wording in 3.2.3 be changed to match the wording
> > in 2.1.2, or is this really picking at words?
> 
> A bit, but sure, why not make it consistent.

We might walk over the whole document for that. I'd give that a low
priority, though.

> 
> > > 
> > > > > What if interrupts are disabled?
> > > > > 
> > > > > > It also SHOULD use
> > > > > > +the CCW_CMD_READ_STATUS command to verify the \field{status} field after it
> > > > > > +issued the CCW_CMD_WRITE_STATUS command.
> > > > > 
> > > > > Seems to conflict with above logic which says
> > > > > assume it retained previous value.
> > > > 
> > > > There are two parts to that:
> > > > - WRITE_STATUS failed -> status field is unchanged
> > > > - WRITE_STATUS did not fail -> status field has been modified; but use
> > > >   READ_STATUS to verify that the device did not modify it itself
> > > > 
> > > > I was thinking of the logic "write status with FEATURES_OK, then read
> > > > it to check whether the device kept FEATURES_OK". But it's probably ok
> > > > to keep that logic inside the generic sections; I'll remove this
> > > > sentence.
> > > 
> > > My question is generally this: what's wrong with
> > > keeping the current logic where  we require FEATURES_OK to fail
> > > if device doesn't set valid features?
> > > 
> > > The result is that CCW_CMD_READ_STATUS is merely there as an option: for
> > > fatal errors or for drivers that don't want to maintain the status
> > > internally.
> > 
> > We still have NEEDS_RESET - the driver needs to be able to retrieve
> > this when the configuration changed indicator is set.
> 
> Yes - that's what I meant when I said "fatal errors".
> My question was about the requirement to do CCW_CMD_READ_STATUS
> after each CCW_CMD_WRITE_STATUS.
> If driver wants to maintain status internally, why not let it.

Yeah, it's free to do so if I drop the sentence. We still need to
require the driver to retrieve status after a configuration change
notification to catch NEEDS_RESET.

> 
> > > > I'll prepare a v2.
> > > 
>