[PATCH 01/13] ARM: suspend: use hash of cpu_logical_map value to index into save array

[Dave Martin]

On Fri, Jul 26, 2013 at 11:43:34AM -0400, Nicolas Pitre wrote:
> On Fri, 26 Jul 2013, Dave Martin wrote:
> 
> > On Fri, Jul 26, 2013 at 10:41:38AM -0400, Nicolas Pitre wrote:
> > > On Fri, 26 Jul 2013, Lorenzo Pieralisi wrote:
> > > 
> > > > On Thu, Jul 25, 2013 at 05:06:51PM +0100, Nicolas Pitre wrote:
> > > > > On Thu, 25 Jul 2013, Dave Martin wrote:
> > > > > 
> > > > > > On Wed, Jul 24, 2013 at 02:55:00PM -0400, Nicolas Pitre wrote:
> > > > > > > On Wed, 24 Jul 2013, Dave Martin wrote:
> > > > > > > 
> > > > > > > > But this patch commits us to requiring that on the suspend path 
> > > > > > > > specifically -- I think that ought to be mentioned somewhere. A 
> > > > > > > > comment in the preamble for __cpu_suspend would be enough, I think.
> > > > > > > 
> > > > > > > What comment would you suggest?  I want to make sure the possible 
> > > > > > > confusion you see is properly addressed.
> > > > > > 
> > > > > > I think we just need to state that the value of
> > > > > > cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical
> > > > > > CPU the suspending logical CPU will resume on.  Consequently, if doing a
> > > > > > migration, cpu_logical_map() must be updated appropriately somewhere
> > > > > > between cpu_pm_enter() and cpu_suspend().
> > > > > 
> > > > > Excellent.  I've amended the patch with this:
> > > > > 
> > > > > diff --git a/arch/arm/kernel/suspend.c b/arch/arm/kernel/suspend.c
> > > > > index 2835d35234..caf938db62 100644
> > > > > --- a/arch/arm/kernel/suspend.c
> > > > > +++ b/arch/arm/kernel/suspend.c
> > > > > @@ -17,6 +17,11 @@ extern void cpu_resume_mmu(void);
> > > > >  /*
> > > > >   * Hide the first two arguments to __cpu_suspend - these are an implementation
> > > > >   * detail which platform code shouldn't have to know about.
> > > > > + *
> > > > > + * On SMP systems, the value of cpu_logical_map(smp_processor_id()) must be
> > > > > + * the MPIDR of the physical CPU the suspending logical CPU will resume on.
> > > > > + * Consequently, if doing a physical CPU migration, cpu_logical_map() must be
> > > > > + * updated appropriately somewhere between cpu_pm_enter() and cpu_suspend().
> > > > >   */
> > > > >  int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
> > > > >  {
> > > > > 
> > > > > I've put it against cpu_suspend() rather than __cpu_suspend(() as this 
> > > > > is what users should care about.
> > > > > 
> > > > > ACK?
> > > > 
> > > > We need this patch to allow IKS to store a cpu context at a specific
> > > > index, let's be honest. It is a moot point and I am not very happy
> > > > about changing this code for a very specific usage, but the way code is
> > > > implemented makes the change acceptable. I really do not think we should
> > > > write guidelines on how cpu_suspend users have to change cpu_logical_map
> > > > though, that's not needed apart from IKS and that should be limited to IKS
> > > > code only.
> > > > 
> > > > Again, that's just my opinion, but cpu_suspend API must be kept as it is
> > > > and we should not encourage people to use it in creative ways.
> > > 
> > > I tend to agree, but I'm now stuck between two conflicting requests.
> > 
> > Would it make sense to keep the same API to cpu_suspend(), but make it
> > a wrapper for another function which has the MPIDR argument?  Then people
> > calling cpu_suspend() continue as normal.  Only IKS needs to know about
> > the underlying MPIDR handling when calling this.
> 
> The fact is that the switcher _does_ need to swizzle the cpu_logical_map 
> anyway before suspending.  Hence really there is no point creating extra
> wrappers for this.
> 
> So Lorenzo's suggestion to simply not advertise this too much for people 
> to not get too creative is probably the best compromize IMHO.

Sure, I don't have a problem with that in light of the discussion.

Cheers
---Dave