drivers/staging/brcm80211: Fix for suspend/resume bug

drivers/staging/brcm80211: Fix for suspend/resume bug

Currently, there are 2 callbacks registered with OS for getting notifications when system goes to suspend/resume.
Racing between these 2 callbacks is the reason for the failure.
With this fix, we avoid registering dhd callback for suspend/resume notification when cfg80211 is used. Relevent functionality in dhd suspend/resume callback function is moved to cfg80211 suspend/resume functions.

Change-Id: Ic29f711eed74d975e6d9558cf93899b85db0678d

BUG=chromium-os-partner:13867, chromium-os-partner:12337
TEST= Manually tested the suspend/resume functionality & Ran 002Suspend test from WiFiRoaming test suite.

Signed-off-by: Sukesh Srikakula <sukeshs@broadcom.com>;
Reviewed-by: 
Tested-by:

[ggg, 2011-04-07 20:28:57]

Just some nits.

In general, use of "volatile" suggests a "home grown" semaphore implementation.
Probably should drop volatile and make use of existing linux kernel semaphores.

http://codereview.chromium.org/6802002/diff/1/drivers/staging/brcm80211/brcmf...
File drivers/staging/brcm80211/brcmfmac/dhd_linux.c (right):

http://codereview.chromium.org/6802002/diff/1/drivers/staging/brcm80211/brcmf...
drivers/staging/brcm80211/brcmfmac/dhd_linux.c:2091: if
(!IS_CFG80211_FAVORITE())
#define FAVORITE_WIFI_CP (!!dhd_cfg80211)
#define IS_CFG80211_FAVORITE() FAVORITE_WIFI_CP

I expect IS_CFG80211_FAVORITE, FAVORITE_WIFI_CP, and dhd_cfg80211 all need to be
removed before this driver can move out of staging-2.6 tree.

http://codereview.chromium.org/6802002/diff/1/drivers/staging/brcm80211/brcmf...
File drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c (right):

http://codereview.chromium.org/6802002/diff/1/drivers/staging/brcm80211/brcmf...
drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c:44: extern volatile bool
dhd_mmc_suspend;
I think dhd_mmc_suspend prototype belongs in brcmfmac/dhd.h.
Then include dhd.h.
bcmsdh_sdmmc.c would benefit from this as well.

[Sam Leffler, 2011-04-07 21:30:07]

What are "whitebox tests"?  Were any of our automated suspend/resume tests run
(that depend on wakealarm)?

[sukeshs, 2011-04-14 01:26:08]

On 2011/04/07 20:28:57, ggg wrote:

Thanks for the comments. 
your first comment regarding the use of IS_CFG80211_FAVORITE, will be taken care
as part of the cleanup which we have targetted before moving to the main kernel
tree.

Second comment regarding dhd_mmc_suspend variable declaration is taken care.

Please find the uploaded cl @ http://codereview.chromium.org/6851008/

[ggg, 2011-04-14 16:38:52]

On 2011/04/14 01:26:08, sukeshs wrote:
> Thanks for the comments. 
> your first comment regarding the use of IS_CFG80211_FAVORITE, will be taken
care
> as part of the cleanup which we have targetted before moving to the main
kernel
> tree.
> 
> Second comment regarding dhd_mmc_suspend variable declaration is taken care.

Hi Sukesh!
Thanks for addressing my comments.

Please respond to Sam's question as well:
| What are "whitebox tests"?

I don't need to know engineering details of the test, just in principle what the
test covers.

> 
> Please find the uploaded cl @ http://codereview.chromium.org/6851008/

Why the new CL number?

You should be able upload to the same CL (6802002) multiple times.
Can you try that please?

thanks!
grant

[Olof Johansson, 2011-04-14 18:01:00]

Hi,

This patch needs some description cleanup before it can be committed, including
a signed-off-by by the original contributor and a proper patch subject line that
specifies subsystem/driver/etc.

Also, this changes code in a driver that is under heavy upstream churn and
cleanup. It must go upstream since we will otherwise forget about it being
needed next time the tree is rebased, and we'll have functional regressions. So,
please upstream.

[sukeshs, 2011-04-14 18:01:08]

> Why the new CL number?

> You should be able upload to the same CL (6802002) multiple times.
> Can you try that please?

Hi Grant,

I deleted the original source code tree on which I created CL 6802002 from my
machine.
So, I created this new cl(6851008) from a newly synced source tree. It looks
like git cl upload has some dependency on which branch it is created to upload
the cl to correct issue no.

Can you please suggest me any other way I can upload this new cl to the older
issue no(6802002)?

Regards,
Sukesh.  


-----Original Message-----
From: grundler@chromium.org [mailto:grundler@chromium.org] 
Sent: Thursday, April 14, 2011 9:39 AM
To: Sukesh Srikakula; grundler@google.com; sleffler@chromium.org
Cc: chromium-os-reviews@chromium.org; vb+kernel@google.com; msb@chromium.org;
olofj@chromium.org
Subject: Re: Fix for suspend/resume bug (issue6802002)

On 2011/04/14 01:26:08, sukeshs wrote:
> Thanks for the comments.
> your first comment regarding the use of IS_CFG80211_FAVORITE, will be  
> taken
care
> as part of the cleanup which we have targetted before moving to the main
kernel
> tree.

> Second comment regarding dhd_mmc_suspend variable declaration is taken  
> care.

Hi Sukesh!
Thanks for addressing my comments.

Please respond to Sam's question as well:
| What are "whitebox tests"?

I don't need to know engineering details of the test, just in principle  
what the
test covers.


> Please find the uploaded cl @ http://codereview.chromium.org/6851008/

Why the new CL number?

You should be able upload to the same CL (6802002) multiple times.
Can you try that please?

thanks!
grant

http://codereview.chromium.org/6802002/

[grundler, 2011-04-14 18:23:06]

Hi Sukesh,

On Thu, Apr 14, 2011 at 11:00 AM, Sukesh Srikakula <sukeshs@broadcom.com> wrote:
...
> I deleted the original source code tree on which I created CL 6802002 from my
machine.
> So, I created this new cl(6851008) from a newly synced source tree. It looks
like git cl upload has some dependency on which branch it is created to upload
the cl to correct issue no.
>
> Can you please suggest me any other way I can upload this new cl to the older
issue no(6802002)?

I think the following two steps will work given you are using the same
branch that you used to upload 6851008:

   git cl issue 6802002  # redirect back to orginal CL
   git cl upload   # update CL6802002 with latest changes


Two more things:
o fix up CL comment as noted by olofj
o respond to "whitebox testing" question by slefller.

Once those two items are addressed I can then push the CL into our tree.

thanks,
grant

[sukeshs, 2011-04-14 19:51:38]

On 2011/04/07 21:30:07, Sam Leffler wrote:
Sorry for not being clear. 
Actually I ran 002Suspend test from WifiRoaming test suite. Now I updated the
issue description correctly.

[sukeshs, 2011-04-14 19:54:40]

On 2011/04/14 18:01:00, Olof Johansson wrote:
Updated the issue description line with module name.

Can you please elaborate on your comment "including a signed-off-by by the
original contributor"?

[ggg, 2011-04-14 21:27:56]

On Thu, Apr 14, 2011 at 12:54 PM,  <sukeshs@broadcom.com> wrote:
> On 2011/04/14 18:01:00, Olof Johansson wrote:
> Updated the issue description line with module name.
>
> Can you please elaborate on your comment "including a signed-off-by by the
> original contributor"?

Olof is asking the comments match the format specified in
Documentation/SubmittingPatches.

Please read this at least once...and again when you plan to submit
patches upstream.

Olof wants something pretty close to this:

    Signed-off-by: Sukesh Srikakula <sukesh@broadcom.com>
    Reviewed-by: Grant Grundler <grundler@chromium.org>
    Tested-by: Grant Grundler <grundler@chromium.org>


cheers,
grant

[Olof Johansson, 2011-04-15 00:34:59]

I had comments on the other CL. Copying them over here:


http://codereview.chromium.org/6851008/diff/1/drivers/staging/brcm80211/brcmf...
File drivers/staging/brcm80211/brcmfmac/dhd.h (right):

http://codereview.chromium.org/6851008/diff/1/drivers/staging/brcm80211/brcmf...
drivers/staging/brcm80211/brcmfmac/dhd.h:42: #endif
No need to ifdef the include.

http://codereview.chromium.org/6851008/diff/1/drivers/staging/brcm80211/brcmf...
drivers/staging/brcm80211/brcmfmac/dhd.h:131: extern volatile bool
dhd_mmc_suspend;
Extern volatile? Huge red flags. Please move to an appropriate header file, and
why is it volatile?

http://codereview.chromium.org/6851008/diff/1/drivers/staging/brcm80211/brcmf...
File drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c (right):

http://codereview.chromium.org/6851008/diff/1/drivers/staging/brcm80211/brcmf...
drivers/staging/brcm80211/brcmfmac/wl_cfg80211.c:1973: #endif	/* 
defined(CONFIG_PM_SLEEP) */
If you move the variable out of ifdef, you can do away with the ifdefs here too
(same for the below).

[Olof Johansson, 2011-04-15 00:36:20]

Also, never ever add someone else name to signed-off/reviewed/tested without
them explicitly offering it.

I have not finished reviewing this, so don't say in your patch description that
I have by adding my name as Reviewed-by. Neither has Grant yet, I believe.

[grundler, 2011-04-15 00:50:56]

On Thu, Apr 14, 2011 at 5:34 PM,  <olofj@chromium.org> wrote:
> I had comments on the other CL. Copying them over here:

thanks olof!

>
http://codereview.chromium.org/6851008/diff/1/drivers/staging/brcm80211/brcmf...
> drivers/staging/brcm80211/brcmfmac/dhd.h:131: extern volatile bool
> dhd_mmc_suspend;
> Extern volatile? Huge red flags. Please move to an appropriate header file,
> and why is it volatile?

Existing code. I've already pointed out this needs to be removed in
previous email and Broadcom indicated it's on the TODO list.

I'd prefer to NOT tie up this CL with another (related issue).

thanks
grant

[grundler, 2011-04-15 00:59:11]

On Thu, Apr 14, 2011 at 5:36 PM,  <olofj@chromium.org> wrote:
> Also, never ever add someone else name to signed-off/reviewed/tested without
> them explicitly offering it.
>
> I have not finished reviewing this, so don't say in your patch description
> that I have by adding my name as Reviewed-by.
> Neither has Grant yet, I believe.

True. I didn't explicitly say that. Please add a "Reviewed-by" line as
I previously posted. I am OK with the changes since they match
existing driver code.

Your comments about removing some of the #ifdef are good. Sukesh
should implement your suggestion or defend his choice as he sees fit.

thanks,
grant