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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
> >
