From patchwork Fri Nov 11 22:05:34 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Norris X-Patchwork-Id: 9423715 X-Patchwork-Delegate: kvalo@adurom.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 160E2601C0 for ; Fri, 11 Nov 2016 22:05:41 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0709A29263 for ; Fri, 11 Nov 2016 22:05:41 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id EE8D229BC7; Fri, 11 Nov 2016 22:05:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4964D29263 for ; Fri, 11 Nov 2016 22:05:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936677AbcKKWFi (ORCPT ); Fri, 11 Nov 2016 17:05:38 -0500 Received: from mail-pg0-f51.google.com ([74.125.83.51]:34122 "EHLO mail-pg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935747AbcKKWFh (ORCPT ); Fri, 11 Nov 2016 17:05:37 -0500 Received: by mail-pg0-f51.google.com with SMTP id x23so14471048pgx.1 for ; Fri, 11 Nov 2016 14:05:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=EmGrWb6yhE+pwQnU4pXHZSBJShaBRFsyqakESeFbazk=; b=fw76aGFnKoFYl069oax/BIaAkigdj4PcOx3Md/5Q8Zaq6qzuZSk94Q0c9D64H31cEM Ft3jJ42/38vFhmd1yz4P2m7zlLoHZ2/UYonA/QVtWIH2+xPIXN5EzpUixbwbnE1PvK9p RlnFF3setC8zApo+ls2v301bw4WLd9MuzJgXw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=EmGrWb6yhE+pwQnU4pXHZSBJShaBRFsyqakESeFbazk=; b=XKo3oinUl5sJNCvTK0CFuEsovh2lMbLgX5UWMdPv0ccBKxrKeTrhDLOyrbCEYjBXLq 7/bHfXKoqER5ADCVv1ceyyVdd/4UOpMLUqrGaSfQGOTNISsiUBhHls/+IXiQCt4lEhSD nrwmfg0EAugy2LX0ovNc3RoEM8OQiXC7u3lxEGGd2K9Qotb07RaFdAFYG1vUfrGGAdbz C8juRpQ6LfU0AY1kgMuOhJxQFdzmGbIK863EKy8Mhd7FpznM0gWEeQs/pcB9/dMPuuAB itmgXiZhZRERrCL0+4Y6E49f9+iLcNJWInJWCDwah9oGX+8qkFB81yW/NIVEKQa/+NjX HyRw== X-Gm-Message-State: ABUngvd0XOIhgYpmk+CwlrDw/wqKEpoafIEJ2klG/MkZwAHnDfoyqusmpwFatZ6UKdoYtB2R X-Received: by 10.98.141.153 with SMTP id p25mr10994236pfk.148.1478901936697; Fri, 11 Nov 2016 14:05:36 -0800 (PST) Received: from google.com ([2620:0:1000:1301:2df8:8593:7b08:2299]) by smtp.gmail.com with ESMTPSA id 74sm17317121pfp.77.2016.11.11.14.05.35 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Fri, 11 Nov 2016 14:05:36 -0800 (PST) Date: Fri, 11 Nov 2016 14:05:34 -0800 From: Brian Norris To: Amitkumar Karwar Cc: linux-wireless@vger.kernel.org, Cathy Luo , Nishant Sarmukadam , rajatja@google.com, dmitry.torokhov@gmail.com Subject: Re: [PATCH v2 3/3] mwifiex: Enable WoWLAN for both sdio and pcie Message-ID: <20161111220530.GA142669@google.com> References: <1478862911-15498-1-git-send-email-akarwar@marvell.com> <1478862911-15498-3-git-send-email-akarwar@marvell.com> <20161111204235.GB111624@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161111204235.GB111624@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, One correction to my review: On Fri, Nov 11, 2016 at 12:42:36PM -0800, Brian Norris wrote: > On Fri, Nov 11, 2016 at 04:45:11PM +0530, Amitkumar Karwar wrote: > > --- a/drivers/net/wireless/marvell/mwifiex/main.c > > +++ b/drivers/net/wireless/marvell/mwifiex/main.c > > @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare) > > } > > EXPORT_SYMBOL_GPL(mwifiex_do_flr); > > > > +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv) > > +{ > > + struct mwifiex_adapter *adapter = priv; > > + > > + if (adapter->irq_wakeup >= 0) { > > + dev_dbg(adapter->dev, "%s: wake by wifi", __func__); > > + adapter->wake_by_wifi = true; > > This flag is unecessary and buggy. IIUC, you're trying to avoid calling > disable_irq() in resume(), if you've called it here? > > > + disable_irq_nosync(irq); > > ...but this is unnecessary, I think, unless you're trying to make up for > buggy wakeup interrupts that keep firing? See my suggestion below. I think I figured out some of the logic here, and my suggestion was somewhat wrong. This 'wake_by_wifi' flag seems to be used here to assist with the fact that we're requesting a level-triggered interrupt, but the card doesn't deassert the interrupt until much later -- when we finish the wakeup handshake. So I guess it is necessary (or at least, expedient) to disable the interrupt here. > > + } > > + > > + /* Notify PM core we are wakeup source */ > > + pm_wakeup_event(adapter->dev, 0); > > + > > + return IRQ_HANDLED; > > +} > > + > > static void mwifiex_probe_of(struct mwifiex_adapter *adapter) > > { > > + int ret; > > struct device *dev = adapter->dev; > > > > if (!dev->of_node) > > return; > > > > adapter->dt_node = dev->of_node; > > + adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0); > > + if (!adapter->irq_wakeup) { > > + dev_info(dev, "fail to parse irq_wakeup from device tree\n"); > > + return; > > + } > > + > > + ret = devm_request_irq(dev, adapter->irq_wakeup, > > + mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW, > > + "wifi_wake", adapter); > > + if (ret) { > > + dev_err(dev, "Failed to request irq_wakeup %d (%d)\n", > > + adapter->irq_wakeup, ret); > > + goto err_exit; > > + } > > + > > + disable_irq(adapter->irq_wakeup); > > + if (device_init_wakeup(dev, true)) { > > + dev_err(dev, "fail to init wakeup for mwifiex\n"); > > + goto err_exit; > > + } > > + return; > > + > > +err_exit: > > + adapter->irq_wakeup = 0; > > } > > > > /* > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h > > index 0c07434..11abc49 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/main.h > > +++ b/drivers/net/wireless/marvell/mwifiex/main.h > > @@ -1011,6 +1011,10 @@ struct mwifiex_adapter { > > bool usb_mc_setup; > > struct cfg80211_wowlan_nd_info *nd_info; > > struct ieee80211_regdomain *regd; > > + > > + /* Wake-on-WLAN (WoWLAN) */ > > + int irq_wakeup; > > + bool wake_by_wifi; > > }; > > > > void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter); > > @@ -1410,6 +1414,27 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status) > > return false; > > } > > > > +/* Disable platform specific wakeup interrupt */ > > +static inline void mwifiex_disable_wake(struct mwifiex_adapter *adapter) > > +{ > > + if (adapter->irq_wakeup >= 0) { > > + disable_irq_wake(adapter->irq_wakeup); > > + if (!adapter->wake_by_wifi) > > You're depending on the wake IRQ handler to set this flag, so you don't > disable the IRQ twice (the calls nest). But what if the IRQ is being > serviced concurrently with this? Then you'll double-disable the IRQ. > > I believe you can just remove the disable_irq_nosync() from the handler, > kill the above flag, and just unconditionally disable the IRQ. So my suggestion here was wrong; we shouldn't completely kill the check for ->wake_by_wifi I don't think, but we *should* wait for the interrupt handler to complete before checking the flag. i.e., synchronize_irq(): I'd appreciate if you bugfix this, either before or after this patch. Brian > > + disable_irq(adapter->irq_wakeup); > > + } > > +} > > + > > +/* Enable platform specific wakeup interrupt */ > > +static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter) > > +{ > > + /* Enable platform specific wakeup interrupt */ > > + if (adapter->irq_wakeup >= 0) { > > + adapter->wake_by_wifi = false; > > + enable_irq(adapter->irq_wakeup); > > + enable_irq_wake(adapter->irq_wakeup); > > + } > > +} > > + > > int mwifiex_init_shutdown_fw(struct mwifiex_private *priv, > > u32 func_init_shutdown); > > int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8, diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c index 4063086ab5b8..e9446eeafb9d 100644 --- a/drivers/net/wireless/marvell/mwifiex/util.c +++ b/drivers/net/wireless/marvell/mwifiex/util.c @@ -841,6 +841,11 @@ void mwifiex_disable_wake(struct mwifiex_plt_wake_cfg *wake_cfg) { if (wake_cfg && wake_cfg->irq_wifi >= 0) { disable_irq_wake(wake_cfg->irq_wifi); + /* + * Disable the wake IRQ only if it didn't already fire (and + * disable itself). + */ + synchronize_irq(wake_cfg->irq_wifi); if (!wake_cfg->wake_by_wifi) disable_irq(wake_cfg->irq_wifi); }