Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752820AbbKKPmD (ORCPT ); Wed, 11 Nov 2015 10:42:03 -0500 Received: from comal.ext.ti.com ([198.47.26.152]:47873 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751826AbbKKPmA (ORCPT ); Wed, 11 Nov 2015 10:42:00 -0500 Subject: Re: [RFC PATCH 1/2] genirq: Add runtime resume/suspend support for IRQ chips To: Jon Hunter , Lars-Peter Clausen , 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> CC: Jason Cooper , Marc Zyngier , Stephen Warren , Thierry Reding , Kevin Hilman , Geert Uytterhoeven , LKML , , Soren Brinkmann , Linus Walleij , Alexandre Courbot From: Grygorii Strashko Message-ID: <564361AE.4070303@ti.com> Date: Wed, 11 Nov 2015 17:41:34 +0200 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: <564314D9.9040502@nvidia.com> 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: 3868 Lines: 102 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. > >>> 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). -- regards, -grygorii -- 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/