Re: [patch 04/15] clocksource: Add module refcount

[John Stultz]

On 04/25/2013 01:31 PM, Thomas Gleixner wrote:
Add a module refcount, so the current clocksource cannot be removed
unconditionally.
Signed-off-by: Thomas Gleixner<t...@linutronix.de>
---
  include/linux/clocksource.h |    3 +++
  kernel/time/timekeeping.c   |   15 ++++++++++-----
  2 files changed, 13 insertions(+), 5 deletions(-)

Index: tip/include/linux/clocksource.h
===================================================================
--- tip.orig/include/linux/clocksource.h
+++ tip/include/linux/clocksource.h
@@ -21,6 +21,7 @@
  /* clocksource cycle base type */
  typedef u64 cycle_t;
  struct clocksource;
+struct module;
  
  #ifdef CONFIG_ARCH_CLOCKSOURCE_DATA
  #include <asm/clocksource.h>
@@ -162,6 +163,7 @@ extern u64 timecounter_cyc2time(struct t
   * @suspend:          suspend function for the clocksource, if necessary
   * @resume:           resume function for the clocksource, if necessary
   * @cycle_last:               most recent cycle counter value seen by ::read()
+ * @owner:             module reference
   */
  struct clocksource {
        /*
@@ -195,6 +197,7 @@ struct clocksource {
        cycle_t cs_last;
        cycle_t wd_last;
  #endif
+       struct module *owner;

Maybe could you add a comment about who is expected to set the owner ptr?



  } ____cacheline_aligned;
  
  /*
Index: tip/kernel/time/timekeeping.c
===================================================================
--- tip.orig/kernel/time/timekeeping.c
+++ tip/kernel/time/timekeeping.c
@@ -627,11 +627,16 @@ static int change_clocksource(void *data
        write_seqcount_begin(&timekeeper_seq);
  
  	timekeeping_forward_now(tk);
-       if (!new->enable || new->enable(new) == 0) {
-               old = tk->clock;
-               tk_setup_internals(tk, new);
-               if (old->disable)
-                       old->disable(old);
+       if (try_module_get(new->owner)) {

Hrm..  So this works, but the interface is a bit non-intuitive, since 
without looking up try_module_get() I'd be surprised that this would 
succeed if the owner ptr is null. Maybe deserves a comment?

+               if (!new->enable || new->enable(new) == 0) {
+                       old = tk->clock;
+                       tk_setup_internals(tk, new);
+                       if (old->disable)
+                               old->disable(old);
+                       module_put(old->owner);
+               } else {
+                       module_put(new->owner);
+               }
        }
        timekeeping_update(tk, true, true);
  

thanks
-john

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/