| Message ID | 20200821071547.18894-1-s.temerkhanov@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Xen: Use a dedicated pointer for IRQ data | expand |
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, 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
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; >