From: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> To: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> Cc: Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Kevin Hilman <khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>, LKML <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Subject: Re: [RFC PATCH 1/2] genirq: Add runtime resume/suspend support for IRQ chips Date: Tue, 10 Nov 2015 19:07:01 +0100 [thread overview] Message-ID: <56423245.1040602@metafoo.de> (raw) In-Reply-To: <56421FA5.4020801-l0cyMroinI0@public.gmane.org> On 11/10/2015 05:47 PM, Grygorii Strashko wrote: [...] >> I was trying to simplify matters by placing the resume call in >> __setup_irq() as opposed to requested_threaded_irq(). However, the would >> mean the resume is inside the bus_lock and may be I should not assume >> that I can sleep here. >> >>> Can you folks please agree on something which is correct and complete? >> >> Soren I am happy to defer to your patch and drop this. My only comment >> would be what about the request_percpu_irq() path in your patch? >> > > I have the same comment here as I asked Soren: > 1) There are no restrictions to call irq set_irq_type() whenever, > as result HW can be accessed before request_x_irq()/__setup_irq(). > And this is used quite widely now :( > Changing the configuration of a resource that is not owned seems to be fairly broken. In the worst case this will overwrite the configuration that was set by owner of the resource. Especially those that call irq_set_irq_type() directly before request_irq(), given that you supply the trigger type to request_irq() which will make sure that there are no conflicts and the configure. This is a bit like calling gpio_set_direction() before you call gpio_request(), which will also have PM issues. > For example, during OF boot: > > [a] irq_create_of_mapping() > - irq_create_fwspec_mapping() > - irq_set_irq_type() > > or > irq_set_irq_type(irq, IRQ_TYPE_LEVEL_HIGH); > irq_set_chained_handler(irq, mx31ads_expio_irq_handler); > > or > irq_set_irq_type(alarm_irq, IRQ_TYPE_EDGE_BOTH); > err = devm_request_irq(&pdev->dev, alarm_irq, fan_alarm_irq_handler, > (there are ~200 occurrences of irq set_irq_type in Kernel) > > 2) if i'm not wrong, the same is valid for irq_set_irq_wake() and irq_set_affinity() > > I'm not saying all these code is correct, but that what's now in kernel :( > I've tried to test Soren's patch with omap-gpio and immediately hit case [a] :.( All functions for which are part of the public API and for which it is legal to call them without calling request_irq() (or similar) first will need to have pm_get()/pm_put().
WARNING: multiple messages have this Message-ID (diff)
From: Lars-Peter Clausen <lars@metafoo.de> To: Grygorii Strashko <grygorii.strashko@ti.com>, Jon Hunter <jonathanh@nvidia.com>, Thomas Gleixner <tglx@linutronix.de> Cc: Jason Cooper <jason@lakedaemon.net>, Marc Zyngier <marc.zyngier@arm.com>, Stephen Warren <swarren@wwwdotorg.org>, Thierry Reding <thierry.reding@gmail.com>, Kevin Hilman <khilman@kernel.org>, Geert Uytterhoeven <geert@linux-m68k.org>, LKML <linux-kernel@vger.kernel.org>, linux-tegra@vger.kernel.org, Soren Brinkmann <soren.brinkmann@xilinx.com>, Linus Walleij <linus.walleij@linaro.org>, Alexandre Courbot <gnurou@gmail.com> Subject: Re: [RFC PATCH 1/2] genirq: Add runtime resume/suspend support for IRQ chips Date: Tue, 10 Nov 2015 19:07:01 +0100 [thread overview] Message-ID: <56423245.1040602@metafoo.de> (raw) In-Reply-To: <56421FA5.4020801@ti.com> On 11/10/2015 05:47 PM, Grygorii Strashko wrote: [...] >> I was trying to simplify matters by placing the resume call in >> __setup_irq() as opposed to requested_threaded_irq(). However, the would >> mean the resume is inside the bus_lock and may be I should not assume >> that I can sleep here. >> >>> Can you folks please agree on something which is correct and complete? >> >> Soren I am happy to defer to your patch and drop this. My only comment >> would be what about the request_percpu_irq() path in your patch? >> > > I have the same comment here as I asked Soren: > 1) There are no restrictions to call irq set_irq_type() whenever, > as result HW can be accessed before request_x_irq()/__setup_irq(). > And this is used quite widely now :( > Changing the configuration of a resource that is not owned seems to be fairly broken. In the worst case this will overwrite the configuration that was set by owner of the resource. Especially those that call irq_set_irq_type() directly before request_irq(), given that you supply the trigger type to request_irq() which will make sure that there are no conflicts and the configure. This is a bit like calling gpio_set_direction() before you call gpio_request(), which will also have PM issues. > For example, during OF boot: > > [a] irq_create_of_mapping() > - irq_create_fwspec_mapping() > - irq_set_irq_type() > > or > irq_set_irq_type(irq, IRQ_TYPE_LEVEL_HIGH); > irq_set_chained_handler(irq, mx31ads_expio_irq_handler); > > or > irq_set_irq_type(alarm_irq, IRQ_TYPE_EDGE_BOTH); > err = devm_request_irq(&pdev->dev, alarm_irq, fan_alarm_irq_handler, > (there are ~200 occurrences of irq set_irq_type in Kernel) > > 2) if i'm not wrong, the same is valid for irq_set_irq_wake() and irq_set_affinity() > > I'm not saying all these code is correct, but that what's now in kernel :( > I've tried to test Soren's patch with omap-gpio and immediately hit case [a] :.( All functions for which are part of the public API and for which it is legal to call them without calling request_irq() (or similar) first will need to have pm_get()/pm_put().
next prev parent reply other threads:[~2015-11-10 18:07 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-11-10 14:39 [RFC PATCH 0/2] Add support for Tegra210 AGIC Jon Hunter 2015-11-10 14:39 ` [RFC PATCH 1/2] genirq: Add runtime resume/suspend support for IRQ chips Jon Hunter [not found] ` <1447166377-19707-2-git-send-email-jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-11-10 15:26 ` Thomas Gleixner 2015-11-10 15:26 ` Thomas Gleixner 2015-11-10 15:58 ` Jon Hunter 2015-11-10 15:58 ` Jon Hunter [not found] ` <56421421.8070807-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-11-10 16:47 ` Grygorii Strashko 2015-11-10 16:47 ` Grygorii Strashko [not found] ` <56421FA5.4020801-l0cyMroinI0@public.gmane.org> 2015-11-10 18:07 ` Lars-Peter Clausen [this message] 2015-11-10 18:07 ` Lars-Peter Clausen [not found] ` <56423245.1040602-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> 2015-11-11 10:13 ` Jon Hunter 2015-11-11 10:13 ` Jon Hunter [not found] ` <564314D9.9040502-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-11-11 15:41 ` Grygorii Strashko 2015-11-11 15:41 ` Grygorii Strashko [not found] ` <564361AE.4070303-l0cyMroinI0@public.gmane.org> 2015-11-12 10:59 ` Jon Hunter 2015-11-12 10:59 ` Jon Hunter [not found] ` <5644710D.7080108-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-11-12 13:20 ` Lars-Peter Clausen 2015-11-12 13:20 ` Lars-Peter Clausen 2015-11-12 13:29 ` Grygorii Strashko 2015-11-12 13:29 ` Grygorii Strashko [not found] ` <5644943E.1060102-l0cyMroinI0@public.gmane.org> 2015-11-12 13:34 ` Lars-Peter Clausen 2015-11-12 13:34 ` Lars-Peter Clausen [not found] ` <5644957D.6060202-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> 2015-11-13 18:07 ` Grygorii Strashko 2015-11-13 18:07 ` Grygorii Strashko 2015-11-12 13:35 ` Jon Hunter 2015-11-12 13:35 ` Jon Hunter 2015-11-12 13:47 ` Lars-Peter Clausen [not found] ` <5644986B.5030901-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> 2015-11-12 14:02 ` Jon Hunter 2015-11-12 14:02 ` Jon Hunter [not found] ` <56449BF0.9090408-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-11-12 14:37 ` Lars-Peter Clausen 2015-11-12 14:37 ` Lars-Peter Clausen [not found] ` <5644A418.2020906-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> 2015-11-12 15:38 ` Jon Hunter 2015-11-12 15:38 ` Jon Hunter 2015-11-12 17:04 ` Sören Brinkmann 2015-11-12 17:04 ` Sören Brinkmann 2015-11-12 23:20 ` Kevin Hilman [not found] ` <7hio56dctz.fsf-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org> 2015-11-13 9:01 ` Jon Hunter 2015-11-13 9:01 ` Jon Hunter [not found] ` <5645A6F6.6020202-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-11-13 20:01 ` Thomas Gleixner 2015-11-13 20:01 ` Thomas Gleixner 2015-11-16 9:46 ` Jon Hunter 2015-11-16 9:46 ` Jon Hunter 2015-11-16 9:49 ` Geert Uytterhoeven [not found] ` <CAMuHMdXazgRcw=+O-w+PsZpwiW8iCwZ8MmhNSjcOoZGRP45Y6Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-11-16 10:34 ` Jon Hunter 2015-11-16 10:34 ` Jon Hunter [not found] ` <5649B135.8050800-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2015-11-16 10:48 ` Geert Uytterhoeven 2015-11-16 10:48 ` Geert Uytterhoeven [not found] ` <CAMuHMdW2L1gYO7cNjeeaBTcQQdEXc9q45E1sZj-=TPwokkGx2g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-11-17 11:57 ` Jon Hunter 2015-11-17 11:57 ` Jon Hunter 2015-11-10 14:39 ` [RFC PATCH 2/2] irqchip/gic: Add support for tegra AGIC interrupt controller Jon Hunter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=56423245.1040602@metafoo.de \ --to=lars-qo5elluwu/uelga04laivw@public.gmane.org \ --cc=geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org \ --cc=gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=grygorii.strashko-l0cyMroinI0@public.gmane.org \ --cc=jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org \ --cc=jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \ --cc=khilman-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ --cc=linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \ --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=marc.zyngier-5wv7dgnIgG8@public.gmane.org \ --cc=soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org \ --cc=swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org \ --cc=tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org \ --cc=thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.