On 20-10-21, 18:10, Andy Shevchenko wrote:> On Wednesday, October 20, 2021, Viresh Kumar <viresh.kumar at linaro.org> > wrote: > > +static int virtio_gpio_irq_set_type(struct irq_data *d, unsigned int > > type) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct virtio_gpio *vgpio = gpiochip_get_data(gc); > > + struct vgpio_irq_line *irq_line = &vgpio->irq_lines[d->hwirq]; > > + > > + switch (type) { > > + case IRQ_TYPE_NONE: > > + type = VIRTIO_GPIO_IRQ_TYPE_NONE; > > + break; > > > 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.> > +static void virtio_gpio_event_vq(struct virtqueue *vq) > > +{> > + irq = irq_find_mapping(vgpio->gc.irq.domain, gpio); > > + WARN_ON(!irq); > > + > > + ret = generic_handle_irq(irq); > > > IIRC there is a new API that basically combines the two above.generic_handle_domain_irq(), thanks.> > 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. -- viresh
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: > > On Wednesday, October 20, 2021, Viresh Kumar <viresh.kumar at linaro.org> > > wrote:...> > > + case IRQ_TYPE_NONE: > > > + type = VIRTIO_GPIO_IRQ_TYPE_NONE; > > > + break; > > > > 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? ...> > > 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? If there is a possibility to have 32-bit to 64-bit or vice versa communication you have a problem. -- With Best Regards, Andy Shevchenko