Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938259AbcKKXev (ORCPT ); Fri, 11 Nov 2016 18:34:51 -0500 Received: from mail-qk0-f193.google.com ([209.85.220.193]:33913 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934163AbcKKXes (ORCPT ); Fri, 11 Nov 2016 18:34:48 -0500 MIME-Version: 1.0 In-Reply-To: <20161111223234.GM7138@atomide.com> References: <1478801227-65527-1-git-send-email-briannorris@chromium.org> <20161110184910.GA135921@google.com> <20161111163147.GC7138@atomide.com> <20161111222911.GL7138@atomide.com> <20161111223234.GM7138@atomide.com> From: "Rafael J. Wysocki" Date: Sat, 12 Nov 2016 00:34:47 +0100 X-Google-Sender-Auth: Jr6AeB8RQv1ZryHYzedRjVuLEqk Message-ID: Subject: Re: [PATCH] PM / wakeirq: report wakeup events in dedicated wake-IRQs To: Tony Lindgren Cc: "Rafael J. Wysocki" , Brian Norris , Dmitry Torokhov , "Rafael J . Wysocki" , Pavel Machek , Len Brown , Greg Kroah-Hartman , lkml , Brian Norris , "linux-pm@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2691 Lines: 54 On Fri, Nov 11, 2016 at 11:32 PM, Tony Lindgren wrote: > * Tony Lindgren [161111 14:29]: >> * Rafael J. Wysocki [161111 13:33]: >> > On Fri, Nov 11, 2016 at 5:31 PM, Tony Lindgren wrote: >> > > * Rafael J. Wysocki [161110 16:06]: >> > >> On Thu, Nov 10, 2016 at 7:49 PM, Brian Norris wrote: >> > >> > On Thu, Nov 10, 2016 at 10:13:55AM -0800, Dmitry Torokhov wrote: >> > >> >> On Thu, Nov 10, 2016 at 10:07 AM, Brian Norris wrote: >> > >> >> > It's important that user space can figure out what device woke the >> > >> >> > system from suspend -- e.g., for debugging, or for implementing >> > >> >> > conditional wake behavior. Dedicated wakeup IRQs don't currently do >> > >> >> > that. >> > >> >> > >> > >> >> > Let's report the event (pm_wakeup_event()) and also allow drivers to >> > >> >> > synchronize with these events in their resume path (hence, disable_irq() >> > >> >> > instead of disable_irq_nosync()). >> > >> >> >> > >> >> Hmm, dev_pm_disable_wake_irq() is called from >> > >> >> rpm_suspend()/rpm_resume() that take dev->power.lock spinlock and >> > >> >> disable interrupts. Dropping _nosync() feels dangerous. >> > >> > >> > >> > Indeed. So how do you suggest we get sane wakeup reports? Every device >> > >> > or bus that's going to use the dedicated wake APIs has to >> > >> > synchronize_irq() [1] in their resume() routine? Seems like an odd >> > >> > implementation detail to have to remember (and therefore most drivers >> > >> > will get it wrong). >> > >> > >> > >> > Brian >> > >> > >> > >> > [1] Or maybe at least create a helper API that will extract the >> > >> > dedicated wake IRQ number and do the synchronize_irq() for us, so >> > >> > drivers don't have to stash this separately (or poke at >> > >> > dev->power.wakeirq->irq) for no good reason. >> > >> >> > >> Well, in the first place, can anyone please refresh my memory on why >> > >> it is necessary to call dev_pm_disable_wake_irq() under power.lock? >> > > >> > > I guess no other reason except we need to manage the wakeirq >> > > for rpm_callback(). So we dev_pm_enable_wake_irq() before >> > > rpm_callback() in rpm_suspend(), then disable on resume. >> > >> > But we drop the lock in rpm_callback(), so can't it be moved to where >> > the callback is invoked? >> >> Then we're back to patching all the drivers again, no? > > Sorry I misunderstood, yeah that should work if rpm_callback() drops > the lock. It still will not re-enable interrupts if the irq_safe flag is set. I wonder if we really care about this case, though. Thanks, Rafael