From: Viresh Kumar <viresh.kumar@linaro.org> To: Jean-Philippe Brucker <jean-philippe@linaro.org> 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>, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-gpio@vger.kernel.org, Stefan Hajnoczi <stefanha@redhat.com>, "Stefano Garzarella --cc virtualization @ lists . linux-foundation . org" <sgarzare@redhat.com>, stratos-dev@op-lists.linaro.org Subject: Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver Date: Fri, 11 Jun 2021 09:09:29 +0530 [thread overview] Message-ID: <20210611033929.ifnafw2gznjklnq2@vireshk-i7> (raw) In-Reply-To: <YMJOk6RWuztRNBXO@myrica> On 10-06-21, 19:40, Jean-Philippe Brucker wrote: > On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote: Fixed everything else you suggested. > > +struct virtio_gpio_config { > > + char name[32]; > > + __u16 ngpio; > > + __u16 padding; > > + __u32 gpio_names_size; > > + char gpio_names[0]; > > A variable-size array here will make it very difficult to append new > fields to virtio_gpio_config, when adding new features to the device. (New > fields cannot be inserted in between, since older drivers will expect > existing fields at a specific offset.) Yes, I thought about that earlier and though maybe we will be able to play with that using the virtio-features, I mean a different layout of config structure if we really need to add a field in config, based on the feature flag. > You could replace it with a reference to the string array, for example > "__u16 gpio_names_offset" declaring the offset between the beginning of > device-specific config and the string array. But, I like this idea more and it does make it very flexible. Will adapt to it. > The 'name' field could also be indirect to avoid setting a fixed > 32-char size, but that's not as important. Yeah, 32 bytes is really enough. One won't be able to make any sense out of a bigger name anyway :) > > +} __packed; > > No need for __packed, because the fields are naturally aligned (as > required by the virtio spec) Yeah, I know, but I tend to add that for structures which aren't very simple (like the request/response ones), just to avoid human errors and hours of debugging someone need to go through. __packed won't harm at least :) -- viresh
WARNING: multiple messages have this Message-ID (diff)
From: Viresh Kumar <viresh.kumar@linaro.org> To: Jean-Philippe Brucker <jean-philippe@linaro.org> Cc: Stefan Hajnoczi <stefanha@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Viresh Kumar <vireshk@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Bartosz Golaszewski <bgolaszewski@baylibre.com>, linux-gpio@vger.kernel.org, stratos-dev@op-lists.linaro.org, "Enrico Weigelt, metux IT consult" <info@metux.net> Subject: Re: [PATCH V3 1/3] gpio: Add virtio-gpio driver Date: Fri, 11 Jun 2021 09:09:29 +0530 [thread overview] Message-ID: <20210611033929.ifnafw2gznjklnq2@vireshk-i7> (raw) In-Reply-To: <YMJOk6RWuztRNBXO@myrica> On 10-06-21, 19:40, Jean-Philippe Brucker wrote: > On Thu, Jun 10, 2021 at 12:16:46PM +0000, Viresh Kumar via Stratos-dev wrote: Fixed everything else you suggested. > > +struct virtio_gpio_config { > > + char name[32]; > > + __u16 ngpio; > > + __u16 padding; > > + __u32 gpio_names_size; > > + char gpio_names[0]; > > A variable-size array here will make it very difficult to append new > fields to virtio_gpio_config, when adding new features to the device. (New > fields cannot be inserted in between, since older drivers will expect > existing fields at a specific offset.) Yes, I thought about that earlier and though maybe we will be able to play with that using the virtio-features, I mean a different layout of config structure if we really need to add a field in config, based on the feature flag. > You could replace it with a reference to the string array, for example > "__u16 gpio_names_offset" declaring the offset between the beginning of > device-specific config and the string array. But, I like this idea more and it does make it very flexible. Will adapt to it. > The 'name' field could also be indirect to avoid setting a fixed > 32-char size, but that's not as important. Yeah, 32 bytes is really enough. One won't be able to make any sense out of a bigger name anyway :) > > +} __packed; > > No need for __packed, because the fields are naturally aligned (as > required by the virtio spec) Yeah, I know, but I tend to add that for structures which aren't very simple (like the request/response ones), just to avoid human errors and hours of debugging someone need to go through. __packed won't harm at least :) -- viresh _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-06-11 3:39 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 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 [this message] 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=20210611033929.ifnafw2gznjklnq2@vireshk-i7 \ --to=viresh.kumar@linaro.org \ --cc=bgolaszewski@baylibre.com \ --cc=info@metux.net \ --cc=jasowang@redhat.com \ --cc=jean-philippe@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=stratos-dev@op-lists.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.