> On July 30, 2019 at 7:11 PM Thiago Jung Bauermann <bauer...@linux.ibm.com>
> wrote:
>
>
>
> Christopher M Riedl <c...@informatik.wtf> writes:
>
> >> On July 30, 2019 at 4:31 PM Thiago Jung Bauermann <bauer...@linux.ibm.com>
> >> wrote:
> >>
> >>
> >>
> >> Christopher M. Riedl <c...@informatik.wtf> writes:
> >>
> >> > Determining if a processor is in shared processor mode is not a constant
> >> > so don't hide it behind a #define.
> >> >
> >> > Signed-off-by: Christopher M. Riedl <c...@informatik.wtf>
> >> > ---
> >> > arch/powerpc/include/asm/spinlock.h | 21 +++++++++++++++------
> >> > 1 file changed, 15 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/include/asm/spinlock.h
> >> > b/arch/powerpc/include/asm/spinlock.h
> >> > index a47f827bc5f1..8631b0b4e109 100644
> >> > --- a/arch/powerpc/include/asm/spinlock.h
> >> > +++ b/arch/powerpc/include/asm/spinlock.h
> >> > @@ -101,15 +101,24 @@ static inline int
> >> > arch_spin_trylock(arch_spinlock_t *lock)
> >> >
> >> > #if defined(CONFIG_PPC_SPLPAR)
> >> > /* We only yield to the hypervisor if we are in shared processor mode */
> >> > -#define SHARED_PROCESSOR (lppaca_shared_proc(local_paca->lppaca_ptr))
> >> > extern void __spin_yield(arch_spinlock_t *lock);
> >> > extern void __rw_yield(arch_rwlock_t *lock);
> >> > #else /* SPLPAR */
> >> > #define __spin_yield(x) barrier()
> >> > #define __rw_yield(x) barrier()
> >> > -#define SHARED_PROCESSOR 0
> >> > #endif
> >> >
> >> > +static inline bool is_shared_processor(void)
> >> > +{
> >> > +/* Only server processors have an lppaca struct */
> >> > +#ifdef CONFIG_PPC_BOOK3S
> >> > + return (IS_ENABLED(CONFIG_PPC_SPLPAR) &&
> >> > + lppaca_shared_proc(local_paca->lppaca_ptr));
> >> > +#else
> >> > + return false;
> >> > +#endif
> >> > +}
> >> > +
> >>
> >> CONFIG_PPC_SPLPAR depends on CONFIG_PPC_PSERIES, which depends on
> >> CONFIG_PPC_BOOK3S so the #ifdef above is unnecessary:
> >>
> >> if CONFIG_PPC_BOOK3S is unset then CONFIG_PPC_SPLPAR will be unset as
> >> well and the return expression should short-circuit to false.
> >>
> >
> > Agreed, but the #ifdef is necessary to compile platforms which include
> > this header but do not implement lppaca_shared_proc(...) and friends.
> > I can reword the comment if that helps.
>
> Ah, indeed. Yes, if you could mention that in the commit I think it
> would help. These #ifdefs are becoming démodé so it's good to know why
> they're there.
>
> Another alternative is to provide a dummy lppaca_shared_proc() which
> always returns false when CONFIG_PPC_BOOK3S isn't set (just mentioning
> it, I don't have a preference).
>
Yeah, I tried that first, but the declaration and definition for
lppaca_shared_proc()
and arguments are nested within several includes and arch/platform #ifdefs that
I
decided the #ifdef in is_shared_processor() is simpler.
I am not sure if unraveling all that makes sense for implementing this fix,
maybe
someone can convince me hah.
In any case, next version will have an improved commit message and comment.
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
