Re: [PATCH 0/2] Xen: Use a dedicated pointer for IRQ data

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 21 Aug 2020, Thomas Gleixner wrote:
> On Fri, Aug 21 2020 at 14:17, Jürgen Groß wrote:
> > On 21.08.20 13:19, Sergei Temerkhanov wrote:
> >>> Did you see any specific problem where handler_data is written by
> >> another component?
> >> 
> >> I've posted this series in the thread
> >> https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg00957.html
> >> where the problem is caused exactly by that behavior
> >> 
> >>> In case this is a real problem I don't think your approach will be 
> >>> accepted
> >> Any comments/suggestions are welcome
> >
> > Not sure if the IRQ maintainers agree with me, but I would add
> > a set_handler_data and get_handler_data function pointer to
> > struct irq_chip. If those are set I'd call them for writing/reading
> > handler_data instead doing it directly. Xen could then specify those
> > and add a field to its own handler data struct for storing the data
> > of the driver coming later.
> >
> > Xen would need another accessor function for its own primary data,
> > of course.
> >
> > Adding the IRQ maintainer as he might have an opinion here. :-)
> 
> Without seeing the patches, and no I'm not going to grab them from a web
> archive, I'd say they are wrong :)
> 
> Fiddling in irqchip is wrong to begin with.
> 
> int irq_set_handler_data(unsigned int irq, void *data);
> static inline void *irq_get_handler_data(unsigned int irq)
> static inline void *irq_data_get_irq_handler_data(struct irq_data *d)
> 
> are accessors to handler_data. Am I missing something?

Hi Thomas,

I think Juergen's suggestion was to use function pointers as accessors.


The underlying problem is that both Xen and GPIO want to use
handler_data.

Xen comes first and uses handler_data to handle Xen events
(drivers/xen/events/events_base.c:xen_irq_init).

Then, the GPIO driver probe function
(drivers/pinctrl/intel/pinctrl-baytrail.c:byt_gpio_probe) calls
gpiochip_set_chained_irqchip, which eventually calls
irq_set_chained_handler_and_data, overwriting handler_data without
checks.


Juergen's suggestion is to replace irq_set_handler_data and
irq_get_handler_data with function pointers.

Xen could install its own irq_set_handler_data and irq_get_handler_data
functions. The Xen implementation would take care of saving other
handler_data pointers on request: when the GPIO driver calls
irq_set_chained_handler_and_data it would end up calling the Xen
implementation of the set_handler_data function that would store the
GPIO pointer in a Xen struct somewhere.