From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755177AbcARNV6 (ORCPT ); Mon, 18 Jan 2016 08:21:58 -0500 Received: from mail-wm0-f47.google.com ([74.125.82.47]:36271 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754902AbcARNV4 (ORCPT ); Mon, 18 Jan 2016 08:21:56 -0500 Subject: Re: [RFC PATCH 2/2] sched: idle: IRQ based next prediction for idle period To: Thomas Gleixner References: <1452093774-17831-1-git-send-email-daniel.lezcano@linaro.org> <1452093774-17831-3-git-send-email-daniel.lezcano@linaro.org> Cc: peterz@infradead.org, rafael@kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, nicolas.pitre@linaro.org, vincent.guittot@linaro.org From: Daniel Lezcano Message-ID: <569CE6F1.2090707@linaro.org> Date: Mon, 18 Jan 2016 14:21:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/08/2016 04:43 PM, Thomas Gleixner wrote: > On Wed, 6 Jan 2016, Daniel Lezcano wrote: [ ... ] >> + /* >> + * For all the irq already setup, assign the timing callback. >> + * All interrupts with their desc NULL will be discarded. >> + */ >> + for_each_irq_desc(irq, desc) >> + sched_irq_timing_setup(irq, desc->action); > > No, no, no. This belongs into the core code register_irq_timings() function > which installs the handler into the irq descs with the proper protections and > once it has done that enables the static key. > > The above is completely unprotected against interrupts being setup or even > freed concurrently. > > Aside of that, you call that setup function in setup_irq for each action() and > here you call it only for the first one. Hi Thomas, I went through the different comments and almost finished the changes but I think the 'register_ops' approach, which happens after some irq were setup, introduces some useless complexity and because of the desc lock section, the ops can't do memory allocation. Before going further, I am wondering if declaring the irq_timings_ops statically (read without 'register_ops' - hence without a init time dependency) and calling the init/free ops from alloc_desc/free_desc wouldn't be cleaner and simpler. What do you think ? -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog