[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, 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? > > > > 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. > > I'll prepare a v2. >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]