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

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

On Mon, Aug 24 2020 at 20:14, Stefano Stabellini wrote:
> On Fri, 21 Aug 2020, Thomas Gleixner wrote:
>> are accessors to handler_data. Am I missing something?
> 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.

I don't think that's a good idea. The point is that we have an irq chip
which wants to have per interrupt data and then an interrupt request
which wants that as well.

Conceptually they are distinct. One belongs to the irq chip and one to
the handler.

So the straight forward solution is to switch XEN to use the
irqdesc::irq_data::chip_data instead of
irqdesc::irq_data_common::handler_data.

Something like the completely untested below.

Thanks,

        tglx
---
 drivers/xen/events/events_base.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -156,7 +156,7 @@ int get_evtchn_to_irq(evtchn_port_t evtc
 /* Get info for IRQ */
 struct irq_info *info_for_irq(unsigned irq)
 {
-       return irq_get_handler_data(irq);
+       return irq_get_chip_data(irq);
 }
 
 /* Constructors for packed IRQ information. */
@@ -377,7 +377,7 @@ static void xen_irq_init(unsigned irq)
        info->type = IRQT_UNBOUND;
        info->refcnt = -1;
 
-       irq_set_handler_data(irq, info);
+       irq_set_chip_data(irq, info);
 
        list_add_tail(&info->list, &xen_irq_list_head);
 }
@@ -426,14 +426,14 @@ static int __must_check xen_allocate_irq
 
 static void xen_free_irq(unsigned irq)
 {
-       struct irq_info *info = irq_get_handler_data(irq);
+       struct irq_info *info = irq_get_chip_data(irq);
 
        if (WARN_ON(!info))
                return;
 
        list_del(&info->list);
 
-       irq_set_handler_data(irq, NULL);
+       irq_set_chip_data(irq, NULL);
 
        WARN_ON(info->refcnt > 0);
 
@@ -603,7 +603,7 @@ EXPORT_SYMBOL_GPL(xen_irq_from_gsi);
 static void __unbind_from_irq(unsigned int irq)
 {
        evtchn_port_t evtchn = evtchn_from_irq(irq);
-       struct irq_info *info = irq_get_handler_data(irq);
+       struct irq_info *info = irq_get_chip_data(irq);
 
        if (info->refcnt > 0) {
                info->refcnt--;
@@ -1108,7 +1108,7 @@ int bind_ipi_to_irqhandler(enum ipi_vect
 
 void unbind_from_irqhandler(unsigned int irq, void *dev_id)
 {
-       struct irq_info *info = irq_get_handler_data(irq);
+       struct irq_info *info = irq_get_chip_data(irq);
 
        if (WARN_ON(!info))
                return;
@@ -1142,7 +1142,7 @@ int evtchn_make_refcounted(evtchn_port_t
        if (irq == -1)
                return -ENOENT;
 
-       info = irq_get_handler_data(irq);
+       info = irq_get_chip_data(irq);
 
        if (!info)
                return -ENOENT;
@@ -1170,7 +1170,7 @@ int evtchn_get(evtchn_port_t evtchn)
        if (irq == -1)
                goto done;
 
-       info = irq_get_handler_data(irq);
+       info = irq_get_chip_data(irq);
 
        if (!info)
                goto done;