Re: b43_suspend problem

2008-01-22 Thread Michael Buesch 
On Tuesday 22 January 2008 00:36:45 Rafael J. Wysocki wrote:
  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.
[snip]

The patch looks good. Did you try it on b43 hardware?

-- 
Greetings Michael.
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev

Re: b43_suspend problem

2008-01-21 Thread Michael Buesch 
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:

Re: b43_suspend problem

2008-01-21 Thread Rafael J. Wysocki 
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);

Re: b43_suspend problem

2008-01-20 Thread Rafael J. Wysocki 
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

2008-01-20 Thread Michael Buesch 
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

2008-01-20 Thread Rafael J. Wysocki 
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

Re: b43_suspend problem

2008-01-19 Thread Rafael J. Wysocki 
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);
+}
+
 /* Initialize a wireless core */
-static int b43_wireless_core_init(struct b43_wldev *dev)
+static int __b43_wireless_core_init(struct b43_wldev *dev, bool no_suspend)
 {
struct b43_wl *wl = dev-wl;
struct ssb_bus *bus = dev-dev-bus;
@@ -3420,11 +3432,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 (no_suspend)
+   b43_rng_init(wl);
 
b43_set_status(dev, B43_STAT_INITIALIZED);
 
-   b43_leds_init(dev);
+   if (no_suspend)
+   b43_leds_init(dev);
 out:
return err;
 
@@ -3442,6 +3456,11 @@ out:
return err;
 }
 
+static int b43_wireless_core_init(struct b43_wldev *dev)
+{
+   return __b43_wireless_core_init(dev, true);
+}
+
 static int b43_op_add_interface(struct ieee80211_hw *hw,
struct ieee80211_if_init_conf *conf)
 {
@@ -4028,7 +4047,7 @@ static int b43_suspend(struct ssb_device
if (wldev-suspend_init_status = B43_STAT_STARTED)
b43_wireless_core_stop(wldev);
if

Re: b43_suspend problem

2008-01-13 Thread Michael Buesch 
On Sunday 13 January 2008 18:08:57 Alan Stern wrote:
 On Sun, 13 Jan 2008, Rafael J. Wysocki wrote:
 
  On Sunday, 13 of January 2008, Michael Buesch wrote:
   On Sunday 13 January 2008 00:08:29 Rafael J. Wysocki wrote:
There is a problem with b43_suspend() that it (indirectly) causes
b43_leds_exit() to be called, which attempts to unregister the leds 
device
objects, which is forbidden (ie. you can't unregister and/or register 
devices
during a suspend or resume).
   
   Why?
  
  Well, the unregistering itself is not really harmful, provided that the 
  device
  is not registered back during the subsequent resume.
  
  The PM core uses a list of active devices that are added to the list in
  device_add().  The ordering of this list is important, because it is 
  expected
  to reflect the order in which the devices are to be suspended.
  
  This list is manipulated during suspend/resume and devices are moved from it
  and back to it, so unregistering devices during a suspend and registering 
  them
  during the subsequent resume generally changes its ordering and may lead to
  problems during the next suspend/resume cycle.
  
  This is also undesirable if we're going to stop using the freezer for
  suspend/resume at one point in the future.
  
  I'm sure Alan can add some more details.
 
 Indeed.  A system suspend is a delicate operation, and the PM core
 needs to access all the devices in the system.  To have devices being
 registered and unregistered at the same time would cause a lot of
 confusion.  In self defense, the PM core starts out by acquiring all 
 the device semaphores before calling the suspend routines.  This means 
 that if a suspend routine tries to unregister anything, it will 
 deadlock at the point where the driver core tries to acquire the device 
 semaphore prior to invoking the driver's remove method.
 
 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.

-- 
Greetings Michael.
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev

Re: b43_suspend problem

2008-01-12 Thread Rafael J. Wysocki 
On Sunday, 13 of January 2008, Michael Buesch wrote:
 On Sunday 13 January 2008 00:08:29 Rafael J. Wysocki wrote:
  There is a problem with b43_suspend() that it (indirectly) causes
  b43_leds_exit() to be called, which attempts to unregister the leds device
  objects, which is forbidden (ie. you can't unregister and/or register 
  devices
  during a suspend or resume).
 
 Why?

Well, the unregistering itself is not really harmful, provided that the device
is not registered back during the subsequent resume.

The PM core uses a list of active devices that are added to the list in
device_add().  The ordering of this list is important, because it is expected
to reflect the order in which the devices are to be suspended.

This list is manipulated during suspend/resume and devices are moved from it
and back to it, so unregistering devices during a suspend and registering them
during the subsequent resume generally changes its ordering and may lead to
problems during the next suspend/resume cycle.

This is also undesirable if we're going to stop using the freezer for
suspend/resume at one point in the future.

I'm sure Alan can add some more details.

Greetings,
Rafael
___
Bcm43xx-dev mailing list
Bcm43xx-dev@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/bcm43xx-dev