* Re: [PATCH] KVM: PPC: Book3S HV: Context switch IAMR on Power9
From: Russell Currey @ 2019-02-20 11:25 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: paulus, kvm-ppc
In-Reply-To: <20190220085500.29837-1-mpe@ellerman.id.au>
On Wed, 2019-02-20 at 19:55 +1100, Michael Ellerman wrote:
> kvmhv_p9_guest_entry() implements a fast-path guest entry for Power9
> when guest and host are both running with the Radix MMU.
>
> Currently in that path we don't save the host AMR (Authority Mask
> Register) value, and we always restore 0 on return to the host. That
> is OK at the moment because the AMR is not used for storage keys with
> the Radix MMU.
>
> However we plan to start using the AMR on Radix to prevent the kernel
> from reading/writing to userspace outside of copy_to/from_user(). In
> order to make that work we need to save/restore the AMR value.
>
> We only restore the value if it is different from the guest value,
> which is already in the register when we exit to the host. This
> should
> mean we rarely need to actually restore the value when running a
> modern Linux as a guest, because it will be using the same value as
> us.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Tested-by: Russell Currey <ruscur@russell.cc>
^ permalink raw reply
* Re: [PATCH v2 2/3] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm()
From: Russell Currey @ 2019-02-20 11:57 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: mikey, kernel-hardening, npiggin
In-Reply-To: <5ff8b24e-a748-19d3-8651-b626dd676ea4@c-s.fr>
On Fri, 2019-01-25 at 12:45 +0100, Christophe Leroy wrote:
> Hi Russel,
>
> Le 17/12/2018 à 08:09, Christophe Leroy a écrit :
> > Hi Russel,
> >
> > Le 10/12/2018 à 08:00, Russell Currey a écrit :
> > > __patch_instruction() is called in early boot, and uses
> > > __put_user_size(), which includes the locks and unlocks for KUAP,
> > > which could either be called too early, or in the Radix case,
> > > forced to
> > > use "early_" versions of functions just to safely handle this one
> > > case.
> >
> > Looking at x86, I see that __put_user_size() doesn't includes the
> > locks.
> > The lock/unlock is do by callers. I'll do the same.
> >
> >
> > > __put_user_asm() does not do this, and thus is safe to use both
> > > in early
> > > boot, and later on since in this case it should only ever be
> > > touching
> > > kernel memory.
> > >
> > > __patch_instruction() was previously refactored to use
> > > __put_user_size()
> > > in order to be able to return -EFAULT, which would allow the
> > > kernel to
> > > patch instructions in userspace, which should never happen. This
> > > has
> > > the functional change of causing faults on userspace addresses if
> > > KUAP
> > > is turned on, which should never happen in practice.
> > >
> > > A future enhancement could be to double check the patch address
> > > is
> > > definitely allowed to be tampered with by the kernel.
> >
> > This makes me realise that we are calling lock_user_access() with
> > kernel
> > addresses. That most likely breaks protection on kernel addresses
> > for
> > book3s/32. I'll have to work around it.
> >
> > Another thing I realised also is that get_user() at least is called
> > in
> > some exceptions/trap handlers. Which means it can be called nested
> > with
> > an ongoing user access. It means that get_paca()-
> > >user_access_allowed
> > might be modified during those exceptions/traps.
>
> Any comment about that ? Isn't it a problem ?
Yes, I think so. I wonder why I haven't hit this issue, though. Which
handlers is this an issue with?
Maybe we could do something like...
unlock_user_access() checks if user access is already unlocked (== 1),
if so sets user_access_allowed to 2
lock_user_access() sees that user_access_allowed is 2, and knows it's
nested and sets user_access_allowed back to 1 instead of its usual 0.
...that's pretty gross, though. It also means that every
implementation has to figure out how to cope with that.
I've done a lot of testing where a) user access hasn't been left
unlocked and b) faults haven't happened where they shouldn't, so I
wonder how I could try and hit such a case.
Could also have a get_user() without locking that's only allowed to be
used by exception handlers...
I dunno. Open to better ideas.
- Russell
>
> Christophe
>
> > Christophe
> >
> > > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > > ---
> > > arch/powerpc/lib/code-patching.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/powerpc/lib/code-patching.c
> > > b/arch/powerpc/lib/code-patching.c
> > > index 89502cbccb1b..15e8c6339960 100644
> > > --- a/arch/powerpc/lib/code-patching.c
> > > +++ b/arch/powerpc/lib/code-patching.c
> > > @@ -26,9 +26,9 @@
> > > static int __patch_instruction(unsigned int *exec_addr,
> > > unsigned int
> > > instr,
> > > unsigned int *patch_addr)
> > > {
> > > - int err;
> > > + int err = 0;
> > > - __put_user_size(instr, patch_addr, 4, err);
> > > + __put_user_asm(instr, patch_addr, err, "stw");
> > > if (err)
> > > return err;
> > >
^ permalink raw reply
* Re: [PATCH] powerpc: fix 32-bit KVM-PR lockup and panic with MacOS guest
From: Michael Ellerman @ 2019-02-20 12:41 UTC (permalink / raw)
To: Mark Cave-Ayland, Christophe Leroy, benh, paulus, npiggin,
linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <6839dfd8-f316-d98a-2ae9-279de956ce28@ilande.co.uk>
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> On 19/02/2019 04:20, Michael Ellerman wrote:
>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
>>>>> unexpectedly removed the MSR_FE0 and MSR_FE1 bits from the bitmask used to
>>>>> update the MSR of the previous thread in __giveup_fpu() causing a KVM-PR MacOS
>>>>> guest to lockup and panic the kernel.
>>
>> Which kernel is panicking? The guest or the host?
>
> It's the host kernel. As long as you occasionally tap a few keys to keep the screen
> blanking disabled then you can see the panic on the physical console.
Ah crap I assumed you meant the guest kernel.
> I've uploaded a photo I took during the bisection containing the panic when booting
> MacOS X 10.2 under qemu-system-ppc to
> https://www.ilande.co.uk/tmp/qemu/macmini-kvm.jpg in case you find it useful.
OK. That's a host crash, but only because init died (systemd). Though
the reason it died is because we didn't clear FE0/1 properly, so still a
kernel bug.
> Given that it's really easy to recreate, let me know if you want me to do a git
> pull/rebuild and/or if you need any debugging information as it's easy for me to
> reproduce.
I think that's OK. It's reasonably clear what's going on.
>>> 2) Is this the right fix? I'm told that MacOS guests already run without this patch
>>> on a G5 under 64-bit KVM-PR which may suggest that this is a workaround for another
>>> bug elsewhere in the 32-bit powerpc code.
>>
>> That's slightly worrying. It's hard to say without more detail on why
>> the guest is crashing.
>>
>> I think your patch looks OK based just on the fact that it restores the
>> previous behaviour, so I'll pick it up and pass it through my usual
>> testing. If nothing breaks I'll merge it.
>
> That would be great! Does it need a CC to stable too? It would be great if this would
> get picked up in the next set of Debian ports kernels, for example.
I'll add Cc stable.
cheers
^ permalink raw reply
* Re: [RFC PATCH 2/2] powerpc/mm/hash64: Map all the kernel mapping in the same 0xc range
From: Michael Ellerman @ 2019-02-20 12:48 UTC (permalink / raw)
To: Nicholas Piggin, Akshay Adiga, Aneesh Kumar K.V, benh,
Daniel Axtens, paulus
Cc: linuxppc-dev
In-Reply-To: <1550559594.nl4vxs2jg6.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Aneesh Kumar K.V's on February 19, 2019 4:00 pm:
>> On 2/19/19 11:28 AM, Nicholas Piggin wrote:
>>> Aneesh Kumar K.V's on February 17, 2019 3:16 pm:
>>>> This patch maps vmap, IO and vmemap regions in the 0xc address range
>>>> instead of the current 0xd and 0xf range. This brings the mapping closer
>>>> to radix translation mode.
>>>
>>> What was the reason for that address layout in the first place?
>>>
>>
>> To bring in closer to radix translation mode and it should also help
>> with KASAN implementation.
>
> No I mean the old implementation. Your change seems fine, it would just
> be good to know what the downside (if any) is from changing. Maybe it
> was just to make debugging slightly simpler?
I don't know what the reason was for using 0xd to begin with. The
original ppc64 port already had it.
Takes a few seconds to load:
https://github.com/mpe/linux-fullhistory/commit/c3aa9878533e724f639852c3d951e6a169e04081#diff-0adfdefb64f0f0ef306be06ce7f953ebR173
#define PAGE_OFFSET 0xC000000000000000
#define KERNELBASE PAGE_OFFSET
#define VMALLOCBASE 0xD000000000000000
#define IOREGIONBASE 0xE000000000000000
cheers
^ permalink raw reply
* Re: [PATCH v2 00/15] powerpc/32s: Use BATs/LTLBs for STRICT_KERNEL_RWX
From: Michael Ellerman @ 2019-02-20 13:23 UTC (permalink / raw)
To: Christophe Leroy, Jonathan Neuschäfer
Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <8b072209-c845-20f5-8ff1-27ecd67b5c72@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 15/01/2019 à 11:22, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>> Le 15/01/2019 à 01:33, Jonathan Neuschäfer a écrit :
>> ...
>>>>
>>>> - patches 7 to 11 fail to build with this error (really a warning, but
>>>> arch/powerpc doesn't allow warnings by default):
>>>>
>>>> CC arch/powerpc/mm/ppc_mmu_32.o
>>>> ../arch/powerpc/mm/ppc_mmu_32.c:133:13: error: ‘clearibat’ defined but not used [-Werror=unused-function]
>>>> static void clearibat(int index)
>>>> ^~~~~~~~~
>>>> ../arch/powerpc/mm/ppc_mmu_32.c:115:13: error: ‘setibat’ defined but not used [-Werror=unused-function]
>>>> static void setibat(int index, unsigned long virt, phys_addr_t phys,
>>>> ^~~~~~~
>>>> cc1: all warnings being treated as errors
>>>
>>> Argh ! I have to squash the patch bringing the new functions with the
>>> one using them (patch 12). The result is a big messy patch which is more
>>> difficult to review but that's life.
>>
>> You don't *have* to squash them.
>>
>> We like to preserve bisectability, but it's not a 100% hard requirement.
>>
>> Someone trying to bisect through those patches can always turn off
>> -Werror with PPC_DISABLE_WERROR. But they probably can just skip them
>> because they just add new code that's not called yet.
>
> Ok thanks for the note.
>
>>
>> So I won't object if you send them as-is.
>
> Good to know. Anyway I think I will at least re-order so that the patch
> using the new functions immediatly follows the one adding the functions.
Based on that I'm expecting a v3 of this series, right?
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Move page table dump files in a dedicated subdirectory
From: Michael Ellerman @ 2019-02-20 13:37 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <913780421c7b73d731bbbe04c215e29667f7c938.1550492751.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> This patch moves the files related to page table dump in a
> dedicated subdirectory.
>
> The purpose is to clean a bit arch/powerpc/mm by regrouping
> multiple files handling a dedicated function.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/Kconfig.debug | 4 ----
> arch/powerpc/mm/Makefile | 10 +---------
> arch/powerpc/mm/ptdump/Makefile | 9 +++++++++
> arch/powerpc/mm/{ => ptdump}/dump_bats.c | 0
> arch/powerpc/mm/{ => ptdump}/dump_hashpagetable.c | 0
> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-8xx.c | 0
> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-book3s64.c | 0
> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-generic.c | 0
> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables.c | 0
> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables.h | 0
> arch/powerpc/mm/{ => ptdump}/dump_sr.c | 0
> 11 files changed, 10 insertions(+), 13 deletions(-)
> create mode 100644 arch/powerpc/mm/ptdump/Makefile
> rename arch/powerpc/mm/{ => ptdump}/dump_bats.c (100%)
> rename arch/powerpc/mm/{ => ptdump}/dump_hashpagetable.c (100%)
> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-8xx.c (100%)
> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-book3s64.c (100%)
> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-generic.c (100%)
> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables.c (100%)
> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables.h (100%)
> rename arch/powerpc/mm/{ => ptdump}/dump_sr.c (100%)
I'd like to shorten the file names as well, now that they're namespaced
in the ptdump directory, how about:
arch/powerpc/Kconfig.debug | 4 ----
arch/powerpc/mm/Makefile | 10 +---------
arch/powerpc/mm/{dump_linuxpagetables-8xx.c => ptdump/8xx.c} | 2 +-
arch/powerpc/mm/ptdump/Makefile | 9 +++++++++
arch/powerpc/mm/{dump_bats.c => ptdump/bats.c} | 0
arch/powerpc/mm/{dump_linuxpagetables-book3s64.c => ptdump/book3s64.c} | 2 +-
arch/powerpc/mm/{dump_hashpagetable.c => ptdump/hashpagetable.c} | 0
arch/powerpc/mm/{dump_linuxpagetables.c => ptdump/ptdump.c} | 2 +-
arch/powerpc/mm/{dump_linuxpagetables.h => ptdump/ptdump.h} | 0
arch/powerpc/mm/{dump_sr.c => ptdump/segment_regs.c} | 0
arch/powerpc/mm/{dump_linuxpagetables-generic.c => ptdump/shared.c} | 2 +-
cheers
^ permalink raw reply
* Re: [PATCH v3 1/7] dump_stack: Support adding to the dump stack arch description
From: Andrea Parri @ 2019-02-20 13:44 UTC (permalink / raw)
To: Michael Ellerman
Cc: linux-arch, Petr Mladek, sergey.senozhatsky, linux-kernel,
Steven Rostedt, linuxppc-dev, tj, akpm, dyoung
In-Reply-To: <87va1e7pw2.fsf@concordia.ellerman.id.au>
> >> > > + * Order the stores above in vsnprintf() vs the store of the
> >> > > + * space below which joins the two strings. Note this doesn't
> >> > > + * make the code truly race free because there is no barrier on
> >> > > + * the read side. ie. Another CPU might load the uninitialised
> >> > > + * tail of the buffer first and then the space below (rather
> >> > > + * than the NULL that was there previously), and so print the
> >> > > + * uninitialised tail. But the whole string lives in BSS so in
> >> > > + * practice it should just see NULLs.
> >> >
> >> > The comment doesn't say _why_ we need to order these stores: IOW, what
> >> > will or can go wrong without this order? This isn't clear to me.
> >> >
> >> > Another good practice when adding smp_*-constructs (as discussed, e.g.,
> >> > at KS'18) is to indicate the matching construct/synch. mechanism.
> >>
> >> Yes, one barrier without a counter-part is suspicious.
> >
> > As is this silence...,
> >
> > Michael, what happened to this patch? did you submit a new version?
>
> No, I'm just busy, it's the merge window next week :)
Got it.
>
> I thought the comment was pretty clear, if the stores are observed out
> of order we might print the uninitialised tail.
>
> And the barrier on the read side would need to be in printk somewhere,
> which is obviously unpleasant.
Indeed.
>
> >> If the parallel access is really needed then we could define the
> >> current length as atomic_t and use:
> >>
> >> + atomic_cmpxchg() to reserve the space for the string
> >> + %*s to limit the printed length
> >>
> >> In the worst case, we would print an incomplete string.
> >> See below for a sample code.
> >
> > Seems worth exploring, IMO; but I'd like to first hear _clear about
> > the _intended semantics (before digging into alternatives)...
>
> It is not my intention to support concurrent updates of the string. The
> idea is you setup the string early in boot.
Understood, thanks for the clarification.
>
> The concern with a concurrent reader is simply that the string is dumped
> in the panic path, and you never really know when you're going to panic.
> Even if you only write to the string before doing SMP bringup you might
> still have another CPU go rogue and panic before then.
>
> But I probably should have just not added the barrier, it's over
> paranoid and will almost certainly never matter in practice.
Oh, well, I can only echo you: if you don't care about the stores being
_observed_ out of order, you could simply remove the barrier; if you do
care, then you need "more paranoid" on the readers side. ;-)
Andrea
>
> cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Move page table dump files in a dedicated subdirectory
From: Christophe Leroy @ 2019-02-20 14:00 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87d0nm7f8l.fsf@concordia.ellerman.id.au>
Le 20/02/2019 à 14:37, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>
>> This patch moves the files related to page table dump in a
>> dedicated subdirectory.
>>
>> The purpose is to clean a bit arch/powerpc/mm by regrouping
>> multiple files handling a dedicated function.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/Kconfig.debug | 4 ----
>> arch/powerpc/mm/Makefile | 10 +---------
>> arch/powerpc/mm/ptdump/Makefile | 9 +++++++++
>> arch/powerpc/mm/{ => ptdump}/dump_bats.c | 0
>> arch/powerpc/mm/{ => ptdump}/dump_hashpagetable.c | 0
>> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-8xx.c | 0
>> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-book3s64.c | 0
>> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-generic.c | 0
>> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables.c | 0
>> arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables.h | 0
>> arch/powerpc/mm/{ => ptdump}/dump_sr.c | 0
>> 11 files changed, 10 insertions(+), 13 deletions(-)
>> create mode 100644 arch/powerpc/mm/ptdump/Makefile
>> rename arch/powerpc/mm/{ => ptdump}/dump_bats.c (100%)
>> rename arch/powerpc/mm/{ => ptdump}/dump_hashpagetable.c (100%)
>> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-8xx.c (100%)
>> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-book3s64.c (100%)
>> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables-generic.c (100%)
>> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables.c (100%)
>> rename arch/powerpc/mm/{ => ptdump}/dump_linuxpagetables.h (100%)
>> rename arch/powerpc/mm/{ => ptdump}/dump_sr.c (100%)
>
> I'd like to shorten the file names as well, now that they're namespaced
> in the ptdump directory, how about:
>
> arch/powerpc/Kconfig.debug | 4 ----
> arch/powerpc/mm/Makefile | 10 +---------
> arch/powerpc/mm/{dump_linuxpagetables-8xx.c => ptdump/8xx.c} | 2 +-
> arch/powerpc/mm/ptdump/Makefile | 9 +++++++++
> arch/powerpc/mm/{dump_bats.c => ptdump/bats.c} | 0
> arch/powerpc/mm/{dump_linuxpagetables-book3s64.c => ptdump/book3s64.c} | 2 +-
> arch/powerpc/mm/{dump_hashpagetable.c => ptdump/hashpagetable.c} | 0
> arch/powerpc/mm/{dump_linuxpagetables.c => ptdump/ptdump.c} | 2 +-
> arch/powerpc/mm/{dump_linuxpagetables.h => ptdump/ptdump.h} | 0
> arch/powerpc/mm/{dump_sr.c => ptdump/segment_regs.c} | 0
> arch/powerpc/mm/{dump_linuxpagetables-generic.c => ptdump/shared.c} | 2 +-
Yes good idea. Do you want a v2 or will you do it ?
Christophe
>
>
> cheers
>
^ permalink raw reply
* Re: use generic DMA mapping code in powerpc V7
From: Christoph Hellwig @ 2019-02-20 14:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Olof Johansson
Cc: iommu, linuxppc-dev, linux-kernel
In-Reply-To: <20190213070133.11259-1-hch@lst.de>
Hi powerpc maintainers,
what is the plan for this serries? I've not received any comments,
and although the series briefly appeared in the powerpc next-test
branch it disappeared again..
^ permalink raw reply
* Re: [PATCH 1/4] powerpc/64s: Fix HV NMI vs HV interrupt recoverability test
From: Nicholas Piggin @ 2019-02-20 14:31 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20190122064618.1510-2-npiggin@gmail.com>
Nicholas Piggin's on January 22, 2019 4:46 pm:
> HV interrupts that use HSRR registers do not clear MSR[RI], but
> NMI entry code is not recoverable early on due to both using HSPRG
> for a scratch register.
>
> This bug means that a system reset or machine check can cause silent
> data corruption (due to loss of r13 register) if it hits in a small
> window when taking an HV interrupt.
>
> Fix this by marking NMIs non-recoverable if they land in HV interrupt
> ranges.
Hum, I had a v2 that I didn't send properly with a small compile fix,
but I've also just noticed this:
> +void hv_nmi_check_nonrecoverable(struct pt_regs *regs)
> +{
> +#ifdef CONFIG_POWERNV
> + unsigned long kbase = (unsigned long)_stext;
> + unsigned long nip = regs->nip;
> +
> + if (!(regs->msr & MSR_RI))
> + return;
> + if (!(regs->msr & MSR_HV))
> + return;
> + if (regs->msr & MSR_PR)
> + return;
> +again:
> + if (nip >= 0x500 && nip < 0x600)
> + goto nonrecoverable;
> + if (nip >= 0x980 && nip < 0xa00)
> + goto nonrecoverable;
> + if (nip >= 0xe00 && nip < 0xec0)
> + goto nonrecoverable;
> + if (nip >= 0xf80 && nip < 0xfa0)
> + goto nonrecoverable;
> + /* Trampolines are not relocated. */
> + if (nip >= real_trampolines_start - kbase &&
> + nip < real_trampolines_end - kbase)
> + goto nonrecoverable;
> + if (nip >= virt_trampolines_start - kbase &&
> + nip < virt_trampolines_end - kbase)
> + goto nonrecoverable;
> + if (nip >= 0xc000000000000000ULL) {
> + nip -= 0xc000000000000000ULL;
> + goto again;
Tried to be a bit too clever here. The 0xc... vectors also have a
+0x4000 offset so this won't catch them properly. I'll respin.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v4 1/3] locking/rwsem: Remove arch specific rwsem files
From: Will Deacon @ 2019-02-20 14:47 UTC (permalink / raw)
To: Waiman Long
Cc: linux-ia64, linux-sh, Peter Zijlstra, linux-mips, H. Peter Anvin,
sparclinux, linux-riscv, linux-arch, linux-s390, Davidlohr Bueso,
linux-c6x-dev, linux-hexagon, x86, Ingo Molnar, uclinux-h8-devel,
linux-xtensa, Arnd Bergmann, linux-um, linux-m68k, openrisc,
Borislav Petkov, Thomas Gleixner, linux-arm-kernel, Tim Chen,
linux-parisc, Linus Torvalds, linux-kernel, linux-alpha,
nios2-dev, Andrew Morton, linuxppc-dev
In-Reply-To: <1550095217-12047-2-git-send-email-longman@redhat.com>
On Wed, Feb 13, 2019 at 05:00:15PM -0500, Waiman Long wrote:
> As the generic rwsem-xadd code is using the appropriate acquire and
> release versions of the atomic operations, the arch specific rwsem.h
> files will not be that much faster than the generic code as long as the
> atomic functions are properly implemented. So we can remove those arch
> specific rwsem.h and stop building asm/rwsem.h to reduce maintenance
> effort.
>
> Currently, only x86, alpha and ia64 have implemented architecture
> specific fast paths. I don't have access to alpha and ia64 systems for
> testing, but they are legacy systems that are not likely to be updated
> to the latest kernel anyway.
>
> By using a rwsem microbenchmark, the total locking rates on a 4-socket
> 56-core 112-thread x86-64 system before and after the patch were as
> follows (mixed means equal # of read and write locks):
>
> Before Patch After Patch
> # of Threads wlock rlock mixed wlock rlock mixed
> ------------ ----- ----- ----- ----- ----- -----
> 1 29,201 30,143 29,458 28,615 30,172 29,201
> 2 6,807 13,299 1,171 7,725 15,025 1,804
> 4 6,504 12,755 1,520 7,127 14,286 1,345
> 8 6,762 13,412 764 6,826 13,652 726
> 16 6,693 15,408 662 6,599 15,938 626
> 32 6,145 15,286 496 5,549 15,487 511
> 64 5,812 15,495 60 5,858 15,572 60
>
> There were some run-to-run variations for the multi-thread tests. For
> x86-64, using the generic C code fast path seems to be a little bit
> faster than the assembly version with low lock contention. Looking at
> the assembly version of the fast paths, there are assembly to/from C
> code wrappers that save and restore all the callee-clobbered registers
> (7 registers on x86-64). The assembly generated from the generic C
> code doesn't need to do that. That may explain the slight performance
> gain here.
>
> The generic asm rwsem.h can also be merged into kernel/locking/rwsem.h
> with no code change as no other code other than those under
> kernel/locking needs to access the internal rwsem macros and functions.
>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> MAINTAINERS | 1 -
> arch/alpha/include/asm/rwsem.h | 211 -----------------------------------
> arch/arm/include/asm/Kbuild | 1 -
> arch/arm64/include/asm/Kbuild | 1 -
> arch/hexagon/include/asm/Kbuild | 1 -
> arch/ia64/include/asm/rwsem.h | 172 -----------------------------
> arch/powerpc/include/asm/Kbuild | 1 -
> arch/s390/include/asm/Kbuild | 1 -
> arch/sh/include/asm/Kbuild | 1 -
> arch/sparc/include/asm/Kbuild | 1 -
> arch/x86/include/asm/rwsem.h | 237 ----------------------------------------
> arch/x86/lib/Makefile | 1 -
> arch/x86/lib/rwsem.S | 156 --------------------------
> arch/x86/um/Makefile | 1 -
> arch/xtensa/include/asm/Kbuild | 1 -
> include/asm-generic/rwsem.h | 140 ------------------------
> include/linux/rwsem.h | 4 +-
> kernel/locking/percpu-rwsem.c | 2 +
> kernel/locking/rwsem.h | 130 ++++++++++++++++++++++
> 19 files changed, 133 insertions(+), 930 deletions(-)
> delete mode 100644 arch/alpha/include/asm/rwsem.h
> delete mode 100644 arch/ia64/include/asm/rwsem.h
> delete mode 100644 arch/x86/include/asm/rwsem.h
> delete mode 100644 arch/x86/lib/rwsem.S
> delete mode 100644 include/asm-generic/rwsem.h
Looks like a nice cleanup, thanks:
Acked-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply
* Re: [PATCH v4 2/3] locking/rwsem: Remove rwsem-spinlock.c & use rwsem-xadd.c for all archs
From: Will Deacon @ 2019-02-20 14:47 UTC (permalink / raw)
To: Waiman Long
Cc: linux-ia64, linux-sh, Peter Zijlstra, linux-mips, H. Peter Anvin,
sparclinux, linux-riscv, linux-arch, linux-s390, Davidlohr Bueso,
linux-c6x-dev, linux-hexagon, x86, Ingo Molnar, uclinux-h8-devel,
linux-xtensa, Arnd Bergmann, linux-um, linux-m68k, openrisc,
Borislav Petkov, Thomas Gleixner, linux-arm-kernel, Tim Chen,
linux-parisc, Linus Torvalds, linux-kernel, linux-alpha,
nios2-dev, Andrew Morton, linuxppc-dev
In-Reply-To: <1550095217-12047-3-git-send-email-longman@redhat.com>
On Wed, Feb 13, 2019 at 05:00:16PM -0500, Waiman Long wrote:
> Currently, we have two different implementation of rwsem:
> 1) CONFIG_RWSEM_GENERIC_SPINLOCK (rwsem-spinlock.c)
> 2) CONFIG_RWSEM_XCHGADD_ALGORITHM (rwsem-xadd.c)
>
> As we are going to use a single generic implementation for rwsem-xadd.c
> and no architecture-specific code will be needed, there is no point
> in keeping two different implementations of rwsem. In most cases, the
> performance of rwsem-spinlock.c will be worse. It also doesn't get all
> the performance tuning and optimizations that had been implemented in
> rwsem-xadd.c over the years.
>
> For simplication, we are going to remove rwsem-spinlock.c and make all
> architectures use a single implementation of rwsem - rwsem-xadd.c.
>
> All references to RWSEM_GENERIC_SPINLOCK and RWSEM_XCHGADD_ALGORITHM
> in the code are removed.
>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
> arch/alpha/Kconfig | 7 -
> arch/arc/Kconfig | 3 -
> arch/arm/Kconfig | 4 -
> arch/arm64/Kconfig | 3 -
> arch/c6x/Kconfig | 3 -
> arch/csky/Kconfig | 3 -
> arch/h8300/Kconfig | 3 -
> arch/hexagon/Kconfig | 6 -
> arch/ia64/Kconfig | 4 -
> arch/m68k/Kconfig | 7 -
> arch/microblaze/Kconfig | 6 -
> arch/mips/Kconfig | 7 -
> arch/nds32/Kconfig | 3 -
> arch/nios2/Kconfig | 3 -
> arch/openrisc/Kconfig | 6 -
> arch/parisc/Kconfig | 6 -
> arch/powerpc/Kconfig | 7 -
> arch/riscv/Kconfig | 3 -
> arch/s390/Kconfig | 6 -
> arch/sh/Kconfig | 6 -
> arch/sparc/Kconfig | 8 -
> arch/unicore32/Kconfig | 6 -
> arch/x86/Kconfig | 3 -
> arch/x86/um/Kconfig | 6 -
> arch/xtensa/Kconfig | 3 -
> include/linux/rwsem-spinlock.h | 47 ------
> include/linux/rwsem.h | 5 -
> kernel/Kconfig.locks | 2 +-
> kernel/locking/Makefile | 4 +-
> kernel/locking/rwsem-spinlock.c | 339 ----------------------------------------
> kernel/locking/rwsem.h | 3 -
> 31 files changed, 2 insertions(+), 520 deletions(-)
> delete mode 100644 include/linux/rwsem-spinlock.h
> delete mode 100644 kernel/locking/rwsem-spinlock.c
Another nice cleanup, and it looks like the optimistic spinning in the xadd
implementation is predicated on ARCH_SUPPORTS_ATOMIC_RMW, so we're all good
there too.
Acked-by: Will Deacon <will.deacon@arm.com>
Will
^ permalink raw reply
* Re: [PATCH] powerpc/64s: Fix possible corruption on big endian due to pgd/pud_present()
From: Segher Boessenkool @ 2019-02-20 14:51 UTC (permalink / raw)
To: Michael Ellerman
Cc: erhard_f, jack, linuxppc-dev, linux-kernel, linux-mm,
aneesh.kumar
In-Reply-To: <87sgwi7lo1.fsf@concordia.ellerman.id.au>
On Wed, Feb 20, 2019 at 10:18:38PM +1100, Michael Ellerman wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Mon, Feb 18, 2019 at 11:49:18AM +1100, Michael Ellerman wrote:
> >> Balbir Singh <bsingharora@gmail.com> writes:
> >> > Fair enough, my point was that the compiler can help out. I'll see what
> >> > -Wconversion finds on my local build :)
> >>
> >> I get about 43MB of warnings here :)
> >
> > Yes, -Wconversion complains about a lot of things that are idiomatic C.
> > There is a reason -Wconversion is not in -Wall or -Wextra.
>
> Actually a lot of those go away when I add -Wno-sign-conversion.
>
> And what's left seems mostly reasonable, they all indicate the
> possibility of a bug I think.
>
> In fact this works and would have caught the bug:
>
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index d8c8d7c9df15..3114e3f368e2 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -904,7 +904,12 @@ static inline int pud_none(pud_t pud)
>
> static inline int pud_present(pud_t pud)
> {
> + __diag_push();
> + __diag_warn(GCC, 8, "-Wconversion", "ulong -> int");
> +
> return !!(pud_raw(pud) & cpu_to_be64(_PAGE_PRESENT));
> +
> + __diag_pop();
> }
>
> extern struct page *pud_page(pud_t pud);
>
>
>
> Obviously we're not going to instrument every function like that. But we
> could start instrumenting particular files.
So you want to instrument the functions that you know are buggy, using some
weird incantations to catch only those errors you already know about?
(I am worried this does not scale, in many dimensions).
Segher
^ permalink raw reply
* Re: [RESEND PATCH 0/7] Add FOLL_LONGTERM to GUP fast and use it
From: Christoph Hellwig @ 2019-02-20 15:19 UTC (permalink / raw)
To: ira.weiny
Cc: linux-fbdev, Michal Hocko, kvm, linux-sh, Peter Zijlstra,
James Hogan, linux-fpga, Heiko Carstens, dri-devel, linux-mips,
linux-mm, Rich Felker, Paul Mackerras, sparclinux, ceph-devel,
devel, linux-s390, rds-devel, Yoshinori Sato, linux-rdma,
Jason Gunthorpe, Ingo Molnar, xen-devel, devel, linux-media,
John Hubbard, kvm-ppc, Borislav Petkov, Thomas Gleixner,
virtualization, linux-scsi, netdev, linux-kernel, Ralf Baechle,
Paul Burton, Martin Schwidefsky, Andrew Morton, linuxppc-dev,
David S. Miller, Kirill A. Shutemov
In-Reply-To: <20190220053040.10831-1-ira.weiny@intel.com>
On Tue, Feb 19, 2019 at 09:30:33PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
>
> Resending these as I had only 1 minor comment which I believe we have covered
> in this series. I was anticipating these going through the mm tree as they
> depend on a cleanup patch there and the IB changes are very minor. But they
> could just as well go through the IB tree.
>
> NOTE: This series depends on my clean up patch to remove the write parameter
> from gup_fast_permitted()[1]
>
> HFI1, qib, and mthca, use get_user_pages_fast() due to it performance
> advantages. These pages can be held for a significant time. But
> get_user_pages_fast() does not protect against mapping of FS DAX pages.
This I don't get - if you do lock down long term mappings performance
of the actual get_user_pages call shouldn't matter to start with.
What do I miss?
^ permalink raw reply
* Re: [PATCH v2 00/15] powerpc/32s: Use BATs/LTLBs for STRICT_KERNEL_RWX
From: Christophe Leroy @ 2019-02-20 15:30 UTC (permalink / raw)
To: Michael Ellerman, Jonathan Neuschäfer
Cc: linuxppc-dev, Paul Mackerras, linux-kernel
In-Reply-To: <87ftsi7fwi.fsf@concordia.ellerman.id.au>
Le 20/02/2019 à 14:23, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>
>> Le 15/01/2019 à 11:22, Michael Ellerman a écrit :
>>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>>> Le 15/01/2019 à 01:33, Jonathan Neuschäfer a écrit :
>>> ...
>>>>>
>>>>> - patches 7 to 11 fail to build with this error (really a warning, but
>>>>> arch/powerpc doesn't allow warnings by default):
>>>>>
>>>>> CC arch/powerpc/mm/ppc_mmu_32.o
>>>>> ../arch/powerpc/mm/ppc_mmu_32.c:133:13: error: ‘clearibat’ defined but not used [-Werror=unused-function]
>>>>> static void clearibat(int index)
>>>>> ^~~~~~~~~
>>>>> ../arch/powerpc/mm/ppc_mmu_32.c:115:13: error: ‘setibat’ defined but not used [-Werror=unused-function]
>>>>> static void setibat(int index, unsigned long virt, phys_addr_t phys,
>>>>> ^~~~~~~
>>>>> cc1: all warnings being treated as errors
>>>>
>>>> Argh ! I have to squash the patch bringing the new functions with the
>>>> one using them (patch 12). The result is a big messy patch which is more
>>>> difficult to review but that's life.
>>>
>>> You don't *have* to squash them.
>>>
>>> We like to preserve bisectability, but it's not a 100% hard requirement.
>>>
>>> Someone trying to bisect through those patches can always turn off
>>> -Werror with PPC_DISABLE_WERROR. But they probably can just skip them
>>> because they just add new code that's not called yet.
>>
>> Ok thanks for the note.
>>
>>>
>>> So I won't object if you send them as-is.
>>
>> Good to know. Anyway I think I will at least re-order so that the patch
>> using the new functions immediatly follows the one adding the functions.
>
> Based on that I'm expecting a v3 of this series, right?
Right. Coming soon.
Christophe
^ permalink raw reply
* [PATCH] sched/topology: fix kernel crash when a CPU is hotplugged in a memoryless node
From: Laurent Vivier @ 2019-02-20 16:37 UTC (permalink / raw)
To: linux-kernel
Cc: Laurent Vivier, Srikar Dronamraju, Michael Bringmann,
Suravee Suthikulpanit, Nathan Fontenot, Borislav Petkov,
linuxppc-dev, David Gibson
When we hotplug a CPU in a memoryless/cpuless node, the kernel crashes when it
rebuilds the sched_domains data.
I reproduce this problem on POWER and with a pseries VM, with the following
QEMU parameters:
-machine pseries -enable-kvm -m 8192 \
-smp 2,maxcpus=8,sockets=4,cores=2,threads=1 \
-numa node,nodeid=0,cpus=0-1,mem=0 \
-numa node,nodeid=1,cpus=2-3,mem=8192 \
-numa node,nodeid=2,cpus=4-5,mem=0 \
-numa node,nodeid=3,cpus=6-7,mem=0
Then I can trigger the crash by hotplugging a CPU on node-id 3:
(qemu) device_add host-spapr-cpu-core,core-id=7,node-id=3
Built 2 zonelists, mobility grouping on. Total pages: 130162
Policy zone: Normal
WARNING: workqueue cpumask: online intersect > possible intersect
BUG: Kernel NULL pointer dereference at 0x00000400
Faulting instruction address: 0xc000000000170edc
Oops: Kernel access of bad area, sig: 11 [#1]
LE SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: ip6t_rpfilter ipt_REJECT nf_reject_ipv4 ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc ip6table_nat nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw iptable_nat nf_nat_ipv4 nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 iptable_mangle iptable_security iptable_raw ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter xts vmx_crypto ip_tables xfs libcrc32c virtio_net net_failover failover virtio_blk virtio_pci virtio_ring virtio dm_mirror dm_region_hash dm_log dm_mod
CPU: 2 PID: 5661 Comm: kworker/2:0 Not tainted 5.0.0-rc6+ #20
Workqueue: events cpuset_hotplug_workfn
NIP: c000000000170edc LR: c000000000170f98 CTR: 0000000000000000
REGS: c000000003e931a0 TRAP: 0380 Not tainted (5.0.0-rc6+)
MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 22284028 XER: 00000000
CFAR: c000000000170f20 IRQMASK: 0
GPR00: c000000000170f98 c000000003e93430 c0000000011ac500 c0000001efe22000
GPR04: 0000000000000001 0000000000000000 0000000000000000 0000000000000010
GPR08: 0000000000000001 0000000000000400 ffffffffffffffff 0000000000000000
GPR12: 0000000000008800 c00000003fffd680 c0000001f14b0000 c0000000011e1bf0
GPR16: c0000000011e61f4 c0000001efe22200 c0000001efe22020 c0000001fba80000
GPR20: c0000001ff567a80 0000000000000001 c000000000e27a80 ffffffffffffe830
GPR24: ffffffffffffec30 000000000000102f 000000000000102f c0000001efca1000
GPR28: c0000001efca0400 c0000001efe22000 c0000001efe23bff c0000001efe22a00
NIP [c000000000170edc] free_sched_groups+0x5c/0xf0
LR [c000000000170f98] destroy_sched_domain+0x28/0x90
Call Trace:
[c000000003e93430] [000000000000102f] 0x102f (unreliable)
[c000000003e93470] [c000000000170f98] destroy_sched_domain+0x28/0x90
[c000000003e934a0] [c0000000001716e0] cpu_attach_domain+0x100/0x920
[c000000003e93600] [c000000000173128] build_sched_domains+0x1228/0x1370
[c000000003e93740] [c00000000017429c] partition_sched_domains+0x23c/0x400
[c000000003e937e0] [c0000000001f5ec8] rebuild_sched_domains_locked+0x78/0xe0
[c000000003e93820] [c0000000001f9ff0] rebuild_sched_domains+0x30/0x50
[c000000003e93850] [c0000000001fa1c0] cpuset_hotplug_workfn+0x1b0/0xb70
[c000000003e93c80] [c00000000012e5a0] process_one_work+0x1b0/0x480
[c000000003e93d20] [c00000000012e8f8] worker_thread+0x88/0x540
[c000000003e93db0] [c00000000013714c] kthread+0x15c/0x1a0
[c000000003e93e20] [c00000000000b55c] ret_from_kernel_thread+0x5c/0x80
Instruction dump:
2e240000 f8010010 f821ffc1 409e0014 48000080 7fbdf040 7fdff378 419e0074
ebdf0000 4192002c e93f0010 7c0004ac <7d404828> 314affff 7d40492d 40c2fff4
---[ end trace f992c4a7d47d602a ]---
Kernel panic - not syncing: Fatal exception
This happens in free_sched_groups() because the linked list of the sched_groups
is corrupted. Here what happens when we hotplug the CPU:
- build_sched_groups() builds a sched_groups linked list for sched_domain D1,
with only one entry A, refcount=1
D1: A(ref=1)
- build_sched_groups() builds a sched_groups linked list for sched_domain D2,
with the same entry A
D2: A(ref=2)
- build_sched_groups() builds a sched_groups linked list for sched_domain D3,
with the same entry A and a new entry B:
D3: A(ref=3) -> B(ref=1)
- destroy_sched_domain() is called for D1:
D1: A(ref=3) -> B(ref=1) and as ref is 1, memory of B is released,
but A->next always points to B
- destroy_sched_domain() is called for D3:
D3: A(ref=2) -> B(ref=0)
kernels crashes when it tries to use data inside B, as the memory has been
corrupted as it has been freed, the linked list (next) is broken too.
This problem appears with commit 051f3ca02e46
("sched/topology: Introduce NUMA identity node sched domain").
If I compare function calls sequence before and after this commit I can see
in the the working case that build_overlap_sched_groups() is called instead of
build_sched_groups() and in this case the reference counters have all the
same value and the linked list can be correctly unallocated.
The problem happens because patch "sched/topology: Introduce NUMA
identity node sched domain" has removed the SDTL_OVERLAP flag
of the first topology level when it has introduced the NODE domain (and thus
build_sched_groups() is used instead of build_overlap_sched_groups()).
As I don't see any reason (and it is not documented in the involved commit)
to remove this flag, this patch re-introduces the SDTL_OVERLAP flag for the
first level. This fixes the problem described above and a CPU can be hotplugged
again without kernel crash.
Fixes: 051f3ca02e46 ("sched/topology: Introduce NUMA identity node sched domain")
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Borislav Petkov <bp@suse.de>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Nathan Fontenot <nfont@linux.vnet.ibm.com>
CC: Michael Bringmann <mwb@linux.vnet.ibm.com>
CC: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
kernel/sched/topology.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 3f35ba1d8fde..372278605f0d 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1651,6 +1651,7 @@ void sched_init_numa(void)
*/
tl[i++] = (struct sched_domain_topology_level){
.mask = sd_numa_mask,
+ .flags = SDTL_OVERLAP,
.numa_level = 0,
SD_INIT_NAME(NODE)
};
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 5/8] iio/counter: add FlexTimer Module Quadrature decoder counter driver
From: Jonathan Cameron @ 2019-02-20 16:41 UTC (permalink / raw)
To: Patrick Havelange
Cc: Mark Rutland, devicetree, Lars-Peter Clausen, linux-pwm,
linux-iio, linux-kernel, Linus Walleij, Daniel Lezcano,
William Breathitt Gray, Li Yang, linuxppc-dev, Rob Herring,
Thierry Reding, linux-arm-kernel, Peter Meerwald-Stadler,
Hartmut Knaack, Thomas Gleixner, Shawn Guo, Esben Haabendal
In-Reply-To: <20190218140321.19166-5-patrick.havelange@essensium.com>
On Mon, 18 Feb 2019 15:03:18 +0100
Patrick Havelange <patrick.havelange@essensium.com> wrote:
> This driver exposes the counter for the quadrature decoder of the
> FlexTimer Module, present in the LS1021A soc.
>
> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> Reviewed-by: Esben Haabendal <esben@haabendal.dk>
Given you cc'd William, I'm guessing you know about the counter
subsystem effort. I would really rather not take any drivers
into IIO if we have any hope of getting that upstreamed soon
(which I personally think we do and should!). The reason is
we end up having to maintain old ABI just because someone might be using
it and it makes the drivers very messy.
I'll review as is though as may be there are some elements that will
cross over.
Comments inline. William: Looks like a straight forward conversion if
it makes sense to get this lined up as part of your initial submission?
You have quite a few drivers so I wouldn't have said it needs to be there
at the start, but good to have it soon after.
Jonathan
> ---
> drivers/iio/counter/Kconfig | 10 +
> drivers/iio/counter/Makefile | 1 +
> drivers/iio/counter/ftm-quaddec.c | 294 ++++++++++++++++++++++++++++++
> 3 files changed, 305 insertions(+)
> create mode 100644 drivers/iio/counter/ftm-quaddec.c
>
> diff --git a/drivers/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
> index bf1e559ad7cd..4641cb2e752a 100644
> --- a/drivers/iio/counter/Kconfig
> +++ b/drivers/iio/counter/Kconfig
> @@ -31,4 +31,14 @@ config STM32_LPTIMER_CNT
>
> To compile this driver as a module, choose M here: the
> module will be called stm32-lptimer-cnt.
> +
> +config FTM_QUADDEC
> + tristate "Flex Timer Module Quadrature decoder driver"
> + help
> + Select this option to enable the Flex Timer Quadrature decoder
> + driver.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ftm-quaddec.
> +
> endmenu
> diff --git a/drivers/iio/counter/Makefile b/drivers/iio/counter/Makefile
> index 1b9a896eb488..757c1f4196af 100644
> --- a/drivers/iio/counter/Makefile
> +++ b/drivers/iio/counter/Makefile
> @@ -6,3 +6,4 @@
>
> obj-$(CONFIG_104_QUAD_8) += 104-quad-8.o
> obj-$(CONFIG_STM32_LPTIMER_CNT) += stm32-lptimer-cnt.o
> +obj-$(CONFIG_FTM_QUADDEC) += ftm-quaddec.o
> diff --git a/drivers/iio/counter/ftm-quaddec.c b/drivers/iio/counter/ftm-quaddec.c
> new file mode 100644
> index 000000000000..ca7e55a9ab3f
> --- /dev/null
> +++ b/drivers/iio/counter/ftm-quaddec.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Flex Timer Module Quadrature decoder
> + *
> + * This module implements a driver for decoding the FTM quadrature
> + * of ex. a LS1021A
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/workqueue.h>
Tidy these up. Not all are used.
> +#include <linux/swait.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mutex.h>
> +#include <linux/fsl/ftm.h>
> +
> +struct ftm_quaddec {
> + struct platform_device *pdev;
> + void __iomem *ftm_base;
> + bool big_endian;
I'm curious. What is the benefit of running in big endian mode?
> + struct mutex ftm_quaddec_mutex;
> +};
> +
> +#define HASFLAGS(flag, bits) ((flag & bits) ? 1 : 0)
Not used.
> +
> +#define DEFAULT_POLL_INTERVAL 100 /* in msec */
> +
> +static void ftm_read(struct ftm_quaddec *ftm, uint32_t offset, uint32_t *data)
> +{
> + if (ftm->big_endian)
> + *data = ioread32be(ftm->ftm_base + offset);
> + else
> + *data = ioread32(ftm->ftm_base + offset);
> +}
> +
> +static void ftm_write(struct ftm_quaddec *ftm, uint32_t offset, uint32_t data)
> +{
> + if (ftm->big_endian)
> + iowrite32be(data, ftm->ftm_base + offset);
> + else
> + iowrite32(data, ftm->ftm_base + offset);
> +}
> +
> +/* take mutex
Tidy this comment up. I would have said the flow as fairly
obvious and the only thing needed here is to document that
the mutex must be held?
> + * call ftm_clear_write_protection
> + * update settings
> + * call ftm_set_write_protection
> + * release mutex
> + */
> +static void ftm_clear_write_protection(struct ftm_quaddec *ftm)
> +{
> + uint32_t flag;
> +
> + /* First see if it is enabled */
> + ftm_read(ftm, FTM_FMS, &flag);
> +
> + if (flag & FTM_FMS_WPEN) {
> + ftm_read(ftm, FTM_MODE, &flag);
> + ftm_write(ftm, FTM_MODE, flag | FTM_MODE_WPDIS);
> + }
> +}
> +
> +static void ftm_set_write_protection(struct ftm_quaddec *ftm)
> +{
> + ftm_write(ftm, FTM_FMS, FTM_FMS_WPEN);
> +}
> +
> +static void ftm_reset_counter(struct ftm_quaddec *ftm)
> +{
> + /* Reset hardware counter to CNTIN */
> + ftm_write(ftm, FTM_CNT, 0x0);
> +}
> +
> +static void ftm_quaddec_init(struct ftm_quaddec *ftm)
> +{
> + ftm_clear_write_protection(ftm);
> +
> + /* Do not write in the region from the CNTIN register through the
IIO multiline syntax is
/*
* Do not write
* PWM..
*/
> + * PWMLOAD register when FTMEN = 0.
> + */
> + ftm_write(ftm, FTM_MODE, FTM_MODE_FTMEN); /* enable FTM */
Drop any comments that are self explanatory.
> + ftm_write(ftm, FTM_CNTIN, 0x0000); /* zero init value */
> + ftm_write(ftm, FTM_MOD, 0xffff); /* max overflow value */
> + ftm_write(ftm, FTM_CNT, 0x0); /* reset counter value */
> + ftm_write(ftm, FTM_SC, FTM_SC_PS_1); /* prescale with x1 */
> + /* Select quad mode */
> + ftm_write(ftm, FTM_QDCTRL, FTM_QDCTRL_QUADEN);
> +
> + /* Unused features and reset to default section */
> + ftm_write(ftm, FTM_POL, 0x0); /* polarity is active high */
> + ftm_write(ftm, FTM_FLTCTRL, 0x0); /* all faults disabled */
> + ftm_write(ftm, FTM_SYNCONF, 0x0); /* disable all sync */
> + ftm_write(ftm, FTM_SYNC, 0xffff);
> +
> + /* Lock the FTM */
> + ftm_set_write_protection(ftm);
> +}
> +
> +static int ftm_quaddec_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ftm_quaddec *ftm = iio_priv(indio_dev);
> + uint32_t counter;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ftm_read(ftm, FTM_CNT, &counter);
> + *val = counter;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
> + uintptr_t private,
> + struct iio_chan_spec const *chan,
> + const char *buf, size_t len)
> +{
> + struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +
> + /* Only "counter reset" is supported for now */
> + if (!sysfs_streq(buf, "0")) {
> + dev_warn(&ftm->pdev->dev, "Reset only accepts '0'\n");
> + return -EINVAL;
Why not just make the channel attribute itself writeable given we are
setting it to 0?
> + }
> +
> + ftm_reset_counter(ftm);
> +
> + return len;
> +}
> +
> +static int ftm_quaddec_get_prescaler(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct ftm_quaddec *ftm = iio_priv(indio_dev);
> + uint32_t scflags;
> +
> + ftm_read(ftm, FTM_SC, &scflags);
> +
> + return scflags & FTM_SC_PS_MASK;
> +}
> +
> +static int ftm_quaddec_set_prescaler(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + unsigned int type)
> +{
> + struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +
> + uint32_t scflags;
> +
> + mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> + ftm_read(ftm, FTM_SC, &scflags);
> +
> + scflags &= ~FTM_SC_PS_MASK;
> + type &= FTM_SC_PS_MASK; /*just to be 100% sure*/
> +
> + scflags |= type;
> +
> + /* Write */
> + ftm_clear_write_protection(ftm);
> + ftm_write(ftm, FTM_SC, scflags);
> + ftm_set_write_protection(ftm);
> +
> + /* Also resets the counter as it is undefined anyway now */
> + ftm_reset_counter(ftm);
> +
> + mutex_unlock(&ftm->ftm_quaddec_mutex);
> + return 0;
> +}
> +
> +static const char * const ftm_quaddec_prescaler[] = {
> + "1", "2", "4", "8", "16", "32", "64", "128"
> +};
> +
> +static const struct iio_enum ftm_quaddec_prescaler_en = {
> + .items = ftm_quaddec_prescaler,
> + .num_items = ARRAY_SIZE(ftm_quaddec_prescaler),
> + .get = ftm_quaddec_get_prescaler,
> + .set = ftm_quaddec_set_prescaler,
> +};
> +
> +static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
> + {
> + .name = "reset",
> + .shared = IIO_SEPARATE,
> + .write = ftm_write_reset,
> + },
> + IIO_ENUM("prescaler", IIO_SEPARATE, &ftm_quaddec_prescaler_en),
This looks like non standard ABI. Needs documentation in
Documentation/ABI/testing/sysfs-bus-iio-*
> + IIO_ENUM_AVAILABLE("prescaler", &ftm_quaddec_prescaler_en),
> + {}
> +};
> +
> +static const struct iio_chan_spec ftm_quaddec_channels = {
> + .type = IIO_COUNT,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .ext_info = ftm_quaddec_ext_info,
> + .indexed = 1,
> +};
> +
> +static const struct iio_info ftm_quaddec_iio_info = {
> + .read_raw = ftm_quaddec_read_raw,
> +};
> +
> +static int ftm_quaddec_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct ftm_quaddec *ftm;
> + int ret;
> +
> + struct device_node *node = pdev->dev.of_node;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*ftm));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + ftm = iio_priv(indio_dev);
> +
> + platform_set_drvdata(pdev, ftm);
> +
> + ftm->pdev = pdev;
> + ftm->big_endian = of_property_read_bool(node, "big-endian");
> + ftm->ftm_base = of_iomap(node, 0);
> + if (!ftm->ftm_base)
> + return -EINVAL;
> +
> + indio_dev->name = dev_name(&pdev->dev);
This should be a nice part number like string. What does
it evaluate to here?
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->info = &ftm_quaddec_iio_info;
> + indio_dev->num_channels = 1;
> + indio_dev->channels = &ftm_quaddec_channels;
> +
> + ftm_quaddec_init(ftm);
> +
> + mutex_init(&ftm->ftm_quaddec_mutex);
> +
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> + if (ret) {
> + mutex_destroy(&ftm->ftm_quaddec_mutex);
Don't have managed devm_ calls after anything unmanaged.
I opens you up to races and is hard to review.
> + iounmap(ftm->ftm_base);
I would suggest not using of_iomap, then you can
use the devm_ioremap form handle this for you.
> + }
> + return ret;
> +}
> +
> +static int ftm_quaddec_remove(struct platform_device *pdev)
> +{
> + struct ftm_quaddec *ftm;
> + struct iio_dev *indio_dev;
> +
> + ftm = (struct ftm_quaddec *)platform_get_drvdata(pdev);
platform_get_drvdata returns a void *.
C spec says you can always cast implicitly from a void * to any
other point type. Hence you don't need the cast.
(I'm too lazy to find the reference now as it's been a long
day of reviewing but google will find it for you if interested)
> + indio_dev = iio_priv_to_dev(ftm);
> + /* This is needed to remove sysfs entries */
It does a lot more than that, so please remove the comment.
Actually this worries me. You should not need to manually
call devm_iio_device_register, but you do because of the ordering
and the fact you want to remove the interfaces before turning the
device off. In that case, do not use the devm_ form but
instead iio_device_register and do the error handling paths
manually. An alternative is to use
devm_add_action_or_reset to call unwind functions automatically
for the few device specific calls you have.
> + devm_iio_device_unregister(&pdev->dev, indio_dev);
> +
> + ftm_write(ftm, FTM_MODE, 0);
> +
> + iounmap(ftm->ftm_base);
> + mutex_destroy(&ftm->ftm_quaddec_mutex);
mutex destroy is only really used in debug paths and is often
skipped in remove functions to simplify things.
(particularly when managed cleanup is going on).
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ftm_quaddec_match[] = {
> + { .compatible = "fsl,ftm-quaddec" },
> + {},
> +};
> +
> +static struct platform_driver ftm_quaddec_driver = {
> + .driver = {
> + .name = "ftm-quaddec",
> + .owner = THIS_MODULE,
> + .of_match_table = ftm_quaddec_match,
> + },
> + .probe = ftm_quaddec_probe,
> + .remove = ftm_quaddec_remove,
> +};
> +
> +module_platform_driver(ftm_quaddec_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Kjeld Flarup <kfa@deif.com");
> +MODULE_AUTHOR("Patrick Havelange <patrick.havelange@essensium.com");
^ permalink raw reply
* Re: [PATCH 8/8] iio/counter/ftm-quaddec: add handling of under/overflow of the counter.
From: Jonathan Cameron @ 2019-02-20 16:54 UTC (permalink / raw)
To: Patrick Havelange
Cc: Mark Rutland, devicetree, Lars-Peter Clausen, linux-pwm,
linux-iio, linux-kernel, Linus Walleij, Daniel Lezcano,
William Breathitt Gray, Li Yang, linuxppc-dev, Rob Herring,
Thierry Reding, linux-arm-kernel, Peter Meerwald-Stadler,
Hartmut Knaack, Thomas Gleixner, Shawn Guo, Esben Haabendal
In-Reply-To: <20190218140321.19166-8-patrick.havelange@essensium.com>
On Mon, 18 Feb 2019 15:03:21 +0100
Patrick Havelange <patrick.havelange@essensium.com> wrote:
> This is implemented by polling the counter value. A new parameter
> "poll-interval" can be set in the device tree, or can be changed
> at runtime. The reason for the polling is to avoid interrupts flooding.
> If the quadrature input is going up and down around the overflow value
> (or around 0), the interrupt will be triggering all the time. Thus,
> polling is an easy way to handle overflow in a consistent way.
> Polling can still be disabled by setting poll-interval to 0.
>
> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> Reviewed-by: Esben Haabendal <esben@haabendal.dk>
Comments inline.
Jonathan
> ---
> drivers/iio/counter/ftm-quaddec.c | 199 +++++++++++++++++++++++++++++-
> 1 file changed, 193 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/counter/ftm-quaddec.c b/drivers/iio/counter/ftm-quaddec.c
> index ca7e55a9ab3f..3a0395c3ef33 100644
> --- a/drivers/iio/counter/ftm-quaddec.c
> +++ b/drivers/iio/counter/ftm-quaddec.c
> @@ -25,11 +25,33 @@
>
> struct ftm_quaddec {
> struct platform_device *pdev;
> + struct delayed_work delayedcounterwork;
> void __iomem *ftm_base;
> bool big_endian;
> +
> + /* Offset added to the counter to adjust for overflows of the
> + * 16 bit HW counter. Only the 16 MSB are set.
Comment syntax.
> + */
> + uint32_t counteroffset;
> +
> + /* Store the counter on each read, this is used to detect
> + * if the counter readout if we over or underflow
> + */
> + uint8_t lastregion;
> +
> + /* Poll-interval, in ms before delayed work must poll counter */
> + uint16_t poll_interval;
> +
> struct mutex ftm_quaddec_mutex;
> };
>
> +struct counter_result {
> + /* 16 MSB are from the counteroffset
> + * 16 LSB are from the hardware counter
> + */
> + uint32_t value;
Why the structure?
> +};
> +
> #define HASFLAGS(flag, bits) ((flag & bits) ? 1 : 0)
>
> #define DEFAULT_POLL_INTERVAL 100 /* in msec */
> @@ -74,8 +96,75 @@ static void ftm_set_write_protection(struct ftm_quaddec *ftm)
> ftm_write(ftm, FTM_FMS, FTM_FMS_WPEN);
> }
>
> +/* must be called with mutex locked */
> +static void ftm_work_reschedule(struct ftm_quaddec *ftm)
> +{
> + cancel_delayed_work(&ftm->delayedcounterwork);
> + if (ftm->poll_interval > 0)
> + schedule_delayed_work(&ftm->delayedcounterwork,
> + msecs_to_jiffies(ftm->poll_interval));
> +}
> +
> +/* Reports the hardware counter added the offset counter.
> + *
> + * The quadrature decodes does not use interrupts, because it cannot be
> + * guaranteed that the counter won't flip between 0xFFFF and 0x0000 at a high
> + * rate, causing Real Time performance degration. Instead the counter must be
> + * read frequently enough - the assumption is 150 KHz input can be handled with
> + * 100 ms read cycles.
> + */
> +static void ftm_work_counter(struct ftm_quaddec *ftm,
> + struct counter_result *returndata)
> +{
> + /* only 16bits filled in*/
> + uint32_t hwcounter;
> + uint8_t currentregion;
> +
> + mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> + ftm_read(ftm, FTM_CNT, &hwcounter);
> +
> + /* Divide the counter in four regions:
> + * 0x0000-0x4000-0x8000-0xC000-0xFFFF
> + * When the hwcounter changes between region 0 and 3 there is an
> + * over/underflow
> + */
> + currentregion = hwcounter / 0x4000;
> +
> + if (ftm->lastregion == 3 && currentregion == 0)
> + ftm->counteroffset += 0x10000;
> +
> + if (ftm->lastregion == 0 && currentregion == 3)
> + ftm->counteroffset -= 0x10000;
> +
> + ftm->lastregion = currentregion;
> +
> + if (returndata)
> + returndata->value = ftm->counteroffset + hwcounter;
> +
> + ftm_work_reschedule(ftm);
> +
> + mutex_unlock(&ftm->ftm_quaddec_mutex);
> +}
> +
> +/* wrapper around the real function */
> +static void ftm_work_counter_delay(struct work_struct *workptr)
> +{
> + struct delayed_work *work;
> + struct ftm_quaddec *ftm;
> +
> + work = container_of(workptr, struct delayed_work, work);
> + ftm = container_of(work, struct ftm_quaddec, delayedcounterwork);
> +
> + ftm_work_counter(ftm, NULL);
> +}
> +
> +/* must be called with mutex locked */
> static void ftm_reset_counter(struct ftm_quaddec *ftm)
> {
> + ftm->counteroffset = 0;
> + ftm->lastregion = 0;
> +
> /* Reset hardware counter to CNTIN */
> ftm_write(ftm, FTM_CNT, 0x0);
> }
> @@ -110,18 +199,91 @@ static int ftm_quaddec_read_raw(struct iio_dev *indio_dev,
> int *val, int *val2, long mask)
> {
> struct ftm_quaddec *ftm = iio_priv(indio_dev);
> - uint32_t counter;
> + struct counter_result counter;
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - ftm_read(ftm, FTM_CNT, &counter);
> - *val = counter;
> + case IIO_CHAN_INFO_PROCESSED:
> + ftm_work_counter(ftm, &counter);
> + if (mask == IIO_CHAN_INFO_RAW)
> + counter.value &= 0xffff;
> +
> + *val = counter.value;
> +
> return IIO_VAL_INT;
> default:
> return -EINVAL;
> }
> }
>
> +static uint32_t ftm_get_default_poll_interval(const struct ftm_quaddec *ftm)
> +{
> + /* Read values from device tree */
> + uint32_t val;
> + const struct device_node *node = ftm->pdev->dev.of_node;
> +
> + if (of_property_read_u32(node, "poll-interval", &val))
> + val = DEFAULT_POLL_INTERVAL;
> +
> + return val;
> +}
> +
> +static ssize_t ftm_read_default_poll_interval(struct iio_dev *indio_dev,
> + uintptr_t private,
> + struct iio_chan_spec const *chan,
> + char *buf)
> +{
> + const struct ftm_quaddec *ftm = iio_priv(indio_dev);
> + uint32_t val = ftm_get_default_poll_interval(ftm);
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", val);
> +}
> +
> +static ssize_t ftm_read_poll_interval(struct iio_dev *indio_dev,
> + uintptr_t private,
> + struct iio_chan_spec const *chan,
> + char *buf)
> +{
> + struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +
> + uint32_t poll_interval = READ_ONCE(ftm->poll_interval);
Why bother with the local variable? I'm not awake enough to see
why the READ_ONCE is necessary here.
If worried about it, just take the lock, we are far from high
performance in this path.
> +
> + return snprintf(buf, PAGE_SIZE, "%u\n", poll_interval);
> +}
> +
> +static ssize_t ftm_write_poll_interval(struct iio_dev *indio_dev,
> + uintptr_t private,
> + struct iio_chan_spec const *chan,
> + const char *buf, size_t len)
> +{
> + struct ftm_quaddec *ftm = iio_priv(indio_dev);
> + uint32_t newpoll_interval;
> + uint32_t default_interval;
> +
> + if (kstrtouint(buf, 10, &newpoll_interval) != 0) {
> + dev_err(&ftm->pdev->dev, "poll_interval not a number: '%s'\n",
> + buf);
> + return -EINVAL;
> + }
> +
> + /* Don't accept polling times below the default value to protect the
> + * system.
> + */
> + default_interval = ftm_get_default_poll_interval(ftm);
> +
> + if (newpoll_interval < default_interval && newpoll_interval != 0)
> + newpoll_interval = default_interval;
> +
> + mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> + WRITE_ONCE(ftm->poll_interval, newpoll_interval);
> + ftm_work_reschedule(ftm);
> +
> + mutex_unlock(&ftm->ftm_quaddec_mutex);
> +
> + return len;
> +}
> +
> static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
> uintptr_t private,
> struct iio_chan_spec const *chan,
> @@ -135,8 +297,11 @@ static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
> return -EINVAL;
> }
>
> + mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> ftm_reset_counter(ftm);
>
> + mutex_unlock(&ftm->ftm_quaddec_mutex);
> return len;
> }
>
> @@ -192,6 +357,17 @@ static const struct iio_enum ftm_quaddec_prescaler_en = {
> };
>
> static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
> + {
> + .name = "default_poll_interval",
> + .shared = IIO_SHARED_BY_TYPE,
> + .read = ftm_read_default_poll_interval,
Why is this relevant if the value is set to something else?
> + },
> + {
> + .name = "poll_interval",
> + .shared = IIO_SHARED_BY_TYPE,
> + .read = ftm_read_poll_interval,
> + .write = ftm_write_poll_interval,
Hmm. New ABI that needs documenting. I'm not sure how we should
handle this. Perhaps William has a good suggestion for how to do it.
> + },
> {
> .name = "reset",
> .shared = IIO_SEPARATE,
> @@ -205,7 +381,8 @@ static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
> static const struct iio_chan_spec ftm_quaddec_channels = {
> .type = IIO_COUNT,
> .channel = 0,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_PROCESSED),
Why? As a general rule, we don't allow bother RAW and processed for
a particular channel. Note the raw value doesn't actually 'have'
to be the value off a sensor - it is just expected to not have
linear scaling and offset applied (which are encode dependent
here so can't be applied). So just use raw for your non overflowing
version.
> .ext_info = ftm_quaddec_ext_info,
> .indexed = 1,
> };
> @@ -232,10 +409,14 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
>
> ftm->pdev = pdev;
> ftm->big_endian = of_property_read_bool(node, "big-endian");
> + ftm->counteroffset = 0;
> + ftm->lastregion = 0;
> ftm->ftm_base = of_iomap(node, 0);
> if (!ftm->ftm_base)
> return -EINVAL;
>
> + ftm->poll_interval = ftm_get_default_poll_interval(ftm);
> +
> indio_dev->name = dev_name(&pdev->dev);
> indio_dev->dev.parent = &pdev->dev;
> indio_dev->info = &ftm_quaddec_iio_info;
> @@ -245,9 +426,13 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
> ftm_quaddec_init(ftm);
>
> mutex_init(&ftm->ftm_quaddec_mutex);
> + INIT_DELAYED_WORK(&ftm->delayedcounterwork, ftm_work_counter_delay);
> +
> + ftm_work_reschedule(ftm);
>
> ret = devm_iio_device_register(&pdev->dev, indio_dev);
> if (ret) {
> + cancel_delayed_work_sync(&ftm->delayedcounterwork);
> mutex_destroy(&ftm->ftm_quaddec_mutex);
> iounmap(ftm->ftm_base);
> }
> @@ -261,13 +446,15 @@ static int ftm_quaddec_remove(struct platform_device *pdev)
>
> ftm = (struct ftm_quaddec *)platform_get_drvdata(pdev);
> indio_dev = iio_priv_to_dev(ftm);
> - /* This is needed to remove sysfs entries */
> + /* Make sure no concurrent attribute reads happen*/
> devm_iio_device_unregister(&pdev->dev, indio_dev);
>
> + cancel_delayed_work_sync(&ftm->delayedcounterwork);
> +
> ftm_write(ftm, FTM_MODE, 0);
>
> - iounmap(ftm->ftm_base);
> mutex_destroy(&ftm->ftm_quaddec_mutex);
> + iounmap(ftm->ftm_base);
Why the reorder? If it was wrong in the first place, fix the
earlier patch not this one.
>
> return 0;
> }
^ permalink raw reply
* [PATCH v2] sched/topology: fix kernel crash when a CPU is hotplugged in a memoryless node
From: Laurent Vivier @ 2019-02-20 16:55 UTC (permalink / raw)
To: linux-kernel
Cc: Laurent Vivier, Srikar Dronamraju, Peter Zijlstra,
Michael Bringmann, Ingo Molnar, Suravee Suthikulpanit,
Nathan Fontenot, Borislav Petkov, linuxppc-dev, David Gibson
When we hotplug a CPU in a memoryless/cpuless node, the kernel crashes when it
rebuilds the sched_domains data.
I reproduce this problem on POWER and with a pseries VM, with the following
QEMU parameters:
-machine pseries -enable-kvm -m 8192 \
-smp 2,maxcpus=8,sockets=4,cores=2,threads=1 \
-numa node,nodeid=0,cpus=0-1,mem=0 \
-numa node,nodeid=1,cpus=2-3,mem=8192 \
-numa node,nodeid=2,cpus=4-5,mem=0 \
-numa node,nodeid=3,cpus=6-7,mem=0
Then I can trigger the crash by hotplugging a CPU on node-id 3:
(qemu) device_add host-spapr-cpu-core,core-id=7,node-id=3
Built 2 zonelists, mobility grouping on. Total pages: 130162
Policy zone: Normal
WARNING: workqueue cpumask: online intersect > possible intersect
BUG: Kernel NULL pointer dereference at 0x00000400
Faulting instruction address: 0xc000000000170edc
Oops: Kernel access of bad area, sig: 11 [#1]
LE SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: ip6t_rpfilter ipt_REJECT nf_reject_ipv4 ip6t_REJECT nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc ip6table_nat nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw iptable_nat nf_nat_ipv4 nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 iptable_mangle iptable_security iptable_raw ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter xts vmx_crypto ip_tables xfs libcrc32c virtio_net net_failover failover virtio_blk virtio_pci virtio_ring virtio dm_mirror dm_region_hash dm_log dm_mod
CPU: 2 PID: 5661 Comm: kworker/2:0 Not tainted 5.0.0-rc6+ #20
Workqueue: events cpuset_hotplug_workfn
NIP: c000000000170edc LR: c000000000170f98 CTR: 0000000000000000
REGS: c000000003e931a0 TRAP: 0380 Not tainted (5.0.0-rc6+)
MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 22284028 XER: 00000000
CFAR: c000000000170f20 IRQMASK: 0
GPR00: c000000000170f98 c000000003e93430 c0000000011ac500 c0000001efe22000
GPR04: 0000000000000001 0000000000000000 0000000000000000 0000000000000010
GPR08: 0000000000000001 0000000000000400 ffffffffffffffff 0000000000000000
GPR12: 0000000000008800 c00000003fffd680 c0000001f14b0000 c0000000011e1bf0
GPR16: c0000000011e61f4 c0000001efe22200 c0000001efe22020 c0000001fba80000
GPR20: c0000001ff567a80 0000000000000001 c000000000e27a80 ffffffffffffe830
GPR24: ffffffffffffec30 000000000000102f 000000000000102f c0000001efca1000
GPR28: c0000001efca0400 c0000001efe22000 c0000001efe23bff c0000001efe22a00
NIP [c000000000170edc] free_sched_groups+0x5c/0xf0
LR [c000000000170f98] destroy_sched_domain+0x28/0x90
Call Trace:
[c000000003e93430] [000000000000102f] 0x102f (unreliable)
[c000000003e93470] [c000000000170f98] destroy_sched_domain+0x28/0x90
[c000000003e934a0] [c0000000001716e0] cpu_attach_domain+0x100/0x920
[c000000003e93600] [c000000000173128] build_sched_domains+0x1228/0x1370
[c000000003e93740] [c00000000017429c] partition_sched_domains+0x23c/0x400
[c000000003e937e0] [c0000000001f5ec8] rebuild_sched_domains_locked+0x78/0xe0
[c000000003e93820] [c0000000001f9ff0] rebuild_sched_domains+0x30/0x50
[c000000003e93850] [c0000000001fa1c0] cpuset_hotplug_workfn+0x1b0/0xb70
[c000000003e93c80] [c00000000012e5a0] process_one_work+0x1b0/0x480
[c000000003e93d20] [c00000000012e8f8] worker_thread+0x88/0x540
[c000000003e93db0] [c00000000013714c] kthread+0x15c/0x1a0
[c000000003e93e20] [c00000000000b55c] ret_from_kernel_thread+0x5c/0x80
Instruction dump:
2e240000 f8010010 f821ffc1 409e0014 48000080 7fbdf040 7fdff378 419e0074
ebdf0000 4192002c e93f0010 7c0004ac <7d404828> 314affff 7d40492d 40c2fff4
---[ end trace f992c4a7d47d602a ]---
Kernel panic - not syncing: Fatal exception
This happens in free_sched_groups() because the linked list of the sched_groups
is corrupted. Here what happens when we hotplug the CPU:
- build_sched_groups() builds a sched_groups linked list for sched_domain D1,
with only one entry A, refcount=1
D1: A(ref=1)
- build_sched_groups() builds a sched_groups linked list for sched_domain D2,
with the same entry A
D2: A(ref=2)
- build_sched_groups() builds a sched_groups linked list for sched_domain D3,
with the same entry A and a new entry B:
D3: A(ref=3) -> B(ref=1)
- destroy_sched_domain() is called for D1:
D1: A(ref=3) -> B(ref=1) and as ref is 1, memory of B is released,
but A->next always points to B
- destroy_sched_domain() is called for D3:
D3: A(ref=2) -> B(ref=0)
kernel crashes when it tries to use data inside B, as the memory has been
corrupted as it has been freed, the linked list (next) is broken too.
This problem appears with commit 051f3ca02e46
("sched/topology: Introduce NUMA identity node sched domain").
If I compare function calls sequence before and after this commit I can see
in the the working case that build_overlap_sched_groups() is called instead of
build_sched_groups() and in this case the reference counters have all the
same value and the linked list can be correctly unallocated.
The problem happens because patch "sched/topology: Introduce NUMA
identity node sched domain" has removed the SDTL_OVERLAP flag
of the first topology level when it has introduced the NODE domain (and thus
build_sched_groups() is used instead of build_overlap_sched_groups()).
As I don't see any reason (and it is not documented in the involved commit)
to remove this flag, this patch re-introduces the SDTL_OVERLAP flag for the
first level. This fixes the problem described above and a CPU can be hotplugged
again without kernel crash.
Fixes: 051f3ca02e46 ("sched/topology: Introduce NUMA identity node sched domain")
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Cc: Michael Bringmann <mwb@linux.vnet.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Notes:
v2: add scheduler maintainers in the CC: list
kernel/sched/topology.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 3f35ba1d8fde..372278605f0d 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1651,6 +1651,7 @@ void sched_init_numa(void)
*/
tl[i++] = (struct sched_domain_topology_level){
.mask = sd_numa_mask,
+ .flags = SDTL_OVERLAP,
.numa_level = 0,
SD_INIT_NAME(NODE)
};
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2] sched/topology: fix kernel crash when a CPU is hotplugged in a memoryless node
From: Peter Zijlstra @ 2019-02-20 17:08 UTC (permalink / raw)
To: Laurent Vivier
Cc: Srikar Dronamraju, linux-kernel, Michael Bringmann, Ingo Molnar,
Suravee Suthikulpanit, Nathan Fontenot, Borislav Petkov,
linuxppc-dev, David Gibson
In-Reply-To: <20190220165520.5241-1-lvivier@redhat.com>
On Wed, Feb 20, 2019 at 05:55:20PM +0100, Laurent Vivier wrote:
> index 3f35ba1d8fde..372278605f0d 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1651,6 +1651,7 @@ void sched_init_numa(void)
> */
> tl[i++] = (struct sched_domain_topology_level){
> .mask = sd_numa_mask,
> + .flags = SDTL_OVERLAP,
This makes no sense what so ever. The numa identify node should not have
overlap with other domains.
Are you sure this is not because of the utterly broken powerpc nonsense
where they move CPUs between nodes?
> .numa_level = 0,
> SD_INIT_NAME(NODE)
> };
> --
> 2.20.1
>
^ permalink raw reply
* [PATCH v3 04/16] powerpc/mm/32s: use generic mmu_mapin_ram() for all blocks.
From: Christophe Leroy @ 2019-02-20 17:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
j.neuschaefer
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1550648295.git.christophe.leroy@c-s.fr>
Now that mmu_mapin_ram() is able to handle other blocks
than the one starting at 0, the WII can use it for all
its blocks.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/pgtable_32.c | 25 +++++++------------------
1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index b4858818523f..fd665c32a1f7 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -254,26 +254,15 @@ static void __init __mapin_ram_chunk(unsigned long offset, unsigned long top)
void __init mapin_ram(void)
{
- unsigned long s, top;
-
-#ifndef CONFIG_WII
- top = total_lowmem;
- s = mmu_mapin_ram(0, top);
- __mapin_ram_chunk(s, top);
-#else
- if (!wii_hole_size) {
- s = mmu_mapin_ram(0, total_lowmem);
- __mapin_ram_chunk(s, total_lowmem);
- } else {
- top = wii_hole_start;
- s = mmu_mapin_ram(0, top);
- __mapin_ram_chunk(s, top);
+ struct memblock_region *reg;
+
+ for_each_memblock(memory, reg) {
+ unsigned long base = reg->base;
+ unsigned long top = base + reg->size;
- top = memblock_end_of_DRAM();
- s = wii_mmu_mapin_mem2(top);
- __mapin_ram_chunk(s, top);
+ base = mmu_mapin_ram(base, top);
+ __mapin_ram_chunk(base, top);
}
-#endif
}
/* Scan the real Linux page tables and return a PTE pointer for
--
2.13.3
^ permalink raw reply related
* [PATCH v3 00/16] powerpc/32: Use BATs/LTLBs for STRICT_KERNEL_RWX
From: Christophe Leroy @ 2019-02-20 17:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
j.neuschaefer
Cc: linuxppc-dev, linux-kernel
The purpose of this serie is to:
- use BATs with STRICT_KERNEL_RWX on book3s (See patch 13 for details.)
- use LTLBs with STRICT_KERNEL_RWX on 8xx (See patch 15 for a few details.)
v3:
- Reordered to avoid build failure due to setibat() not being used for several steps in the serie.
Now the patch using setibat() is next to the one adding setibat().
- Fixed mmu_mapin_ram() in patch 3 to return base in all cases, thanks Jonathan for the test
- Fixed build failure on 8xx when CONFIG_PERF_EVENTS is set due to too many instructions in Exception 0x1200
- Made 8M alignment for data the default on 8xx when STRICT_KERNEL_RWX is selected.
- Added patch 1 to not set additionnal bat on the wii when requesting nobats. The only purpose of this patch
is to be backported, as this function is removed later in the series.
v2:
- Fix patch 2 (was patch 3 in v1) based on feedback from Jonathan.
- Added support for 8xx with LTLBs.
- Added systematic population of pagetables for Abatron BDI.
Christophe Leroy (16):
powerpc/wii: properly disable use of BATs when requested.
powerpc/mm/32: add base address to mmu_mapin_ram()
powerpc/mm/32s: rework mmu_mapin_ram()
powerpc/mm/32s: use generic mmu_mapin_ram() for all blocks.
powerpc/32: always populate page tables for Abatron BDI.
powerpc/wii: remove wii_mmu_mapin_mem2()
powerpc/mm/32s: use _PAGE_EXEC in setbat()
powerpc/32: add helper to write into segment registers
powerpc/mmu: add is_strict_kernel_rwx() helper
powerpc/kconfig: define PAGE_SHIFT inside Kconfig
powerpc/kconfig: define CONFIG_DATA_SHIFT and CONFIG_ETEXT_SHIFT
powerpc/mm/32s: add setibat() clearibat() and update_bats()
powerpc/mm/32s: Use BATs for STRICT_KERNEL_RWX
powerpc/kconfig: make _etext and data areas alignment configurable on
Book3s 32
powerpc/8xx: don't disable large TLBs with CONFIG_STRICT_KERNEL_RWX
powerpc/kconfig: make _etext and data areas alignment configurable on
8xx
arch/powerpc/Kconfig | 60 +++++++++
arch/powerpc/include/asm/book3s/32/mmu-hash.h | 2 +
arch/powerpc/include/asm/book3s/32/pgtable.h | 11 ++
arch/powerpc/include/asm/mmu.h | 11 ++
arch/powerpc/include/asm/nohash/32/mmu-8xx.h | 3 +-
arch/powerpc/include/asm/page.h | 13 +-
arch/powerpc/include/asm/reg.h | 5 +
arch/powerpc/kernel/head_32.S | 35 +++++
arch/powerpc/kernel/head_8xx.S | 54 ++++++--
arch/powerpc/kernel/vmlinux.lds.S | 9 +-
arch/powerpc/mm/40x_mmu.c | 2 +-
arch/powerpc/mm/44x_mmu.c | 2 +-
arch/powerpc/mm/8xx_mmu.c | 33 ++++-
arch/powerpc/mm/fsl_booke_mmu.c | 2 +-
arch/powerpc/mm/init_32.c | 6 +-
arch/powerpc/mm/mmu_decl.h | 10 +-
arch/powerpc/mm/pgtable_32.c | 38 +++---
arch/powerpc/mm/ppc_mmu_32.c | 180 ++++++++++++++++++++++----
arch/powerpc/platforms/embedded6xx/wii.c | 24 ----
19 files changed, 390 insertions(+), 110 deletions(-)
--
2.13.3
^ permalink raw reply
* [PATCH v3 01/16] powerpc/wii: properly disable use of BATs when requested.
From: Christophe Leroy @ 2019-02-20 17:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
j.neuschaefer
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1550648295.git.christophe.leroy@c-s.fr>
'nobats' kernel parameter or some options like CONFIG_DEBUG_PAGEALLOC
deny the use of BATS for mapping memory.
This patch makes sure that the specific wii RAM mapping function
takes it into account as well.
Fixes: de32400dd26e ("wii: use both mem1 and mem2 as ram")
Cc: stable@vger.kernel.org
Reviewed-by: Jonathan Neuschafer <j.neuschaefer@gmx.net>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/platforms/embedded6xx/wii.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/platforms/embedded6xx/wii.c b/arch/powerpc/platforms/embedded6xx/wii.c
index ecf703ee3a76..ac4ee88efc80 100644
--- a/arch/powerpc/platforms/embedded6xx/wii.c
+++ b/arch/powerpc/platforms/embedded6xx/wii.c
@@ -83,6 +83,10 @@ unsigned long __init wii_mmu_mapin_mem2(unsigned long top)
/* MEM2 64MB@0x10000000 */
delta = wii_hole_start + wii_hole_size;
size = top - delta;
+
+ if (__map_without_bats)
+ return delta;
+
for (bl = 128<<10; bl < max_size; bl <<= 1) {
if (bl * 2 > size)
break;
--
2.13.3
^ permalink raw reply related
* [PATCH v3 03/16] powerpc/mm/32s: rework mmu_mapin_ram()
From: Christophe Leroy @ 2019-02-20 17:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
j.neuschaefer
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1550648295.git.christophe.leroy@c-s.fr>
This patch reworks mmu_mapin_ram() to be more generic and map as much
blocks as possible. It now supports blocks not starting at address 0.
It scans DBATs array to find free ones instead of forcing the use of
BAT2 and BAT3.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/ppc_mmu_32.c | 63 ++++++++++++++++++++++++++++----------------
1 file changed, 41 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/mm/ppc_mmu_32.c b/arch/powerpc/mm/ppc_mmu_32.c
index b260ced065b4..5fc59b195fef 100644
--- a/arch/powerpc/mm/ppc_mmu_32.c
+++ b/arch/powerpc/mm/ppc_mmu_32.c
@@ -73,39 +73,58 @@ unsigned long p_block_mapped(phys_addr_t pa)
return 0;
}
+static int find_free_bat(void)
+{
+ int b;
+
+ if (cpu_has_feature(CPU_FTR_601)) {
+ for (b = 0; b < 4; b++) {
+ struct ppc_bat *bat = BATS[b];
+
+ if (!(bat[0].batl & 0x40))
+ return b;
+ }
+ } else {
+ int n = mmu_has_feature(MMU_FTR_USE_HIGH_BATS) ? 8 : 4;
+
+ for (b = 0; b < n; b++) {
+ struct ppc_bat *bat = BATS[b];
+
+ if (!(bat[1].batu & 3))
+ return b;
+ }
+ }
+ return -1;
+}
+
+static unsigned int block_size(unsigned long base, unsigned long top)
+{
+ unsigned int max_size = (cpu_has_feature(CPU_FTR_601) ? 8 : 256) << 20;
+ unsigned int base_shift = (fls(base) - 1) & 31;
+ unsigned int block_shift = (fls(top - base) - 1) & 31;
+
+ return min3(max_size, 1U << base_shift, 1U << block_shift);
+}
+
unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
{
- unsigned long tot, bl, done;
- unsigned long max_size = (256<<20);
+ int idx;
if (__map_without_bats) {
printk(KERN_DEBUG "RAM mapped without BATs\n");
- return 0;
+ return base;
}
- /* Set up BAT2 and if necessary BAT3 to cover RAM. */
+ while ((idx = find_free_bat()) != -1 && base != top) {
+ unsigned int size = block_size(base, top);
- /* Make sure we don't map a block larger than the
- smallest alignment of the physical address. */
- tot = top;
- for (bl = 128<<10; bl < max_size; bl <<= 1) {
- if (bl * 2 > tot)
+ if (size < 128 << 10)
break;
+ setbat(idx, PAGE_OFFSET + base, base, size, PAGE_KERNEL_X);
+ base += size;
}
- setbat(2, PAGE_OFFSET, 0, bl, PAGE_KERNEL_X);
- done = (unsigned long)bat_addrs[2].limit - PAGE_OFFSET + 1;
- if ((done < tot) && !bat_addrs[3].limit) {
- /* use BAT3 to cover a bit more */
- tot -= done;
- for (bl = 128<<10; bl < max_size; bl <<= 1)
- if (bl * 2 > tot)
- break;
- setbat(3, PAGE_OFFSET+done, done, bl, PAGE_KERNEL_X);
- done = (unsigned long)bat_addrs[3].limit - PAGE_OFFSET + 1;
- }
-
- return done;
+ return base;
}
/*
--
2.13.3
^ permalink raw reply related
* [PATCH v3 02/16] powerpc/mm/32: add base address to mmu_mapin_ram()
From: Christophe Leroy @ 2019-02-20 17:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
j.neuschaefer
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1550648295.git.christophe.leroy@c-s.fr>
At the time being, mmu_mapin_ram() always maps RAM from the beginning.
But some platforms like the WII have to map a second block of RAM.
This patch adds to mmu_mapin_ram() the base address of the block.
At the moment, only base address 0 is supported.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/40x_mmu.c | 2 +-
arch/powerpc/mm/44x_mmu.c | 2 +-
arch/powerpc/mm/8xx_mmu.c | 2 +-
arch/powerpc/mm/fsl_booke_mmu.c | 2 +-
arch/powerpc/mm/mmu_decl.h | 2 +-
arch/powerpc/mm/pgtable_32.c | 6 +++---
arch/powerpc/mm/ppc_mmu_32.c | 2 +-
7 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/mm/40x_mmu.c b/arch/powerpc/mm/40x_mmu.c
index 61ac468c87c6..b9cf6f8764b0 100644
--- a/arch/powerpc/mm/40x_mmu.c
+++ b/arch/powerpc/mm/40x_mmu.c
@@ -93,7 +93,7 @@ void __init MMU_init_hw(void)
#define LARGE_PAGE_SIZE_16M (1<<24)
#define LARGE_PAGE_SIZE_4M (1<<22)
-unsigned long __init mmu_mapin_ram(unsigned long top)
+unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
{
unsigned long v, s, mapped;
phys_addr_t p;
diff --git a/arch/powerpc/mm/44x_mmu.c b/arch/powerpc/mm/44x_mmu.c
index ea2b9af08a48..aad127acdbaa 100644
--- a/arch/powerpc/mm/44x_mmu.c
+++ b/arch/powerpc/mm/44x_mmu.c
@@ -170,7 +170,7 @@ void __init MMU_init_hw(void)
flush_instruction_cache();
}
-unsigned long __init mmu_mapin_ram(unsigned long top)
+unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
{
unsigned long addr;
unsigned long memstart = memstart_addr & ~(PPC_PIN_SIZE - 1);
diff --git a/arch/powerpc/mm/8xx_mmu.c b/arch/powerpc/mm/8xx_mmu.c
index e2c32bdb6023..46bc26ef71e9 100644
--- a/arch/powerpc/mm/8xx_mmu.c
+++ b/arch/powerpc/mm/8xx_mmu.c
@@ -99,7 +99,7 @@ static void __init mmu_patch_cmp_limit(s32 *site, unsigned long mapped)
modify_instruction_site(site, 0xffff, (unsigned long)__va(mapped) >> 16);
}
-unsigned long __init mmu_mapin_ram(unsigned long top)
+unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
{
unsigned long mapped;
diff --git a/arch/powerpc/mm/fsl_booke_mmu.c b/arch/powerpc/mm/fsl_booke_mmu.c
index 080d49b26c3a..210cbc1faf63 100644
--- a/arch/powerpc/mm/fsl_booke_mmu.c
+++ b/arch/powerpc/mm/fsl_booke_mmu.c
@@ -221,7 +221,7 @@ unsigned long map_mem_in_cams(unsigned long ram, int max_cam_idx, bool dryrun)
#error "LOWMEM_CAM_NUM must be less than NUM_TLBCAMS"
#endif
-unsigned long __init mmu_mapin_ram(unsigned long top)
+unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
{
return tlbcam_addrs[tlbcam_index - 1].limit - PAGE_OFFSET + 1;
}
diff --git a/arch/powerpc/mm/mmu_decl.h b/arch/powerpc/mm/mmu_decl.h
index c4a717da65eb..61730023dde3 100644
--- a/arch/powerpc/mm/mmu_decl.h
+++ b/arch/powerpc/mm/mmu_decl.h
@@ -130,7 +130,7 @@ extern void wii_memory_fixups(void);
*/
#ifdef CONFIG_PPC32
extern void MMU_init_hw(void);
-extern unsigned long mmu_mapin_ram(unsigned long top);
+unsigned long mmu_mapin_ram(unsigned long base, unsigned long top);
#endif
#ifdef CONFIG_PPC_FSL_BOOK3E
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index ded71126ce4c..b4858818523f 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -258,15 +258,15 @@ void __init mapin_ram(void)
#ifndef CONFIG_WII
top = total_lowmem;
- s = mmu_mapin_ram(top);
+ s = mmu_mapin_ram(0, top);
__mapin_ram_chunk(s, top);
#else
if (!wii_hole_size) {
- s = mmu_mapin_ram(total_lowmem);
+ s = mmu_mapin_ram(0, total_lowmem);
__mapin_ram_chunk(s, total_lowmem);
} else {
top = wii_hole_start;
- s = mmu_mapin_ram(top);
+ s = mmu_mapin_ram(0, top);
__mapin_ram_chunk(s, top);
top = memblock_end_of_DRAM();
diff --git a/arch/powerpc/mm/ppc_mmu_32.c b/arch/powerpc/mm/ppc_mmu_32.c
index 3f4193201ee7..b260ced065b4 100644
--- a/arch/powerpc/mm/ppc_mmu_32.c
+++ b/arch/powerpc/mm/ppc_mmu_32.c
@@ -73,7 +73,7 @@ unsigned long p_block_mapped(phys_addr_t pa)
return 0;
}
-unsigned long __init mmu_mapin_ram(unsigned long top)
+unsigned long __init mmu_mapin_ram(unsigned long base, unsigned long top)
{
unsigned long tot, bl, done;
unsigned long max_size = (256<<20);
--
2.13.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox