Re: [PATCH V6] gpio: virtio: Add IRQ support

[Andy Shevchenko]

On Thu, Oct 21, 2021 at 12:52 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> On 21-10-21, 12:42, Andy Shevchenko wrote:
> > On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > > On 20-10-21, 18:10, Andy Shevchenko wrote:

...

> > > > >  struct virtio_gpio_config {
> > > > >         __le16 ngpio;
> > > > >         __u8 padding[2];
> > > > > @@ -44,4 +56,17 @@ struct virtio_gpio_response_get_names {
> > > > >         __u8 value[];
> > > > >  };
> > > > >
> > > > > +/* Virtio GPIO IRQ Request / Response */
> > > > > +struct virtio_gpio_irq_request {
> > > > > +       __le16 gpio;
> > > > > +};
> > > > > +
> > > > > +struct virtio_gpio_irq_response {
> > > > > +       __u8 status;
> > > > > +};
> > > > >
> > > > I’m wondering if those above should be packed.
> > >
> > > You are talking about the newly added ones or the ones before ?
> > >
> > > In any case, they are all already packed (i.e. they have explicit
> > > padding wherever required) and properly aligned. Compiler won't add
> > > any other padding to them.
> >
> > Is it only for 64-bit to 64-bit communications?
>
> That's what I have been looking at.
>
> > If there is a possibility to have 32-bit to 64-bit or vice versa
> > communication you have a problem.
>
> This should work as well.
>
> The structure will get aligned to the size of largest element and each
> element will be aligned to itself. I don't see how this will break
> even in case of 32/64 bit communication.

I admit I haven't looked into the specification, but in the past we
had had quite an issue exactly in GPIO on kernel side because of this
kind of design mistake. The problem here if in the future one wants to
supply more than one item at a time, it will be not possible with this
interface. Yes, I understand that in current design it's rather missed
scalability, but hey, I believe in the future we may need
performance-wise calls.

-- 
With Best Regards,
Andy Shevchenko