On 21-10-21, 12:42, Andy Shevchenko wrote:> On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar <viresh.kumar at linaro.org> wrote: > > On 20-10-21, 18:10, Andy Shevchenko wrote: > > > IIRC you add dead code. IRQ framework never calls this if type is not set. > > > > Yes, but it is allowed to call > > > > irq_set_irq_type(irq, IRQ_TYPE_NONE); > > > > and the irq framework won't disallow it AFAICT. > > That's true, but how you may end up in this callback with a such? > What the meaning of that call to the user?I can see few calls like this in the kernel (mostly from irq-providers only), but yeah sure I can drop it. We will error out if it ever gets called and so can get it back later if required.> > > > 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. -- viresh
On Thu, Oct 21, 2021 at 12:52 PM Viresh Kumar <viresh.kumar at linaro.org> wrote:> On 21-10-21, 12:42, Andy Shevchenko wrote: > > On Thu, Oct 21, 2021 at 7:34 AM Viresh Kumar <viresh.kumar at linaro.org> 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
Hi Viresh, On Thu, Oct 21, 2021 at 11:52 AM Viresh Kumar <viresh.kumar at linaro.org> wrote:> 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.Structures are not aligned to the size of the largest element, but to the largest alignment needed for each member. This can be smaller than the size of the largest element. E.g. alignof(long long) might be 4, not 8. And m68k aligns to two bytes at most. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds