Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754253AbbKLNfh (ORCPT ); Thu, 12 Nov 2015 08:35:37 -0500 Received: from hqemgate15.nvidia.com ([216.228.121.64]:16743 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753690AbbKLNfc (ORCPT ); Thu, 12 Nov 2015 08:35:32 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Thu, 12 Nov 2015 05:24:23 -0800 Subject: Re: [RFC PATCH 1/2] genirq: Add runtime resume/suspend support for IRQ chips To: Lars-Peter Clausen , Grygorii Strashko , Thomas Gleixner References: <1447166377-19707-1-git-send-email-jonathanh@nvidia.com> <1447166377-19707-2-git-send-email-jonathanh@nvidia.com> <56421421.8070807@nvidia.com> <56421FA5.4020801@ti.com> <56423245.1040602@metafoo.de> <564314D9.9040502@nvidia.com> <564361AE.4070303@ti.com> <5644710D.7080108@nvidia.com> <5644920C.8080007@metafoo.de> CC: Jason Cooper , Marc Zyngier , Stephen Warren , Thierry Reding , Kevin Hilman , Geert Uytterhoeven , LKML , , Soren Brinkmann , Linus Walleij , Alexandre Courbot From: Jon Hunter Message-ID: <5644959D.2020207@nvidia.com> Date: Thu, 12 Nov 2015 13:35:25 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <5644920C.8080007@metafoo.de> X-Originating-IP: [10.21.132.159] X-ClientProxiedBy: UKMAIL101.nvidia.com (10.26.138.13) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6155 Lines: 145 On 12/11/15 13:20, Lars-Peter Clausen wrote: > On 11/12/2015 11:59 AM, Jon Hunter wrote: >> >> On 11/11/15 15:41, Grygorii Strashko wrote: >>> On 11/11/2015 12:13 PM, Jon Hunter wrote: >>>> >>>> On 10/11/15 18:07, Lars-Peter Clausen wrote: >>>>> 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. >>>> >>>> Yes, I agree that this does sound a bit odd, but ... >>>> >>>>>> For example, during OF boot: >>>>>> >>>>>> [a] irq_create_of_mapping() >>>>>> - irq_create_fwspec_mapping() >>>>>> - irq_set_irq_type() >>>> >>>> The above means that if someone calls of_irq_get() (or >>>> platform_get_irq()), before request_irq(), then this will call >>>> irq_create_of_mapping() and hence, call irq_set_irq_type. So should >>>> irq_create_fwspec_mapping() be setting the type in the first place? I >>>> can see it is convenient to do it here. >>> >>> In general there is another option - save OF-flags and pass them to >>> __setup_irq() where they can be processed. >> >> Right, we could look at doing something like this. >> >>>>>> or >>> [b] >>>>>> irq_set_irq_type(irq, IRQ_TYPE_LEVEL_HIGH); >>>>>> irq_set_chained_handler(irq, mx31ads_expio_irq_handler); >>> >>> option: add "flag" parameter to irq_set_chained_handler >>> >>>>>> >>>>>> or >>> [c] >>>>>> 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(). >>>> >>>> Right. May be we can look at the various entry points to the chip >>>> operators to get a feel for which public APIs need to be handled. >>> >>> >>> Seems yes. But we need to be very careful with this, some of functions could be >>> called recursively (nested), like: >>> [d] >>> static int pcf857x_irq_set_wake(struct irq_data *data, unsigned int on) >>> { >>> ... >>> error = irq_set_irq_wake(gpio->irq_parent, on); >>> >>> >>> Personally, I have nothing against irq_pm_(get|put) :) and thought about similar things >>> when tried to solve the same problem for omap-gpio driver. >>> But :(, I have to fall back to irq_bus_lock/sync_unlock, because of [a,b,c] - all above >>> APIs surrounded by chip_bus_lock/chip_bus_sync_unlock. ([d] - I've not hit it just because >>> I was lucky). >> >> I had a quick peek at the omap-gpio driver and I see that internally you >> are using the gpio ref-count to manage RPM and use the bus-lock hooks to >> invoke RPM. >> >> This can definitely be complex when considering all the potential paths, >> but I think that we need to a look at this from a chip-ops perspective >> because only the chip knows if it is accessible or not. That said, what >> we need to assess is: >> >> 1. Which chip-ops should ONLY be called after an IRQ has been allocated >> (eg, enable/disable, mask/unmask, type, etc). These chip-ops should >> not try to control the chip PM, but should possibly WARN if called >> when the chip is not accessible. >> 2. For chip-ops that may be called without allocating an IRQ (eg. >> bus_lock, irq_suspend, etc), can these be called from an atomic >> context? If they might be called from an atomic context then these >> are the chip-ops which will cause problems as we cannot guarantee >> that all IRQ chips can support irq-safe RPM. > > They can't. chip_bus_lock() can sleep, so anything that locks the bus can't > be called from atomic context. Sorry, what can't? Yes I understand that we cannot call anything that locks the bus from an atomic context. > One easy way out might be to always call pm_get/pm_but from > bus_lock,/bus_unlock. This way the chip is guaranteed to be powered up when > accessed happens. In addition pm_get is called when the IRQ is request and > pm_put is called when the IRQ is release, this is to ensure the chip stays > powered when it is actively monitoring the IRQ lines. Yes I had thought about that, but it is not quite that easy, because in the case of request_irq() you don't want to pm_put() after the bus_unlock(). However, the bus_lock/unlock() are good indicators of different paths. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/