Use a dedicated pointer for IRQ data to avoid conflicts with some
other parts of the kernel code which my use handler_data for their
own purposes while still running on Xen

Sergey Temerkhanov (2):
  Xen: Use a dedicated irq_info structure pointer
  Xen: Rename irq_info structure

 drivers/xen/events/events_2l.c       |  2 +-
 drivers/xen/events/events_base.c     | 80 +++++++++++++---------------
 drivers/xen/events/events_fifo.c     |  5 +-
 drivers/xen/events/events_internal.h | 12 ++---
 include/linux/irq.h                  | 17 ++++++
 kernel/irq/chip.c                    | 14 +++++
 6 files changed, 78 insertions(+), 52 deletions(-)

-- 
2.26.2

On 21.08.20 09:15, Sergey Temerkhanov wrote:
> Use a dedicated pointer for IRQ data to avoid conflicts with some
> other parts of the kernel code which my use handler_data for their
> own purposes while still running on Xen
> 
> Sergey Temerkhanov (2):
>    Xen: Use a dedicated irq_info structure pointer
>    Xen: Rename irq_info structure
> 
>   drivers/xen/events/events_2l.c       |  2 +-
>   drivers/xen/events/events_base.c     | 80 +++++++++++++---------------
>   drivers/xen/events/events_fifo.c     |  5 +-
>   drivers/xen/events/events_internal.h | 12 ++---
>   include/linux/irq.h                  | 17 ++++++
>   kernel/irq/chip.c                    | 14 +++++
>   6 files changed, 78 insertions(+), 52 deletions(-)
> 

Did you see any specific problem where handler_data is written by
another component?

In case this is a real problem I don't think your approach will be
accepted, especially the IRQ subsystem maintainers probably won't
like it.

And please include the maintainers of the files you are modifying in
the recipients list of the patch(es).


Juergen

>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

Regards,
Sergey

On Fri, Aug 21, 2020 at 1:18 PM Jürgen Groß <jgross@suse.com> wrote:
>
> On 21.08.20 09:15, Sergey Temerkhanov wrote:
> > Use a dedicated pointer for IRQ data to avoid conflicts with some
> > other parts of the kernel code which my use handler_data for their
> > own purposes while still running on Xen
> >
> > Sergey Temerkhanov (2):
> >    Xen: Use a dedicated irq_info structure pointer
> >    Xen: Rename irq_info structure
> >
> >   drivers/xen/events/events_2l.c       |  2 +-
> >   drivers/xen/events/events_base.c     | 80 +++++++++++++---------------
> >   drivers/xen/events/events_fifo.c     |  5 +-
> >   drivers/xen/events/events_internal.h | 12 ++---
> >   include/linux/irq.h                  | 17 ++++++
> >   kernel/irq/chip.c                    | 14 +++++
> >   6 files changed, 78 insertions(+), 52 deletions(-)
> >
>
> Did you see any specific problem where handler_data is written by
> another component?
>
> In case this is a real problem I don't think your approach will be
> accepted, especially the IRQ subsystem maintainers probably won't
> like it.
>
> And please include the maintainers of the files you are modifying in
> the recipients list of the patch(es).
>
>
> Juergen

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. :-)


Juergen

> 
> Regards,
> Sergey
> 
> On Fri, Aug 21, 2020 at 1:18 PM Jürgen Groß <jgross@suse.com> wrote:
>>
>> On 21.08.20 09:15, Sergey Temerkhanov wrote:
>>> Use a dedicated pointer for IRQ data to avoid conflicts with some
>>> other parts of the kernel code which my use handler_data for their
>>> own purposes while still running on Xen
>>>
>>> Sergey Temerkhanov (2):
>>>     Xen: Use a dedicated irq_info structure pointer
>>>     Xen: Rename irq_info structure
>>>
>>>    drivers/xen/events/events_2l.c       |  2 +-
>>>    drivers/xen/events/events_base.c     | 80 +++++++++++++---------------
>>>    drivers/xen/events/events_fifo.c     |  5 +-
>>>    drivers/xen/events/events_internal.h | 12 ++---
>>>    include/linux/irq.h                  | 17 ++++++
>>>    kernel/irq/chip.c                    | 14 +++++
>>>    6 files changed, 78 insertions(+), 52 deletions(-)
>>>
>>
>> Did you see any specific problem where handler_data is written by
>> another component?
>>
>> In case this is a real problem I don't think your approach will be
>> accepted, especially the IRQ subsystem maintainers probably won't
>> like it.
>>
>> And please include the maintainers of the files you are modifying in
>> the recipients list of the patch(es).
>>
>>
>> Juergen
> 

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?

Thanks,

        tglx

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.

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;

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

On Fri, Aug 21, 2020 at 11:07 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> 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?

Well, the patches in question are meant to solve the issue discussed
in this thread:
https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg00957.html
All in all, the Dom0 kernel crashes due to a conflict between Xen and
GPIO pinctrl code both trying to use hander data to track some private
data. The patch series adds a separate pointer along with a few
functions for Xen. There may be other ideas on how to resolve this
issue though
Here are web archive :-) links:
https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg01144.html
https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg01145.html

Regards,
Sergey

On Fri, Aug 21 2020 at 23:38, Sergei Temerkhanov wrote:
> On Fri, Aug 21, 2020 at 11:07 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> 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?
>
> Well, the patches in question are meant to solve the issue discussed
> in this thread:
> https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg00957.html
> All in all, the Dom0 kernel crashes due to a conflict between Xen and
> GPIO pinctrl code both trying to use hander data to track some private
> data. The patch series adds a separate pointer along with a few
> functions for Xen. There may be other ideas on how to resolve this
> issue though
> Here are web archive :-) links:
> https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg01144.html
> https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg01145.html

As I said before, I'm not going to wade through a web archive simply
because it's tideous and I can't reply and quote properly.

Can you please just resend this series with the appropriate maintainers
and mailing lists in Cc which you should have done in the first place,
instead of offering me web links?

Just upfront, adding something just for XEN is wrong to begin with. This
is a conceptual problem and XEN is just triggering it, but the
argumentation must be on the concept level, not at the XEN level.

Without having looked at the patches, I bet that this new pointer has a
XEN name tag on it and the changelogs do not explain anything about the
conceptual problem the patches try to solve.

If I'm right, you might want to fix that before resending. If I'm wrong,
then I owe you a beer.

Thanks,

        tglx