Re: b43_suspend problem
On Sunday, 20 of January 2008, Michael Buesch wrote:
On Sunday 20 January 2008, Rafael J. Wysocki wrote:
On Sunday, 13 of January 2008, Alan Stern wrote:
On Sun, 13 Jan 2008, Michael Buesch wrote:
Besides, if you're going to register the device right back again
during
the subsequent resume, then why go to the trouble of unregistering it
during suspend? Why not just leave it registered the whole time and
avoid all the complication (and excess meaningless uevents)?
Well, because not doing it complicates code :)
Currently suspend/resume calls the same code as init/exit.
The b43 init/exit code is really complicated, compared to other
drivers, due to dozens of hardware versions. So I just avoided
having yet other codepaths for suspend/resume. But I will add
a flag to the device structure that's used to avoid unregistering stuff.
Instead of adding an extra flag you should refactor the code. Have a
common enable subroutine that can be called for both init and resume,
and a common disable subroutine that can be called for both exit and
suspend. Then the method routines themselves will contain only the
portions unique to their particular functions, making them shorter and
simpler.
Well, it doesn't seem to be that easy.
I tried to fix the issue myself and finally obtained the appended,
apparently
working, patch (against 2.6.24-rc8-mm1). Well, it should have been a series
of patches against multiple subsystems, but I thought it would be
instructive
to put everything needed into one bigger patch for starters.
Greetings,
Rafael
---
drivers/base/power/main.c |1
drivers/base/power/power.h |1
drivers/char/hw_random/core.c | 10 -
drivers/char/misc.c | 13
drivers/leds/led-class.c| 13
drivers/net/wireless/b43/leds.c | 17 +--
drivers/net/wireless/b43/leds.h | 14 +++--
drivers/net/wireless/b43/main.c | 43
+---
include/linux/device.h |6 +
include/linux/hw_random.h | 10 -
include/linux/leds.h| 10 -
include/linux/miscdevice.h | 10 -
12 files changed, 111 insertions(+), 37 deletions(-)
Index: linux-2.6.24-rc8-mm1/drivers/net/wireless/b43/main.c
===
--- linux-2.6.24-rc8-mm1.orig/drivers/net/wireless/b43/main.c
+++ linux-2.6.24-rc8-mm1/drivers/net/wireless/b43/main.c
@@ -2470,10 +2470,15 @@ static int b43_rng_read(struct hwrng *rn
return (sizeof(u16));
}
-static void b43_rng_exit(struct b43_wl *wl)
+static void __b43_rng_exit(struct b43_wl *wl, bool suspended)
{
if (wl-rng_initialized)
- hwrng_unregister(wl-rng);
+ __hwrng_unregister(wl-rng, suspended);
+}
+
+static void b43_rng_exit(struct b43_wl *wl)
+{
+ __b43_rng_exit(wl, false);
}
static int b43_rng_init(struct b43_wl *wl)
@@ -3289,7 +3294,7 @@ static void b43_set_retry_limits(struct
/* Shutdown a wireless core */
/* Locking: wl-mutex */
-static void b43_wireless_core_exit(struct b43_wldev *dev)
+static void __b43_wireless_core_exit(struct b43_wldev *dev, bool
no_suspend)
{
struct b43_phy *phy = dev-phy;
@@ -3298,8 +3303,10 @@ static void b43_wireless_core_exit(struc
return;
b43_set_status(dev, B43_STAT_UNINIT);
- b43_leds_exit(dev);
- b43_rng_exit(dev-wl);
+ if (no_suspend) {
+ b43_leds_exit(dev);
+ b43_rng_exit(dev-wl);
+ }
b43_pio_free(dev);
b43_dma_free(dev);
b43_chip_exit(dev);
@@ -3313,8 +3320,13 @@ static void b43_wireless_core_exit(struc
ssb_bus_may_powerdown(dev-dev-bus);
}
+static void b43_wireless_core_exit(struct b43_wldev *dev)
+{
+ __b43_wireless_core_exit(dev, true);
+}
Nah, please don't obfuscate the code.
Better add a flag to struct b43_wldev and check that in the few places
that need different behaviour.
I can do that, if you prefer, but that will look worse, IMHO.
Thanks,
Rafael
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: b43_suspend problem
On Sunday 20 January 2008, Rafael J. Wysocki wrote: Nah, please don't obfuscate the code. Better add a flag to struct b43_wldev and check that in the few places that need different behaviour. I can do that, if you prefer, but that will look worse, IMHO. I'm pretty sure it won't. We had such a flag in the past for firmware. (Fixed that differently now). You simply have do do dev-suspending = 1; at the beginning of suspend/resume and dev-suspending = 0; at the end. The if() checks in the code remain the same. The only thing that this approach won't do it clutter the (already hard to understand) interface up/down API. And that is good. We already have enough special cases for this stuff due to device weirdness. Let's not make it worse. I had a hard time to make a sane API for this (look at bcm43xx to compare to the old crap that doesn't work on lots of devices. ;) ) Thanks for doing this patch. ___ Bcm43xx-dev mailing list Bcm43xx-dev@lists.berlios.de https://lists.berlios.de/mailman/listinfo/bcm43xx-dev
Re: b43_suspend problem
On Sunday, 20 of January 2008, Michael Buesch wrote:
On Sunday 20 January 2008, Rafael J. Wysocki wrote:
Nah, please don't obfuscate the code.
Better add a flag to struct b43_wldev and check that in the few places
that need different behaviour.
I can do that, if you prefer, but that will look worse, IMHO.
I'm pretty sure it won't. We had such a flag in the past for firmware.
(Fixed that differently now).
You simply have do do dev-suspending = 1; at the beginning of suspend/resume
and dev-suspending = 0; at the end. The if() checks in the code remain the
same.
The only thing that this approach won't do it clutter the (already hard
to understand) interface up/down API. And that is good. We already have
enough special cases for this stuff due to device weirdness. Let's not make
it worse.
I had a hard time to make a sane API for this (look at bcm43xx to compare to
the old crap that doesn't work on lots of devices. ;) )
I modified the patch to implement something like this. This still is one big
patch against everything what's necessary. [BTW, in the current version
of the code, b43_resume() may leave wl-mutex locked in the error paths,
which also is fixed by the patch.]
Please have a look.
Thanks for doing this patch.
You're welcome. :-)
Thanks,
Rafael
---
drivers/base/power/main.c |1 +
drivers/base/power/power.h |1 -
drivers/char/hw_random/core.c | 10 +-
drivers/char/misc.c | 13 +
drivers/leds/led-class.c| 13 +
drivers/net/wireless/b43/b43.h |1 +
drivers/net/wireless/b43/leds.c | 17 ++---
drivers/net/wireless/b43/leds.h | 14 --
drivers/net/wireless/b43/main.c | 25 -
include/linux/device.h |6 ++
include/linux/hw_random.h | 10 +-
include/linux/leds.h| 10 +-
include/linux/miscdevice.h | 10 +-
13 files changed, 96 insertions(+), 35 deletions(-)
Index: linux-2.6.24-rc8-mm1/drivers/net/wireless/b43/main.c
===
--- linux-2.6.24-rc8-mm1.orig/drivers/net/wireless/b43/main.c
+++ linux-2.6.24-rc8-mm1/drivers/net/wireless/b43/main.c
@@ -2470,10 +2470,10 @@ static int b43_rng_read(struct hwrng *rn
return (sizeof(u16));
}
-static void b43_rng_exit(struct b43_wl *wl)
+static void b43_rng_exit(struct b43_wl *wl, bool suspended)
{
if (wl-rng_initialized)
- hwrng_unregister(wl-rng);
+ __hwrng_unregister(wl-rng, suspended);
}
static int b43_rng_init(struct b43_wl *wl)
@@ -3298,8 +3298,10 @@ static void b43_wireless_core_exit(struc
return;
b43_set_status(dev, B43_STAT_UNINIT);
- b43_leds_exit(dev);
- b43_rng_exit(dev-wl);
+ if (!dev-suspend_in_progress) {
+ b43_leds_exit(dev);
+ b43_rng_exit(dev-wl, false);
+ }
b43_pio_free(dev);
b43_dma_free(dev);
b43_chip_exit(dev);
@@ -3420,11 +3422,13 @@ static int b43_wireless_core_init(struct
memset(wl-mac_addr, 0, ETH_ALEN);
b43_upload_card_macaddress(dev);
b43_security_init(dev);
- b43_rng_init(wl);
+ if (!dev-suspend_in_progress)
+ b43_rng_init(wl);
b43_set_status(dev, B43_STAT_INITIALIZED);
- b43_leds_init(dev);
+ if (!dev-suspend_in_progress)
+ b43_leds_init(dev);
out:
return err;
@@ -4024,6 +4028,7 @@ static int b43_suspend(struct ssb_device
b43dbg(wl, Suspending...\n);
mutex_lock(wl-mutex);
+ wldev-suspend_in_progress = true;
wldev-suspend_init_status = b43_status(wldev);
if (wldev-suspend_init_status = B43_STAT_STARTED)
b43_wireless_core_stop(wldev);
@@ -4055,15 +4060,17 @@ static int b43_resume(struct ssb_device
if (wldev-suspend_init_status = B43_STAT_STARTED) {
err = b43_wireless_core_start(wldev);
if (err) {
+ b43_leds_resume_exit(wldev);
+ b43_rng_exit(wldev-wl, true);
b43_wireless_core_exit(wldev);
b43err(wl, Resume failed at core start\n);
goto out;
}
}
- mutex_unlock(wl-mutex);
-
b43dbg(wl, Device resumed.\n);
- out:
+ out:
+ wldev-suspend_in_progress = false;
+ mutex_unlock(wl-mutex);
return err;
}
Index: linux-2.6.24-rc8-mm1/drivers/net/wireless/b43/leds.h
===
--- linux-2.6.24-rc8-mm1.orig/drivers/net/wireless/b43/leds.h
+++ linux-2.6.24-rc8-mm1/drivers/net/wireless/b43/leds.h
@@ -43,8 +43,15 @@ enum b43_led_behaviour {
};
void b43_leds_init(struct b43_wldev *dev);
-void b43_leds_exit(struct b43_wldev *dev);
-
+void __b43_leds_exit(struct b43_wldev *dev, bool
