Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1559209955-10089-1-git-send-email-gbhat@marvell.com> <MN2PR18MB26379DF16EADA38F72A87412A0180@MN2PR18MB2637.namprd18.prod.outlook.com>
In-Reply-To: <MN2PR18MB26379DF16EADA38F72A87412A0180@MN2PR18MB2637.namprd18.prod.outlook.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Fri, 31 May 2019 09:47:33 -0700
Message-ID: <CA+ASDXPDtr7jo=Tt=53VaLNYFwz2MgoSAFxTMLvxvVOMTuL2Cg@mail.gmail.com>
Subject: Re: [PATCH 1/2] mwifiex: add support for host wakeup via PCIE wake#
To:     Ganapathi Bhat <gbhat@marvell.com>
Cc:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        Cathy Luo <cluo@marvell.com>,
        Zhiyuan Yang <yangzy@marvell.com>,
        James Cao <jcao@marvell.com>,
        Rakesh Parmar <rakeshp@marvell.com>,
        Sharvari Harisangam <sharvari@marvell.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

(You really should re-send your patches, as they didn't make it to the
list. But while you're here:)

On Thu, May 30, 2019 at 3:01 AM Ganapathi Bhat <gbhat@marvell.com> wrote:
> > From: Sharvari Harisangam <sharvari@marvell.com>
> >
> > PCIE WAKE# is asserted by firmware, when WoWLAN conditions are
> > matched. Current driver does not enable PME bit needed for WAKE#
> > assertion, causing host to remain in sleep even after WoWLAN conditions are
> > matched. This commit fixes it by enabling wakeup (PME bit) in suspend
> > handler.

Are you sure said devices actually have their 'wakeup' attribute set
to 'enabled' (e.g., in sysfs)? I think the PCI core should probably be
taking care of this for you already. See below.

> > Signed-off-by: Sharvari Harisangam <sharvari@marvell.com>
> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c | 27 +++++++++++++++------------
> >  1 file changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index 3fe81b2..0bd81d4 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
...
> > @@ -181,6 +180,10 @@ static int mwifiex_pcie_suspend(struct device *dev)
> >       set_bit(MWIFIEX_IS_SUSPENDED, &adapter->work_flags);
> >       clear_bit(MWIFIEX_IS_HS_ENABLING, &adapter->work_flags);
> >
> > +     pci_enable_wake(pdev, pci_choose_state(pdev, state), 1);
> > +     pci_save_state(pdev);
> > +     pci_set_power_state(pdev, pci_choose_state(pdev, state));

From Documentation/power/pci.txt:

"""
3.1.2. suspend()
...
This callback is expected to quiesce the device and prepare it to be put into a
low-power state by the PCI subsystem.  It is not required (in fact it even is
not recommended) that a PCI driver's suspend() callback save the standard
configuration registers of the device, prepare it for waking up the system, or
put it into a low-power state.  All of these operations can very well be taken
care of by the PCI subsystem, without the driver's participation.

However, in some rare case it is convenient to carry out these operations in
a PCI driver. [...]
"""

I think you need to do a little more work to justify why you are one
of those rare cases.

On a related note: some of the existing "configure wakeup" stuff we do
here should probably be gated behind a call to device_may_wakeup().
That would help make it more obvious that such configuration is
futile, if the device was marked as wake-disabled.

Brian

> > +
> >       return 0;
> >  }
> >