Re: b43_suspend problem
On Monday 21 January 2008, Rafael J. Wysocki wrote:
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.]
Whoopsy, thanks for catching this.
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;
}
This part looks OK.
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 suspended);
+static inline void b43_leds_exit(struct b43_wldev *dev)
+{
+ __b43_leds_exit(dev, false);
+}
+static inline void b43_leds_resume_exit(struct b43_wldev *dev)
+{
+ __b43_leds_exit(dev, true);
+}
I still don't like this function wrapping.
I'm pretty sure the additional parameter to the function is not
needed. We can check dev-suspend_in_progress to find out
if we are in a up/down or in a suspend/resume cycle.
-static void b43_unregister_led(struct b43_led *led)
+static void b43_unregister_led(struct b43_led *led, bool suspended)
{
if (!led-dev)
return;
- led_classdev_unregister(led-led_dev);
+ if (suspended)
You can check led-dev-suspend_in_progress here.
+ led_classdev_unregister_suspended(led-led_dev);
+ else
+ led_classdev_unregister(led-led_dev);
b43_led_turn_off(led-dev, led-index, led-activelow);
led-dev = NULL;
}
@@ -230,10 +233,10 @@ 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 suspended)
{
- b43_unregister_led(dev-led_tx);
- b43_unregister_led(dev-led_rx);
- b43_unregister_led(dev-led_assoc);
- b43_unregister_led(dev-led_radio);
+ b43_unregister_led(dev-led_tx, suspended);
+ b43_unregister_led(dev-led_rx, suspended);
+ b43_unregister_led(dev-led_assoc, suspended);
+ b43_unregister_led(dev-led_radio, suspended);
}
Don't need this hunk. Check led-dev-suspend_in_progress in
b43_unregister_led.
#endif /* __KERNEL__ */
#endif /* LINUX_HWRANDOM_H_ */
Index:
[PATCH] b43: Fix firmware caching
We must also store the ID string (filename) for the cached firmware blobs
and verify that we really have the right firmware cached before using it.
If we don't have the right fw cached, we must free it and request the
correct blobs.
This fixes bandswitch on A/B/G multi-PHY devices.
Signed-off-by: Michael Buesch [EMAIL PROTECTED]
---
John, as we don't support A-PHY operation, yet, we don't need to
apply this patch to 2.6.24. Please queue this patch for 2.6.25.
Index: wireless-2.6/drivers/net/wireless/b43/b43.h
===
--- wireless-2.6.orig/drivers/net/wireless/b43/b43.h2008-01-16
23:26:30.0 +0100
+++ wireless-2.6/drivers/net/wireless/b43/b43.h 2008-01-21 18:38:33.0
+0100
@@ -663,22 +663,29 @@ struct b43_wl {
* This beacon stuff is protected by the irq_lock. */
struct sk_buff *current_beacon;
bool beacon0_uploaded;
bool beacon1_uploaded;
};
+/* In-memory representation of a cached microcode file. */
+struct b43_firmware_file {
+ const char *filename;
+ const struct firmware *data;
+};
+
/* Pointers to the firmware data and meta information about it. */
struct b43_firmware {
/* Microcode */
- const struct firmware *ucode;
+ struct b43_firmware_file ucode;
/* PCM code */
- const struct firmware *pcm;
+ struct b43_firmware_file pcm;
/* Initial MMIO values for the firmware */
- const struct firmware *initvals;
+ struct b43_firmware_file initvals;
/* Initial MMIO values for the firmware, band-specific */
- const struct firmware *initvals_band;
+ struct b43_firmware_file initvals_band;
+
/* Firmware revision */
u16 rev;
/* Firmware patchlevel */
u16 patch;
};
Index: wireless-2.6/drivers/net/wireless/b43/main.c
===
--- wireless-2.6.orig/drivers/net/wireless/b43/main.c 2008-01-21
18:28:36.0 +0100
+++ wireless-2.6/drivers/net/wireless/b43/main.c2008-01-21
19:42:15.0 +0100
@@ -1554,22 +1554,25 @@ static irqreturn_t b43_interrupt_handler
mmiowb();
spin_unlock(dev-wl-irq_lock);
return ret;
}
+static void do_release_fw(struct b43_firmware_file *fw)
+{
+ release_firmware(fw-data);
+ fw-data = NULL;
+ fw-filename = NULL;
+}
+
static void b43_release_firmware(struct b43_wldev *dev)
{
- release_firmware(dev-fw.ucode);
- dev-fw.ucode = NULL;
- release_firmware(dev-fw.pcm);
- dev-fw.pcm = NULL;
- release_firmware(dev-fw.initvals);
- dev-fw.initvals = NULL;
- release_firmware(dev-fw.initvals_band);
- dev-fw.initvals_band = NULL;
+ do_release_fw(dev-fw.ucode);
+ do_release_fw(dev-fw.pcm);
+ do_release_fw(dev-fw.initvals);
+ do_release_fw(dev-fw.initvals_band);
}
static void b43_print_fw_helptext(struct b43_wl *wl, bool error)
{
const char *text;
@@ -1581,155 +1584,169 @@ static void b43_print_fw_helptext(struct
else
b43warn(wl, text);
}
static int do_request_fw(struct b43_wldev *dev,
const char *name,
-const struct firmware **fw)
+struct b43_firmware_file *fw)
{
char path[sizeof(modparam_fwpostfix) + 32];
+ const struct firmware *blob;
struct b43_fw_header *hdr;
u32 size;
int err;
- if (!name)
+ if (!name) {
+ /* Don't fetch anything. Free possibly cached firmware. */
+ do_release_fw(fw);
return 0;
+ }
+ if (fw-filename) {
+ if (strcmp(fw-filename, name) == 0)
+ return 0; /* Already have this fw. */
+ /* Free the cached firmware first. */
+ do_release_fw(fw);
+ }
snprintf(path, ARRAY_SIZE(path),
b43%s/%s.fw,
modparam_fwpostfix, name);
- err = request_firmware(fw, path, dev-dev-dev);
+ err = request_firmware(blob, path, dev-dev-dev);
if (err) {
b43err(dev-wl, Firmware file \%s\ not found
or load failed.\n, path);
return err;
}
- if ((*fw)-size sizeof(struct b43_fw_header))
+ if (blob-size sizeof(struct b43_fw_header))
goto err_format;
- hdr = (struct b43_fw_header *)((*fw)-data);
+ hdr = (struct b43_fw_header *)(blob-data);
switch (hdr-type) {
case B43_FW_TYPE_UCODE:
case B43_FW_TYPE_PCM:
size = be32_to_cpu(hdr-size);
- if (size != (*fw)-size - sizeof(struct b43_fw_header))
+ if (size != blob-size - sizeof(struct b43_fw_header))
goto err_format;
/* fallthrough */
case B43_FW_TYPE_IV:
Re: b43_suspend problem
On Monday, 21 of January 2008, Michael Buesch wrote:
On Monday 21 January 2008, Rafael J. Wysocki wrote:
[--snip--]
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 suspended);
+static inline void b43_leds_exit(struct b43_wldev *dev)
+{
+ __b43_leds_exit(dev, false);
+}
+static inline void b43_leds_resume_exit(struct b43_wldev *dev)
+{
+ __b43_leds_exit(dev, true);
+}
I still don't like this function wrapping.
I'm pretty sure the additional parameter to the function is not
needed. We can check dev-suspend_in_progress to find out
if we are in a up/down or in a suspend/resume cycle.
You're right, I overlooked that.
-static void b43_unregister_led(struct b43_led *led)
+static void b43_unregister_led(struct b43_led *led, bool suspended)
{
if (!led-dev)
return;
- led_classdev_unregister(led-led_dev);
+ if (suspended)
You can check led-dev-suspend_in_progress here.
+ led_classdev_unregister_suspended(led-led_dev);
+ else
+ led_classdev_unregister(led-led_dev);
b43_led_turn_off(led-dev, led-index, led-activelow);
led-dev = NULL;
}
@@ -230,10 +233,10 @@ 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 suspended)
{
- b43_unregister_led(dev-led_tx);
- b43_unregister_led(dev-led_rx);
- b43_unregister_led(dev-led_assoc);
- b43_unregister_led(dev-led_radio);
+ b43_unregister_led(dev-led_tx, suspended);
+ b43_unregister_led(dev-led_rx, suspended);
+ b43_unregister_led(dev-led_assoc, suspended);
+ b43_unregister_led(dev-led_radio, suspended);
}
Don't need this hunk. Check led-dev-suspend_in_progress in
b43_unregister_led.
#endif /* __KERNEL__ */
#endif /* LINUX_HWRANDOM_H_ */
Index: linux-2.6.24-rc8-mm1/drivers/net/wireless/b43/b43.h
===
--- linux-2.6.24-rc8-mm1.orig/drivers/net/wireless/b43/b43.h
+++ linux-2.6.24-rc8-mm1/drivers/net/wireless/b43/b43.h
@@ -706,6 +706,7 @@ struct b43_wldev {
bool short_preamble;/* TRUE, if short preamble is enabled. */
bool short_slot;/* TRUE, if short slot timing is enabled. */
bool radio_hw_enable; /* saved state of radio hardware enabled state
*/
+ bool suspend_in_progress;
Please add a comment like: /* TRUE, if we are in a suspend/resume cycle */
Sure, I just didn't know what to write in the comment. :-)
Revised patch below, please have a look.
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 |5 -
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 +-
12 files changed, 78 insertions(+), 27 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);
