Re: [PATCH] virtio: Fix various coding style issues

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jul 19, 2024 at 07:48:29PM +0200, Eugenio Perez Martin wrote:
> On Fri, Jul 19, 2024 at 7:49 AM Juan José Arboleda
> <soyjuanarbol@gmail.com> wrote:
> >
> > Address various coding style warnings in the virtio_mmio driver:

warnings from which tool?

> >
> > - Move trailing `*/` to a new line in block comments.

don't unless you fix the comment completely.

> > - Use parentheses with sizeof for consistency.

for consistency with what? don't see the point.

> > - Add missing blank line after declarations.
> > - Replace 'S_IRUSR' with '0400' for permissions.
> >
> > Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>


> > ---
> >  drivers/virtio/virtio_mmio.c | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> > index 173596589c71..7e6e41b2fb5a 100644
> > --- a/drivers/virtio/virtio_mmio.c
> > +++ b/drivers/virtio/virtio_mmio.c
> > @@ -74,7 +74,8 @@
> >
> >
> >  /* The alignment to use between consumer and producer parts of vring.
> > - * Currently hardcoded to the page size. */
> > + * Currently hardcoded to the page size.
> > + */
> >  #define VIRTIO_MMIO_VRING_ALIGN                PAGE_SIZE
> >
> >
> > @@ -167,21 +168,21 @@ static void vm_get(struct virtio_device *vdev, unsigned int offset,
> >         switch (len) {
> >         case 1:
> >                 b = readb(base + offset);
> > -               memcpy(buf, &b, sizeof b);
> > +               memcpy(buf, &b, sizeof(b));
> >                 break;
> >         case 2:
> >                 w = cpu_to_le16(readw(base + offset));
> > -               memcpy(buf, &w, sizeof w);
> > +               memcpy(buf, &w, sizeof(w));
> >                 break;
> >         case 4:
> >                 l = cpu_to_le32(readl(base + offset));
> > -               memcpy(buf, &l, sizeof l);
> > +               memcpy(buf, &l, sizeof(l));
> >                 break;
> >         case 8:
> >                 l = cpu_to_le32(readl(base + offset));
> > -               memcpy(buf, &l, sizeof l);
> > -               l = cpu_to_le32(ioread32(base + offset + sizeof l));
> > -               memcpy(buf + sizeof l, &l, sizeof l);
> > +               memcpy(buf, &l, sizeof(l));
> > +               l = cpu_to_le32(ioread32(base + offset + sizeof(l)));
> > +               memcpy(buf + sizeof(l), &l, sizeof(l));
> >                 break;
> >         default:
> >                 BUG();
> > @@ -209,22 +210,22 @@ static void vm_set(struct virtio_device *vdev, unsigned int offset,
> >
> >         switch (len) {
> >         case 1:
> > -               memcpy(&b, buf, sizeof b);
> > +               memcpy(&b, buf, sizeof(b));
> >                 writeb(b, base + offset);
> >                 break;
> >         case 2:
> > -               memcpy(&w, buf, sizeof w);
> > +               memcpy(&w, buf, sizeof(w));
> >                 writew(le16_to_cpu(w), base + offset);
> >                 break;
> >         case 4:
> > -               memcpy(&l, buf, sizeof l);
> > +               memcpy(&l, buf, sizeof(l));
> >                 writel(le32_to_cpu(l), base + offset);
> >                 break;
> >         case 8:
> > -               memcpy(&l, buf, sizeof l);
> > +               memcpy(&l, buf, sizeof(l));
> >                 writel(le32_to_cpu(l), base + offset);
> > -               memcpy(&l, buf + sizeof l, sizeof l);
> > -               writel(le32_to_cpu(l), base + offset + sizeof l);
> > +               memcpy(&l, buf + sizeof(l), sizeof(l));
> > +               writel(le32_to_cpu(l), base + offset + sizeof(l));
> >                 break;
> >         default:
> >                 BUG();
> > @@ -281,7 +282,8 @@ static bool vm_notify(struct virtqueue *vq)
> >         struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vq->vdev);
> >
> >         /* We write the queue's selector into the notification register to
> > -        * signal the other end */
> > +        * signal the other end
> > +        */
> >         writel(vq->index, vm_dev->base + VIRTIO_MMIO_QUEUE_NOTIFY);
> >         return true;
> >  }
> > @@ -699,6 +701,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> >  static void virtio_mmio_remove(struct platform_device *pdev)
> >  {
> >         struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > +
> >         unregister_virtio_device(&vm_dev->vdev);
> >  }
> >
> > @@ -799,7 +802,7 @@ static const struct kernel_param_ops vm_cmdline_param_ops = {
> >         .get = vm_cmdline_get,
> >  };
> >
> > -device_param_cb(device, &vm_cmdline_param_ops, NULL, S_IRUSR);
> > +device_param_cb(device, &vm_cmdline_param_ops, NULL, 0400);
> 
> Hi Juan José,
> 
> What is this warning? It sounds more reasonable to me to go from
> hardcoded value to macro, am I missing something?
> 
> The rest looks good to me.
> 
> Thanks!
> 
> >
> >  static int vm_unregister_cmdline_device(struct device *dev,
> >                 void *data)
> > --
> > 2.45.2
> >