* [PATCH] virtio: Fix various coding style issues
@ 2024-07-19 5:46 Juan José Arboleda
2024-07-19 17:48 ` Eugenio Perez Martin
0 siblings, 1 reply; 8+ messages in thread
From: Juan José Arboleda @ 2024-07-19 5:46 UTC (permalink / raw)
To: mst, jasowang, xuanzhuo, eperezma; +Cc: virtualization, trivial
Address various coding style warnings in the virtio_mmio driver:
- Move trailing `*/` to a new line in block comments.
- Use parentheses with sizeof for consistency.
- 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);
static int vm_unregister_cmdline_device(struct device *dev,
void *data)
--
2.45.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio: Fix various coding style issues
2024-07-19 5:46 [PATCH] virtio: Fix various coding style issues Juan José Arboleda
@ 2024-07-19 17:48 ` Eugenio Perez Martin
2024-07-19 18:11 ` Michael S. Tsirkin
2024-07-19 22:17 ` Juan José Arboleda
0 siblings, 2 replies; 8+ messages in thread
From: Eugenio Perez Martin @ 2024-07-19 17:48 UTC (permalink / raw)
To: Juan José Arboleda; +Cc: mst, jasowang, xuanzhuo, virtualization, trivial
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:
>
> - Move trailing `*/` to a new line in block comments.
> - Use parentheses with sizeof for consistency.
> - 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio: Fix various coding style issues
2024-07-19 17:48 ` Eugenio Perez Martin
@ 2024-07-19 18:11 ` Michael S. Tsirkin
2024-07-19 22:28 ` Juan José Arboleda
2024-07-19 22:17 ` Juan José Arboleda
1 sibling, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2024-07-19 18:11 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: Juan José Arboleda, jasowang, xuanzhuo, virtualization,
trivial
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
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio: Fix various coding style issues
2024-07-19 17:48 ` Eugenio Perez Martin
2024-07-19 18:11 ` Michael S. Tsirkin
@ 2024-07-19 22:17 ` Juan José Arboleda
1 sibling, 0 replies; 8+ messages in thread
From: Juan José Arboleda @ 2024-07-19 22:17 UTC (permalink / raw)
To: Eugenio Perez Martin; +Cc: virtualization
> > -device_param_cb(device, &vm_cmdline_param_ops, NULL, S_IRUSR);
> > +device_param_cb(device, &vm_cmdline_param_ops, NULL, 0400);
>
> What is this warning? It sounds more reasonable to me to go from
> hardcoded value to macro, am I missing something?
Hey! I believe that the hardcoded value seems better as well. I got the
warning from this:
./scripts/checkpatch.pl --file --terse drivers/virtio/virtio_mmio.c
...
drivers/virtio/virtio_mmio.c:802: WARNING: Symbolic permissions 'S_IRUSR' are
not preferred. Consider using octal permissions '0400'.
...
I can bring the 'S_IRUSR' again in a V2 patch if you prefer that.
>
> The rest looks good to me.
>
> Thanks!
>
Thanks for taking the time to review!
Regards,
-Juan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio: Fix various coding style issues
2024-07-19 18:11 ` Michael S. Tsirkin
@ 2024-07-19 22:28 ` Juan José Arboleda
2024-07-24 14:01 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Juan José Arboleda @ 2024-07-19 22:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
> warnings from which tool?
Hi! Thanks for taking the time to review this!
From the 'checkpatch.pl' script
Regards,
-Juan José
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio: Fix various coding style issues
2024-07-19 22:28 ` Juan José Arboleda
@ 2024-07-24 14:01 ` Michael S. Tsirkin
2024-07-24 17:22 ` Juan José Arboleda
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2024-07-24 14:01 UTC (permalink / raw)
To: Juan José Arboleda; +Cc: virtualization
On Fri, Jul 19, 2024 at 05:28:41PM -0500, Juan José Arboleda wrote:
> > warnings from which tool?
>
> Hi! Thanks for taking the time to review this!
>
> >From the 'checkpatch.pl' script
>
> Regards,
> -Juan José
As a rule, we don't just tweak coding style by running checkpatch
on existing code. Makes backports harder for little gain.
Code gets tweaked naturally as someone works on it.
--
MST
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio: Fix various coding style issues
2024-07-24 14:01 ` Michael S. Tsirkin
@ 2024-07-24 17:22 ` Juan José Arboleda
2024-07-25 6:02 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Juan José Arboleda @ 2024-07-24 17:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
> Makes backports harder for little gain.
> Code gets tweaked naturally as someone works on it.
I understand your concern about backports. However, this patch only updates
the coding style to match kernel-wide standards without altering behavior.
Consistent style improves readability and maintainability without impacting
functionality, thus not complicating backports.
Thank you for considering this.
Best,
- Juan José
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtio: Fix various coding style issues
2024-07-24 17:22 ` Juan José Arboleda
@ 2024-07-25 6:02 ` Michael S. Tsirkin
0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2024-07-25 6:02 UTC (permalink / raw)
To: Juan José Arboleda; +Cc: virtualization
On Wed, Jul 24, 2024 at 12:22:07PM -0500, Juan José Arboleda wrote:
> > Makes backports harder for little gain.
> > Code gets tweaked naturally as someone works on it.
>
> I understand your concern about backports. However, this patch only updates
> the coding style to match kernel-wide standards without altering behavior.
> Consistent style improves readability and maintainability without impacting
> functionality, thus not complicating backports.
>
> Thank you for considering this.
>
> Best,
> - Juan José
checkpatch keeps adding warnings, they are there to help patch
submitters especially new ones. It's not a holy script and there is
little interest in rewriting all of kernel each time this happens.
As contributors start being active in a specific area,
they will tend to also adjust the style to make it easier to
work with, and this is fine.
Thanks,
--
MST
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-07-25 6:02 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-19 5:46 [PATCH] virtio: Fix various coding style issues Juan José Arboleda
2024-07-19 17:48 ` Eugenio Perez Martin
2024-07-19 18:11 ` Michael S. Tsirkin
2024-07-19 22:28 ` Juan José Arboleda
2024-07-24 14:01 ` Michael S. Tsirkin
2024-07-24 17:22 ` Juan José Arboleda
2024-07-25 6:02 ` Michael S. Tsirkin
2024-07-19 22:17 ` Juan José Arboleda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).