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

["Jürgen Groß" <jgross@suse.com>]

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@suse.com>


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