RE: [PATCH 1/2] mwifiex: add support for host wakeup via PCIE wake#

RE: [PATCH 1/2] mwifiex: add support for host wakeup via PCIE wake#

[Ganapathi Bhat]

Hi Kalle,

I have pushed two patches usingn 'git send-mail' and below patch is the first one; 

I could not see the patches in patchwork page(https://patchwork.kernel.org/project/linux-wireless/list/); Did you get this patch? 

Regards,
Ganapathi
> -----Original Message-----
> From: Ganapathi Bhat <gbhat@marvell.com>
> Sent: Thursday, May 30, 2019 3:23 PM
> To: linux-wireless@vger.kernel.org
> Cc: 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>; Ganapathi Bhat
> <gbhat@marvell.com>
> Subject: [PATCH 1/2] mwifiex: add support for host wakeup via PCIE wake#
> 
> 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.
> 
> 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
> @@ -147,11 +147,10 @@ static bool mwifiex_pcie_ok_to_access_hw(struct
> mwifiex_adapter *adapter)
>   * If already not suspended, this function allocates and sends a host
>   * sleep activate request to the firmware and turns off the traffic.
>   */
> -static int mwifiex_pcie_suspend(struct device *dev)
> +static int mwifiex_pcie_suspend(struct pci_dev *pdev, pm_message_t
> +state)
>  {
>  	struct mwifiex_adapter *adapter;
>  	struct pcie_service_card *card;
> -	struct pci_dev *pdev = to_pci_dev(dev);
> 
>  	card = pci_get_drvdata(pdev);
> 
> @@ -160,7 +159,7 @@ static int mwifiex_pcie_suspend(struct device *dev)
> 
>  	adapter = card->adapter;
>  	if (!adapter) {
> -		dev_err(dev, "adapter is not valid\n");
> +		dev_err(&pdev->dev, "adapter is not valid\n");
>  		return 0;
>  	}
> 
> @@ -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));
> +
>  	return 0;
>  }
> 
> @@ -192,16 +195,20 @@ static int mwifiex_pcie_suspend(struct device
> *dev)
>   * If already not resumed, this function turns on the traffic and
>   * sends a host sleep cancel request to the firmware.
>   */
> -static int mwifiex_pcie_resume(struct device *dev)
> +static int mwifiex_pcie_resume(struct pci_dev *pdev)
>  {
>  	struct mwifiex_adapter *adapter;
>  	struct pcie_service_card *card;
> -	struct pci_dev *pdev = to_pci_dev(dev);
> +
> +	pci_set_power_state(pdev, PCI_D0);
> +	pci_restore_state(pdev);
> +	pci_enable_wake(pdev, PCI_D0, 0);
> +
> 
>  	card = pci_get_drvdata(pdev);
> 
>  	if (!card->adapter) {
> -		dev_err(dev, "adapter structure is not valid\n");
> +		dev_err(&pdev->dev, "adapter structure is not valid\n");
>  		return 0;
>  	}
> 
> @@ -416,11 +423,6 @@ static void mwifiex_pcie_reset_done(struct pci_dev
> *pdev)
>  	.reset_done		= mwifiex_pcie_reset_done,
>  };
> 
> -#ifdef CONFIG_PM_SLEEP
> -/* Power Management Hooks */
> -static SIMPLE_DEV_PM_OPS(mwifiex_pcie_pm_ops, mwifiex_pcie_suspend,
> -				mwifiex_pcie_resume);
> -#endif
> 
>  /* PCI Device Driver */
>  static struct pci_driver __refdata mwifiex_pcie = { @@ -431,7 +433,8 @@
> static SIMPLE_DEV_PM_OPS(mwifiex_pcie_pm_ops, mwifiex_pcie_suspend,
>  	.driver   = {
>  		.coredump = mwifiex_pcie_coredump,
>  #ifdef CONFIG_PM_SLEEP
> -		.pm = &mwifiex_pcie_pm_ops,
> +	.suspend  = mwifiex_pcie_suspend,
> +	.resume   = mwifiex_pcie_resume,
>  #endif
>  	},
>  	.shutdown = mwifiex_pcie_shutdown,
> --
> 1.9.1

Re: [PATCH 1/2] mwifiex: add support for host wakeup via PCIE wake#

[Kalle Valo]

Ganapathi Bhat <gbhat@marvell.com> writes:

> I have pushed two patches usingn 'git send-mail' and below patch is the first one; 
>
> I could not see the patches in patchwork page(https://patchwork.kernel.org/project/linux-wireless/list/); Did you get this patch? 

I take patches directly from patchwork and if it's not there I don't see
it. So you need to resend.

-- 
Kalle Valo

Re: [PATCH 1/2] mwifiex: add support for host wakeup via PCIE wake#

[Brian Norris]

(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;
> >  }
> >

RE: [PATCH 2/2] mwifiex: add support for WIPHY_WOWLAN_ANY

[Ganapathi Bhat]

Hi Kalle,

Below is the second patch in the series; Kindly help to merge both;

Regards,
Ganapathi
> -----Original Message-----
> From: Ganapathi Bhat <gbhat@marvell.com>
> Sent: Thursday, May 30, 2019 3:23 PM
> To: linux-wireless@vger.kernel.org
> Cc: 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>; Ganapathi Bhat
> <gbhat@marvell.com>
> Subject: [PATCH 2/2] mwifiex: add support for WIPHY_WOWLAN_ANY
> 
> From: Sharvari Harisangam <sharvari@marvell.com>
> 
> Advertise support for WIPHY_WOWLAN_ANY trigger. Update default
> hostsleep condition to handle magic packet.
> 
> Signed-off-by: Sharvari Harisangam <sharvari@marvell.com>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 6 +++---
>  drivers/net/wireless/marvell/mwifiex/fw.h       | 4 +++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index e11a4bb..f23bb9c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -3492,7 +3492,7 @@ static int mwifiex_cfg80211_suspend(struct wiphy
> *wiphy,
>  						  wowlan->nd_config);
>  	}
> 
> -	if (wowlan->disconnect) {
> +	if (wowlan->disconnect || wowlan->any || wowlan->magic_pkt) {
>  		hs_cfg.conditions |= HS_CFG_COND_MAC_EVENT;
>  		mwifiex_dbg(sta_priv->adapter, INFO, "Wake on device
> disconnect\n");
>  	}
> @@ -4232,7 +4232,7 @@ static int mwifiex_tm_cmd(struct wiphy *wiphy,
> struct wireless_dev *wdev,  static const struct wiphy_wowlan_support
> mwifiex_wowlan_support = {
>  	.flags = WIPHY_WOWLAN_MAGIC_PKT |
> WIPHY_WOWLAN_DISCONNECT |
>  		WIPHY_WOWLAN_NET_DETECT |
> WIPHY_WOWLAN_SUPPORTS_GTK_REKEY |
> -		WIPHY_WOWLAN_GTK_REKEY_FAILURE,
> +		WIPHY_WOWLAN_GTK_REKEY_FAILURE |
> WIPHY_WOWLAN_ANY,
>  	.n_patterns = MWIFIEX_MEF_MAX_FILTERS,
>  	.pattern_min_len = 1,
>  	.pattern_max_len = MWIFIEX_MAX_PATTERN_LEN, @@ -4242,7
> +4242,7 @@ static int mwifiex_tm_cmd(struct wiphy *wiphy, struct
> wireless_dev *wdev,
> 
>  static const struct wiphy_wowlan_support
> mwifiex_wowlan_support_no_gtk = {
>  	.flags = WIPHY_WOWLAN_MAGIC_PKT |
> WIPHY_WOWLAN_DISCONNECT |
> -		 WIPHY_WOWLAN_NET_DETECT,
> +		 WIPHY_WOWLAN_NET_DETECT | WIPHY_WOWLAN_ANY,
>  	.n_patterns = MWIFIEX_MEF_MAX_FILTERS,
>  	.pattern_min_len = 1,
>  	.pattern_max_len = MWIFIEX_MAX_PATTERN_LEN, diff --git
> a/drivers/net/wireless/marvell/mwifiex/fw.h
> b/drivers/net/wireless/marvell/mwifiex/fw.h
> index b73f99d..ad34b25 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -478,7 +478,9 @@ enum mwifiex_channel_flags {
>  #define HostCmd_SCAN_RADIO_TYPE_A           1
> 
>  #define HS_CFG_CANCEL			0xffffffff
> -#define HS_CFG_COND_DEF			0x00000000
> +#define HS_CFG_COND_DEF		(HS_CFG_COND_BROADCAST_DATA
> |\
> +				 HS_CFG_COND_UNICAST_DATA |\
> +				 HS_CFG_COND_MULTICAST_DATA)
>  #define HS_CFG_GPIO_DEF			0xff
>  #define HS_CFG_GAP_DEF			0xff
>  #define HS_CFG_COND_BROADCAST_DATA	0x00000001
> --
> 1.9.1