Re: [virtio-comment] [PATCH v3] virtio-net: fix Driver Notification description related to VIRTIO_F_NOTIF_CONFIG_DATA

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 3 Feb 2021 01:30:08 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 7 Jan 2021 17:39:48 +0000
> Vitaly Mireyno <vmireyno@marvell.com> wrote:
> 
> > Incorporated comments for the "[PATCH v9] virtio-net: Add support for the flexible driver notification structure".
> > Made Driver Notifications description more consistent throughout the document wrt VIRTIO_F_NOTIF_CONFIG_DATA.
> > 
> > Changes from v2:
> >  * Reworked 'vqn' name and definition
> >   
> 
> First sorry for being late.

This had somehow fallen through the cracks on my side as well.

> 
> > 
> > Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> > ---
> >  content.tex        | 30 ++++++++++++++----------------
> >  notifications-be.c |  2 +-
> >  notifications-le.c |  2 +-
> >  3 files changed, 16 insertions(+), 18 deletions(-)
> > 
> > diff --git a/content.tex b/content.tex
> > index 00bc050..19f09d9 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -337,8 +337,12 @@ \section{Driver Notifications} \label{sec:Virtqueues / Driver notifications}
> >  notification to the device.
> >  
> >  When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> > -this notification involves sending the
> > -virtqueue number to the device (method depending on the transport).
> > +this notification involves sending to the device the virtqueue token
> > +(method depending on the transport).  
> 
> I don't think changing the word order is a good idea. I would prefer
> 'this notification involves sending the virtqueue token to the device'

Agreed.

> 
> > +if VIRTIO_F_NOTIF_CONFIG_DATA has not been negotiated, the virtqueue token is  
> 
> This is a new sentence. You should start it with a capital letter.

Nod.

> 
> > +the virtqueue number to be notified  
> 
> To be notified is off here. I believe it can be omitted, or we need
> something like 'the virtqueue number of the virtqueue to be notified'

"the number of the virtqueue to be notified" ?

> 
> > +if VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated, the virtqueue token is  
> 
> This is probably supposed to be a new sentence as well, but you lack
> both the period and the capital initial letter.

Nod.

> 
> > +the queue notification data of the virtqueue to be notified.
> >