brcmfmac: avoid duplicated suspend/resume operation

PATCH In review Fu, Zhonghui 5 participants 13 messages Feb 12, 2015 – Mar 3, 2015

Subsystems (3)

Networking Network drivers Networking drivers (wireless)

lore

Share & tools

lore

Fu, Zhonghui

author

2015-02-15 02:40 UTC #1 lore raw

Any comments to this patch? Can it be accepted?

Thanks,
Zhonghui

Quoted 46 lines · Zhonghui

From a05d35ab334c20970c236fb971dae88810078c88 Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <zhonghui.fu-VuQAYsv1563Yd54FQh9/[email protected]>
Date: Thu, 12 Feb 2015 10:49:35 +0800
Subject: [PATCH v3] brcmfmac: avoid duplicated suspend/resume operation
WiFi chip has 2 SDIO functions, and PM core will trigger twice suspend/resume operations for one WiFi chip to do the same things. This patch avoid this case.
Acked-by Arend van Spriel <arend-dY08KVG/lbpWk0Htik3J/[email protected]>
Signed-off-by Fu Zhonghui <zhonghui.fu-VuQAYsv1563Yd54FQh9/[email protected]>
---
Changes in v3:
- Rebase to wireless-drivers-next/master branch
 drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +6 -1
index 7944224..b8832a7 100644
@@ -1117,9 +1117,13 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
 	struct brcmf_bus *bus_if;
 	struct brcmf_sdio_dev *sdiodev;
 	mmc_pm_flag_t sdio_flags;
+	struct sdio_func *func = dev_to_sdio_func(dev);
 
 	brcmf_dbg(SDIO, "Enter\n");
 
+	if (func->num == 2)
+		return 0;
+
 	bus_if = dev_get_drvdata(dev);
 	sdiodev = bus_if->bus_priv.sdio;
 
@@ -1148,9 +1152,16 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
 static int brcmf_ops_sdio_resume(struct device *dev)
 {
 	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
-	struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+	struct brcmf_sdio_dev *sdiodev;
+	struct sdio_func *func = dev_to_sdio_func(dev);
 
 	brcmf_dbg(SDIO, "Enter\n");

drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c static int brcmf_ops_sdio_suspend(struct device *dev)

Quoted 10 lines

drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +6 -1

+
+	if (func->num == 2)
+		return 0;
+
+	sdiodev = bus_if->bus_priv.sdio;
+
 	if (sdiodev->pdata && sdiodev->pdata->oob_irq_supported)
 		disable_irq_wake(sdiodev->pdata->oob_irq_nr);
 	brcmf_sdio_wd_timer(sdiodev->bus, BRCMF_WD_POLL_MS);
-- 1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Pat Erley

2015-02-15 03:27 UTC #2 lore raw

Quoted 35 lines · Zhonghui

Any comments to this patch? Can it be accepted?
Thanks,
Zhonghui
Quoted 28 lines · Zhonghui
From a05d35ab334c20970c236fb971dae88810078c88 Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <[email protected]>
Date: Thu, 12 Feb 2015 10:49:35 +0800
Subject: [PATCH v3] brcmfmac: avoid duplicated suspend/resume operation
WiFi chip has 2 SDIO functions, and PM core will trigger twice suspend/resume operations for one WiFi chip to do the same things. This patch avoid this case.
Acked-by Arend van Spriel <[email protected]>
Signed-off-by Fu Zhonghui <[email protected]>
---
Changes in v3:
- Rebase to wireless-drivers-next/master branch
  drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +1 -0
index 7944224..b8832a7 100644
@@ -1117,9 +1117,13 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
  	struct brcmf_bus *bus_if;
  	struct brcmf_sdio_dev *sdiodev;
  	mmc_pm_flag_t sdio_flags;
+	struct sdio_func *func = dev_to_sdio_func(dev);
 

Quoted 5 lines

drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c static int brcmf_ops_sdio_suspend(struct device *dev) · line 1126
Quoted 5 lines
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +3 -0
  	brcmf_dbg(SDIO, "Enter\n");
 
+	if (func->num == 2)
+		return 0;
+

Should it be >= 2 instead of == 2 so that if, in the future, a 3+ SDIO function chip comes out, it's already handled? Not that that should hold up the patch or anything, just a curiosity.

Quoted 29 lines

Quoted 13 lines
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +2 -1
  	bus_if = dev_get_drvdata(dev);
  	sdiodev = bus_if->bus_priv.sdio;
 
@@ -1148,9 +1152,16 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
  static int brcmf_ops_sdio_resume(struct device *dev)
  {
  	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
-	struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+	struct brcmf_sdio_dev *sdiodev;
+	struct sdio_func *func = dev_to_sdio_func(dev);
 
  	brcmf_dbg(SDIO, "Enter\n");
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c static int brcmf_ops_sdio_suspend(struct device *dev)
Quoted 10 lines
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +6 -1
+
+	if (func->num == 2)
+		return 0;
+
+	sdiodev = bus_if->bus_priv.sdio;
+
  	if (sdiodev->pdata && sdiodev->pdata->oob_irq_supported)
  		disable_irq_wake(sdiodev->pdata->oob_irq_nr);
  	brcmf_sdio_wd_timer(sdiodev->bus, BRCMF_WD_POLL_MS);
-- 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Arend van Spriel

2015-02-15 08:31 UTC #3 lore raw

Quoted 3 lines · Pat Erley

Quoted 2 lines · Zhonghui
Any comments to this patch? Can it be accepted?

I assume that patches are queued up until after the merge window that we are currently in.

Quoted 36 lines

Quoted 36 lines
Thanks,
Zhonghui
Quoted 32 lines · Zhonghui
From a05d35ab334c20970c236fb971dae88810078c88 Mon Sep 17 00:00:00 2001
From: Fu Zhonghui <zhonghui.fu-VuQAYsv1563Yd54FQh9/[email protected]>
Date: Thu, 12 Feb 2015 10:49:35 +0800
Subject: [PATCH v3] brcmfmac: avoid duplicated suspend/resume operation
WiFi chip has 2 SDIO functions, and PM core will trigger twice suspend/resume operations for one WiFi chip to do the same things. This patch avoid this case.
Acked-by Arend van Spriel <arend-dY08KVG/lbpWk0Htik3J/[email protected]>
Signed-off-by Fu Zhonghui <zhonghui.fu-VuQAYsv1563Yd54FQh9/[email protected]>
---
Changes in v3:
- Rebase to wireless-drivers-next/master branch
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +1 -0
index 7944224..b8832a7 100644
@@ -1117,9 +1117,13 @@ static int brcmf_ops_sdio_suspend(struct
 device *dev)
 struct brcmf_bus *bus_if;
 struct brcmf_sdio_dev *sdiodev;
 mmc_pm_flag_t sdio_flags;
+ struct sdio_func *func = dev_to_sdio_func(dev);
 
 brcmf_dbg(SDIO, "Enter\n");
 

Quoted 7 lines

Quoted 3 lines
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c static int brcmf_ops_sdio_suspend(struct device *dev) · line 1126
Quoted 3 lines
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +3 -0
+ if (func->num == 2)
+ return 0;
+
Should it be >= 2 instead of == 2 so that if, in the future, a 3+ SDIO function chip comes out, it's already handled? Not that that should hold up the patch or anything, just a curiosity.

The driver only claims functions 1 and 2 during the probe so that assure it works for SDIO devices that have more than two functions.

Regards,
Arend

Quoted 23 lines

drivers/net/wireless/brcm80211/brcmfmac/bcmsdh.c +8 -1

 bus_if = dev_get_drvdata(dev);
 sdiodev = bus_if->bus_priv.sdio;

@@ -1148,9 +1152,16 @@ static int brcmf_ops_sdio_suspend(struct
 device *dev)
 static int brcmf_ops_sdio_resume(struct device *dev)
 {
 struct brcmf_bus *bus_if = dev_get_drvdata(dev);
- struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
+ struct brcmf_sdio_dev *sdiodev;
+ struct sdio_func *func = dev_to_sdio_func(dev);
 
 brcmf_dbg(SDIO, "Enter\n");
+
+ if (func->num == 2)
+ return 0;
+
+ sdiodev = bus_if->bus_priv.sdio;
+
 if (sdiodev->pdata && sdiodev->pdata->oob_irq_supported)
 disable_irq_wake(sdiodev->pdata->oob_irq_nr);

brcmf_sdio_wd_timer(sdiodev->bus, BRCMF_WD_POLL_MS); -- 1.9.1

Quoted 9 lines

Quoted 8 lines
Quoted 1 line
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Kalle Valo

2015-02-15 14:54 UTC #4 lore raw

Quoted 7 lines · Arend van Spriel

Quoted 3 lines · Pat Erley
Quoted 2 lines · Zhonghui
Any comments to this patch? Can it be accepted?
I assume that patches are queued up until after the merge window that we are currently in.

That's right. In the future I will most likely apply patches also during the merge window, but as I'm still a greenhorn I'll be on the safe and wait for the merge window to end.

Signature · 1 line

Kalle Valo

Fu, Zhonghui

author

2015-02-16 07:34 UTC #5 lore raw

Quoted 10 lines · Kalle Valo

Quoted 5 lines · Arend van Spriel
Quoted 2 lines · Pat Erley
Quoted 1 line · Zhonghui
Any comments to this patch? Can it be accepted?
I assume that patches are queued up until after the merge window that we are currently in.
That's right. In the future I will most likely apply patches also during the merge window, but as I'm still a greenhorn I'll be on the safe and wait for the merge window to end.

I am very glad to see this.
Could you please tell which release candidate this patch will be likely merged into now?

Thanks,
Zhonghui

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Arend van Spriel

2015-02-16 09:35 UTC #6 lore raw

Quoted 6 lines · Zhonghui

Quoted 4 lines · Kalle Valo
Quoted 2 lines · Arend van Spriel
Quoted 1 line · Pat Erley
On 02/14/2015 08:40 PM, Fu, Zhonghui wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Any comments to this patch? Can it be accepted?
I assume that patches are queued up until after the merge window that we are currently in.
That's right. In the future I will most likely apply patches also during the merge window, but as I'm still a greenhorn I'll be on the safe and wait for the merge window to end.
I am very glad to see this.
Could you please tell which release candidate this patch will be likely merged into now?

For which tree are you asking this? When the merge window ends and linus' tree has moved to 3.20-rc1, the wireless-drivers-next will move to that -rc1 as well and pending/accepted patches will be applied for the next kernel release. If you are asking when they will be in linus' tree than the answer is 3.21-rc1. Now if you say this patch solves a real problem for you (providing usual proof like log with stack trace) you can request it to go on the wireless-drivers tree to be fixed for 3.20.

Regards,
Arend

Fu, Zhonghui

author

2015-02-27 07:53 UTC #7 lore raw

Quoted 10 lines · Arend van Spriel

Quoted 9 lines · Zhonghui
Quoted 7 lines · Kalle Valo
Quoted 5 lines · Arend van Spriel
Quoted 2 lines · Pat Erley
Quoted 1 line · Zhonghui
Any comments to this patch? Can it be accepted?
I assume that patches are queued up until after the merge window that we are currently in.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
That's right. In the future I will most likely apply patches also during the merge window, but as I'm still a greenhorn I'll be on the safe and wait for the merge window to end.
I am very glad to see this.
Could you please tell which release candidate this patch will be likely merged into now?
For which tree are you asking this? When the merge window ends and linus' tree has moved to 3.20-rc1, the wireless-drivers-next will move to that -rc1 as well and pending/accepted patches will be applied for the next kernel release. If you are asking when they will be in linus' tree than the answer is 3.21-rc1. Now if you say this patch solves a real problem for you (providing usual proof like log with stack trace) you can request it to go on the wireless-drivers tree to be fixed for 3.20.

I was in Chinese new-year vacation these few days. So sorry for late response.

Many thanks for your explanation first.

Now that there is not 3.20 version. My understanding is that this patch will be in linus' tree 4.1-rc1, right?

Thanks,
Zhonghui

Quoted 7 lines

Regards,
Arend

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arend van Spriel

2015-02-27 09:08 UTC #8 lore raw

Quoted 18 lines · Zhonghui

Quoted 16 lines · Arend van Spriel
Quoted 14 lines · Zhonghui
Quoted 10 lines · Kalle Valo
Quoted 5 lines · Arend van Spriel
Quoted 2 lines · Pat Erley
Quoted 1 line · Zhonghui
Any comments to this patch? Can it be accepted?
I assume that patches are queued up until after the merge window that we are currently in.
That's right. In the future I will most likely apply patches also during the merge window, but as I'm still a greenhorn I'll be on the safe and wait for the merge window to end.
I am very glad to see this.
Could you please tell which release candidate this patch will be likely merged into now?

Quoted 7 lines

Quoted 1 line
For which tree are you asking this? When the merge window ends and linus' tree has moved to 3.20-rc1, the wireless-drivers-next will move to that -rc1 as well and pending/accepted patches will be applied for the next kernel release. If you are asking when they will be in linus' tree than the answer is 3.21-rc1. Now if you say this patch solves a real problem for you (providing usual proof like log with stack trace) you can request it to go on the wireless-drivers tree to be fixed for 3.20.
I was in Chinese new-year vacation these few days. So sorry for late response.
Many thanks for your explanation first.
Now that there is not 3.20 version. My understanding is that this patch will be in linus' tree 4.1-rc1, right?

Yes. It will go into linux-next first, which you can consider to be an incubator where all stuff for the next release is integrated. Stuff will be added there until 4.0 is released. At that moment the merge window starts which moves all the stuff from linux-next into the mainline linux repo to prepare 4.1-rc1.

Now regarding your patch I have to give a heads up. Our pending patches have been applied by Kalle and includes similar fix.

Regards,
Arend

Quoted 16 lines

Thanks,
Zhonghui
Quoted 7 lines
Regards,
Arend
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kalle Valo

2015-03-02 15:08 UTC #9 lore raw

Quoted 11 lines · Arend van Spriel

Quoted 2 lines
Now that there is not 3.20 version. My understanding is that this patch will be in linus' tree 4.1-rc1, right?
Yes. It will go into linux-next first, which you can consider to be an incubator where all stuff for the next release is integrated. Stuff will be added there until 4.0 is released. At that moment the merge window starts which moves all the stuff from linux-next into the mainline linux repo to prepare 4.1-rc1.
Now regarding your patch I have to give a heads up. Our pending patches have been applied by Kalle and includes similar fix.

Yeah, Zhonghui's patch doesn't apply anymore. There is similar code in wireless-drivers-next but still a bit different. So what should we do? Is the driver ok now?

Signature · 1 line

Kalle Valo

Arend van Spriel

2015-03-03 19:49 UTC #10 lore raw

Quoted 11 lines · Kalle Valo

Quoted 9 lines · Arend van Spriel
Quoted 2 lines
Now that there is not 3.20 version. My understanding is that this patch will be in linus' tree 4.1-rc1, right?
Yes. It will go into linux-next first, which you can consider to be an incubator where all stuff for the next release is integrated. Stuff will be added there until 4.0 is released. At that moment the merge window starts which moves all the stuff from linux-next into the mainline linux repo to prepare 4.1-rc1.

Quoted 6 lines

Quoted 2 lines
Now regarding your patch I have to give a heads up. Our pending patches have been applied by Kalle and includes similar fix.
Yeah, Zhonghui's patch doesn't apply anymore. There is similar code in wireless-drivers-next but still a bit different. So what should we do? Is the driver ok now?

The idea of Zhonghui's patch was to avoid entering suspend and resume callbacks twice. The current behavior is same as his patch intended so yes the driver is ok (pending undiscovered bugs :-p ).

Regards,
Arend

David Laight

2015-02-16 09:50 UTC #11 lore raw

In reply to Fu, Zhonghui

Quoted 3 lines

Quoted 3 lines
WiFi chip has 2 SDIO functions, and PM core will trigger twice suspend/resume operations for one WiFi chip to do the same things. This patch avoid this case.

Do you want to suspend on the first or last request?

In general it might be that one function is in use and something wants to suspend the other (as inactive).

If they suspend together you might need to pretend the first function is suspended but only do the real power-saving device suspend when all the functions have been suspended.

David

Fu, Zhonghui

author

2015-02-27 07:42 UTC #12 lore raw

Quoted 11 lines · David Laight

Quoted 3 lines
Quoted 3 lines
WiFi chip has 2 SDIO functions, and PM core will trigger twice suspend/resume operations for one WiFi chip to do the same things. This patch avoid this case.
Do you want to suspend on the first or last request?
In general it might be that one function is in use and something wants to suspend the other (as inactive).
If they suspend together you might need to pretend the first function is suspended but only do the real power-saving device suspend when all the functions have been suspended.

I was in Chinese new-year vacation these few days. So sorry for late response.

Suspend/Resume entry functions of brcmfmac driver does not differentiate between two functions, performs the same operations instead.

Thanks,
Zhonghui

Quoted 4 lines

David
N��r��y��b�X��ǧv�^�)޺{.n�+��{��*ޕ�,�{ay�ʇڙ�,j��f��h��z��w��j:+v��w�j�m��zZ+��ݢj"��!tml=