Message-ID: <6fc9f00aa0b0867029fb6406a55c1e72d4c13af6.camel@egauge.net>
Subject: Re: [PATCH] wilc1000: Allow setting power_save before driver is
 initialized
From:   David Mosberger-Tang <davidm@egauge.net>
To:     Ajay Singh <ajay.kathat@microchip.com>
Cc:     Claudiu Beznea <claudiu.beznea@microchip.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Sun, 12 Dec 2021 14:20:29 -0700
In-Reply-To: <20211212011835.3719001-1-davidm@egauge.net>
References: <20211212011835.3719001-1-davidm@egauge.net>
Organization: eGauge Systems LLC
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Unfortunately, this patch doesn't seem to be sufficient.  From what I
can tell, if power-save mode is turned on before a station is
associated with an access-point, there is no actual power savings.  If
I issue the command after the station is associated, it works perfectly
fine.

Ajay, does this make sense to you?

Best regards,

  --david

On Sun, 2021-12-12 at 01:18 +0000, David Mosberger-Tang wrote:
> Without this patch, trying to use:
> 
> 	iw dev wlan0 set power_save on
> 
> before the driver is initialized results in an EIO error.  It is more
> useful to simply remember the desired setting and establish it when
> the driver is initialized.
> 
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
>  drivers/net/wireless/microchip/wilc1000/cfg80211.c | 3 ---
>  drivers/net/wireless/microchip/wilc1000/hif.c      | 8 ++++++++
>  drivers/net/wireless/microchip/wilc1000/netdev.c   | 3 ++-
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> index dc4bfe7be378..01d607fa2ded 100644
> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> @@ -1280,9 +1280,6 @@ static int set_power_mgmt(struct wiphy *wiphy, struct net_device *dev,
>  	struct wilc_vif *vif = netdev_priv(dev);
>  	struct wilc_priv *priv = &vif->priv;
>  
> -	if (!priv->hif_drv)
> -		return -EIO;
> -
>  	wilc_set_power_mgmt(vif, enabled, timeout);
>  
>  	return 0;
> diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
> index 29a42bc47017..66fd77c816f7 100644
> --- a/drivers/net/wireless/microchip/wilc1000/hif.c
> +++ b/drivers/net/wireless/microchip/wilc1000/hif.c
> @@ -1934,6 +1934,14 @@ int wilc_set_power_mgmt(struct wilc_vif *vif, bool enabled, u32 timeout)
>  	int result;
>  	s8 power_mode;
>  
> +	if (!wilc->initialized) {
> +		/* Simply remember the desired setting for now; will be
> +		 * established by wilc_init_fw_config().
> +		 */
> +		wilc->power_save_mode = enabled;
> +		return 0;
> +	}
> +
>  	if (enabled)
>  		power_mode = WILC_FW_MIN_FAST_PS;
>  	else
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
> index 4712cd7dff9f..082bed26a981 100644
> --- a/drivers/net/wireless/microchip/wilc1000/netdev.c
> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
> @@ -244,6 +244,7 @@ static int wilc1000_firmware_download(struct net_device *dev)
>  static int wilc_init_fw_config(struct net_device *dev, struct wilc_vif *vif)
>  {
>  	struct wilc_priv *priv = &vif->priv;
> +	struct wilc *wilc = vif->wilc;
>  	struct host_if_drv *hif_drv;
>  	u8 b;
>  	u16 hw;
> @@ -305,7 +306,7 @@ static int wilc_init_fw_config(struct net_device *dev, struct wilc_vif *vif)
>  	if (!wilc_wlan_cfg_set(vif, 0, WID_QOS_ENABLE, &b, 1, 0, 0))
>  		goto fail;
>  
> -	b = WILC_FW_NO_POWERSAVE;
> +	b = wilc->power_save_mode ? WILC_FW_MIN_FAST_PS : WILC_FW_NO_POWERSAVE;
>  	if (!wilc_wlan_cfg_set(vif, 0, WID_POWER_MANAGEMENT, &b, 1, 0, 0))
>  		goto fail;
>