[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio] [PATCH 1/1] ccw: add CCW_CMD_READ_STATUS
On Mon, Oct 12, 2015 at 06:53:52PM +0200, Cornelia Huck wrote: > On Mon, 12 Oct 2015 15:34:18 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Oct 12, 2015 at 01:44:33PM +0200, Cornelia Huck wrote: > > > On Sun, 11 Oct 2015 18:41:52 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Fri, Sep 11, 2015 at 03:52:27PM +0200, Cornelia Huck wrote: > > > > > > \drivernormative{\paragraph}{Communicating Status Information}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Communicating Status Information} > > > > > > > > > > If the device posts a unit check with command reject in response to the > > > > > CCW_CMD_WRITE_STATUS command, the driver MUST assume that the device failed > > > > > to set the status and the \field{status} field retained its previous value. > > > > > > > > > > +If at least revision 2 has been negotiated, the driver MUST use the > > > > > > > > Why is this MUST here? > > > > > > The idea was that the device can rely on the driver having fetched the > > > new value and not needing to accommodate drivers that are not aware of > > > recent status changes. > > > > > > If we consider it the driver's responsibility to keep abreast of > > > changes (so that the device does not really need to care whether the > > > driver fetches all changes), we can change it to SHOULD. > > > > I this so - see below: > > > > > > > > > > > > > +CCW_CMD_READ_STATUS command to retrieve the \field{status} field after > > > > > +a configuration change interrupt has been retrieved. > > > > > > > > received? > > > > Maybe better "after a configuration change is detected". > > > > > > Hm... although I'm not sure how a driver may detect a configuration > > > change other than through an interrupt. > > > > It reads the indicator, isn't this right? > > Hm, yes, it might poll the configuration changed indicator. > > 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. > > > > > > 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. > > > I'll prepare a v2. > >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]