Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
Benjamin Herrenschmidt b...@kernel.crashing.org wrote on 03/10/2009 12:57:28:
On Sat, 2009-10-03 at 11:24 +0200, Joakim Tjernlund wrote:
So yes, there is a missing _tlbil_va() missing for 8xx somewhere
but there is something more too.
Maybe your new filter functions and my
powerpc, 8xx: DTLB Error must check for more errors.
will do the trick?
Well, if we can't tell between a load and a store on a TLB miss, then
we should probably let it create an unpopulated entry in all cases,
so that we do take a proper DSI/ISI the second time around, which
would then tell us where we come from...
While looking closer on 8xx TLB handling I noticed we were taking
an extra TLB Error when making a page dirty. Tracked it down to:
static inline pte_t pte_mkdirty(pte_t pte) {
pte_val(pte) |= _PAGE_DIRTY; return pte; }
pte_mkdirty does not set HWWRITE thus forcing a new
TLB error to clear it. Adding HWWRITE to pte_mkdirty fixes the
problem.
Looking at (especially pte_mkclean):
static inline pte_t pte_wrprotect(pte_t pte) {
pte_val(pte) = ~(_PAGE_RW | _PAGE_HWWRITE); return pte; }
static inline pte_t pte_mkclean(pte_t pte) {
pte_val(pte) = ~(_PAGE_DIRTY | _PAGE_HWWRITE); return pte; }
it looks like a mistake not to include HWWRITE in pte_mkdirty(),
what do you think?
Jocke
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] sound: Don't assume i2c device probing always succeeds
Hi Takashi,
On Thu, 01 Oct 2009 08:52:59 +0200, Takashi Iwai wrote:
At Wed, 30 Sep 2009 18:55:05 +0200,
Jean Delvare wrote:
On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
Yes, indeed I prefer NULL check because the user can know the error
at the right place. I share your concern about the code addition,
though :)
I already made a patch below, but it's totally untested.
It'd be helpful if someone can do review and build-test it.
thanks,
Takashi
---
diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
index f0ebc97..0f810c8 100644
--- a/sound/aoa/codecs/tas.c
+++ b/sound/aoa/codecs/tas.c
@@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
client = i2c_new_device(adapter, info);
if (!client)
return -ENODEV;
+ if (!client-driver) {
+ i2c_unregister_device(client);
+ return -ENODEV;
+ }
/*
* Let i2c-core delete that device on driver removal.
diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
index 835fa19..473c5a6 100644
--- a/sound/ppc/keywest.c
+++ b/sound/ppc/keywest.c
@@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter
*adapter)
strlcpy(info.type, keywest, I2C_NAME_SIZE);
info.addr = keywest_ctx-addr;
keywest_ctx-client = i2c_new_device(adapter, info);
+ if (!keywest_ctx-client)
+ return -ENODEV;
+ if (!keywest_ctx-client-driver) {
+ i2c_unregister_device(keywest_ctx-client);
+ keywest_ctx-client = NULL;
+ return -ENODEV;
+ }
/*
* Let i2c-core delete that device on driver removal.
This looks good to me. Please add the following comment before the
client-driver check in both drivers:
/*
* We know the driver is already loaded, so the device should be
* already bound. If not it means binding failed, and then there
* is no point in keeping the device instantiated.
*/
Otherwise it's a little difficult to understand why the check is there.
Fair enough. I applied the patch with the comment now.
Thanks!
I see this is upstream now. While the keywest fix was essentially
theoretical, the tas one addresses a crash which really could happen,
so I think it would be worth sending to stable for 2.6.31. What do you
think? Will you take care, or do you want me to?
Thanks,
--
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [patch 1/3] powerpc: Move 64bit VDSO to improve context switch performance
Benjamin Herrenschmidt b...@kernel.crashing.org writes:
Maybe a better fix is to force alignment in the kernel by requesting
size + 64k - 4k and aligning it.
Sure you are right.
Andreas.
From b9441a3d2148d439e2730def3222a7b70dccc432 Mon Sep 17 00:00:00 2001
From: Andreas Schwab sch...@linux-m68k.org
Date: Sun, 4 Oct 2009 14:29:04 +0200
Subject: [PATCH] powerpc: align vDSO base address
Signed-off-by: Andreas Schwab sch...@linux-m68k.org
---
arch/powerpc/kernel/vdso.c | 11 ++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index 94e2df3..137dc22 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -50,6 +50,9 @@
/* Max supported size for symbol names */
#define MAX_SYMNAME64
+/* The alignment of the vDSO */
+#define VDSO_ALIGNMENT (1 16)
+
extern char vdso32_start, vdso32_end;
static void *vdso32_kbase = vdso32_start;
static unsigned int vdso32_pages;
@@ -231,15 +234,21 @@ int arch_setup_additional_pages(struct linux_binprm
*bprm, int uses_interp)
* pick a base address for the vDSO in process space. We try to put it
* at vdso_base which is the natural base for it, but we might fail
* and end up putting it elsewhere.
+* Add enough to the size so that the result can be aligned.
*/
down_write(mm-mmap_sem);
vdso_base = get_unmapped_area(NULL, vdso_base,
- vdso_pages PAGE_SHIFT, 0, 0);
+ (vdso_pages PAGE_SHIFT) +
+ ((VDSO_ALIGNMENT - 1) PAGE_MASK),
+ 0, 0);
if (IS_ERR_VALUE(vdso_base)) {
rc = vdso_base;
goto fail_mmapsem;
}
+ /* Add required alignment. */
+ vdso_base = ALIGN(vdso_base, VDSO_ALIGNMENT);
+
/*
* Put vDSO base into mm struct. We need to do this before calling
* install_special_mapping or the perf counter mmap tracking code
--
1.6.4.4
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
Scott Wood scottw...@freescale.com wrote on 02/10/2009 23:49:49: On Thu, Oct 01, 2009 at 08:35:59AM +1000, Benjamin Herrenschmidt wrote: From what I can see, the TLB miss code will check _PAGE_PRESENT, and when not set, it will -still- insert something into the TLB (unlike all other CPU types that go straight to data access faults from there). So we end up populating with those unpopulated entries that will then cause us to take a DSI (or ISI) instead of a TLB miss the next time around ... and so we need to remove them once we do that, no ? IE. Once we have actually put a valid PTE in. At least that's my understanding and why I believe we should always have tlbil_va() in set_pte() and ptep_set_access_flags(), which boils down in putting it into the 2 filter functions in the new code. Well.. actually, the ptep_set_access_flags() will already do a flush_tlb_page_nohash() when the PTE is changed. So I suppose all we really need here would be in set_pte_filter(), to do the same if we are writing a PTE that has _PAGE_PRESENT, at least on 8xx. But just unconditionally doing a tlbil_va() in both the filter functions shouldn't harm and also fix the problem, though Rex seems to indicate that is not the case. Adding a tlbil_va to do_page_fault makes the problem go away for me (on top of your merge branch) -- none of the other changes in this thread do (assuming I didn't miss any). FWIW, when it gets stuck on a fault, DSISR is 0xc000, and handle_mm_fault returns zero. Scott and Rex I have managed to update the TLB code to make proper use of dirty and accessed states. Advantages are: - I/D TLB Miss never needs to write to the linux pte, saving a few cycles - Accessed is only set by I/D TLB Error, should be a plus when SWAP is used. - _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly and there will be no extra DTLB Error to actually set the changed bit when a page has been made dirty. - Proper RO/RW mapping of user space. Cons: - 4 more insn in TLB Miss handlers, but the since the linux pte isn't written it should still be a win. However, I did this on my 2.4 tree but I can port it to 2.6 if you guys can test it for me. Jocke ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
On Sun, 2009-10-04 at 10:35 +0200, Joakim Tjernlund wrote:
Benjamin Herrenschmidt b...@kernel.crashing.org wrote on 03/10/2009
12:57:28:
On Sat, 2009-10-03 at 11:24 +0200, Joakim Tjernlund wrote:
So yes, there is a missing _tlbil_va() missing for 8xx somewhere
but there is something more too.
Maybe your new filter functions and my
powerpc, 8xx: DTLB Error must check for more errors.
will do the trick?
Well, if we can't tell between a load and a store on a TLB miss, then
we should probably let it create an unpopulated entry in all cases,
so that we do take a proper DSI/ISI the second time around, which
would then tell us where we come from...
While looking closer on 8xx TLB handling I noticed we were taking
an extra TLB Error when making a page dirty. Tracked it down to:
static inline pte_t pte_mkdirty(pte_t pte) {
pte_val(pte) |= _PAGE_DIRTY; return pte; }
pte_mkdirty does not set HWWRITE thus forcing a new
TLB error to clear it. Adding HWWRITE to pte_mkdirty fixes the
problem.
We should just get rid of HWWRITE like I did for 44x and BookE.
Looking at (especially pte_mkclean):
static inline pte_t pte_wrprotect(pte_t pte) {
pte_val(pte) = ~(_PAGE_RW | _PAGE_HWWRITE); return pte; }
static inline pte_t pte_mkclean(pte_t pte) {
pte_val(pte) = ~(_PAGE_DIRTY | _PAGE_HWWRITE); return pte; }
it looks like a mistake not to include HWWRITE in pte_mkdirty(),
what do you think?
Maybe yes. No big deal right now, we have more important problems
to fix no ? :-)
Ben.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
I have managed to update the TLB code to make proper use of dirty and accessed states. Advantages are: - I/D TLB Miss never needs to write to the linux pte, saving a few cycles That's good, that leaves us with only 40x to fix now. Also we can remove atomic updates of PTEs for all non-hash. It's pointless on those CPUs anyway. - Accessed is only set by I/D TLB Error, should be a plus when SWAP is used. No need for that neither. ISI/DSI shouldn't touch the PTE. They should just fall back to C code which takes care of it all.l - _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly and there will be no extra DTLB Error to actually set the changed bit when a page has been made dirty. - Proper RO/RW mapping of user space. Cons: - 4 more insn in TLB Miss handlers, but the since the linux pte isn't written it should still be a win. However, I did this on my 2.4 tree but I can port it to 2.6 if you guys can test it for me. Why don't you use and test 2.6 ? :-) Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
Benjamin Herrenschmidt b...@kernel.crashing.org wrote on 04/10/2009 22:26:42:
On Sun, 2009-10-04 at 10:35 +0200, Joakim Tjernlund wrote:
Benjamin Herrenschmidt b...@kernel.crashing.org wrote on 03/10/2009
12:57:28:
On Sat, 2009-10-03 at 11:24 +0200, Joakim Tjernlund wrote:
So yes, there is a missing _tlbil_va() missing for 8xx somewhere
but there is something more too.
Maybe your new filter functions and my
powerpc, 8xx: DTLB Error must check for more errors.
will do the trick?
Well, if we can't tell between a load and a store on a TLB miss, then
we should probably let it create an unpopulated entry in all cases,
so that we do take a proper DSI/ISI the second time around, which
would then tell us where we come from...
While looking closer on 8xx TLB handling I noticed we were taking
an extra TLB Error when making a page dirty. Tracked it down to:
static inline pte_t pte_mkdirty(pte_t pte) {
pte_val(pte) |= _PAGE_DIRTY; return pte; }
pte_mkdirty does not set HWWRITE thus forcing a new
TLB error to clear it. Adding HWWRITE to pte_mkdirty fixes the
problem.
We should just get rid of HWWRITE like I did for 44x and BookE.
I am trying :)
Looking at (especially pte_mkclean):
static inline pte_t pte_wrprotect(pte_t pte) {
pte_val(pte) = ~(_PAGE_RW | _PAGE_HWWRITE); return pte; }
static inline pte_t pte_mkclean(pte_t pte) {
pte_val(pte) = ~(_PAGE_DIRTY | _PAGE_HWWRITE); return pte; }
it looks like a mistake not to include HWWRITE in pte_mkdirty(),
what do you think?
Maybe yes. No big deal right now, we have more important problems
to fix no ? :-)
The missing invalidate you guys need to work out. I have no clue
where to put it. If we are really lucky, getting rid of HWWRITE
might help :)
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/8xx: fix regression introduced by cache coherency rewrite
Benjamin Herrenschmidt b...@kernel.crashing.org wrote on 04/10/2009 22:28:38: I have managed to update the TLB code to make proper use of dirty and accessed states. Advantages are: - I/D TLB Miss never needs to write to the linux pte, saving a few cycles That's good, that leaves us with only 40x to fix now. Also we can remove atomic updates of PTEs for all non-hash. It's pointless on those CPUs anyway. - Accessed is only set by I/D TLB Error, should be a plus when SWAP is used. No need for that neither. Since 8xx lacks HW support for ACCESSED, the only way is map the page NoAccess and take a TLB Error on first access that sets access bit (or bails to do_page_fault) ISI/DSI shouldn't touch the PTE. They should just fall back to C code which takes care of it all.l Yes, that is what I do now(i.e I only read the pte). ISI and DSI is the TLB Miss handlers on 8xx. - _PAGE_DIRTY is mapped to 0x100, the changed bit, and is set directly and there will be no extra DTLB Error to actually set the changed bit when a page has been made dirty. - Proper RO/RW mapping of user space. Cons: - 4 more insn in TLB Miss handlers, but the since the linux pte isn't written it should still be a win. However, I did this on my 2.4 tree but I can port it to 2.6 if you guys can test it for me. Why don't you use and test 2.6 ? :-) Because porting my 8xx board to 2.6 isn't going to be easy so I havn't yet. One day I might when we can't get away with 2.4 on our old boards. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] sound: Don't assume i2c device probing always succeeds
At Sun, 4 Oct 2009 11:35:21 +0200,
Jean Delvare wrote:
Hi Takashi,
On Thu, 01 Oct 2009 08:52:59 +0200, Takashi Iwai wrote:
At Wed, 30 Sep 2009 18:55:05 +0200,
Jean Delvare wrote:
On Wed, 30 Sep 2009 17:15:49 +0200, Takashi Iwai wrote:
Yes, indeed I prefer NULL check because the user can know the error
at the right place. I share your concern about the code addition,
though :)
I already made a patch below, but it's totally untested.
It'd be helpful if someone can do review and build-test it.
thanks,
Takashi
---
diff --git a/sound/aoa/codecs/tas.c b/sound/aoa/codecs/tas.c
index f0ebc97..0f810c8 100644
--- a/sound/aoa/codecs/tas.c
+++ b/sound/aoa/codecs/tas.c
@@ -897,6 +897,10 @@ static int tas_create(struct i2c_adapter *adapter,
client = i2c_new_device(adapter, info);
if (!client)
return -ENODEV;
+ if (!client-driver) {
+ i2c_unregister_device(client);
+ return -ENODEV;
+ }
/*
* Let i2c-core delete that device on driver removal.
diff --git a/sound/ppc/keywest.c b/sound/ppc/keywest.c
index 835fa19..473c5a6 100644
--- a/sound/ppc/keywest.c
+++ b/sound/ppc/keywest.c
@@ -59,6 +59,13 @@ static int keywest_attach_adapter(struct i2c_adapter
*adapter)
strlcpy(info.type, keywest, I2C_NAME_SIZE);
info.addr = keywest_ctx-addr;
keywest_ctx-client = i2c_new_device(adapter, info);
+ if (!keywest_ctx-client)
+ return -ENODEV;
+ if (!keywest_ctx-client-driver) {
+ i2c_unregister_device(keywest_ctx-client);
+ keywest_ctx-client = NULL;
+ return -ENODEV;
+ }
/*
* Let i2c-core delete that device on driver removal.
This looks good to me. Please add the following comment before the
client-driver check in both drivers:
/*
* We know the driver is already loaded, so the device should be
* already bound. If not it means binding failed, and then there
* is no point in keeping the device instantiated.
*/
Otherwise it's a little difficult to understand why the check is there.
Fair enough. I applied the patch with the comment now.
Thanks!
I see this is upstream now. While the keywest fix was essentially
theoretical, the tas one addresses a crash which really could happen,
so I think it would be worth sending to stable for 2.6.31. What do you
think? Will you take care, or do you want me to?
Agreed, it's safer to send the patch to stable tree.
I'm going to submit it.
thanks,
Takashi
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
