[PATCH] Libertas: Fix issues while configuring host sleep

[Bing Zhao]

Hi John,

> -----Original Message-----
> From: John W. Linville [mailto:linville at tuxdriver.com]
> Sent: Friday, November 06, 2009 12:19 PM
> To: Bing Zhao
> Cc: libertas-dev at lists.infradead.org; linux-wireless at vger.kernel.org; Amitkumar Karwar
> Subject: Re: [PATCH] Libertas: Fix issues while configuring host sleep
> 
> On Wed, Nov 04, 2009 at 05:06:35PM -0800, Bing Zhao wrote:
> > From: Amitkumar Karwar <akarwar at marvell.com>
> >
> > Configuration of wake-on-lan for unicast, multicast, broadcast, physical
> > activity was not working. Kernel panic issue was there when user tries to
> > disable WOL. Fixed them.
> >
> > Signed-off-by: Amitkumar Karwar <akarwar at marvell.com>
> > Signed-off-by: Bing Zhao <bzhao at marvell.com>
> > ---
> >  drivers/net/wireless/libertas/ethtool.c |   16 +++++++++++-----
> >  1 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/wireless/libertas/ethtool.c b/drivers/net/wireless/libertas/ethtool.c
> > index 039b555..eeda6d7 100644
> > --- a/drivers/net/wireless/libertas/ethtool.c
> > +++ b/drivers/net/wireless/libertas/ethtool.c
> > @@ -169,16 +169,22 @@ static int lbs_ethtool_set_wol(struct net_device *dev,
> >  	struct lbs_private *priv = dev->ml_priv;
> >  	uint32_t criteria = 0;
> >
> > -	if (priv->wol_criteria == 0xffffffff && wol->wolopts)
> > +	if (priv->wol_criteria != 0xffffffff && wol->wolopts)
> >  		return -EOPNOTSUPP;
> 
> Are you sure about this?  This makes me think that you won't be able
> to change WoL parameters without going through a disable step first.
> Am I misreading?

I'm not sure if this is the best fix or not. But without this change, "ethtool -s wlan0 wol u" would return "Cannot set new wake-on-lan settings: Operation not supported". Yes, you have to disable it first in order to change the WoL parameters.

> 
> >  	if (wol->wolopts & ~(WAKE_UCAST|WAKE_MCAST|WAKE_BCAST|WAKE_PHY))
> >  		return -EOPNOTSUPP;
> >
> > -	if (wol->wolopts & WAKE_UCAST) criteria |= EHS_WAKE_ON_UNICAST_DATA;
> > -	if (wol->wolopts & WAKE_MCAST) criteria |= EHS_WAKE_ON_MULTICAST_DATA;
> > -	if (wol->wolopts & WAKE_BCAST) criteria |= EHS_WAKE_ON_BROADCAST_DATA;
> > -	if (wol->wolopts & WAKE_PHY)   criteria |= EHS_WAKE_ON_MAC_EVENT;
> > +	if (wol->wolopts & WAKE_UCAST)
> > +		criteria |= EHS_WAKE_ON_UNICAST_DATA;
> > +	if (wol->wolopts & WAKE_MCAST)
> > +		criteria |= EHS_WAKE_ON_MULTICAST_DATA;
> > +	if (wol->wolopts & WAKE_BCAST)
> > +		criteria |= EHS_WAKE_ON_BROADCAST_DATA;
> > +	if (wol->wolopts & WAKE_PHY)
> > +		criteria |= EHS_WAKE_ON_MAC_EVENT;
> > +	if (wol->wolopts == 0)
> > +		criteria |= EHS_REMOVE_WAKEUP;
> >
> >  	return lbs_host_sleep_cfg(priv, criteria, (struct wol_config *)NULL);
> >  }
> 
> The reformatting is a distraction.  It would be better to do just
> the fix part separately, especially if you are targetting 2.6.32.

Without the reformatting, the change would be like this:

+	if (wol->wolopts == 0)    criteria |= EHS_REMOVE_WAKEUP;

But the "checkpatch.pl" script gave me an error on that:
"ERROR: trailing statements should be on next line"


Thanks,

Bing

> John
> --
> John W. Linville		Someday the world will need a hero, and you
> linville at tuxdriver.com			might be all we have.  Be ready.