On Wed, 2016-05-11 at 17:44 +1000, Rashmica wrote:
> On 11/05/16 15:03, Balbir Singh wrote:
> > On 11/05/16 14:55, Rashmica Gupta wrote:
> > > The same logic for tm_abort appears twice, so pull it out into a
> > > function.
> > >
> > > diff --git a/arch/powerpc/mm/hash_utils_64.c
> > > b/arch/powerpc/mm/hash_utils_64.c
> > > index 7635b1c6b5da..1cef8f5aee9b 100644
> > > --- a/arch/powerpc/mm/hash_utils_64.c
> > > +++ b/arch/powerpc/mm/hash_utils_64.c
> > > @@ -1318,6 +1318,25 @@ out_exit:
> > > local_irq_restore(flags);
> > > }
> > >
> > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > > + /* Transactions are not aborted by tlbiel, only tlbie.
Can you fix up the comment formatting while you're there, should be:
/*
* Transactions are not aborted by tlbiel, only tlbie.
> > > + * Without, syncing a page back to a block device w/ PIO could pick up
> > > + * transactional data (bad!) so we force an abort here. Before the
> > > + * sync the page will be made read-only, which will flush_hash_page.
> > > + * BIG ISSUE here: if the kernel uses a page from userspace without
> > > + * unmapping it first, it may see the speculated version.
> > > + */
> > > +static inline void abort_tm(int local)
> > > +{
> > > + if (local && cpu_has_feature(CPU_FTR_TM) &&
> > > + current->thread.regs &&
> > > + MSR_TM_ACTIVE(current->thread.regs->msr)) {
> > > + tm_enable();
> > > + tm_abort(TM_CAUSE_TLBI);
> > > + }
> > > +}
> > While your at this do
> >
> > #else
> >
> > static inline void abort_tm(int local)
> > {
> > }
> If I'm doing that, wouldn't it make more sense to write:
>
> +static inline void abort_tm(int local)
It needs a better name. Perhaps tlb_flush_abort_tm() ?
> +{
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> + if (local && cpu_has_feature(CPU_FTR_TM) &&
> + current->thread.regs &&
> + MSR_TM_ACTIVE(current->thread.regs->msr)) {
> + tm_enable();
> + tm_abort(TM_CAUSE_TLBI);
> + }
> +#endif
> +}
In this case it would make some sense.
But the goal is "no #ifdef's in C code", which that would violate.
And if you have two functions that need to be inside the #ifdef then the above
doesn't work anymore.
So the preferred style is:
#ifdef CONFIG_FOO
static void foo(int bar)
{
...
}
#else /* !CONFIG_FOO */
static inline foo(int bar) { }
#endif
And when it expands you end up with:
#ifdef CONFIG_FOO
static void foo(int bar)
{
...
}
static void foo_new(int bar)
{
...
}
#else /* !CONFIG_FOO */
static inline foo(int bar) { }
static inline foo_new(int bar) { }
#endif
cheers
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
