| Message ID | alpine.LFD.2.03.1307251203550.15022@syhkavp.arg (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Lorenzo
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. Are you saying you are willing to give me your ACK if I revert the suggested change? Nicolas
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. Cheers ---Dave > > Are you saying you are willing to give me your ACK if I revert the > suggested change? > > > Nicolas
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. Nicolas
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
On Fri, Jul 26, 2013 at 04:43:34PM +0100, 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. We could add a function cpu_migrate, almost identical to cpu_suspend but accepting an MPIDR parameter as Dave said. cpu_suspend would read its own MPIDR using read_cpuid_mpidr() and pass it to __cpu_suspend, keeping the current behaviour unchanged (well, MPIDR is now read in cpu_suspend instead of __cpu_suspend). cpu_migrate will pass the mpidr to __cpu_suspend, to say "migrate this CPU to the CPU identified by this MPIDR"; IKS can swizzle cpu_logical_map internally. Looks like lot of churn though, probably the intent is clearer and closer to PSCI. Just an idea, probably useless. I will apply and test the patch as it is first thing next week. Lorenzo
On Fri, 26 Jul 2013, Lorenzo Pieralisi wrote: > On Fri, Jul 26, 2013 at 04:43:34PM +0100, 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. > > We could add a function cpu_migrate, almost identical to cpu_suspend > but accepting an MPIDR parameter as Dave said. cpu_suspend would read its > own MPIDR using read_cpuid_mpidr() and pass it to __cpu_suspend, keeping the > current behaviour unchanged (well, MPIDR is now read in cpu_suspend instead > of __cpu_suspend). > cpu_migrate will pass the mpidr to __cpu_suspend, to say "migrate this CPU to > the CPU identified by this MPIDR"; IKS can swizzle cpu_logical_map internally. Sorry but aren't we getting overboard with this? - IKS _has_ to swizzle cpu_logical_map - The suspend path has to get the MPIDR somehow - This patch keeps the change to a minimum - We don't necessarily want to "publish" a formal interface for this trick > Looks like lot of churn though, probably the intent is clearer and closer to > PSCI. Just an idea, probably useless. PSCI has to be invoked at a different level. And frankly we don't need to look for analogies to justify an extra interface which we may as well dispense with. > I will apply and test the patch as it is first thing next week. Thanks. FYI this patch is already in Linaro's kernel and passed all our testing. Nicolas
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)) {