| Message ID | 20210906104318.1569967-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/virtio: Minor housekeeping patches | expand |
On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Reported-by: Stefano Garzarella <sgarzare@redhat.com> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/hw/virtio/virtio.h | 7 +++++++ > hw/virtio/virtio.c | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 8bab9cfb750..c1c5f6e53c8 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); > > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > +/** > + * virtqueue_flush: > + * @vq: The #VirtQueue > + * @count: Number of elements to flush > + * > + * Must be called within RCU critical section. > + */ Hm... do these doc comments belong into .h files, or rather into .c files? > void virtqueue_flush(VirtQueue *vq, unsigned int count); > void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 3a1f6c520cb..97f60017466 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) > } > } > > +/* Called within rcu_read_lock(). */ > void virtqueue_flush(VirtQueue *vq, unsigned int count) > { > if (virtio_device_disabled(vq->vdev)) { The content of the change looks good to me, I'm only wondering about the style...
On 9/27/21 12:18, Cornelia Huck wrote: > On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> Reported-by: Stefano Garzarella <sgarzare@redhat.com> >> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/hw/virtio/virtio.h | 7 +++++++ >> hw/virtio/virtio.c | 1 + >> 2 files changed, 8 insertions(+) >> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index 8bab9cfb750..c1c5f6e53c8 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); >> >> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, >> unsigned int len); >> +/** >> + * virtqueue_flush: >> + * @vq: The #VirtQueue >> + * @count: Number of elements to flush >> + * >> + * Must be called within RCU critical section. >> + */ > > Hm... do these doc comments belong into .h files, or rather into .c files? Maybe we should restart this old thread, vote(?) and settle on a style? https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/ >> void virtqueue_flush(VirtQueue *vq, unsigned int count); >> void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem, >> unsigned int len); >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 3a1f6c520cb..97f60017466 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) >> } >> } >> >> +/* Called within rcu_read_lock(). */ >> void virtqueue_flush(VirtQueue *vq, unsigned int count) >> { >> if (virtio_device_disabled(vq->vdev)) { > > The content of the change looks good to me, I'm only wondering about > the style... >
On Mon, Sep 27 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 9/27/21 12:18, Cornelia Huck wrote: >> On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >>> Reported-by: Stefano Garzarella <sgarzare@redhat.com> >>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> include/hw/virtio/virtio.h | 7 +++++++ >>> hw/virtio/virtio.c | 1 + >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >>> index 8bab9cfb750..c1c5f6e53c8 100644 >>> --- a/include/hw/virtio/virtio.h >>> +++ b/include/hw/virtio/virtio.h >>> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); >>> >>> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, >>> unsigned int len); >>> +/** >>> + * virtqueue_flush: >>> + * @vq: The #VirtQueue >>> + * @count: Number of elements to flush >>> + * >>> + * Must be called within RCU critical section. >>> + */ >> >> Hm... do these doc comments belong into .h files, or rather into .c files? > > Maybe we should restart this old thread, vote(?) and settle on a style? > > https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/ My vote would still go to putting this into .c files :) Not sure if we want to go through the hassle of a wholesale cleanup; but if others agree, we could at least start with putting new doc comments next to the implementation. Do we actually consume these comments in an automated way somewhere?
On Mon, Sep 27, 2021 at 01:29:46PM +0200, Cornelia Huck wrote: > On Mon, Sep 27 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > On 9/27/21 12:18, Cornelia Huck wrote: > >> On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> > >>> Reported-by: Stefano Garzarella <sgarzare@redhat.com> > >>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>> --- > >>> include/hw/virtio/virtio.h | 7 +++++++ > >>> hw/virtio/virtio.c | 1 + > >>> 2 files changed, 8 insertions(+) > >>> > >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > >>> index 8bab9cfb750..c1c5f6e53c8 100644 > >>> --- a/include/hw/virtio/virtio.h > >>> +++ b/include/hw/virtio/virtio.h > >>> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); > >>> > >>> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > >>> unsigned int len); > >>> +/** > >>> + * virtqueue_flush: > >>> + * @vq: The #VirtQueue > >>> + * @count: Number of elements to flush > >>> + * > >>> + * Must be called within RCU critical section. > >>> + */ > >> > >> Hm... do these doc comments belong into .h files, or rather into .c files? > > > > Maybe we should restart this old thread, vote(?) and settle on a style? > > > > https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/ > > My vote would still go to putting this into .c files :) Not sure if we > want to go through the hassle of a wholesale cleanup; but if others > agree, we could at least start with putting new doc comments next to the > implementation. In the virtio.c/h case doc comments (and especially the RCU-related ones) are in the .c file. The exception is that constants and structs are documented in the header file. I would follow that and avoid duplicating doc comments into the .h file. Stefan
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 8bab9cfb750..c1c5f6e53c8 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len); +/** + * virtqueue_flush: + * @vq: The #VirtQueue + * @count: Number of elements to flush + * + * Must be called within RCU critical section. + */ void virtqueue_flush(VirtQueue *vq, unsigned int count); void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len); diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 3a1f6c520cb..97f60017466 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) } } +/* Called within rcu_read_lock(). */ void virtqueue_flush(VirtQueue *vq, unsigned int count) { if (virtio_device_disabled(vq->vdev)) {
Reported-by: Stefano Garzarella <sgarzare@redhat.com> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/hw/virtio/virtio.h | 7 +++++++ hw/virtio/virtio.c | 1 + 2 files changed, 8 insertions(+)