Re: [PATCH v2] Libertas: Added callback functions to support SDIO suspend/resume.

[Sascha Silbe <sascha-ml-linux-kernel-libertas-dev@silbe.org>]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

On Wed, May 12, 2010 at 10:07:55PM -0700, Kiran Divekar wrote:

> In suspend() host sleep is activated using already configured
> host sleep parameters through wol command, and in resume() host
> sleep is cancelled. Earlier priv->fw_ready flag used to reset and
> set in suspend and resume handler respectively. Since after suspend
> only host goes into sleep state and firmware is always ready, those
> changes in flag state are removed.

This patch looks a lot like the one that broke suspend on XO-1. I see 
you've made changes that affect this, but it doesn't seem to be actually 
fixed; see my comment on if_usb_suspend() below.


> diff --git a/drivers/net/wireless/libertas/if_sdio.c 
> b/drivers/net/wireless/libertas/if_sdio.c
> index 64dd345..6b1048b 100644
> --- a/drivers/net/wireless/libertas/if_sdio.c
> +++ b/drivers/net/wireless/libertas/if_sdio.c

[if_sdio_suspend()]
> +	if (card->priv->wol_criteria == 0xffffffff) {
For consistency, the literal value should be replaced with 
EHS_REMOVE_WAKEUP.

> +		lbs_pr_info("Suspend without wake params -- "
> +						"powering down card.");
So with "ethtool eth0 d" an SDIO-connected libertas chip will get 
powered down during suspend? OK.


> diff --git a/drivers/net/wireless/libertas/if_usb.c 
> b/drivers/net/wireless/libertas/if_usb.c
> index fcea574..e7a483c 100644
> --- a/drivers/net/wireless/libertas/if_usb.c
> +++ b/drivers/net/wireless/libertas/if_usb.c

[if_usb_suspend()]
> +	if (priv->wol_criteria == 0xffffffff) {
Again, s/0xffffffff/EHS_REMOVE_WAKEUP/.

> +		lbs_pr_info("Suspend attempt without "
> +						"configuring wake params!\n");
> +		return -ENOSYS;
> +	}
AFAICT this would cause a USB-connected libertas chip with WOL disabled 
("ethtool eth0 d") to prevent suspend, instead of just powering down 
like an SDIO-connected chip. So instead of failing during 
lbs_host_sleep_cfg() because the 8388 doesn't support EHS_REMOVE_WAKEUP, 
you just bail out earlier, still breaking suspend.


> diff --git a/drivers/net/wireless/libertas/main.c 
> b/drivers/net/wireless/libertas/main.c
> index d35ebca..dcbff80 100644
> --- a/drivers/net/wireless/libertas/main.c
> +++ b/drivers/net/wireless/libertas/main.c

[lbs_resume()]
> +	priv->wol_criteria |= EHS_REMOVE_WAKEUP;
This resets an ethtool-set WOL policy on each resume, requiring user 
space to call ethtool before every suspend (not just once during 
startup). I still don't understand why we do that (I'd expect anything 
set by user space to be active until re-set by user space), but I've 
been told it has been this way before (i.e. your patch doesn't change 
the behaviour).


CU Sascha

-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]