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

[Jürgen Groß <jgross@xxxxxxxx>]

On 25.08.20 10:58, Thomas Gleixner wrote:
> 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.
Of course. I must have been blind not to spot chip_data to exist.

> Something like the completely untested below.
A short test showed no problems. Would you mind sending it as a proper
patch? You can add my

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

> 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;