Re: [virtio-dev] [PATCH 1/6] notifications: unify notifications wording in core

[Halil Pasic <pasic@linux.ibm.com>]

On 05/14/2018 06:36 PM, Stefan Hajnoczi wrote:
> On Thu, Apr 26, 2018 at 12:59:57PM +0200, Halil Pasic wrote:
> > @@ -235,12 +236,13 @@ transmit and one for receive.}.
> >   Driver makes requests available to device by adding
> >   an available buffer to the queue - i.e. adding a buffer
> >   describing the request to a virtqueue, and optionally triggering
> > -a driver event - i.e. sending a notification to the device.
> > +a driver event - i.e. sending an available buffer notification
> > +to the device.
> >   
> >   Device executes the requests and - when complete - adds
> >   a used buffer to the queue - i.e. lets the driver
> >   know by marking the buffer as used. Device can then trigger
> > -a device event - i.e. send an interrupt to the driver.
> > +a device event - i.e. send an used buffer notification to the driver.
>
> I would say "a used buffer notification".  "A" vs "an" depends on the
> sound of the initial syllable, not the spelling.  So "a used car" vs "an
> upper body".
>
> There are several instances of this in this patch.

Right. Will try to hunt all of them down.

> >   
> >   Device reports the number of bytes it has written to memory for
> >   each buffer it uses. This is referred to as ``used length''.
> > @@ -330,7 +332,8 @@ set the FAILED status bit to indicate that it has given up on the
> >   device (it can reset the device later to restart if desired).  The
> >   driver MUST NOT continue initialization in that case.
> >   
> > -The driver MUST NOT notify the device before setting DRIVER_OK.
> > +The driver MUST NOT send any buffer available notifications to
> > +the device before setting DRIVER_OK.
> >   
> >   \subsection{Legacy Interface: Device Initialization}\label{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
> >   Legacy devices did not support the FEATURES_OK status bit, and thus did
> > @@ -388,10 +391,11 @@ reads unless notified.
> >   
> >   \subsection{Notification of Device Configuration Changes}\label{sec:General Initialization And Device Operation / Device Operation / Notification of Device Configuration Changes}
> >   
> > -For devices where the device-specific configuration information can be changed, an
> > -interrupt is delivered when a device-specific configuration change occurs.
> > +For devices where the device-specific configuration information can be
> > +changed, a notification is delivered when a device-specific configuration
> > +change occurs.
>
> Unlike used/available buffer notifications, the text here just says
> "notification" without explicitly saying "configuration change
> notification".  I think it makes the spec slightly clearer (and easier
> to search) to name the exact type of notification.

I decided to not spell it out for stylistic reasons. If I do we end
up with 3xconfiguration and 3xchange in a single sentence. But I'm also
a fan of searching and finding all instances. And I used to say for
specifications it's consistency over style.

I will change it according to your request.


> > @@ -309,22 +310,20 @@ in the ring.
> >   
> >   \subsection{Driver and Device Event Suppression}
> >   \label{sec:Packed Virtqueues / Driver and Device Event Suppression}
> > -In many systems driver and device notifications involve
> > +In many systems used and available buffer  notifications involve
>
> s/  / /
>
> > @@ -352,9 +351,9 @@ matches this value and a descriptor is
> >   made available/used respectively.
> >   \end{description}
> >   
> > -After writing out some descriptors, both the device and the driver
> > +After writing out some descriptors,the device/driver
>
> s/,/, /
>
> > @@ -562,8 +570,8 @@ The driver offers buffers to one of the device's virtqueues as follows:
> >   \item The driver performs a suitable memory barrier to ensure that it updates
> >     the \field{idx} field before checking for notification suppression.
> >   
> > -\item If notifications are not suppressed, the driver notifies the device
> > -    of the new available buffers.
> > +\item The driver sends an available buffers notification to the device if
>
> s/available buffers notification/available buffer notification/

Will apply all of these.

Thank you for your review!

Regards,
Halil