From: Viresh Kumar <viresh.kumar@linaro.org> To: "Enrico Weigelt, metux IT consult" <lkml@metux.net> Cc: "Bartosz Golaszewski" <bgolaszewski@baylibre.com>, "Linus Walleij" <linus.walleij@linaro.org>, "Enrico Weigelt, metux IT consult" <info@metux.net>, "Viresh Kumar" <vireshk@kernel.org>, "Michael S. Tsirkin" <mst@redhat.com>, "Jason Wang" <jasowang@redhat.com>, "Vincent Guittot" <vincent.guittot@linaro.org>, "Bill Mills" <bill.mills@linaro.org>, "Alex Bennée" <alex.bennee@linaro.org>, "Stratos Mailing List" <stratos-dev@op-lists.linaro.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, "Stefan Hajnoczi" <stefanha@redhat.com>, "Stefano Garzarella --cc virtualization @ lists . linux-foundation . org" <sgarzare@redhat.com>, virtualization@lists.linux-foundation.org Subject: Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver Date: Thu, 10 Jun 2021 22:27:05 +0530 [thread overview] Message-ID: <CAKohpokxSoSVtAJkL-T_OOoS8dW-gYG1Gs5=_DwebvJETE48Xw@mail.gmail.com> (raw) In-Reply-To: <96994f4c-8755-90a8-0c50-4e21c436f137@metux.net> Hi Enrico, On Thu, 10 Jun 2021 at 21:24, Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > On 10.06.21 14:16, Viresh Kumar wrote: > > From: "Enrico Weigelt, metux IT consult" <info@metux.net> > > > > This patch adds a new driver for Virtio based GPIO devices. > > > > This allows a guest VM running Linux to access GPIO device provided by > > the host. It supports all basic operations for now, except interrupts > > for the GPIO lines. > > What exactly did you change here from my original driver ? I didn't write it from scratch and used your patch only to start with, and so you are still the Author of this particular patch. This just followed the specification updates and so changes only the stuff that was different from your original specs. Apart from that as you know, the irqs weren't working in your case as they needed to be implemented differently (patch 2 does that) here. > Your already changed the spec havily (w/o consulting me first), Isn't that what we are doing on the list? I believe that's why the lists exist, so people don't need to discuss in person, offline. I am happy to make changes in spec if you want to suggest something and if something breaks it for you. > so I guess this driver hasn't so much in common w/ my original design. It actually has as I said earlier, it is still based on your patch. > Note that I made my original design decisions for good reaons > (see virtio-dev list). I tried to follow your patches from December on the Linux kernel list and the ID allocation one on virtio-comments list. I wasn't able to search for any other attempt at sending the specification, so may have missed something. I do understand that there were reasons why you (and me) chose a particular way of doing things and if there is a good reason of following that, then we can still modify the spec and fix things for everyone. We just need to discuss our reasoning on the list and get through with a specfication which works for everyone as this will become a standard interface for many in the future and it needs to be robust and efficient for everyone. > It already support async notifications > (IOW: irqs), had an easy upgrade path for future additions, etc. I haven't changed irqs path, we still have a separate virtqueue (named "interrupt" vq) which handles just the interrupts now. And so will be faster than what you initially suggested. In your original design you also received the responses for the requests on this virtqueue, wihch I have changed to get the response synchronously on the "command" virtqueue only. This is from one of your earlier replies: " I've been under the impression that queues only work in only one direction. (at least that's what my web research was telling). Could you please give an example how bi-directional transmission within the same queue could look like ? " It is actually possible and the right thing to do here as we aren't starting multiple requests together. The operation needs to be synchronous anyway and both request/response on the same channel work better there. Also that makes the interrupts reach faster, without additional delay due to responses. > Note #2: it's already implemented and running in the field (different > kernels, different hypervisors, ...) - it just lacked the going through > virtio comitte's formal specification process, which blocked mainlining. I understand the pain you would be reqiured to go through because of this, but this is how any open source community will work, like Linux. There will be changes in specification and code once you post it and any solutions already working in the field won't really matter during the discussion. That is why it is always the right thing to _upstream first_, so one can avoid these problems later on. Though I understand that the real world needs to move faster than community. But no one can really help in that. > Is there anything fundamentally wrong w/ my original design, why you > invented a completely new one ? Not much, and I haven't changed a lot as well. Lemme point out few things which have changed in specification since your earlier version (the code just followed the specification, that's it). - config structure - version information is replaced with virtio-features. - Irq support is added as a feature, as you suggested. - extra padding space (24 bytes) removed. If you see this patch we can now read the entire config structure in a single go. Why should anyone be required to copy extra 24 bytes? If we need more fields later, we can always do that with help of another feature-flag. So this is still extendable. - Virtqueues: we still have two virtqueues (command and interrupt), just responses are moved to the command virtqueue only, as that is more efficient. - Request/response: Request layout is same, added a new layout for response as the same layout is inefficient. - IRQ support: Initial specification had no interface for configuring irqs from the guest, I added that. That's all I can gather right now. I don't think that's a lot and it is mostly improvements only. But if there is a good reason on why we should do things differently, we can still discuss that. I am open to suggestions. Thanks -- Viresh
WARNING: multiple messages have this Message-ID (diff)
From: Viresh Kumar <viresh.kumar@linaro.org> To: "Enrico Weigelt, metux IT consult" <lkml@metux.net> Cc: Vincent Guittot <vincent.guittot@linaro.org>, Stefan Hajnoczi <stefanha@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Viresh Kumar <vireshk@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, virtualization@lists.linux-foundation.org, Bartosz Golaszewski <bgolaszewski@baylibre.com>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, Stratos Mailing List <stratos-dev@op-lists.linaro.org>, "Enrico Weigelt, metux IT consult" <info@metux.net>, Bill Mills <bill.mills@linaro.org> Subject: Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver Date: Thu, 10 Jun 2021 22:27:05 +0530 [thread overview] Message-ID: <CAKohpokxSoSVtAJkL-T_OOoS8dW-gYG1Gs5=_DwebvJETE48Xw@mail.gmail.com> (raw) In-Reply-To: <96994f4c-8755-90a8-0c50-4e21c436f137@metux.net> Hi Enrico, On Thu, 10 Jun 2021 at 21:24, Enrico Weigelt, metux IT consult <lkml@metux.net> wrote: > On 10.06.21 14:16, Viresh Kumar wrote: > > From: "Enrico Weigelt, metux IT consult" <info@metux.net> > > > > This patch adds a new driver for Virtio based GPIO devices. > > > > This allows a guest VM running Linux to access GPIO device provided by > > the host. It supports all basic operations for now, except interrupts > > for the GPIO lines. > > What exactly did you change here from my original driver ? I didn't write it from scratch and used your patch only to start with, and so you are still the Author of this particular patch. This just followed the specification updates and so changes only the stuff that was different from your original specs. Apart from that as you know, the irqs weren't working in your case as they needed to be implemented differently (patch 2 does that) here. > Your already changed the spec havily (w/o consulting me first), Isn't that what we are doing on the list? I believe that's why the lists exist, so people don't need to discuss in person, offline. I am happy to make changes in spec if you want to suggest something and if something breaks it for you. > so I guess this driver hasn't so much in common w/ my original design. It actually has as I said earlier, it is still based on your patch. > Note that I made my original design decisions for good reaons > (see virtio-dev list). I tried to follow your patches from December on the Linux kernel list and the ID allocation one on virtio-comments list. I wasn't able to search for any other attempt at sending the specification, so may have missed something. I do understand that there were reasons why you (and me) chose a particular way of doing things and if there is a good reason of following that, then we can still modify the spec and fix things for everyone. We just need to discuss our reasoning on the list and get through with a specfication which works for everyone as this will become a standard interface for many in the future and it needs to be robust and efficient for everyone. > It already support async notifications > (IOW: irqs), had an easy upgrade path for future additions, etc. I haven't changed irqs path, we still have a separate virtqueue (named "interrupt" vq) which handles just the interrupts now. And so will be faster than what you initially suggested. In your original design you also received the responses for the requests on this virtqueue, wihch I have changed to get the response synchronously on the "command" virtqueue only. This is from one of your earlier replies: " I've been under the impression that queues only work in only one direction. (at least that's what my web research was telling). Could you please give an example how bi-directional transmission within the same queue could look like ? " It is actually possible and the right thing to do here as we aren't starting multiple requests together. The operation needs to be synchronous anyway and both request/response on the same channel work better there. Also that makes the interrupts reach faster, without additional delay due to responses. > Note #2: it's already implemented and running in the field (different > kernels, different hypervisors, ...) - it just lacked the going through > virtio comitte's formal specification process, which blocked mainlining. I understand the pain you would be reqiured to go through because of this, but this is how any open source community will work, like Linux. There will be changes in specification and code once you post it and any solutions already working in the field won't really matter during the discussion. That is why it is always the right thing to _upstream first_, so one can avoid these problems later on. Though I understand that the real world needs to move faster than community. But no one can really help in that. > Is there anything fundamentally wrong w/ my original design, why you > invented a completely new one ? Not much, and I haven't changed a lot as well. Lemme point out few things which have changed in specification since your earlier version (the code just followed the specification, that's it). - config structure - version information is replaced with virtio-features. - Irq support is added as a feature, as you suggested. - extra padding space (24 bytes) removed. If you see this patch we can now read the entire config structure in a single go. Why should anyone be required to copy extra 24 bytes? If we need more fields later, we can always do that with help of another feature-flag. So this is still extendable. - Virtqueues: we still have two virtqueues (command and interrupt), just responses are moved to the command virtqueue only, as that is more efficient. - Request/response: Request layout is same, added a new layout for response as the same layout is inefficient. - IRQ support: Initial specification had no interface for configuring irqs from the guest, I added that. That's all I can gather right now. I don't think that's a lot and it is mostly improvements only. But if there is a good reason on why we should do things differently, we can still discuss that. I am open to suggestions. Thanks -- Viresh _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-06-10 16:58 UTC|newest] Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-10 12:09 [PATCH V3 0/3] gpio: Add virtio based driver Viresh Kumar 2021-06-10 12:09 ` Viresh Kumar 2021-06-10 12:16 ` [PATCH V3 1/3] gpio: Add virtio-gpio driver Viresh Kumar 2021-06-10 12:16 ` Viresh Kumar 2021-06-10 13:22 ` Arnd Bergmann 2021-06-10 16:00 ` Enrico Weigelt, metux IT consult [not found] ` <01000179f6a7715c-cd106846-7770-4088-bb7c-a696bfcbf83e-000000@email.amazonses.com> 2021-06-10 17:03 ` [Stratos-dev] " Jean-Philippe Brucker 2021-06-10 17:03 ` Jean-Philippe Brucker 2021-06-10 19:41 ` Arnd Bergmann 2021-06-14 10:21 ` Viresh Kumar 2021-06-14 10:21 ` Viresh Kumar 2021-06-14 12:31 ` Arnd Bergmann 2021-06-14 12:49 ` Vincent Guittot [not found] ` <0100017a0a9264cc-57668c56-fdbf-412a-9f82-9bf95f5c653e-000000@email.amazonses.com> 2021-06-14 12:58 ` [Stratos-dev] " Arnd Bergmann 2021-06-14 13:24 ` Vincent Guittot 2021-06-14 20:54 ` Arnd Bergmann 2021-06-15 7:30 ` Vincent Guittot 2021-06-10 15:54 ` Enrico Weigelt, metux IT consult 2021-06-10 16:57 ` Viresh Kumar [this message] 2021-06-10 16:57 ` Viresh Kumar 2021-06-10 20:46 ` Linus Walleij 2021-06-10 20:46 ` Linus Walleij 2021-06-11 3:56 ` Viresh Kumar 2021-06-11 3:56 ` Viresh Kumar 2021-06-11 7:42 ` Geert Uytterhoeven 2021-06-11 7:42 ` Geert Uytterhoeven 2021-06-11 8:01 ` Viresh Kumar 2021-06-11 8:01 ` Viresh Kumar 2021-06-11 8:22 ` Geert Uytterhoeven 2021-06-11 8:22 ` Geert Uytterhoeven 2021-06-15 11:15 ` Viresh Kumar 2021-06-15 11:15 ` Viresh Kumar 2021-06-15 11:37 ` Geert Uytterhoeven 2021-06-15 11:37 ` Geert Uytterhoeven 2021-06-15 20:03 ` Linus Walleij 2021-06-15 20:03 ` Linus Walleij 2021-06-16 1:45 ` Viresh Kumar 2021-06-16 1:45 ` Viresh Kumar 2021-06-14 8:07 ` Enrico Weigelt, metux IT consult 2021-06-14 8:12 ` Andy Shevchenko 2021-06-14 8:12 ` Andy Shevchenko 2021-06-14 9:14 ` Viresh Kumar 2021-06-14 9:14 ` Viresh Kumar 2021-06-14 9:17 ` Enrico Weigelt, metux IT consult 2021-06-14 9:52 ` Viresh Kumar 2021-06-14 9:52 ` Viresh Kumar 2021-06-14 9:12 ` Viresh Kumar 2021-06-14 9:12 ` Viresh Kumar 2021-06-14 9:29 ` Enrico Weigelt, metux IT consult 2021-06-14 8:03 ` Enrico Weigelt, metux IT consult 2021-06-14 9:24 ` Viresh Kumar 2021-06-14 9:24 ` Viresh Kumar 2021-06-16 3:30 ` Bjorn Andersson 2021-06-16 3:30 ` Bjorn Andersson 2021-06-16 15:52 ` Enrico Weigelt, metux IT consult 2021-06-18 9:13 ` Linus Walleij 2021-06-18 9:13 ` Linus Walleij 2021-06-21 17:25 ` Bjorn Andersson 2021-06-21 17:25 ` Bjorn Andersson 2021-06-10 12:16 ` [PATCH V3 2/3] gpio: virtio: Add IRQ support Viresh Kumar 2021-06-10 12:16 ` Viresh Kumar 2021-06-10 21:30 ` Linus Walleij 2021-06-10 21:30 ` Linus Walleij 2021-06-14 7:08 ` Viresh Kumar 2021-06-14 7:08 ` Viresh Kumar 2021-06-10 12:16 ` [PATCH V3 3/3] MAINTAINERS: Add entry for Virtio-gpio Viresh Kumar [not found] ` <01000179f5da7763-2ea817c6-e176-423a-952e-de02443f71e2-000000@email.amazonses.com> 2021-06-10 17:40 ` [PATCH V3 1/3] gpio: Add virtio-gpio driver Jean-Philippe Brucker 2021-06-10 17:40 ` Jean-Philippe Brucker 2021-06-11 3:39 ` Viresh Kumar 2021-06-11 3:39 ` Viresh Kumar [not found] ` <01000179f9276678-ae2bb25f-4c0c-4176-b906-650c585b9753-000000@email.amazonses.com> 2021-06-11 8:34 ` [Stratos-dev] " Arnd Bergmann 2021-06-14 5:26 ` Viresh Kumar 2021-06-14 5:26 ` Viresh Kumar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAKohpokxSoSVtAJkL-T_OOoS8dW-gYG1Gs5=_DwebvJETE48Xw@mail.gmail.com' \ --to=viresh.kumar@linaro.org \ --cc=alex.bennee@linaro.org \ --cc=bgolaszewski@baylibre.com \ --cc=bill.mills@linaro.org \ --cc=info@metux.net \ --cc=jasowang@redhat.com \ --cc=linus.walleij@linaro.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lkml@metux.net \ --cc=mst@redhat.com \ --cc=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=stratos-dev@op-lists.linaro.org \ --cc=vincent.guittot@linaro.org \ --cc=vireshk@kernel.org \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.