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. >> 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(). 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. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
