Re: [RFC][PATCH 0/8] PM: Implement autosleep and "wake locks"

[Rafael J. Wysocki]

On Friday, February 17, 2012, Arve Hjønnevåg wrote:
> 2012/2/15 Rafael J. Wysocki <rjw@xxxxxxx>:
> > On Wednesday, February 15, 2012, Arve Hjønnevåg wrote:
> >> 2012/2/14 Rafael J. Wysocki <rjw@xxxxxxx>:
> >> > On Tuesday, February 14, 2012, Arve Hjønnevåg wrote:
> >> >> On Mon, Feb 6, 2012 at 5:00 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> >> >> ...
> >> >> but the wake-source timeout feature has some bugs or incompatible apis. An
> >> >> init api would also be useful for embedding wake-sources in other data
> >> >> structures without adding another memory allocation. Your patch to
> >> >> move the spinlock init to wakeup_source_add still require the struct
> >> >> to be zero initialized and the name set manually.
> >> >
> >> > That should be easy to fix.  What about the appended patch?
> >> >
> >>
> >> That works, but I still have to call more than one function before I
> >> can use the wakeup-source (wakeup_source_init and wakeup_source_add)
> >> and more than one function before I can free it (__pm_relax,
> >> wakeup_source_remove and wakeup_source_drop). Is there any reason to
> >> keep these separate?
> >
> > Yes, there is.  I think that wakeup_source_create/_destroy() should
> > use the same initialization functions internally that will be used for
> > externally allocated wakeup sources (to make sure that all wakeup source
> > objects are initialized in exactly the same way).
> >
> 
> I agree with that, but is it useful to export these helper functions?

Well, we need to export either them or the ones that will call them internally
and in principle someone may want to do something between _prepare() and _add()
sometimes ...

> >> Also, not copying the name when the caller provides the memory for the
> >> wakeup-source would be a closer match to the wakelock api. Most of our
> >> wakelocks pass a string constant as the name, and making a copy of
> >> that string is not useful. wake_lock_init is also safe to call from
> >> atomic context, but I don't know if anyone relies on this.
> >
> > OK, below is another go.  It doesn't copy the name if wakeup_source_init() is
> > used (which also does the _add this time).  I think, though, that copying
> > the name is generally safer, because someone might use wakeup_source_init()
> > with the name string allocated on the stack or otherwise temporary, which would
> > be a bug with the new version.
> >
> 
> I prefer this version. I have not seen a bug where someone passed a
> temporary as the wakelock name, I assume since this will show up
> immediately in the stats file.

OK

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