* [PATCH v2 01/12] memory-hotplug: try to offline the memory twice to avoid dependence
From: wency @ 2012-10-23 10:30 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: len.brown, Wen Congyang, isimatu.yasuaki, paulus, minchan.kim,
kosaki.motohiro, rientjes, cl, akpm, liuj97
In-Reply-To: <1350988250-31294-1-git-send-email-wency@cn.fujitsu.com>
From: Wen Congyang <wency@cn.fujitsu.com>
memory can't be offlined when CONFIG_MEMCG is selected.
For example: there is a memory device on node 1. The address range
is [1G, 1.5G). You will find 4 new directories memory8, memory9, memory10,
and memory11 under the directory /sys/devices/system/memory/.
If CONFIG_MEMCG is selected, we will allocate memory to store page cgroup
when we online pages. When we online memory8, the memory stored page cgroup
is not provided by this memory device. But when we online memory9, the memory
stored page cgroup may be provided by memory8. So we can't offline memory8
now. We should offline the memory in the reversed order.
When the memory device is hotremoved, we will auto offline memory provided
by this memory device. But we don't know which memory is onlined first, so
offlining memory may fail. In such case, iterate twice to offline the memory.
1st iterate: offline every non primary memory block.
2nd iterate: offline primary (i.e. first added) memory block.
This idea is suggested by KOSAKI Motohiro.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
mm/memory_hotplug.c | 16 ++++++++++++++--
1 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 56b758a..600e200 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1019,10 +1019,13 @@ int remove_memory(u64 start, u64 size)
unsigned long start_pfn, end_pfn;
unsigned long pfn, section_nr;
int ret;
+ int return_on_error = 0;
+ int retry = 0;
start_pfn = PFN_DOWN(start);
end_pfn = start_pfn + PFN_DOWN(size);
+repeat:
for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
section_nr = pfn_to_section_nr(pfn);
if (!present_section_nr(section_nr))
@@ -1041,14 +1044,23 @@ int remove_memory(u64 start, u64 size)
ret = offline_memory_block(mem);
if (ret) {
- kobject_put(&mem->dev.kobj);
- return ret;
+ if (return_on_error) {
+ kobject_put(&mem->dev.kobj);
+ return ret;
+ } else {
+ retry = 1;
+ }
}
}
if (mem)
kobject_put(&mem->dev.kobj);
+ if (retry) {
+ return_on_error = 1;
+ goto repeat;
+ }
+
return 0;
}
#else
--
1.7.1
^ permalink raw reply related
* [PATCH v2 03/12] memory-hotplug: remove redundant codes
From: wency @ 2012-10-23 10:30 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: len.brown, Wen Congyang, isimatu.yasuaki, paulus, minchan.kim,
kosaki.motohiro, rientjes, cl, akpm, liuj97
In-Reply-To: <1350988250-31294-1-git-send-email-wency@cn.fujitsu.com>
From: Wen Congyang <wency@cn.fujitsu.com>
offlining memory blocks and checking whether memory blocks are offlined
are very similar. This patch introduces a new function to remove
redundant codes.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
mm/memory_hotplug.c | 101 ++++++++++++++++++++++++++++-----------------------
1 files changed, 55 insertions(+), 46 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f4fdedd..80fc70c 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1012,20 +1012,14 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
return __offline_pages(start_pfn, start_pfn + nr_pages, 120 * HZ);
}
-int remove_memory(u64 start, u64 size)
+static int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
+ void *arg, int (*func)(struct memory_block *, void *))
{
struct memory_block *mem = NULL;
struct mem_section *section;
- unsigned long start_pfn, end_pfn;
unsigned long pfn, section_nr;
int ret;
- int return_on_error = 0;
- int retry = 0;
-
- start_pfn = PFN_DOWN(start);
- end_pfn = start_pfn + PFN_DOWN(size);
-repeat:
for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
section_nr = pfn_to_section_nr(pfn);
if (!present_section_nr(section_nr))
@@ -1042,22 +1036,61 @@ repeat:
if (!mem)
continue;
- ret = offline_memory_block(mem);
+ ret = func(mem, arg);
if (ret) {
- if (return_on_error) {
- kobject_put(&mem->dev.kobj);
- return ret;
- } else {
- retry = 1;
- }
+ kobject_put(&mem->dev.kobj);
+ return ret;
}
}
if (mem)
kobject_put(&mem->dev.kobj);
- if (retry) {
- return_on_error = 1;
+ return 0;
+}
+
+static int offline_memory_block_cb(struct memory_block *mem, void *arg)
+{
+ int *ret = arg;
+ int error = offline_memory_block(mem);
+
+ if (error != 0 && *ret == 0)
+ *ret = error;
+
+ return 0;
+}
+
+static int is_memblock_offlined_cb(struct memory_block *mem, void *arg)
+{
+ int ret = !is_memblock_offlined(mem);
+
+ if (unlikely(ret))
+ pr_warn("removing memory fails, because memory "
+ "[%#010llx-%#010llx] is onlined\n",
+ PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)),
+ PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1))-1);
+
+ return ret;
+}
+
+int remove_memory(u64 start, u64 size)
+{
+ unsigned long start_pfn, end_pfn;
+ int ret = 0;
+ int retry = 1;
+
+ start_pfn = PFN_DOWN(start);
+ end_pfn = start_pfn + PFN_DOWN(size);
+
+repeat:
+ walk_memory_range(start_pfn, end_pfn, &ret,
+ offline_memory_block_cb);
+ if (ret) {
+ if (!retry)
+ return ret;
+
+ retry = 0;
+ ret = 0;
goto repeat;
}
@@ -1075,37 +1108,13 @@ repeat:
* memory blocks are offlined.
*/
- for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
- section_nr = pfn_to_section_nr(pfn);
- if (!present_section_nr(section_nr))
- continue;
-
- section = __nr_to_section(section_nr);
- /* same memblock? */
- if (mem)
- if ((section_nr >= mem->start_section_nr) &&
- (section_nr <= mem->end_section_nr))
- continue;
-
- mem = find_memory_block_hinted(section, mem);
- if (!mem)
- continue;
-
- ret = is_memblock_offlined(mem);
- if (!ret) {
- pr_warn("removing memory fails, because memory "
- "[%#010llx-%#010llx] is onlined\n",
- PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)),
- PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1)) - 1);
-
- kobject_put(&mem->dev.kobj);
- unlock_memory_hotplug();
- return ret;
- }
+ ret = walk_memory_range(start_pfn, end_pfn, NULL,
+ is_memblock_offlined_cb);
+ if (ret) {
+ unlock_memory_hotplug();
+ return ret;
}
- if (mem)
- kobject_put(&mem->dev.kobj);
unlock_memory_hotplug();
return 0;
--
1.7.1
^ permalink raw reply related
* [RFC PATCH powerpc] Fix MAX_STACK_TRACE_ENTRIES too low!
From: Li Zhong @ 2012-10-23 9:46 UTC (permalink / raw)
To: PowerPC email list; +Cc: Paul Mackerras
This patch tries to fix the following BUG report:
[ 0.012313] BUG: MAX_STACK_TRACE_ENTRIES too low!
[ 0.012318] turning off the locking correctness validator.
[ 0.012321] Call Trace:
[ 0.012330] [c00000017666f6d0] [c000000000012128] .show_stack+0x78/0x184 (unreliable)
[ 0.012339] [c00000017666f780] [c0000000000b6348] .save_trace+0x12c/0x14c
[ 0.012345] [c00000017666f800] [c0000000000b7448] .mark_lock+0x2bc/0x710
[ 0.012351] [c00000017666f8b0] [c0000000000bb198] .__lock_acquire+0x748/0xaec
[ 0.012357] [c00000017666f9b0] [c0000000000bb684] .lock_acquire+0x148/0x194
[ 0.012365] [c00000017666fa80] [c00000000069371c] .mutex_lock_nested+0x84/0x4ec
[ 0.012372] [c00000017666fb90] [c000000000096998] .smpboot_register_percpu_thread+0x3c/0x10c
[ 0.012380] [c00000017666fc30] [c0000000009ba910] .spawn_ksoftirqd+0x28/0x48
[ 0.012386] [c00000017666fcb0] [c00000000000a98c] .do_one_initcall+0xd8/0x1d0
[ 0.012392] [c00000017666fd60] [c00000000000b1f8] .kernel_init+0x120/0x398
[ 0.012398] [c00000017666fe30] [c000000000009ad4] .ret_from_kernel_thread+0x5c/0x64
[ 0.012404] [c00000017666fa00] [c00000017666fb20] 0xc00000017666fb20
[ 0.012410] [c00000017666fa80] [c00000000069371c] .mutex_lock_nested+0x84/0x4ec
[ 0.012416] [c00000017666fb90] [c000000000096998] .smpboot_register_percpu_thread+0x3c/0x10c
[ 0.012422] [c00000017666fc30] [c0000000009ba910] .spawn_ksoftirqd+0x28/0x48
[ 0.012427] [c00000017666fcb0] [c00000000000a98c] .do_one_initcall+0xd8/0x1d0
[ 0.012433] [c00000017666fd60] [c00000000000b1f8] .kernel_init+0x120/0x398
[ 0.012439] [c00000017666fe30] [c000000000009ad4] .ret_from_kernel_thread+0x5c/0x64
.......
The reason is that the back chain of c00000017666fe30
(ret_from_kernel_thread) contains some invalid value, which might form a
loop.
Not very sure whether it is good to clear the back chain to 0 here.
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
arch/powerpc/kernel/entry_64.S | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/kernel/entry_64.S
b/arch/powerpc/kernel/entry_64.S
index 56e0ff0..ad76666 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -374,6 +374,8 @@ _GLOBAL(ret_from_kernel_thread)
bl .schedule_tail
REST_NVGPRS(r1)
REST_GPR(2,r1)
+ li r3,0
+ std r3,0(r1)
mtlr r14
mr r3,r15
blrl
--
1.7.1
^ permalink raw reply related
* RE: [PATCH] powerpc/esdhc: enable the card insert/remove interrupt
From: Huang Changming-R66093 @ 2012-10-23 8:39 UTC (permalink / raw)
To: Anton Vorontsov
Cc: linuxppc-dev@lists.ozlabs.org, Chris Ball,
linux-mmc@vger.kernel.org
In-Reply-To: <20121023082648.GA21690@lizard>
SGksIEFudG9uDQpGb3IgRlNMIFNPQ3MsIHRoZSBpbnRlcnJ1cHQgbW9kZSBpcyBzdXBwb3J0IGZv
ciBjYXJkIGRldGVjdC4NCkZvciBzb21lIHJlYXNvbnMsIHNvbWUgYm9hcmQgY2FuJ3Qgc3VwcG9y
dCB0aGlzIGZlYXR1cmUsIHN1Y2ggYXMgbXBjODM3eG1kcyBib2FyZCwNCmJ1dCBtcGM4Mzd4cmRi
IGJvYXJkIHN1cHBvcnQgaXQuDQoNCkkgc2VlIHlvdXIgbGlua3MsIGl0IGlzIGFib3V0IHRoZSBE
TUEgKHRoZSBmaXJzdCB2ZXJzaW9uIG9mIHAyMDIwIGRvbid0IHN1cHBvcnQgRE1BLCBuZWVkIHRv
IGVuYWJsZSB0aGUgUElPIG1vZGUsIGJ1dCB0aGUgbGF0ZXIgdmVyc2lvbiBoYXMgZml4ZWQgdGhp
cyBpc3N1ZSkuDQoNCkJlc3QgUmVnYXJkcw0KSmVycnkgSHVhbmcNCg0KDQo+IC0tLS0tT3JpZ2lu
YWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IGxpbnV4LW1tYy1vd25lckB2Z2VyLmtlcm5lbC5vcmcg
W21haWx0bzpsaW51eC1tbWMtDQo+IG93bmVyQHZnZXIua2VybmVsLm9yZ10gT24gQmVoYWxmIE9m
IEFudG9uIFZvcm9udHNvdg0KPiBTZW50OiBUdWVzZGF5LCBPY3RvYmVyIDIzLCAyMDEyIDQ6Mjcg
UE0NCj4gVG86IEh1YW5nIENoYW5nbWluZy1SNjYwOTMNCj4gQ2M6IGxpbnV4LW1tY0B2Z2VyLmtl
cm5lbC5vcmc7IEh1YW5nIENoYW5nbWluZy1SNjYwOTM7IENocmlzIEJhbGw7IEt1bWFyDQo+IEdh
bGE7IGxpbnV4cHBjLWRldkBsaXN0cy5vemxhYnMub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0hd
IHBvd2VycGMvZXNkaGM6IGVuYWJsZSB0aGUgY2FyZCBpbnNlcnQvcmVtb3ZlDQo+IGludGVycnVw
dA0KPiANCj4gT24gVHVlLCBPY3QgMjMsIDIwMTIgYXQgMDM6MDE6MTdQTSArMDgwMCwgcjY2MDkz
QGZyZWVzY2FsZS5jb20gd3JvdGU6DQo+ID4gRnJvbTogSmVycnkgSHVhbmcgPENoYW5nLU1pbmcu
SHVhbmdAZnJlZXNjYWxlLmNvbT4NCj4gPg0KPiA+IFRoZSBjdXJyZW50IGVTREhDIGRyaXZlciB1
c2UgdGhlIHBvbGwgbW9kZSB0byBkZXRlY3QgaWYgdGhlIFNEL01NQw0KPiA+IGNhcmQgaXMgaW5z
ZXJ0ZWQgb3IgcmVtb3ZlZCwgd2hpY2ggd2lsbCBnZW5lcmF0ZSBtYW55IGludGVycnVwdHMgYW5k
DQo+ID4gaW1wYWN0IHRoZSBwZXJmb3JtYW5jZS4NCj4gPiBUaGVyZWZvcmUsIGNoYW5nZSB0aGUg
ZGVmYXVsdCBjYXJkIGRldGVjdCB0byBpbnRlcnJ1cHQgbW9kZSwgaWYgdGhlDQo+ID4gYm9hcmQg
Y2FuJ3Qgc3VwcG9ydCB0aGlzIG1vZGUsIHdlIHN0aWxsIHVzZSB0aGUgcG9sbCBtb2RlLg0KPiA+
DQo+ID4gU2lnbmVkLW9mZi1ieTogSmVycnkgSHVhbmcgPENoYW5nLU1pbmcuSHVhbmdAZnJlZXNj
YWxlLmNvbT4NCj4gPiBDQzogQW50b24gVm9yb250c292IDxjYm91YXRtYWlscnVAZ21haWwuY29t
Pg0KPiA+IENDOiBDaHJpcyBCYWxsIDxjamJAbGFwdG9wLm9yZz4NCj4gPiAtLS0NCj4gDQo+IElJ
UkMsIHRoZSBjYXJkIGRldGVjdGlvbiBpcyBicm9rZW4gU09DLXJldmlzaW9uLXdpc2UsIG5vdCBi
b2FyZC13aXNlLiBTbw0KPiB0aGUgY2hhbmdlIHNlZW1zIHdyb25nLg0KPiANCj4gQWxzbywgdGFr
ZSBhIGxvb2sgYXQgdGhpczoNCj4gDQo+IAlodHRwOi8vbGttbC5vcmcvbGttbC8yMDEwLzcvMTQv
MTI3DQo+IA0KPiBJIHN0YXJ0ZWQgdGhlIHdvcmsgYnV0IG5ldmVyIGZpbmlzaGVkLCB1bmZvcnR1
bmF0ZWx5IGl0IGNhdXNlZCBzb21lDQo+IHJlZ3Jlc3Npb25zIGZvciBub24tRlNMIGhhcmR3YXJl
Li4uDQo+IA0KPiA+ICBkcml2ZXJzL21tYy9ob3N0L3NkaGNpLW9mLWVzZGhjLmMgfCAgICA3ICsr
KysrKy0NCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDYgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigt
KQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbW1jL2hvc3Qvc2RoY2ktb2YtZXNkaGMu
Yw0KPiA+IGIvZHJpdmVycy9tbWMvaG9zdC9zZGhjaS1vZi1lc2RoYy5jDQo+ID4gaW5kZXggZmZj
MTIyNi4uNWRjMzYyZiAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL21tYy9ob3N0L3NkaGNpLW9m
LWVzZGhjLmMNCj4gPiArKysgYi9kcml2ZXJzL21tYy9ob3N0L3NkaGNpLW9mLWVzZGhjLmMNCj4g
PiBAQCAtMTk2LDYgKzE5NiwxMSBAQCBzdGF0aWMgdm9pZCBlc2RoY19vZl9kZXRlY3RfbGltaXRh
dGlvbihzdHJ1Y3QNCj4gcGxhdGZvcm1fZGV2aWNlICpwZGV2LA0KPiA+ICAJaWYgKHZ2biA9PSBW
RU5ET1JfVl8yMikNCj4gPiAgCQlwZGF0YS0+cXVpcmtzMiB8PSBTREhDSV9RVUlSSzJfSE9TVF9O
T19DTUQyMzsNCj4gPg0KPiA+ICsJLyogUDQwODBEUyBhbmQgTVBDODM3WE1EUyBib2FyZCBkb24n
dCBzdXBwb3J0IGludGVycnVwdCBtb2RlICovDQo+ID4gKwlpZiAob2ZfbWFjaGluZV9pc19jb21w
YXRpYmxlKCJmc2wsbXBjODM3eG1kcyIpIHx8DQo+ID4gKwkgICAgb2ZfbWFjaGluZV9pc19jb21w
YXRpYmxlKCJmc2wsUDQwODBEUyIpKQ0KPiA+ICsJCXBkYXRhLT5xdWlya3MgfD0gU0RIQ0lfUVVJ
UktfQlJPS0VOX0NBUkRfREVURUNUSU9OOw0KPiA+ICsNCj4gPiAgCWlvdW5tYXAoaW9hZGRyKTsN
Cj4gPiAgZW5kOg0KPiA+ICAJcmV0dXJuOw0KPiA+IEBAIC0yMjMsNyArMjI4LDcgQEAgc3RhdGlj
IHN0cnVjdCBzZGhjaV9wbHRmbV9kYXRhIHNkaGNpX2VzZGhjX3BkYXRhID0NCj4gew0KPiA+ICAJ
ICogY2FyZCBkZXRlY3Rpb24gY291bGQgYmUgaGFuZGxlZCB2aWEgR1BJTw0KPiA+ICAJICogZVNE
SEMgY2Fubm90IHN1cHBvcnQgRW5kIEF0dHJpYnV0ZSBpbiBOT1AgQURNQSBkZXNjcmlwdG9yDQo+
ID4gIAkgKi8NCj4gPiAtCS5xdWlya3MgPSBFU0RIQ19ERUZBVUxUX1FVSVJLUyB8IFNESENJX1FV
SVJLX0JST0tFTl9DQVJEX0RFVEVDVElPTg0KPiA+ICsJLnF1aXJrcyA9IEVTREhDX0RFRkFVTFRf
UVVJUktTDQo+ID4gIAkJfCBTREhDSV9RVUlSS19OT19DQVJEX05PX1JFU0VUDQo+ID4gIAkJfCBT
REhDSV9RVUlSS19OT19FTkRBVFRSX0lOX05PUERFU0MsDQo+ID4gIAkub3BzID0gJnNkaGNpX2Vz
ZGhjX29wcywNCj4gPiAtLQ0KPiA+IDEuNy45LjUNCj4gLS0NCj4gVG8gdW5zdWJzY3JpYmUgZnJv
bSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4LW1tYyIgaW4NCj4g
dGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5vcmcgTW9yZSBt
YWpvcmRvbW8gaW5mbyBhdA0KPiBodHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9tby1pbmZv
Lmh0bWwNCg0K
^ permalink raw reply
* Re: [PATCH] powerpc/esdhc: enable the card insert/remove interrupt
From: Anton Vorontsov @ 2012-10-23 8:26 UTC (permalink / raw)
To: r66093; +Cc: Jerry Huang, Chris Ball, linux-mmc, linuxppc-dev
In-Reply-To: <1350975677-8195-1-git-send-email-r66093@freescale.com>
On Tue, Oct 23, 2012 at 03:01:17PM +0800, r66093@freescale.com wrote:
> From: Jerry Huang <Chang-Ming.Huang@freescale.com>
>
> The current eSDHC driver use the poll mode to detect
> if the SD/MMC card is inserted or removed, which will generate
> many interrupts and impact the performance.
> Therefore, change the default card detect to interrupt mode,
> if the board can't support this mode, we still use the poll mode.
>
> Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> CC: Anton Vorontsov <cbouatmailru@gmail.com>
> CC: Chris Ball <cjb@laptop.org>
> ---
IIRC, the card detection is broken SOC-revision-wise, not board-wise. So
the change seems wrong.
Also, take a look at this:
http://lkml.org/lkml/2010/7/14/127
I started the work but never finished, unfortunately it caused some
regressions for non-FSL hardware...
> drivers/mmc/host/sdhci-of-esdhc.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index ffc1226..5dc362f 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -196,6 +196,11 @@ static void esdhc_of_detect_limitation(struct platform_device *pdev,
> if (vvn == VENDOR_V_22)
> pdata->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23;
>
> + /* P4080DS and MPC837XMDS board don't support interrupt mode */
> + if (of_machine_is_compatible("fsl,mpc837xmds") ||
> + of_machine_is_compatible("fsl,P4080DS"))
> + pdata->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +
> iounmap(ioaddr);
> end:
> return;
> @@ -223,7 +228,7 @@ static struct sdhci_pltfm_data sdhci_esdhc_pdata = {
> * card detection could be handled via GPIO
> * eSDHC cannot support End Attribute in NOP ADMA descriptor
> */
> - .quirks = ESDHC_DEFAULT_QUIRKS | SDHCI_QUIRK_BROKEN_CARD_DETECTION
> + .quirks = ESDHC_DEFAULT_QUIRKS
> | SDHCI_QUIRK_NO_CARD_NO_RESET
> | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> .ops = &sdhci_esdhc_ops,
> --
> 1.7.9.5
^ permalink raw reply
* Re: [PATCH 8/10] memory-hotplug : remove page table of x86_64 architecture
From: Wen Congyang @ 2012-10-23 7:41 UTC (permalink / raw)
To: wujianguo
Cc: linux-s390, linux-ia64, jiang.liu, len.brown, linux-acpi,
linux-sh, wujianguo, x86, linux-kernel, cmetcalf, linux-mm,
Yasuaki Ishimatsu, minchan.kim, kosaki.motohiro, rientjes,
sparclinux, qiuxishi, cl, linuxppc-dev, akpm, liuj97
In-Reply-To: <50864297.9010708@gmail.com>
At 10/23/2012 03:09 PM, wujianguo Wrote:
> On 2012-10-22 15:11, Wen Congyang wrote:
>> Hi, Wu
>>
>> Sorry for late reply.
>>
>> At 10/09/2012 04:26 PM, wujianguo Wrote:
>>> Hi Congyang,
>>> I think we should also free pages which are used by page tables after removing
>>> page tables of the memory.
>>
>> It is OK to do it.
>>
>>>
>>> From: Jianguo Wu <wujianguo@huawei.com>
>>>
>>> Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>> ---
>>> arch/x86/mm/init_64.c | 110 +++++++++++++++++++++++++++++++++++++++---------
>>> 1 files changed, 89 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>>> index 5596dfa..81f9c3b 100644
>>> --- a/arch/x86/mm/init_64.c
>>> +++ b/arch/x86/mm/init_64.c
>>> @@ -675,6 +675,74 @@ int arch_add_memory(int nid, u64 start, u64 size)
>>> }
>>> EXPORT_SYMBOL_GPL(arch_add_memory);
>>>
>>> +static inline void free_pagetable(struct page *page)
>>> +{
>>> + struct zone *zone;
>>> +
>>> + __ClearPageReserved(page);
>>> + __free_page(page);
>>> +
>>> + zone = page_zone(page);
>>> + zone_span_writelock(zone);
>>> + zone->present_pages++;
>>> + zone_span_writeunlock(zone);
>>> + totalram_pages++;
>>
>> Why do you update zone and totalram_pages here?
> Sorry, I made a mistake here. Only if the page was allocated at booting, we should update
> zone and totalram_pages(zone->present_pages and totalram_pages mean pages which are
> managed by buddy system).
>
> How about:
> static inline void free_pagetable(struct page *page)
> {
> struct zone *zone;
> bool bootmem = false;
>
> /* bootmem page has reserved flag */
> if (PageReserved(page)) {
> __ClearPageReserved(page);
> bootmem = true;
> }
>
> __free_page(page);
>
> if (bootmem) {
> zone = page_zone(page);
> zone_span_writelock(zone);
> zone->present_pages++;
> zone_span_writeunlock(zone);
> totalram_pages++;
> }
> }
This verson looks fine to me.
>
>>
>>> +}
>>> +
>>> +static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
>>> +{
>>> + pte_t *pte;
>>> + int i;
>>> +
>>> + for (i = 0; i < PTRS_PER_PTE; i++) {
>>> + pte = pte_start + i;
>>> + if (pte_val(*pte))
>>> + break;
>>> + }
>>> +
>>> + /* free a pte talbe */
>>> + if (i == PTRS_PER_PTE) {
>>> + free_pagetable(pmd_page(*pmd));
>>
>> The memory may be allocated at booting. So it is very dangerous to
>> free it without any check.
> The page is only used by page table, so is safe to free it when all the page table
> entries have been cleared, right?
Yes, but I guess we need to do more. For example, all boot memory are in memblock.reserved,
and you don't update memblock here.
Thanks
Wen Congyang
>
>>
>>> + pmd_clear(pmd);
>>> + }
>>> +}
>>> +
>>> +static void free_pmd_table(pmd_t *pmd_start, pud_t *pud)
>>> +{
>>> + pmd_t *pmd;
>>> + int i;
>>> +
>>> + for (i = 0; i < PTRS_PER_PMD; i++) {
>>> + pmd = pmd_start + i;
>>> + if (pmd_val(*pmd))
>>> + break;
>>> + }
>>> +
>>> + /* free a pmd talbe */
>>> + if (i == PTRS_PER_PMD) {
>>> + free_pagetable(pud_page(*pud));
>>> + pud_clear(pud);
>>> + }
>>> +}
>>> +
>>> +static void free_pud_table(pud_t *pud_start, pgd_t *pgd)
>>> +{
>>> + pud_t *pud;
>>> + int i;
>>> +
>>> + for (i = 0; i < PTRS_PER_PUD; i++) {
>>> + pud = pud_start + i;
>>> + if (pud_val(*pud))
>>> + break;
>>> + }
>>> +
>>> + /* free a pud table */
>>> + if (i == PTRS_PER_PUD) {
>>> + free_pagetable(pgd_page(*pgd));
>>> + pgd_clear(pgd);
>>> + }
>>> +}
>>> +
>>> static void __meminit
>>> phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
>>> {
>>> @@ -704,21 +772,19 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>>> unsigned long pages = 0, next;
>>> int i = pmd_index(addr);
>>>
>>> - for (; i < PTRS_PER_PMD; i++, addr = next) {
>>> + for (; i < PTRS_PER_PMD && addr < end; i++, addr = next) {
>>> unsigned long pte_phys;
>>> pmd_t *pmd = pmd_page + pmd_index(addr);
>>> pte_t *pte;
>>>
>>> - if (addr >= end)
>>> - break;
>>> -
>>> - next = (addr & PMD_MASK) + PMD_SIZE;
>>> + next = pmd_addr_end(addr, end);
>>>
>>> if (!pmd_present(*pmd))
>>> continue;
>>>
>>> if (pmd_large(*pmd)) {
>>> - if ((addr & ~PMD_MASK) == 0 && next <= end) {
>>> + if (IS_ALIGNED(addr, PMD_SIZE) &&
>>> + IS_ALIGNED(next, PMD_SIZE)) {
>>> set_pmd(pmd, __pmd(0));
>>> pages++;
>>> continue;
>>> @@ -729,7 +795,8 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>>> * so split 2M page to 4K page.
>>> */
>>> pte = alloc_low_page(&pte_phys);
>>> - __split_large_page((pte_t *)pmd, addr, pte);
>>> + __split_large_page((pte_t *)pmd,
>>> + (unsigned long)__va(addr), pte);
>>>
>>> spin_lock(&init_mm.page_table_lock);
>>> pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
>>> @@ -738,7 +805,8 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>>>
>>> spin_lock(&init_mm.page_table_lock);
>>> pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
>>> - phys_pte_remove(pte, addr, end);
>>> + phys_pte_remove(pte, addr, next);
>>> + free_pte_table(pte, pmd);
>>> unmap_low_page(pte);
>>> spin_unlock(&init_mm.page_table_lock);
>>> }
>>> @@ -751,21 +819,19 @@ phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
>>> unsigned long pages = 0, next;
>>> int i = pud_index(addr);
>>>
>>> - for (; i < PTRS_PER_PUD; i++, addr = next) {
>>> + for (; i < PTRS_PER_PUD && addr < end; i++, addr = next) {
>>> unsigned long pmd_phys;
>>> pud_t *pud = pud_page + pud_index(addr);
>>> pmd_t *pmd;
>>>
>>> - if (addr >= end)
>>> - break;
>>> -
>>> - next = (addr & PUD_MASK) + PUD_SIZE;
>>> + next = pud_addr_end(addr, end);
>>>
>>> if (!pud_present(*pud))
>>> continue;
>>>
>>> if (pud_large(*pud)) {
>>> - if ((addr & ~PUD_MASK) == 0 && next <= end) {
>>> + if (IS_ALIGNED(addr, PUD_SIZE) &&
>>> + IS_ALIGNED(next, PUD_SIZE)) {
>>> set_pud(pud, __pud(0));
>>> pages++;
>>> continue;
>>> @@ -776,15 +842,18 @@ phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
>>> * so split 1G page to 2M page.
>>> */
>>> pmd = alloc_low_page(&pmd_phys);
>>> - __split_large_page((pte_t *)pud, addr, (pte_t *)pmd);
>>> + __split_large_page((pte_t *)pud,
>>> + (unsigned long)__va(addr),
>>> + (pte_t *)pmd);
>>>
>>> spin_lock(&init_mm.page_table_lock);
>>> pud_populate(&init_mm, pud, __va(pmd_phys));
>>> spin_unlock(&init_mm.page_table_lock);
>>> }
>>>
>>> - pmd = map_low_page(pmd_offset(pud, 0));
>>> - phys_pmd_remove(pmd, addr, end);
>>> + pmd = map_low_page((pmd_t *)pud_page_vaddr(*pud));
>>
>> Hmm, pmd_offset(pud, 0) is equal to (pmd_t *)pud_page_vaddr(*pud).
>>
>> Is it OK to merge your patch into my patch?
>>
> Yes, sure.
>
> Thanks,
> Jianguo Wu
>
>> Thanks
>> Wen Congyang
>>
>>> + phys_pmd_remove(pmd, addr, next);
>>> + free_pmd_table(pmd, pud);
>>> unmap_low_page(pmd);
>>> __flush_tlb_all();
>>> }
>>> @@ -805,15 +874,14 @@ kernel_physical_mapping_remove(unsigned long start, unsigned long end)
>>> pgd_t *pgd = pgd_offset_k(start);
>>> pud_t *pud;
>>>
>>> - next = (start + PGDIR_SIZE) & PGDIR_MASK;
>>> - if (next > end)
>>> - next = end;
>>> + next = pgd_addr_end(start, end);
>>>
>>> if (!pgd_present(*pgd))
>>> continue;
>>>
>>> pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
>>> - phys_pud_remove(pud, __pa(start), __pa(end));
>>> + phys_pud_remove(pud, __pa(start), __pa(next));
>>> + free_pud_table(pud, pgd);
>>> unmap_low_page(pud);
>>> }
>>>
>>> -- 1.7.6.1 .
>>>
>>>
>>> On 2012-10-5 10:36, Yasuaki Ishimatsu wrote:
>>>> From: Wen Congyang <wency@cn.fujitsu.com>
>>>>
>>>> For hot removing memory, we sholud remove page table about the memory.
>>>> So the patch searches a page table about the removed memory, and clear
>>>> page table.
>>>>
>>>> CC: David Rientjes <rientjes@google.com>
>>>> CC: Jiang Liu <liuj97@gmail.com>
>>>> CC: Len Brown <len.brown@intel.com>
>>>> CC: Christoph Lameter <cl@linux.com>
>>>> Cc: Minchan Kim <minchan.kim@gmail.com>
>>>> CC: Andrew Morton <akpm@linux-foundation.org>
>>>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>>> CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>>> ---
>>>> arch/x86/include/asm/pgtable_types.h | 1
>>>> arch/x86/mm/init_64.c | 147 +++++++++++++++++++++++++++++++++++
>>>> arch/x86/mm/pageattr.c | 47 +++++------
>>>> 3 files changed, 173 insertions(+), 22 deletions(-)
>>>>
>>>> Index: linux-3.6/arch/x86/mm/init_64.c
>>>> ===================================================================
>>>> --- linux-3.6.orig/arch/x86/mm/init_64.c 2012-10-04 18:30:21.171698416 +0900
>>>> +++ linux-3.6/arch/x86/mm/init_64.c 2012-10-04 18:30:27.317704652 +0900
>>>> @@ -675,6 +675,151 @@ int arch_add_memory(int nid, u64 start,
>>>> }
>>>> EXPORT_SYMBOL_GPL(arch_add_memory);
>>>>
>>>> +static void __meminit
>>>> +phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
>>>> +{
>>>> + unsigned pages = 0;
>>>> + int i = pte_index(addr);
>>>> +
>>>> + pte_t *pte = pte_page + pte_index(addr);
>>>> +
>>>> + for (; i < PTRS_PER_PTE; i++, addr += PAGE_SIZE, pte++) {
>>>> +
>>>> + if (addr >= end)
>>>> + break;
>>>> +
>>>> + if (!pte_present(*pte))
>>>> + continue;
>>>> +
>>>> + pages++;
>>>> + set_pte(pte, __pte(0));
>>>> + }
>>>> +
>>>> + update_page_count(PG_LEVEL_4K, -pages);
>>>> +}
>>>> +
>>>> +static void __meminit
>>>> +phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>>>> +{
>>>> + unsigned long pages = 0, next;
>>>> + int i = pmd_index(addr);
>>>> +
>>>> + for (; i < PTRS_PER_PMD; i++, addr = next) {
>>>> + unsigned long pte_phys;
>>>> + pmd_t *pmd = pmd_page + pmd_index(addr);
>>>> + pte_t *pte;
>>>> +
>>>> + if (addr >= end)
>>>> + break;
>>>> +
>>>> + next = (addr & PMD_MASK) + PMD_SIZE;
>>>> +
>>>> + if (!pmd_present(*pmd))
>>>> + continue;
>>>> +
>>>> + if (pmd_large(*pmd)) {
>>>> + if ((addr & ~PMD_MASK) == 0 && next <= end) {
>>>> + set_pmd(pmd, __pmd(0));
>>>> + pages++;
>>>> + continue;
>>>> + }
>>>> +
>>>> + /*
>>>> + * We use 2M page, but we need to remove part of them,
>>>> + * so split 2M page to 4K page.
>>>> + */
>>>> + pte = alloc_low_page(&pte_phys);
>>>> + __split_large_page((pte_t *)pmd, addr, pte);
>>>> +
>>>> + spin_lock(&init_mm.page_table_lock);
>>>> + pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
>>>> + spin_unlock(&init_mm.page_table_lock);
>>>> + }
>>>> +
>>>> + spin_lock(&init_mm.page_table_lock);
>>>> + pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
>>>> + phys_pte_remove(pte, addr, end);
>>>> + unmap_low_page(pte);
>>>> + spin_unlock(&init_mm.page_table_lock);
>>>> + }
>>>> + update_page_count(PG_LEVEL_2M, -pages);
>>>> +}
>>>> +
>>>> +static void __meminit
>>>> +phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
>>>> +{
>>>> + unsigned long pages = 0, next;
>>>> + int i = pud_index(addr);
>>>> +
>>>> + for (; i < PTRS_PER_PUD; i++, addr = next) {
>>>> + unsigned long pmd_phys;
>>>> + pud_t *pud = pud_page + pud_index(addr);
>>>> + pmd_t *pmd;
>>>> +
>>>> + if (addr >= end)
>>>> + break;
>>>> +
>>>> + next = (addr & PUD_MASK) + PUD_SIZE;
>>>> +
>>>> + if (!pud_present(*pud))
>>>> + continue;
>>>> +
>>>> + if (pud_large(*pud)) {
>>>> + if ((addr & ~PUD_MASK) == 0 && next <= end) {
>>>> + set_pud(pud, __pud(0));
>>>> + pages++;
>>>> + continue;
>>>> + }
>>>> +
>>>> + /*
>>>> + * We use 1G page, but we need to remove part of them,
>>>> + * so split 1G page to 2M page.
>>>> + */
>>>> + pmd = alloc_low_page(&pmd_phys);
>>>> + __split_large_page((pte_t *)pud, addr, (pte_t *)pmd);
>>>> +
>>>> + spin_lock(&init_mm.page_table_lock);
>>>> + pud_populate(&init_mm, pud, __va(pmd_phys));
>>>> + spin_unlock(&init_mm.page_table_lock);
>>>> + }
>>>> +
>>>> + pmd = map_low_page(pmd_offset(pud, 0));
>>>> + phys_pmd_remove(pmd, addr, end);
>>>> + unmap_low_page(pmd);
>>>> + __flush_tlb_all();
>>>> + }
>>>> + __flush_tlb_all();
>>>> +
>>>> + update_page_count(PG_LEVEL_1G, -pages);
>>>> +}
>>>> +
>>>> +void __meminit
>>>> +kernel_physical_mapping_remove(unsigned long start, unsigned long end)
>>>> +{
>>>> + unsigned long next;
>>>> +
>>>> + start = (unsigned long)__va(start);
>>>> + end = (unsigned long)__va(end);
>>>> +
>>>> + for (; start < end; start = next) {
>>>> + pgd_t *pgd = pgd_offset_k(start);
>>>> + pud_t *pud;
>>>> +
>>>> + next = (start + PGDIR_SIZE) & PGDIR_MASK;
>>>> + if (next > end)
>>>> + next = end;
>>>> +
>>>> + if (!pgd_present(*pgd))
>>>> + continue;
>>>> +
>>>> + pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
>>>> + phys_pud_remove(pud, __pa(start), __pa(end));
>>>> + unmap_low_page(pud);
>>>> + }
>>>> +
>>>> + __flush_tlb_all();
>>>> +}
>>>> +
>>>> #ifdef CONFIG_MEMORY_HOTREMOVE
>>>> int __ref arch_remove_memory(u64 start, u64 size)
>>>> {
>>>> @@ -687,6 +832,8 @@ int __ref arch_remove_memory(u64 start,
>>>> ret = __remove_pages(zone, start_pfn, nr_pages);
>>>> WARN_ON_ONCE(ret);
>>>>
>>>> + kernel_physical_mapping_remove(start, start + size);
>>>> +
>>>> return ret;
>>>> }
>>>> #endif
>>>> Index: linux-3.6/arch/x86/include/asm/pgtable_types.h
>>>> ===================================================================
>>>> --- linux-3.6.orig/arch/x86/include/asm/pgtable_types.h 2012-10-04 18:26:51.925486954 +0900
>>>> +++ linux-3.6/arch/x86/include/asm/pgtable_types.h 2012-10-04 18:30:27.322704656 +0900
>>>> @@ -334,6 +334,7 @@ static inline void update_page_count(int
>>>> * as a pte too.
>>>> */
>>>> extern pte_t *lookup_address(unsigned long address, unsigned int *level);
>>>> +extern int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase);
>>>>
>>>> #endif /* !__ASSEMBLY__ */
>>>>
>>>> Index: linux-3.6/arch/x86/mm/pageattr.c
>>>> ===================================================================
>>>> --- linux-3.6.orig/arch/x86/mm/pageattr.c 2012-10-04 18:26:51.923486952 +0900
>>>> +++ linux-3.6/arch/x86/mm/pageattr.c 2012-10-04 18:30:27.328704662 +0900
>>>> @@ -501,21 +501,13 @@ out_unlock:
>>>> return do_split;
>>>> }
>>>>
>>>> -static int split_large_page(pte_t *kpte, unsigned long address)
>>>> +int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase)
>>>> {
>>>> unsigned long pfn, pfninc = 1;
>>>> unsigned int i, level;
>>>> - pte_t *pbase, *tmp;
>>>> + pte_t *tmp;
>>>> pgprot_t ref_prot;
>>>> - struct page *base;
>>>> -
>>>> - if (!debug_pagealloc)
>>>> - spin_unlock(&cpa_lock);
>>>> - base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
>>>> - if (!debug_pagealloc)
>>>> - spin_lock(&cpa_lock);
>>>> - if (!base)
>>>> - return -ENOMEM;
>>>> + struct page *base = virt_to_page(pbase);
>>>>
>>>> spin_lock(&pgd_lock);
>>>> /*
>>>> @@ -523,10 +515,11 @@ static int split_large_page(pte_t *kpte,
>>>> * up for us already:
>>>> */
>>>> tmp = lookup_address(address, &level);
>>>> - if (tmp != kpte)
>>>> - goto out_unlock;
>>>> + if (tmp != kpte) {
>>>> + spin_unlock(&pgd_lock);
>>>> + return 1;
>>>> + }
>>>>
>>>> - pbase = (pte_t *)page_address(base);
>>>> paravirt_alloc_pte(&init_mm, page_to_pfn(base));
>>>> ref_prot = pte_pgprot(pte_clrhuge(*kpte));
>>>> /*
>>>> @@ -579,17 +572,27 @@ static int split_large_page(pte_t *kpte,
>>>> * going on.
>>>> */
>>>> __flush_tlb_all();
>>>> + spin_unlock(&pgd_lock);
>>>>
>>>> - base = NULL;
>>>> + return 0;
>>>> +}
>>>>
>>>> -out_unlock:
>>>> - /*
>>>> - * If we dropped out via the lookup_address check under
>>>> - * pgd_lock then stick the page back into the pool:
>>>> - */
>>>> - if (base)
>>>> +static int split_large_page(pte_t *kpte, unsigned long address)
>>>> +{
>>>> + pte_t *pbase;
>>>> + struct page *base;
>>>> +
>>>> + if (!debug_pagealloc)
>>>> + spin_unlock(&cpa_lock);
>>>> + base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
>>>> + if (!debug_pagealloc)
>>>> + spin_lock(&cpa_lock);
>>>> + if (!base)
>>>> + return -ENOMEM;
>>>> +
>>>> + pbase = (pte_t *)page_address(base);
>>>> + if (__split_large_page(kpte, address, pbase))
>>>> __free_page(base);
>>>> - spin_unlock(&pgd_lock);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> --
>>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>>> the body to majordomo@kvack.org. For more info on Linux MM,
>>>> see: http://www.linux-mm.org/ .
>>>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>>>
>>>
>>>
>>
>>
>
>
^ permalink raw reply
* Re: [PATCH 8/10] memory-hotplug : remove page table of x86_64 architecture
From: wujianguo @ 2012-10-23 7:09 UTC (permalink / raw)
To: Wen Congyang
Cc: linux-s390, linux-ia64, jiang.liu, len.brown, linux-acpi,
linux-sh, wujianguo, x86, linux-kernel, cmetcalf, linux-mm,
Yasuaki Ishimatsu, minchan.kim, kosaki.motohiro, rientjes,
sparclinux, qiuxishi, cl, linuxppc-dev, akpm, liuj97
In-Reply-To: <5084F195.6030908@cn.fujitsu.com>
On 2012-10-22 15:11, Wen Congyang wrote:
> Hi, Wu
>
> Sorry for late reply.
>
> At 10/09/2012 04:26 PM, wujianguo Wrote:
>> Hi Congyang,
>> I think we should also free pages which are used by page tables after removing
>> page tables of the memory.
>
> It is OK to do it.
>
>>
>> From: Jianguo Wu <wujianguo@huawei.com>
>>
>> Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>> arch/x86/mm/init_64.c | 110 +++++++++++++++++++++++++++++++++++++++---------
>> 1 files changed, 89 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>> index 5596dfa..81f9c3b 100644
>> --- a/arch/x86/mm/init_64.c
>> +++ b/arch/x86/mm/init_64.c
>> @@ -675,6 +675,74 @@ int arch_add_memory(int nid, u64 start, u64 size)
>> }
>> EXPORT_SYMBOL_GPL(arch_add_memory);
>>
>> +static inline void free_pagetable(struct page *page)
>> +{
>> + struct zone *zone;
>> +
>> + __ClearPageReserved(page);
>> + __free_page(page);
>> +
>> + zone = page_zone(page);
>> + zone_span_writelock(zone);
>> + zone->present_pages++;
>> + zone_span_writeunlock(zone);
>> + totalram_pages++;
>
> Why do you update zone and totalram_pages here?
Sorry, I made a mistake here. Only if the page was allocated at booting, we should update
zone and totalram_pages(zone->present_pages and totalram_pages mean pages which are
managed by buddy system).
How about:
static inline void free_pagetable(struct page *page)
{
struct zone *zone;
bool bootmem = false;
/* bootmem page has reserved flag */
if (PageReserved(page)) {
__ClearPageReserved(page);
bootmem = true;
}
__free_page(page);
if (bootmem) {
zone = page_zone(page);
zone_span_writelock(zone);
zone->present_pages++;
zone_span_writeunlock(zone);
totalram_pages++;
}
}
>
>> +}
>> +
>> +static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
>> +{
>> + pte_t *pte;
>> + int i;
>> +
>> + for (i = 0; i < PTRS_PER_PTE; i++) {
>> + pte = pte_start + i;
>> + if (pte_val(*pte))
>> + break;
>> + }
>> +
>> + /* free a pte talbe */
>> + if (i == PTRS_PER_PTE) {
>> + free_pagetable(pmd_page(*pmd));
>
> The memory may be allocated at booting. So it is very dangerous to
> free it without any check.
The page is only used by page table, so is safe to free it when all the page table
entries have been cleared, right?
>
>> + pmd_clear(pmd);
>> + }
>> +}
>> +
>> +static void free_pmd_table(pmd_t *pmd_start, pud_t *pud)
>> +{
>> + pmd_t *pmd;
>> + int i;
>> +
>> + for (i = 0; i < PTRS_PER_PMD; i++) {
>> + pmd = pmd_start + i;
>> + if (pmd_val(*pmd))
>> + break;
>> + }
>> +
>> + /* free a pmd talbe */
>> + if (i == PTRS_PER_PMD) {
>> + free_pagetable(pud_page(*pud));
>> + pud_clear(pud);
>> + }
>> +}
>> +
>> +static void free_pud_table(pud_t *pud_start, pgd_t *pgd)
>> +{
>> + pud_t *pud;
>> + int i;
>> +
>> + for (i = 0; i < PTRS_PER_PUD; i++) {
>> + pud = pud_start + i;
>> + if (pud_val(*pud))
>> + break;
>> + }
>> +
>> + /* free a pud table */
>> + if (i == PTRS_PER_PUD) {
>> + free_pagetable(pgd_page(*pgd));
>> + pgd_clear(pgd);
>> + }
>> +}
>> +
>> static void __meminit
>> phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
>> {
>> @@ -704,21 +772,19 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>> unsigned long pages = 0, next;
>> int i = pmd_index(addr);
>>
>> - for (; i < PTRS_PER_PMD; i++, addr = next) {
>> + for (; i < PTRS_PER_PMD && addr < end; i++, addr = next) {
>> unsigned long pte_phys;
>> pmd_t *pmd = pmd_page + pmd_index(addr);
>> pte_t *pte;
>>
>> - if (addr >= end)
>> - break;
>> -
>> - next = (addr & PMD_MASK) + PMD_SIZE;
>> + next = pmd_addr_end(addr, end);
>>
>> if (!pmd_present(*pmd))
>> continue;
>>
>> if (pmd_large(*pmd)) {
>> - if ((addr & ~PMD_MASK) == 0 && next <= end) {
>> + if (IS_ALIGNED(addr, PMD_SIZE) &&
>> + IS_ALIGNED(next, PMD_SIZE)) {
>> set_pmd(pmd, __pmd(0));
>> pages++;
>> continue;
>> @@ -729,7 +795,8 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>> * so split 2M page to 4K page.
>> */
>> pte = alloc_low_page(&pte_phys);
>> - __split_large_page((pte_t *)pmd, addr, pte);
>> + __split_large_page((pte_t *)pmd,
>> + (unsigned long)__va(addr), pte);
>>
>> spin_lock(&init_mm.page_table_lock);
>> pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
>> @@ -738,7 +805,8 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>>
>> spin_lock(&init_mm.page_table_lock);
>> pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
>> - phys_pte_remove(pte, addr, end);
>> + phys_pte_remove(pte, addr, next);
>> + free_pte_table(pte, pmd);
>> unmap_low_page(pte);
>> spin_unlock(&init_mm.page_table_lock);
>> }
>> @@ -751,21 +819,19 @@ phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
>> unsigned long pages = 0, next;
>> int i = pud_index(addr);
>>
>> - for (; i < PTRS_PER_PUD; i++, addr = next) {
>> + for (; i < PTRS_PER_PUD && addr < end; i++, addr = next) {
>> unsigned long pmd_phys;
>> pud_t *pud = pud_page + pud_index(addr);
>> pmd_t *pmd;
>>
>> - if (addr >= end)
>> - break;
>> -
>> - next = (addr & PUD_MASK) + PUD_SIZE;
>> + next = pud_addr_end(addr, end);
>>
>> if (!pud_present(*pud))
>> continue;
>>
>> if (pud_large(*pud)) {
>> - if ((addr & ~PUD_MASK) == 0 && next <= end) {
>> + if (IS_ALIGNED(addr, PUD_SIZE) &&
>> + IS_ALIGNED(next, PUD_SIZE)) {
>> set_pud(pud, __pud(0));
>> pages++;
>> continue;
>> @@ -776,15 +842,18 @@ phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
>> * so split 1G page to 2M page.
>> */
>> pmd = alloc_low_page(&pmd_phys);
>> - __split_large_page((pte_t *)pud, addr, (pte_t *)pmd);
>> + __split_large_page((pte_t *)pud,
>> + (unsigned long)__va(addr),
>> + (pte_t *)pmd);
>>
>> spin_lock(&init_mm.page_table_lock);
>> pud_populate(&init_mm, pud, __va(pmd_phys));
>> spin_unlock(&init_mm.page_table_lock);
>> }
>>
>> - pmd = map_low_page(pmd_offset(pud, 0));
>> - phys_pmd_remove(pmd, addr, end);
>> + pmd = map_low_page((pmd_t *)pud_page_vaddr(*pud));
>
> Hmm, pmd_offset(pud, 0) is equal to (pmd_t *)pud_page_vaddr(*pud).
>
> Is it OK to merge your patch into my patch?
>
Yes, sure.
Thanks,
Jianguo Wu
> Thanks
> Wen Congyang
>
>> + phys_pmd_remove(pmd, addr, next);
>> + free_pmd_table(pmd, pud);
>> unmap_low_page(pmd);
>> __flush_tlb_all();
>> }
>> @@ -805,15 +874,14 @@ kernel_physical_mapping_remove(unsigned long start, unsigned long end)
>> pgd_t *pgd = pgd_offset_k(start);
>> pud_t *pud;
>>
>> - next = (start + PGDIR_SIZE) & PGDIR_MASK;
>> - if (next > end)
>> - next = end;
>> + next = pgd_addr_end(start, end);
>>
>> if (!pgd_present(*pgd))
>> continue;
>>
>> pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
>> - phys_pud_remove(pud, __pa(start), __pa(end));
>> + phys_pud_remove(pud, __pa(start), __pa(next));
>> + free_pud_table(pud, pgd);
>> unmap_low_page(pud);
>> }
>>
>> -- 1.7.6.1 .
>>
>>
>> On 2012-10-5 10:36, Yasuaki Ishimatsu wrote:
>>> From: Wen Congyang <wency@cn.fujitsu.com>
>>>
>>> For hot removing memory, we sholud remove page table about the memory.
>>> So the patch searches a page table about the removed memory, and clear
>>> page table.
>>>
>>> CC: David Rientjes <rientjes@google.com>
>>> CC: Jiang Liu <liuj97@gmail.com>
>>> CC: Len Brown <len.brown@intel.com>
>>> CC: Christoph Lameter <cl@linux.com>
>>> Cc: Minchan Kim <minchan.kim@gmail.com>
>>> CC: Andrew Morton <akpm@linux-foundation.org>
>>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>> CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> ---
>>> arch/x86/include/asm/pgtable_types.h | 1
>>> arch/x86/mm/init_64.c | 147 +++++++++++++++++++++++++++++++++++
>>> arch/x86/mm/pageattr.c | 47 +++++------
>>> 3 files changed, 173 insertions(+), 22 deletions(-)
>>>
>>> Index: linux-3.6/arch/x86/mm/init_64.c
>>> ===================================================================
>>> --- linux-3.6.orig/arch/x86/mm/init_64.c 2012-10-04 18:30:21.171698416 +0900
>>> +++ linux-3.6/arch/x86/mm/init_64.c 2012-10-04 18:30:27.317704652 +0900
>>> @@ -675,6 +675,151 @@ int arch_add_memory(int nid, u64 start,
>>> }
>>> EXPORT_SYMBOL_GPL(arch_add_memory);
>>>
>>> +static void __meminit
>>> +phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
>>> +{
>>> + unsigned pages = 0;
>>> + int i = pte_index(addr);
>>> +
>>> + pte_t *pte = pte_page + pte_index(addr);
>>> +
>>> + for (; i < PTRS_PER_PTE; i++, addr += PAGE_SIZE, pte++) {
>>> +
>>> + if (addr >= end)
>>> + break;
>>> +
>>> + if (!pte_present(*pte))
>>> + continue;
>>> +
>>> + pages++;
>>> + set_pte(pte, __pte(0));
>>> + }
>>> +
>>> + update_page_count(PG_LEVEL_4K, -pages);
>>> +}
>>> +
>>> +static void __meminit
>>> +phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>>> +{
>>> + unsigned long pages = 0, next;
>>> + int i = pmd_index(addr);
>>> +
>>> + for (; i < PTRS_PER_PMD; i++, addr = next) {
>>> + unsigned long pte_phys;
>>> + pmd_t *pmd = pmd_page + pmd_index(addr);
>>> + pte_t *pte;
>>> +
>>> + if (addr >= end)
>>> + break;
>>> +
>>> + next = (addr & PMD_MASK) + PMD_SIZE;
>>> +
>>> + if (!pmd_present(*pmd))
>>> + continue;
>>> +
>>> + if (pmd_large(*pmd)) {
>>> + if ((addr & ~PMD_MASK) == 0 && next <= end) {
>>> + set_pmd(pmd, __pmd(0));
>>> + pages++;
>>> + continue;
>>> + }
>>> +
>>> + /*
>>> + * We use 2M page, but we need to remove part of them,
>>> + * so split 2M page to 4K page.
>>> + */
>>> + pte = alloc_low_page(&pte_phys);
>>> + __split_large_page((pte_t *)pmd, addr, pte);
>>> +
>>> + spin_lock(&init_mm.page_table_lock);
>>> + pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
>>> + spin_unlock(&init_mm.page_table_lock);
>>> + }
>>> +
>>> + spin_lock(&init_mm.page_table_lock);
>>> + pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
>>> + phys_pte_remove(pte, addr, end);
>>> + unmap_low_page(pte);
>>> + spin_unlock(&init_mm.page_table_lock);
>>> + }
>>> + update_page_count(PG_LEVEL_2M, -pages);
>>> +}
>>> +
>>> +static void __meminit
>>> +phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
>>> +{
>>> + unsigned long pages = 0, next;
>>> + int i = pud_index(addr);
>>> +
>>> + for (; i < PTRS_PER_PUD; i++, addr = next) {
>>> + unsigned long pmd_phys;
>>> + pud_t *pud = pud_page + pud_index(addr);
>>> + pmd_t *pmd;
>>> +
>>> + if (addr >= end)
>>> + break;
>>> +
>>> + next = (addr & PUD_MASK) + PUD_SIZE;
>>> +
>>> + if (!pud_present(*pud))
>>> + continue;
>>> +
>>> + if (pud_large(*pud)) {
>>> + if ((addr & ~PUD_MASK) == 0 && next <= end) {
>>> + set_pud(pud, __pud(0));
>>> + pages++;
>>> + continue;
>>> + }
>>> +
>>> + /*
>>> + * We use 1G page, but we need to remove part of them,
>>> + * so split 1G page to 2M page.
>>> + */
>>> + pmd = alloc_low_page(&pmd_phys);
>>> + __split_large_page((pte_t *)pud, addr, (pte_t *)pmd);
>>> +
>>> + spin_lock(&init_mm.page_table_lock);
>>> + pud_populate(&init_mm, pud, __va(pmd_phys));
>>> + spin_unlock(&init_mm.page_table_lock);
>>> + }
>>> +
>>> + pmd = map_low_page(pmd_offset(pud, 0));
>>> + phys_pmd_remove(pmd, addr, end);
>>> + unmap_low_page(pmd);
>>> + __flush_tlb_all();
>>> + }
>>> + __flush_tlb_all();
>>> +
>>> + update_page_count(PG_LEVEL_1G, -pages);
>>> +}
>>> +
>>> +void __meminit
>>> +kernel_physical_mapping_remove(unsigned long start, unsigned long end)
>>> +{
>>> + unsigned long next;
>>> +
>>> + start = (unsigned long)__va(start);
>>> + end = (unsigned long)__va(end);
>>> +
>>> + for (; start < end; start = next) {
>>> + pgd_t *pgd = pgd_offset_k(start);
>>> + pud_t *pud;
>>> +
>>> + next = (start + PGDIR_SIZE) & PGDIR_MASK;
>>> + if (next > end)
>>> + next = end;
>>> +
>>> + if (!pgd_present(*pgd))
>>> + continue;
>>> +
>>> + pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
>>> + phys_pud_remove(pud, __pa(start), __pa(end));
>>> + unmap_low_page(pud);
>>> + }
>>> +
>>> + __flush_tlb_all();
>>> +}
>>> +
>>> #ifdef CONFIG_MEMORY_HOTREMOVE
>>> int __ref arch_remove_memory(u64 start, u64 size)
>>> {
>>> @@ -687,6 +832,8 @@ int __ref arch_remove_memory(u64 start,
>>> ret = __remove_pages(zone, start_pfn, nr_pages);
>>> WARN_ON_ONCE(ret);
>>>
>>> + kernel_physical_mapping_remove(start, start + size);
>>> +
>>> return ret;
>>> }
>>> #endif
>>> Index: linux-3.6/arch/x86/include/asm/pgtable_types.h
>>> ===================================================================
>>> --- linux-3.6.orig/arch/x86/include/asm/pgtable_types.h 2012-10-04 18:26:51.925486954 +0900
>>> +++ linux-3.6/arch/x86/include/asm/pgtable_types.h 2012-10-04 18:30:27.322704656 +0900
>>> @@ -334,6 +334,7 @@ static inline void update_page_count(int
>>> * as a pte too.
>>> */
>>> extern pte_t *lookup_address(unsigned long address, unsigned int *level);
>>> +extern int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase);
>>>
>>> #endif /* !__ASSEMBLY__ */
>>>
>>> Index: linux-3.6/arch/x86/mm/pageattr.c
>>> ===================================================================
>>> --- linux-3.6.orig/arch/x86/mm/pageattr.c 2012-10-04 18:26:51.923486952 +0900
>>> +++ linux-3.6/arch/x86/mm/pageattr.c 2012-10-04 18:30:27.328704662 +0900
>>> @@ -501,21 +501,13 @@ out_unlock:
>>> return do_split;
>>> }
>>>
>>> -static int split_large_page(pte_t *kpte, unsigned long address)
>>> +int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase)
>>> {
>>> unsigned long pfn, pfninc = 1;
>>> unsigned int i, level;
>>> - pte_t *pbase, *tmp;
>>> + pte_t *tmp;
>>> pgprot_t ref_prot;
>>> - struct page *base;
>>> -
>>> - if (!debug_pagealloc)
>>> - spin_unlock(&cpa_lock);
>>> - base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
>>> - if (!debug_pagealloc)
>>> - spin_lock(&cpa_lock);
>>> - if (!base)
>>> - return -ENOMEM;
>>> + struct page *base = virt_to_page(pbase);
>>>
>>> spin_lock(&pgd_lock);
>>> /*
>>> @@ -523,10 +515,11 @@ static int split_large_page(pte_t *kpte,
>>> * up for us already:
>>> */
>>> tmp = lookup_address(address, &level);
>>> - if (tmp != kpte)
>>> - goto out_unlock;
>>> + if (tmp != kpte) {
>>> + spin_unlock(&pgd_lock);
>>> + return 1;
>>> + }
>>>
>>> - pbase = (pte_t *)page_address(base);
>>> paravirt_alloc_pte(&init_mm, page_to_pfn(base));
>>> ref_prot = pte_pgprot(pte_clrhuge(*kpte));
>>> /*
>>> @@ -579,17 +572,27 @@ static int split_large_page(pte_t *kpte,
>>> * going on.
>>> */
>>> __flush_tlb_all();
>>> + spin_unlock(&pgd_lock);
>>>
>>> - base = NULL;
>>> + return 0;
>>> +}
>>>
>>> -out_unlock:
>>> - /*
>>> - * If we dropped out via the lookup_address check under
>>> - * pgd_lock then stick the page back into the pool:
>>> - */
>>> - if (base)
>>> +static int split_large_page(pte_t *kpte, unsigned long address)
>>> +{
>>> + pte_t *pbase;
>>> + struct page *base;
>>> +
>>> + if (!debug_pagealloc)
>>> + spin_unlock(&cpa_lock);
>>> + base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
>>> + if (!debug_pagealloc)
>>> + spin_lock(&cpa_lock);
>>> + if (!base)
>>> + return -ENOMEM;
>>> +
>>> + pbase = (pte_t *)page_address(base);
>>> + if (__split_large_page(kpte, address, pbase))
>>> __free_page(base);
>>> - spin_unlock(&pgd_lock);
>>>
>>> return 0;
>>> }
>>>
>>> --
>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>> the body to majordomo@kvack.org. For more info on Linux MM,
>>> see: http://www.linux-mm.org/ .
>>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>>
>>
>>
>
>
^ permalink raw reply
* Re: Build regressions/improvements in v3.7-rc2
From: Heiko Carstens @ 2012-10-23 6:55 UTC (permalink / raw)
To: Geert Uytterhoeven, Martin Schwidefsky
Cc: linux-s390, linuxppc-dev, Linux Kernel Development
In-Reply-To: <CAMuHMdVS-qz0BAHXFGD01+w1GxyQ2M7DpLnLRQKtdZSiDfYgHw@mail.gmail.com>
On Mon, Oct 22, 2012 at 09:50:26PM +0200, Geert Uytterhoeven wrote:
> On Mon, Oct 22, 2012 at 9:47 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > JFYI, when comparing v3.7-rc2 to v3.7-rc1[3], the summaries are:
> > - build errors: +4/-44
>
> + arch/s390/include/asm/kvm_para.h: error: redefinition of
> 'kvm_arch_para_features': => 147:28, 147:99
> + arch/s390/include/asm/kvm_para.h: error: redefinition of
> 'kvm_check_and_clear_guest_paused': => 152:91, 152:20
>
> s390-allmodconfig/s390-allyesconfig/s390-defconfig
Thanks Geert. We have already a build fix for this from David Howells
which is waiting to be merged upstream.
^ permalink raw reply
* Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
From: Scott Wood @ 2012-10-22 23:53 UTC (permalink / raw)
To: Tabi Timur-B04825
Cc: Sethi Varun-B16395, iommu@lists.linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
joerg.roedel@amd.com
In-Reply-To: <6AE080B68D46FC4BA2D2769E68D765B7081084A7@039-SN2MPN1-023.039d.mgd.msft.net>
On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote:
> On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi =20
> <Varun.Sethi@freescale.com> wrote:
> > +}
> > +
> > +static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
> > +{
>=20
> subwin_cnt should probably be an unsigned int.
>=20
> This function needs to be documented. What value is being returned?
spaact offset (yes, this needs to be documented)
> > + unsigned long spaace_addr;
> > +
> > + spaace_addr =3D gen_pool_alloc(spaace_pool, subwin_cnt * =20
> sizeof(paace_t));
> > + if (!spaace_addr)
> > + return ULONG_MAX;
>=20
> What's wrong with returning 0 on error?
0 is a valid spaact offset
> > +
> > + return (spaace_addr - (unsigned long)spaact) / =20
> (sizeof(paace_t));
>=20
> Is this supposed to be a virtual address? If so, then return void*
> instead of an unsigned long.
It's not a virtual address. How often does subtraction followed by =20
division result in a valid virtual address?
> > +int pamu_update_paace_stash(int liodn, u32 subwin, u32 value)
Whitespace
> > +#define PAMU_PAGE_SHIFT 12
> > +#define PAMU_PAGE_SIZE 4096ULL
>=20
> 4096ULL? Why not just 4096?
This lets it be used in phys_addr_t expressions without needing casts
everywhere or dropping bits.
> > +/* This bitmap advertises the page sizes supported by PAMU hardware
> > + * to the IOMMU API.
> > + */
> > +#define FSL_PAMU_PGSIZES (~0xFFFUL)
>=20
> There should be a better way to define this. ~(PAMU_PAGE_SIZE-1) =20
> maybe?
Is it even true? We don't support IOMMU pages larger than the SoC can
address.
The (~0xFFFUL) version also discards some valid IOMMU page sizes on
32-bit kernels. One use case for windows larger than the CPU virtual
address space is creating one big identity-map window to effectively
disable translation. If we're to support that, the size of =20
pgsize_bitmap
will need to change as well.
> > +static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
> > +{
> > + u32 subwin_cnt =3D dma_domain->subwin_cnt;
> > + unsigned long rpn;
> > + int ret =3D 0, i;
> > +
> > + if (subwin_cnt) {
> > + struct dma_subwindow *sub_win_ptr =3D
> > + &dma_domain->sub_win_arr[0];
> > + for (i =3D 0; i < subwin_cnt; i++) {
> > + if (sub_win_ptr[i].valid) {
> > + rpn =3D sub_win_ptr[i].paddr >>
> > + PAMU_PAGE_SHIFT,
> > + spin_lock(&iommu_lock);
> > + ret =3D pamu_config_spaace(liodn, =20
> subwin_cnt, i,
> > + =20
> sub_win_ptr[i].size,
> > + -1,
> > + rpn,
> > + =20
> dma_domain->snoop_id,
> > + =20
> dma_domain->stash_id,
> > + (i > 0) ? =20
> 1 : 0,
> > + =20
> sub_win_ptr[i].prot);
> > + spin_unlock(&iommu_lock);
> > + if (ret) {
> > + pr_err("PAMU SPAACE =20
> configuration failed for liodn %d\n",
> > + liodn);
> > + return ret;
> > + }
> > + }
> > + }
Break up that nesting with some subfunctions.
> > + while (!list_empty(&dma_domain->devices)) {
> > + info =3D list_entry(dma_domain->devices.next,
> > + struct device_domain_info, link);
> > + remove_domain_ref(info, dma_domain->subwin_cnt);
> > + }
>=20
> I wonder if you should use list_for_each_safe() instead.
The above is simpler if you're destroying the entire list.
> > +}
> > +
> > +static int configure_domain_dma_state(struct fsl_dma_domain =20
> *dma_domain, int enable)
>=20
> bool enable
>=20
> Finally, please CC: me on all IOMMU and PAMU patches you post =20
> upstream.
Me too.
-Scott=
^ permalink raw reply
* Re: [PATCH 2/3 v3] iommu/fsl: Add iommu domain attributes required by fsl PAMU driver.
From: Scott Wood @ 2012-10-22 22:05 UTC (permalink / raw)
To: Varun Sethi; +Cc: joerg.roedel, iommu, linuxppc-dev, linux-kernel, Varun Sethi
In-Reply-To: <1350495170-4593-3-git-send-email-Varun.Sethi@freescale.com>
On 10/17/2012 12:32:49 PM, Varun Sethi wrote:
> Added the following domain attributes required by FSL PAMU driver:
> 1. Subwindows field added to the iommu domain geometry attribute.
> 2. Added new iommu stash attribute, which allows setting of the
> LIODN specific stash id parameter through IOMMU API.
> 3. Added an attribute for enabling/disabling DMA to a particular
> memory window.
>=20
> Signed-off-by: Varun Sethi <Varun.Sethi@freescale.com>
> ---
> change in v3:
> -renamed the stash attribute targets
>=20
> include/linux/iommu.h | 35 +++++++++++++++++++++++++++++++++++
> 1 files changed, 35 insertions(+), 0 deletions(-)
>=20
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f3b99e1..c3b9d73 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -44,6 +44,33 @@ struct iommu_domain_geometry {
> dma_addr_t aperture_start; /* First address that can be =20
> mapped */
> dma_addr_t aperture_end; /* Last address that can be =20
> mapped */
> bool force_aperture; /* DMA only allowed in mappable =20
> range? */
> +
> + /* The subwindows field indicates number of DMA subwindows =20
> supported
> + * by the geometry. Following is the interpretation of
> + * values for this field:
> + * 0 : This implies that the supported geometry size is 1 MB
> + * with each subwindow size being 4KB. Thus number of =20
> subwindows
Whitespace
> + * being =3D 1MB/4KB =3D 256.
> + * 1 : Only one DMA window i.e. no subwindows.
> + * value other than 0 or 1 would indicate actual number of =20
> subwindows.
> + */
This language is way too specific for the generic geometry struct =20
(especially when you start talking about specific sizes). Please =20
explain in implementation-neutral terms what this field means.
> @@ -60,6 +87,14 @@ struct iommu_domain {
> enum iommu_attr {
> DOMAIN_ATTR_MAX,
> DOMAIN_ATTR_GEOMETRY,
> + /* Set the IOMMU hardware stashing
> + * parameters.
> + */
> + DOMAIN_ATTR_STASH,
> + /* Explicity enable/disable DMA for a
> + * particular memory window.
> + */
> + DOMAIN_ATTR_ENABLE,
> };
Whitespace
-Scott=
^ permalink raw reply
* Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
From: Tabi Timur-B04825 @ 2012-10-22 21:18 UTC (permalink / raw)
To: Sethi Varun-B16395
Cc: joerg.roedel@amd.com, iommu@lists.linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <1350495170-4593-4-git-send-email-Varun.Sethi@freescale.com>
On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi <Varun.Sethi@freescale.com> w=
rote:
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/iommu.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/mm.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/of_platform.h>
> +#include <linux/bootmem.h>
> +#include <linux/genalloc.h>
> +#include <asm/io.h>
> +#include <asm/bitops.h>
> +
> +#include "fsl_pamu.h"
> +
> +/* PAMU bypass enbale register contains control bits for
> + * enabling bypass mode on each PAMU.
> + */
/*
* Linux multi-line comments
* look like this.
*/
> +#define PAMUBYPENR 0x604
Update struct ccsr_guts instead.
http://patchwork.ozlabs.org/patch/141649/
> +
> +/* define indexes for each operation mapping scenario */
> +#define OMI_QMAN 0x00
> +#define OMI_FMAN 0x01
> +#define OMI_QMAN_PRIV 0x02
> +#define OMI_CAAM 0x03
> +
> +static paace_t *ppaact;
> +static paace_t *spaact;
> +static struct ome *omt;
> +unsigned int max_subwindow_count;
> +
> +struct gen_pool *spaace_pool;
> +
> +static paace_t *pamu_get_ppaace(int liodn)
> +{
> + if (!ppaact) {
> + pr_err("PPAACT doesn't exist\n");
pr_err("fsl-pamu: PPAACT has not been initialized\n");
Make sure ALL pr_xxx() messages in this file start with "fsl-pamu: "
> + return NULL;
> + }
> +
> + return &ppaact[liodn];
Bounds checking?
> +}
> +
> +/** Sets validation bit of PACCE
> + *
> + * @parm[in] liodn PAACT index for desired PAACE
> + *
> + * @return Returns 0 upon success else error code < 0 returned
> + */
> +int pamu_enable_liodn(int liodn)
> +{
> + paace_t *ppaace;
> +
> + ppaace =3D pamu_get_ppaace(liodn);
> + if (!ppaace)
> + return -ENOENT;
error message?
> +
> + if (!get_bf(ppaace->addr_bitfields, PPAACE_AF_WSE)) {
> + pr_err("liodn %d not configured\n", liodn);
> + return -EINVAL;
> + }
> +
> + /* Ensure that all other stores to the ppaace complete first */
> + mb();
> +
> + ppaace->addr_bitfields |=3D PAACE_V_VALID;
> + mb();
> +
> + return 0;
> +}
> +
> +/** Clears validation bit of PACCE
> + *
> + * @parm[in] liodn PAACT index for desired PAACE
> + *
> + * @return Returns 0 upon success else error code < 0 returned
This is not proper kerneldoc format.
> + */
> +int pamu_disable_liodn(int liodn)
> +{
> + paace_t *ppaace;
> +
> + ppaace =3D pamu_get_ppaace(liodn);
> + if (!ppaace)
> + return -ENOENT;
error message?
> +
> + set_bf(ppaace->addr_bitfields, PAACE_AF_V, PAACE_V_INVALID);
> + mb();
> +
> + return 0;
> +}
> +
> +
> +static unsigned int map_addrspace_size_to_wse(phys_addr_t addrspace_size=
)
> +{
> + BUG_ON((addrspace_size & (addrspace_size - 1)));
> +
> + /* window size is 2^(WSE+1) bytes */
> + return __ffs(addrspace_size >> PAMU_PAGE_SHIFT) + PAMU_PAGE_SHIFT=
- 1;
> +}
> +
> +static unsigned int map_subwindow_cnt_to_wce(u32 subwindow_cnt)
> +{
> + /* window count is 2^(WCE+1) bytes */
> + return __ffs(subwindow_cnt) - 1;
> +}
> +
> +static void pamu_setup_default_xfer_to_host_ppaace(paace_t *ppaace)
> +{
> + set_bf(ppaace->addr_bitfields, PAACE_AF_PT, PAACE_PT_PRIMARY);
> +
> + set_bf(ppaace->domain_attr.to_host.coherency_required, PAACE_DA_H=
OST_CR,
> + PAACE_M_COHERENCE_REQ);
> +}
> +
> +static void pamu_setup_default_xfer_to_host_spaace(paace_t *spaace)
> +{
> + set_bf(spaace->addr_bitfields, PAACE_AF_PT, PAACE_PT_SECONDARY);
> + set_bf(spaace->domain_attr.to_host.coherency_required, PAACE_DA_H=
OST_CR,
> + PAACE_M_COHERENCE_REQ);
> +}
> +
> +static paace_t *pamu_get_spaace(u32 fspi_index, u32 wnum)
> +{
> + return &spaact[fspi_index + wnum];
bounds checking?
> +}
> +
> +static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
> +{
subwin_cnt should probably be an unsigned int.
This function needs to be documented. What value is being returned?
> + unsigned long spaace_addr;
> +
> + spaace_addr =3D gen_pool_alloc(spaace_pool, subwin_cnt * sizeof(p=
aace_t));
> + if (!spaace_addr)
> + return ULONG_MAX;
What's wrong with returning 0 on error?
> +
> + return (spaace_addr - (unsigned long)spaact) / (sizeof(paace_t));
Is this supposed to be a virtual address? If so, then return void*
instead of an unsigned long.
> +}
> +
> +void pamu_free_subwins(int liodn)
> +{
> + paace_t *ppaace;
> + u32 subwin_cnt, size;
subwin_cnt should probably be an unsigned int.
> +
> + ppaace =3D pamu_get_ppaace(liodn);
> + if (!ppaace)
> + return;
error message
> +
> + if (get_bf(ppaace->addr_bitfields, PPAACE_AF_MW)) {
> + subwin_cnt =3D 1UL << (get_bf(ppaace->impl_attr, PAACE_IA=
_WCE) + 1);
> + size =3D (subwin_cnt - 1) * sizeof(paace_t);
> + gen_pool_free(spaace_pool, (unsigned long)&spaact[ppaace-=
>fspi], size);
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_MW, 0);
> + }
> +}
> +
> +/* Function used for updating stash destination for the coresspong LIODN=
.
> + */
> +int pamu_update_paace_stash(int liodn, u32 subwin, u32 value)
> +{
> + paace_t *paace;
> +
> + paace =3D pamu_get_ppaace(liodn);
> + if (!paace) {
> + return -ENOENT;
> + }
Error message?
> + if (subwin) {
> + paace =3D pamu_get_spaace(paace->fspi, subwin - 1);
> + if (!paace) {
> + return -ENOENT;
Error message?
> + }
> + }
> + set_bf(paace->impl_attr, PAACE_IA_CID, value);
> +
> + return 0;
> +}
> +
> +/** Sets up PPAACE entry for specified liodn
> + *
> + * @param[in] liodn Logical IO device number
> + * @param[in] win_addr starting address of DSA window
> + * @param[in] win-size size of DSA window
> + * @param[in] omi Operation mapping index -- if ~omi =3D=3D 0 the=
n omi not defined
> + * @param[in] rpn real (true physical) page number
> + * @param[in] stashid cache stash id for associated cpu -- if ~stashi=
d =3D=3D 0 then
> + * stashid not defined
> + * @param[in] snoopid snoop id for hardware coherency -- if ~snoopid =
=3D=3D 0 then
> + * snoopid not defined
> + * @param[in] subwin_cnt number of sub-windows
> + * @param[in] prot window permissions
> + *
> + * @return Returns 0 upon success else error code < 0 returned
> + */
Please provide proper kerneldoc comments for all of the functions in this f=
ile.
> +int pamu_config_ppaace(int liodn, phys_addr_t win_addr, phys_addr_t win_=
size,
> + u32 omi, unsigned long rpn, u32 snoopid, u32 stash=
id,
> + u32 subwin_cnt, int prot)
> +{
> + paace_t *ppaace;
> + unsigned long fspi;
> +
> + if ((win_size & (win_size - 1)) || win_size < PAMU_PAGE_SIZE) {
> + pr_err("window size too small or not a power of two %llx\=
n", win_size);
> + return -EINVAL;
> + }
> +
> + if (win_addr & (win_size - 1)) {
> + pr_err("window address is not aligned with window size\n"=
);
> + return -EINVAL;
> + }
> +
> + ppaace =3D pamu_get_ppaace(liodn);
> + if (!ppaace) {
> + return -ENOENT;
> + }
> +
> + /* window size is 2^(WSE+1) bytes */
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_WSE,
> + map_addrspace_size_to_wse(win_size));
> +
> + pamu_setup_default_xfer_to_host_ppaace(ppaace);
> +
> + ppaace->wbah =3D win_addr >> (PAMU_PAGE_SHIFT + 20);
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_WBAL,
> + (win_addr >> PAMU_PAGE_SHIFT));
> +
> + /* set up operation mapping if it's configured */
> + if (omi < OME_NUMBER_ENTRIES) {
> + set_bf(ppaace->impl_attr, PAACE_IA_OTM, PAACE_OTM_INDEXED=
);
> + ppaace->op_encode.index_ot.omi =3D omi;
> + } else if (~omi !=3D 0) {
> + pr_err("bad operation mapping index: %d\n", omi);
> + return -EINVAL;
> + }
> +
> + /* configure stash id */
> + if (~stashid !=3D 0)
> + set_bf(ppaace->impl_attr, PAACE_IA_CID, stashid);
> +
> + /* configure snoop id */
> + if (~snoopid !=3D 0)
> + ppaace->domain_attr.to_host.snpid =3D snoopid;
> +
> + if (subwin_cnt) {
> + /* The first entry is in the primary PAACE instead */
> + fspi =3D pamu_get_fspi_and_allocate(subwin_cnt - 1);
> + if (fspi =3D=3D ULONG_MAX) {
> + pr_err("spaace indexes exhausted\n");
> + return -EINVAL;
> + }
Indentation problem.
> +
> + /* window count is 2^(WCE+1) bytes */
> + set_bf(ppaace->impl_attr, PAACE_IA_WCE,
> + map_subwindow_cnt_to_wce(subwin_cnt));
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_MW, 0x1);
> + ppaace->fspi =3D fspi;
> + } else {
> + set_bf(ppaace->impl_attr, PAACE_IA_ATM, PAACE_ATM_WINDOW_=
XLATE);
> + ppaace->twbah =3D rpn >> 20;
> + set_bf(ppaace->win_bitfields, PAACE_WIN_TWBAL, rpn);
> + set_bf(ppaace->addr_bitfields, PAACE_AF_AP, prot);
> + set_bf(ppaace->impl_attr, PAACE_IA_WCE, 0);
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_MW, 0);
> + }
> + mb();
> +
> + return 0;
> +}
> +
> +/** Sets up SPAACE entry for specified subwindow
> + *
> + * @param[in] liodn Logical IO device number
> + * @param[in] subwin_cnt number of sub-windows associated with dma-wind=
ow
> + * @param[in] subwin_addr starting address of subwindow
> + * @param[in] subwin_size size of subwindow
> + * @param[in] omi Operation mapping index
> + * @param[in] rpn real (true physical) page number
> + * @param[in] snoopid snoop id for hardware coherency -- if ~snoopid=
=3D=3D 0 then
> + * snoopid not defined
> + * @param[in] stashid cache stash id for associated cpu
> + * @param[in] enable enable/disable subwindow after reconfiguration
> + * @param[in] prot sub window permissions
> + *
> + * @return Returns 0 upon success else error code < 0 returned
> + */
> +int pamu_config_spaace(int liodn, u32 subwin_cnt, u32 subwin_addr,
> + phys_addr_t subwin_size, u32 omi, unsigned long rp=
n,
> + u32 snoopid, u32 stashid, int enable, int prot)
> +{
> + paace_t *paace;
> + unsigned long fspi;
> +
> + /* setup sub-windows */
> + if (!subwin_cnt) {
> + pr_err("Invalid subwindow count\n");
> + return -EINVAL;
> + }
> +
> + paace =3D pamu_get_ppaace(liodn);
> + if (subwin_addr > 0 && paace) {
> + fspi =3D paace->fspi;
> + paace =3D pamu_get_spaace(fspi, subwin_addr - 1);
> +
> + if (paace && !(paace->addr_bitfields & PAACE_V_VALID)) {
> + pamu_setup_default_xfer_to_host_spaace(paace);
> + set_bf(paace->addr_bitfields, SPAACE_AF_LIODN, li=
odn);
> + }
> + }
> +
> + if (!paace)
> + return -ENOENT;
Error message?
> +
> + if (!enable && prot =3D=3D PAACE_AP_PERMS_DENIED) {
> + if (subwin_addr > 0)
> + set_bf(paace->addr_bitfields, PAACE_AF_V,
> + PAACE_V_INVALID);
> + else
> + set_bf(paace->addr_bitfields, PAACE_AF_AP,
> + prot);
> + mb();
> + return 0;
> + }
> +
> + if (subwin_size & (subwin_size - 1) || subwin_size < PAMU_PAGE_SI=
ZE) {
> + pr_err("subwindow size out of range, or not a power of 2\=
n");
> + return -EINVAL;
> + }
> +
> + if (rpn =3D=3D ULONG_MAX) {
> + pr_err("real page number out of range\n");
> + return -EINVAL;
> + }
> +
> + /* window size is 2^(WSE+1) bytes */
> + set_bf(paace->win_bitfields, PAACE_WIN_SWSE,
> + map_addrspace_size_to_wse(subwin_size));
> +
> + set_bf(paace->impl_attr, PAACE_IA_ATM, PAACE_ATM_WINDOW_XLATE);
> + paace->twbah =3D rpn >> 20;
> + set_bf(paace->win_bitfields, PAACE_WIN_TWBAL, rpn);
> + set_bf(paace->addr_bitfields, PAACE_AF_AP, prot);
> +
> + /* configure snoop id */
> + if (~snoopid !=3D 0)
> + paace->domain_attr.to_host.snpid =3D snoopid;
> +
> + /* set up operation mapping if it's configured */
> + if (omi < OME_NUMBER_ENTRIES) {
> + set_bf(paace->impl_attr, PAACE_IA_OTM, PAACE_OTM_INDEXED)=
;
> + paace->op_encode.index_ot.omi =3D omi;
> + } else if (~omi !=3D 0) {
> + pr_err("bad operation mapping index: %d\n", omi);
> + return -EINVAL;
> + }
> +
> + if (~stashid !=3D 0)
> + set_bf(paace->impl_attr, PAACE_IA_CID, stashid);
> +
> + smp_wmb();
> +
> + if (enable)
> + paace->addr_bitfields |=3D PAACE_V_VALID;
> +
> + mb();
> +
> + return 0;
> +}
> +
> +void get_ome_index(u32 *omi_index, struct device *dev)
> +{
> + if (of_device_is_compatible(dev->of_node, "fsl,qman-portal"))
> + *omi_index =3D OMI_QMAN;
> + if (of_device_is_compatible(dev->of_node, "fsl,qman"))
> + *omi_index =3D OMI_QMAN_PRIV;
> +}
Documentation?
> +
> +u32 get_stash_id(u32 stash_dest_hint, u32 vcpu)
Can we make the stash_id a signed integer, and return -1 as an error?
Making it a u32 is awkward because we keep doing stuff like this:
if (~stashid !=3D 0)
and
return ~(u32)0;
> +{
> + const u32 *prop;
> + struct device_node *node;
> + u32 cache_level;
> + int len;
> +
> + /* Fastpath, exit early if L3/CPC cache is target for stashing */
> + if (stash_dest_hint =3D=3D IOMMU_ATTR_CACHE_L3) {
> + node =3D of_find_compatible_node(NULL, NULL,
> + "fsl,p4080-l3-cache-controller");
> + if (node) {
if (!node) {
pr_err( "no cpc node" );
return ~(u32)0;
}
> + prop =3D of_get_property(node, "cache-stash-id", =
0);
> + if (!prop) {
> + pr_err("missing cache-stash-id at %s\n", =
node->full_name);
> + of_node_put(node);
> + return ~(u32)0;
> + }
> + of_node_put(node);
> + return be32_to_cpup(prop);
> + }
> + return ~(u32)0;
> + }
> +
> + for_each_node_by_type(node, "cpu") {
> + prop =3D of_get_property(node, "reg", &len);
> + if (be32_to_cpup(prop) =3D=3D vcpu)
> + break;
> + }
> +
> + /* find the hwnode that represents the cache */
> + for (cache_level =3D IOMMU_ATTR_CACHE_L1; cache_level <=3D IOMMU_=
ATTR_CACHE_L3; cache_level++) {
Shouldn't this be < IOMMU_ATTR_CACHE_L3, since we already handled the
CPC case above?
> + if (stash_dest_hint =3D=3D cache_level) {
> + prop =3D of_get_property(node, "cache-stash-id", =
0);
> + if (!prop) {
> + pr_err("missing cache-stash-id at %s\n", =
node->full_name);
> + of_node_put(node);
> + return ~(u32)0;
> + }
> + of_node_put(node);
> + return be32_to_cpup(prop);
> + }
> +
> + prop =3D of_get_property(node, "next-level-cache", 0);
> + if (!prop) {
> + pr_err("can't find next-level-cache at %s\n",
> + node->full_name);
> + of_node_put(node);
> + return ~(u32)0; /* can't traverse any further */
> + }
> + of_node_put(node);
> +
> + /* advance to next node in cache hierarchy */
> + node =3D of_find_node_by_phandle(*prop);
> + if (!node) {
> + pr_err("bad cpu reference %d\n", vcpu);
print the full path of any node that has a broken property
> + return ~(u32)0;
> + }
> + }
> +
> + pr_err("stash dest not found for %d on vcpu %d\n",
> + stash_dest_hint, vcpu);
> + return ~(u32)0;
> +}
> +
> +#define QMAN_PAACE 1
> +#define QMAN_PORTAL_PAACE 2
> +#define BMAN_PAACE 3
Documentation?
> +
> +static void setup_qbman_paace(paace_t *ppaace, int paace_type)
> +{
> + switch(paace_type) {
> + case QMAN_PAACE:
> + set_bf(ppaace->impl_attr, PAACE_IA_OTM, PAACE_OTM=
_INDEXED);
> + ppaace->op_encode.index_ot.omi =3D OMI_QMAN_PRIV;
> + /* setup QMAN Private data stashing for the L3 ca=
che */
> + set_bf(ppaace->impl_attr, PAACE_IA_CID, get_stash=
_id(IOMMU_ATTR_CACHE_L3, 0));
> + set_bf(ppaace->domain_attr.to_host.coherency_requ=
ired, PAACE_DA_HOST_CR,
> + 0);
> + break;
> + case QMAN_PORTAL_PAACE:
> + set_bf(ppaace->impl_attr, PAACE_IA_OTM, PAACE_OTM=
_INDEXED);
> + ppaace->op_encode.index_ot.omi =3D OMI_QMAN;
> + /*Set DQRR and Frame stashing for the L3 cache */
> + set_bf(ppaace->impl_attr, PAACE_IA_CID, get_stash=
_id(IOMMU_ATTR_CACHE_L3, 0));
> + break;
> + case BMAN_PAACE:
> + set_bf(ppaace->domain_attr.to_host.coherency_requ=
ired, PAACE_DA_HOST_CR,
> + 0);
> + break;
> + }
> +}
Seriously, you need to document these functions.
> +
> +static void __init setup_omt(struct ome *omt)
> +{
> + struct ome *ome;
> +
> + /* Configure OMI_QMAN */
> + ome =3D &omt[OMI_QMAN];
> +
> + ome->moe[IOE_READ_IDX] =3D EOE_VALID | EOE_READ;
> + ome->moe[IOE_EREAD0_IDX] =3D EOE_VALID | EOE_RSA;
> + ome->moe[IOE_WRITE_IDX] =3D EOE_VALID | EOE_WRITE;
> + ome->moe[IOE_EWRITE0_IDX] =3D EOE_VALID | EOE_WWSAO;
> +
> + ome->moe[IOE_DIRECT0_IDX] =3D EOE_VALID | EOE_LDEC;
> + ome->moe[IOE_DIRECT1_IDX] =3D EOE_VALID | EOE_LDECPE;
> +
> + /* Configure OMI_FMAN */
> + ome =3D &omt[OMI_FMAN];
> + ome->moe[IOE_READ_IDX] =3D EOE_VALID | EOE_READI;
> + ome->moe[IOE_WRITE_IDX] =3D EOE_VALID | EOE_WRITE;
> +
> + /* Configure OMI_QMAN private */
> + ome =3D &omt[OMI_QMAN_PRIV];
> + ome->moe[IOE_READ_IDX] =3D EOE_VALID | EOE_READ;
> + ome->moe[IOE_WRITE_IDX] =3D EOE_VALID | EOE_WRITE;
> + ome->moe[IOE_EREAD0_IDX] =3D EOE_VALID | EOE_RSA;
> + ome->moe[IOE_EWRITE0_IDX] =3D EOE_VALID | EOE_WWSA;
> +
> + /* Configure OMI_CAAM */
> + ome =3D &omt[OMI_CAAM];
> + ome->moe[IOE_READ_IDX] =3D EOE_VALID | EOE_READI;
> + ome->moe[IOE_WRITE_IDX] =3D EOE_VALID | EOE_WRITE;
> +}
> +
> +int setup_one_pamu(unsigned long pamu_reg_base, unsigned long pamu_reg_s=
ize,
> + phys_addr_t ppaact_phys, phys_addr_t spaact_phys,
> + phys_addr_t omt_phys)
> +{
> + u32 *pc;
> + struct pamu_mmap_regs *pamu_regs;
> + u32 pc3_val;
> +
> + pc3_val =3D in_be32((u32 *)(pamu_reg_base + PAMU_PC3));
pamu_reg_base should be a void*. Or even better, create a struct.
> +
> +#define PAMU_PAGE_SHIFT 12
> +#define PAMU_PAGE_SIZE 4096ULL
4096ULL? Why not just 4096?
> +
> +/* This bitmap advertises the page sizes supported by PAMU hardware
> + * to the IOMMU API.
> + */
> +#define FSL_PAMU_PGSIZES (~0xFFFUL)
There should be a better way to define this. ~(PAMU_PAGE_SIZE-1) maybe?
> +
> +static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
> +{
> + u32 subwin_cnt =3D dma_domain->subwin_cnt;
> + unsigned long rpn;
> + int ret =3D 0, i;
> +
> + if (subwin_cnt) {
> + struct dma_subwindow *sub_win_ptr =3D
> + &dma_domain->sub_win_arr[0];
> + for (i =3D 0; i < subwin_cnt; i++) {
> + if (sub_win_ptr[i].valid) {
> + rpn =3D sub_win_ptr[i].paddr >>
> + PAMU_PAGE_SHIFT,
> + spin_lock(&iommu_lock);
> + ret =3D pamu_config_spaace(liodn, subwin_=
cnt, i,
> + sub_win_ptr[i].s=
ize,
> + -1,
> + rpn,
> + dma_domain->snoo=
p_id,
> + dma_domain->stas=
h_id,
> + (i > 0) ? 1 : 0,
> + sub_win_ptr[i].p=
rot);
> + spin_unlock(&iommu_lock);
> + if (ret) {
> + pr_err("PAMU SPAACE configuration=
failed for liodn %d\n",
> + liodn);
> + return ret;
> + }
> + }
> + }
> + } else {
> +
Blank line
> +
> +
> +static struct fsl_dma_domain *iommu_alloc_dma_domain(void)
> +{
> + struct fsl_dma_domain *domain;
> +
> + domain =3D kmem_cache_zalloc(fsl_pamu_domain_cache, GFP_KERNEL);
> + if (!domain)
> + return NULL;
> +
> + domain->stash_id =3D -1;
> + domain->snoop_id =3D -1;
Here, stash_id is set to -1, but in other places, you use ~0. Please
be consistent.
> +
> + INIT_LIST_HEAD(&domain->devices);
> +
> + spin_lock_init(&domain->domain_lock);
> +
> + return domain;
> +}
> +
> +static inline struct fsl_dma_domain *find_domain(struct device *dev)
> +{
> + return dev->archdata.iommu_domain;
> +}
> +
> +static void remove_domain_ref(struct device_domain_info *info, u32 subwi=
n_cnt)
> +{
> + list_del(&info->link);
> + spin_lock(&iommu_lock);
> + if (subwin_cnt)
> + pamu_free_subwins(info->liodn);
> + pamu_disable_liodn(info->liodn);
> + spin_unlock(&iommu_lock);
> + spin_lock(&device_domain_lock);
> + info->dev->archdata.iommu_domain =3D NULL;
> + free_devinfo_mem(info);
> + spin_unlock(&device_domain_lock);
> +}
> +
> +static void destroy_domain(struct fsl_dma_domain *dma_domain)
> +{
> + struct device_domain_info *info;
> +
> + while (!list_empty(&dma_domain->devices)) {
> + info =3D list_entry(dma_domain->devices.next,
> + struct device_domain_info, link);
> + remove_domain_ref(info, dma_domain->subwin_cnt);
> + }
I wonder if you should use list_for_each_safe() instead.
> +
> +static int get_subwin_cnt(dma_addr_t geom_size, u32 subwin, u32 *subwin_=
cnt)
> +{
> +
blank line
> + switch (subwin) {
> + case 0:
> + /* We can't support geometry size > 1MB*/
> + if (geom_size !=3D 1024 * 1024)
Instead of doing 1024*1024, use math that reflects the hardware
limitation of the PAMU: 256 * PAMU_PAGE_SIZE. Create a macro for 256,
like PAMU_MAX_SUBWINDOWS or something.
> + return 0;
> + *subwin_cnt =3D 256;
> + break;
> + case 1:
> + /* No subwindows only a single PAMU window */
> + *subwin_cnt =3D 0;
> + break;
> + default:
> + if (subwin > max_subwindow_count ||
> + (subwin & (subwin - 1)))
> + return 0;
> + *subwin_cnt =3D subwin;
> + }
> + return 1;
> +}
> +
> +static int configure_domain_geometry(struct iommu_domain *domain, void =
*data)
> +{
> + int ret =3D 0;
I don't think you need to initialize 'ret'
> +}
> +
> +static int configure_domain_dma_state(struct fsl_dma_domain *dma_domain,=
int enable)
bool enable
Finally, please CC: me on all IOMMU and PAMU patches you post upstream.
--=20
Timur Tabi
Linux kernel developer at Freescale=
^ permalink raw reply
* Re: Build regressions/improvements in v3.7-rc2
From: Geert Uytterhoeven @ 2012-10-22 19:50 UTC (permalink / raw)
To: Linux Kernel Development; +Cc: linux-s390, linuxppc-dev
In-Reply-To: <alpine.DEB.2.00.1210222146440.12209@ayla.of.borg>
On Mon, Oct 22, 2012 at 9:47 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> JFYI, when comparing v3.7-rc2 to v3.7-rc1[3], the summaries are:
> - build errors: +4/-44
+ arch/s390/include/asm/kvm_para.h: error: redefinition of
'kvm_arch_para_features': => 147:28, 147:99
+ arch/s390/include/asm/kvm_para.h: error: redefinition of
'kvm_check_and_clear_guest_paused': => 152:91, 152:20
s390-allmodconfig/s390-allyesconfig/s390-defconfig
+ error: phy_n.c: relocation truncated to fit: R_PPC64_REL24 against
symbol `._mcount' defined in .text section in
arch/powerpc/kernel/entry_64.o: => (.text+0x1ff9500)
+ error: relocation truncated to fit: R_PPC64_REL24 against symbol
`_savegpr0_24' defined in .text.save.restore section in
arch/powerpc/lib/built-in.o: => (.text+0x1ffac7c)
powerpc-allyesconfig
> [1] http://kisskb.ellerman.id.au/kisskb/head/5550/ (all 117 configs)
> [3] http://kisskb.ellerman.id.au/kisskb/head/5526/ (all 117 configs)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Simon Haggett @ 2012-10-22 11:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Li Yang-R58472, Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, balbi@ti.com,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1535820.NU10ADjvOZ@avalon>
Hi
On 22/10/12 11:47, Laurent Pinchart wrote:
> Hi,
>
> On Monday 22 October 2012 03:33:19 Li Yang-R58472 wrote:
>> On Saturday, October 20, 2012 1:37 AM Felipe Balbi wrote:
>>> On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
>>>> Some gadget drivers may attempt to dequeue requests for an endpoint
>>>> that has already been disabled. For example, in the UVC gadget driver,
>>>> uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
>>>> is selected. When the userspace application subsequently issues the
>>>> VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
>>>
>>> ensure that all requests have been cancelled.
>>>
>>> bug is on uvc gadget, then. Laurent ?
>
> We've discussed this topic a couple of months before. I believe that's not a
> bug.
>
> http://68.183.106.108/lists/linux-usb/msg68869.html
>
>>> Also, fsl should be removed from the tree, I'm trying to persuade iMX
>>> folks to use drivers/usb/chipidea instead.
>>
>> Besides the iMX usage, the driver is also being used by many Freescale
>> PowerPC/Coldfire SoCs. I agree that it's ideal to move to a common driver.
>> But it is a large task to make the chipidea driver works for all the
>> hardware that fsl_udc had supported and been tested on.
>>
>>>> For the Freescale High Speed Dual-Role USB controller,
>>>> fsl_ep_dequeue() provides the implementation of usb_ep_dequeue(). If
>>>> this is called for a disabled endpoint, a kernel oops will occur when
>>>
>>> the ep->ep.desc field is dereferenced (by ep_index()).
>>>
>>>> fsl_ep_disable() sets this field to NULL, as well as deleting all
>>>> pending requests for the endpoint.
>>>>
>>>> This patch adds an additional check to fsl_ep_dequeue() to ensure that
>>>> the endpoint has not already been disabled before attempting to dequeue
>>>
>>> requests.
>>>
>>>> Signed-off-by: Simon Haggett <simon.haggett@realvnc.com>
>>>> ---
>>>>
>>>> drivers/usb/gadget/fsl_udc_core.c | 5 ++++-
>>>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/fsl_udc_core.c
>>>> b/drivers/usb/gadget/fsl_udc_core.c
>>>> index 6ae70cb..acd513b 100644
>>>> --- a/drivers/usb/gadget/fsl_udc_core.c
>>>> +++ b/drivers/usb/gadget/fsl_udc_core.c
>>>> @@ -955,7 +955,10 @@ static int fsl_ep_dequeue(struct usb_ep *_ep,
>>> struct usb_request *_req)
>>>
>>>> int ep_num, stopped, ret = 0;
>>>> u32 epctrl;
>>>>
>>>> - if (!_ep || !_req)
>>>> + /* Ensure that the ep and request are valid, and the ep is not
>>>> + * disabled
>>>> + */
>>>> + if (!_ep || !_req || !ep->ep.desc)
>>>> return -EINVAL;
>
> Shouldn't that last check be done with a lock taken ?
I had presumed a lock wasn't necessary because ep->ep.desc is only set
to NULL by fsl_ep_disable() which, since it is called by
usb_ep_disable(), should only be invoked when no other task is using the
endpoint (according to include/linux/usb/gadget.h). Furthermore, the
chipidea UDC driver does check the equivalent of this field is not NULL
without taking a lock (ep_dequeue() in drivers/usb/chipidea/udc.c).
However, it is possible that I'm misunderstanding something here, so
apologies if I am.
>
>>>> spin_lock_irqsave(&ep->udc->lock, flags);
>
Many thanks
Simon
^ permalink raw reply
* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Felipe Balbi @ 2012-10-22 10:56 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Simon Haggett, Li Yang-R58472, Greg Kroah-Hartman,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
balbi@ti.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1535820.NU10ADjvOZ@avalon>
[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]
Hi,
On Mon, Oct 22, 2012 at 12:47:21PM +0200, Laurent Pinchart wrote:
> Hi,
>
> On Monday 22 October 2012 03:33:19 Li Yang-R58472 wrote:
> > On Saturday, October 20, 2012 1:37 AM Felipe Balbi wrote:
> > > On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> > > > Some gadget drivers may attempt to dequeue requests for an endpoint
> > > > that has already been disabled. For example, in the UVC gadget driver,
> > > > uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
> > > > is selected. When the userspace application subsequently issues the
> > > > VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
> > >
> > > ensure that all requests have been cancelled.
> > >
> > > bug is on uvc gadget, then. Laurent ?
>
> We've discussed this topic a couple of months before. I believe that's not a
> bug.
>
> http://68.183.106.108/lists/linux-usb/msg68869.html
fair enough :-)
That's a different case, however. At the link above we're discussing
dequeueing a request which is already being dequeued. $SUBJECT is trying
to fix dequeueing of a request for an endpoint which isn't even enabled.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Laurent Pinchart @ 2012-10-22 10:47 UTC (permalink / raw)
To: Li Yang-R58472
Cc: Simon Haggett, Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, balbi@ti.com,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <94F013E7935FF44C83EBE7784D62AD3F094730DB@039-SN2MPN1-023.039d.mgd.msft.net>
Hi,
On Monday 22 October 2012 03:33:19 Li Yang-R58472 wrote:
> On Saturday, October 20, 2012 1:37 AM Felipe Balbi wrote:
> > On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> > > Some gadget drivers may attempt to dequeue requests for an endpoint
> > > that has already been disabled. For example, in the UVC gadget driver,
> > > uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
> > > is selected. When the userspace application subsequently issues the
> > > VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
> >
> > ensure that all requests have been cancelled.
> >
> > bug is on uvc gadget, then. Laurent ?
We've discussed this topic a couple of months before. I believe that's not a
bug.
http://68.183.106.108/lists/linux-usb/msg68869.html
> > Also, fsl should be removed from the tree, I'm trying to persuade iMX
> > folks to use drivers/usb/chipidea instead.
>
> Besides the iMX usage, the driver is also being used by many Freescale
> PowerPC/Coldfire SoCs. I agree that it's ideal to move to a common driver.
> But it is a large task to make the chipidea driver works for all the
> hardware that fsl_udc had supported and been tested on.
>
> > > For the Freescale High Speed Dual-Role USB controller,
> > > fsl_ep_dequeue() provides the implementation of usb_ep_dequeue(). If
> > > this is called for a disabled endpoint, a kernel oops will occur when
> >
> > the ep->ep.desc field is dereferenced (by ep_index()).
> >
> > > fsl_ep_disable() sets this field to NULL, as well as deleting all
> > > pending requests for the endpoint.
> > >
> > > This patch adds an additional check to fsl_ep_dequeue() to ensure that
> > > the endpoint has not already been disabled before attempting to dequeue
> >
> > requests.
> >
> > > Signed-off-by: Simon Haggett <simon.haggett@realvnc.com>
> > > ---
> > >
> > > drivers/usb/gadget/fsl_udc_core.c | 5 ++++-
> > > 1 files changed, 4 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/fsl_udc_core.c
> > > b/drivers/usb/gadget/fsl_udc_core.c
> > > index 6ae70cb..acd513b 100644
> > > --- a/drivers/usb/gadget/fsl_udc_core.c
> > > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > > @@ -955,7 +955,10 @@ static int fsl_ep_dequeue(struct usb_ep *_ep,
> > struct usb_request *_req)
> >
> > > int ep_num, stopped, ret = 0;
> > > u32 epctrl;
> > >
> > > - if (!_ep || !_req)
> > > + /* Ensure that the ep and request are valid, and the ep is not
> > > + * disabled
> > > + */
> > > + if (!_ep || !_req || !ep->ep.desc)
> > > return -EINVAL;
Shouldn't that last check be done with a lock taken ?
> > > spin_lock_irqsave(&ep->udc->lock, flags);
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 5/7] usb: gadget: fsl_udc: postpone freeing current dTD
From: Felipe Balbi @ 2012-10-22 7:54 UTC (permalink / raw)
To: Christoph Fritz
Cc: Estevam Fabio-R49496, Li Yang-R58472, Greg Kroah-Hartman,
Chen Peter-B29397, Sascha Hauer, linux-usb, balbi, Hans J. Koch,
Daniel Mack, Christian Hemp, linuxppc-dev, Teresa Gamez,
Sebastian Andrzej Siewior
In-Reply-To: <1350760347.3669.3.camel@mars>
[-- Attachment #1: Type: text/plain, Size: 559 bytes --]
Hi,
On Sat, Oct 20, 2012 at 09:12:27PM +0200, Christoph Fritz wrote:
> On Fri, 2012-10-19 at 13:44 +0300, Felipe Balbi wrote:
> > > I thought about this too but wasn't able to use chipidea with
> > > MXC_EHCI_INTERNAL_PHY as it's called in fsl_udc.
> >
> > that's a matter of writing the PHY driver, right ;-) It has nothing to
> > do with chipidea, actually :-)
>
> Okay, I'll do. But then we should purge the old buggy fsl_udc.
I'm all for that. We shouldn't have multiple copies of a single driver
hanging around.
cheers
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Felipe Balbi @ 2012-10-22 7:53 UTC (permalink / raw)
To: Li Yang-R58472
Cc: Simon Haggett, Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, balbi@ti.com, Laurent Pinchart,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <94F013E7935FF44C83EBE7784D62AD3F094730DB@039-SN2MPN1-023.039d.mgd.msft.net>
[-- Attachment #1: Type: text/plain, Size: 1964 bytes --]
On Mon, Oct 22, 2012 at 03:33:19AM +0000, Li Yang-R58472 wrote:
>
>
> > -----Original Message-----
> > From: Felipe Balbi [mailto:balbi@ti.com]
> > Sent: Saturday, October 20, 2012 1:37 AM
> > To: Simon Haggett
> > Cc: Li Yang-R58472; Felipe Balbi; Greg Kroah-Hartman; linux-
> > usb@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > kernel@vger.kernel.org; Laurent Pinchart
> > Subject: Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests
> > for a disabled USB endpoint on Freescale hardware
> >
> > Hi,
> >
> > On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> > > Some gadget drivers may attempt to dequeue requests for an endpoint
> > > that has already been disabled. For example, in the UVC gadget driver,
> > > uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
> > > is selected. When the userspace application subsequently issues the
> > > VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
> > ensure that all requests have been cancelled.
> >
> > bug is on uvc gadget, then. Laurent ?
> >
> > Also, fsl should be removed from the tree, I'm trying to persuade iMX
> > folks to use drivers/usb/chipidea instead.
>
> Besides the iMX usage, the driver is also being used by many Freescale
> PowerPC/Coldfire SoCs. I agree that it's ideal to move to a common
> driver. But it is a large task to make the chipidea driver works for
> all the hardware that fsl_udc had supported and been tested on.
I understand that, but we just can't keep so many duplicated drivers in
mainline. chipidea udc had at least 3 different implementations. Now
it's the time to combine all of those and stick to a single driver.
Just make a plan to slowly move towards chipidea in the upcoming few
merge windows. I can continue to take in bugfixes for fsl_udc, but only
if I see that you guys are working towards merging with chipidea driver.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 8/10] memory-hotplug : remove page table of x86_64 architecture
From: Wen Congyang @ 2012-10-22 7:11 UTC (permalink / raw)
To: wujianguo
Cc: linux-s390, linux-ia64, jiang.liu, len.brown, linux-acpi,
linux-sh, wujianguo, x86, linux-kernel, cmetcalf, linux-mm,
Yasuaki Ishimatsu, minchan.kim, kosaki.motohiro, rientjes,
sparclinux, qiuxishi, cl, linuxppc-dev, akpm, liuj97
In-Reply-To: <5073DFC0.3010400@gmail.com>
Hi, Wu
Sorry for late reply.
At 10/09/2012 04:26 PM, wujianguo Wrote:
> Hi Congyang,
> I think we should also free pages which are used by page tables after removing
> page tables of the memory.
It is OK to do it.
>
> From: Jianguo Wu <wujianguo@huawei.com>
>
> Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
> arch/x86/mm/init_64.c | 110 +++++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 89 insertions(+), 21 deletions(-)
>
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 5596dfa..81f9c3b 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -675,6 +675,74 @@ int arch_add_memory(int nid, u64 start, u64 size)
> }
> EXPORT_SYMBOL_GPL(arch_add_memory);
>
> +static inline void free_pagetable(struct page *page)
> +{
> + struct zone *zone;
> +
> + __ClearPageReserved(page);
> + __free_page(page);
> +
> + zone = page_zone(page);
> + zone_span_writelock(zone);
> + zone->present_pages++;
> + zone_span_writeunlock(zone);
> + totalram_pages++;
Why do you update zone and totalram_pages here?
> +}
> +
> +static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
> +{
> + pte_t *pte;
> + int i;
> +
> + for (i = 0; i < PTRS_PER_PTE; i++) {
> + pte = pte_start + i;
> + if (pte_val(*pte))
> + break;
> + }
> +
> + /* free a pte talbe */
> + if (i == PTRS_PER_PTE) {
> + free_pagetable(pmd_page(*pmd));
The memory may be allocated at booting. So it is very dangerous to
free it without any check.
> + pmd_clear(pmd);
> + }
> +}
> +
> +static void free_pmd_table(pmd_t *pmd_start, pud_t *pud)
> +{
> + pmd_t *pmd;
> + int i;
> +
> + for (i = 0; i < PTRS_PER_PMD; i++) {
> + pmd = pmd_start + i;
> + if (pmd_val(*pmd))
> + break;
> + }
> +
> + /* free a pmd talbe */
> + if (i == PTRS_PER_PMD) {
> + free_pagetable(pud_page(*pud));
> + pud_clear(pud);
> + }
> +}
> +
> +static void free_pud_table(pud_t *pud_start, pgd_t *pgd)
> +{
> + pud_t *pud;
> + int i;
> +
> + for (i = 0; i < PTRS_PER_PUD; i++) {
> + pud = pud_start + i;
> + if (pud_val(*pud))
> + break;
> + }
> +
> + /* free a pud table */
> + if (i == PTRS_PER_PUD) {
> + free_pagetable(pgd_page(*pgd));
> + pgd_clear(pgd);
> + }
> +}
> +
> static void __meminit
> phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
> {
> @@ -704,21 +772,19 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
> unsigned long pages = 0, next;
> int i = pmd_index(addr);
>
> - for (; i < PTRS_PER_PMD; i++, addr = next) {
> + for (; i < PTRS_PER_PMD && addr < end; i++, addr = next) {
> unsigned long pte_phys;
> pmd_t *pmd = pmd_page + pmd_index(addr);
> pte_t *pte;
>
> - if (addr >= end)
> - break;
> -
> - next = (addr & PMD_MASK) + PMD_SIZE;
> + next = pmd_addr_end(addr, end);
>
> if (!pmd_present(*pmd))
> continue;
>
> if (pmd_large(*pmd)) {
> - if ((addr & ~PMD_MASK) == 0 && next <= end) {
> + if (IS_ALIGNED(addr, PMD_SIZE) &&
> + IS_ALIGNED(next, PMD_SIZE)) {
> set_pmd(pmd, __pmd(0));
> pages++;
> continue;
> @@ -729,7 +795,8 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
> * so split 2M page to 4K page.
> */
> pte = alloc_low_page(&pte_phys);
> - __split_large_page((pte_t *)pmd, addr, pte);
> + __split_large_page((pte_t *)pmd,
> + (unsigned long)__va(addr), pte);
>
> spin_lock(&init_mm.page_table_lock);
> pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
> @@ -738,7 +805,8 @@ phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>
> spin_lock(&init_mm.page_table_lock);
> pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
> - phys_pte_remove(pte, addr, end);
> + phys_pte_remove(pte, addr, next);
> + free_pte_table(pte, pmd);
> unmap_low_page(pte);
> spin_unlock(&init_mm.page_table_lock);
> }
> @@ -751,21 +819,19 @@ phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
> unsigned long pages = 0, next;
> int i = pud_index(addr);
>
> - for (; i < PTRS_PER_PUD; i++, addr = next) {
> + for (; i < PTRS_PER_PUD && addr < end; i++, addr = next) {
> unsigned long pmd_phys;
> pud_t *pud = pud_page + pud_index(addr);
> pmd_t *pmd;
>
> - if (addr >= end)
> - break;
> -
> - next = (addr & PUD_MASK) + PUD_SIZE;
> + next = pud_addr_end(addr, end);
>
> if (!pud_present(*pud))
> continue;
>
> if (pud_large(*pud)) {
> - if ((addr & ~PUD_MASK) == 0 && next <= end) {
> + if (IS_ALIGNED(addr, PUD_SIZE) &&
> + IS_ALIGNED(next, PUD_SIZE)) {
> set_pud(pud, __pud(0));
> pages++;
> continue;
> @@ -776,15 +842,18 @@ phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
> * so split 1G page to 2M page.
> */
> pmd = alloc_low_page(&pmd_phys);
> - __split_large_page((pte_t *)pud, addr, (pte_t *)pmd);
> + __split_large_page((pte_t *)pud,
> + (unsigned long)__va(addr),
> + (pte_t *)pmd);
>
> spin_lock(&init_mm.page_table_lock);
> pud_populate(&init_mm, pud, __va(pmd_phys));
> spin_unlock(&init_mm.page_table_lock);
> }
>
> - pmd = map_low_page(pmd_offset(pud, 0));
> - phys_pmd_remove(pmd, addr, end);
> + pmd = map_low_page((pmd_t *)pud_page_vaddr(*pud));
Hmm, pmd_offset(pud, 0) is equal to (pmd_t *)pud_page_vaddr(*pud).
Is it OK to merge your patch into my patch?
Thanks
Wen Congyang
> + phys_pmd_remove(pmd, addr, next);
> + free_pmd_table(pmd, pud);
> unmap_low_page(pmd);
> __flush_tlb_all();
> }
> @@ -805,15 +874,14 @@ kernel_physical_mapping_remove(unsigned long start, unsigned long end)
> pgd_t *pgd = pgd_offset_k(start);
> pud_t *pud;
>
> - next = (start + PGDIR_SIZE) & PGDIR_MASK;
> - if (next > end)
> - next = end;
> + next = pgd_addr_end(start, end);
>
> if (!pgd_present(*pgd))
> continue;
>
> pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
> - phys_pud_remove(pud, __pa(start), __pa(end));
> + phys_pud_remove(pud, __pa(start), __pa(next));
> + free_pud_table(pud, pgd);
> unmap_low_page(pud);
> }
>
> -- 1.7.6.1 .
>
>
> On 2012-10-5 10:36, Yasuaki Ishimatsu wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> For hot removing memory, we sholud remove page table about the memory.
>> So the patch searches a page table about the removed memory, and clear
>> page table.
>>
>> CC: David Rientjes <rientjes@google.com>
>> CC: Jiang Liu <liuj97@gmail.com>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>> arch/x86/include/asm/pgtable_types.h | 1
>> arch/x86/mm/init_64.c | 147 +++++++++++++++++++++++++++++++++++
>> arch/x86/mm/pageattr.c | 47 +++++------
>> 3 files changed, 173 insertions(+), 22 deletions(-)
>>
>> Index: linux-3.6/arch/x86/mm/init_64.c
>> ===================================================================
>> --- linux-3.6.orig/arch/x86/mm/init_64.c 2012-10-04 18:30:21.171698416 +0900
>> +++ linux-3.6/arch/x86/mm/init_64.c 2012-10-04 18:30:27.317704652 +0900
>> @@ -675,6 +675,151 @@ int arch_add_memory(int nid, u64 start,
>> }
>> EXPORT_SYMBOL_GPL(arch_add_memory);
>>
>> +static void __meminit
>> +phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
>> +{
>> + unsigned pages = 0;
>> + int i = pte_index(addr);
>> +
>> + pte_t *pte = pte_page + pte_index(addr);
>> +
>> + for (; i < PTRS_PER_PTE; i++, addr += PAGE_SIZE, pte++) {
>> +
>> + if (addr >= end)
>> + break;
>> +
>> + if (!pte_present(*pte))
>> + continue;
>> +
>> + pages++;
>> + set_pte(pte, __pte(0));
>> + }
>> +
>> + update_page_count(PG_LEVEL_4K, -pages);
>> +}
>> +
>> +static void __meminit
>> +phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
>> +{
>> + unsigned long pages = 0, next;
>> + int i = pmd_index(addr);
>> +
>> + for (; i < PTRS_PER_PMD; i++, addr = next) {
>> + unsigned long pte_phys;
>> + pmd_t *pmd = pmd_page + pmd_index(addr);
>> + pte_t *pte;
>> +
>> + if (addr >= end)
>> + break;
>> +
>> + next = (addr & PMD_MASK) + PMD_SIZE;
>> +
>> + if (!pmd_present(*pmd))
>> + continue;
>> +
>> + if (pmd_large(*pmd)) {
>> + if ((addr & ~PMD_MASK) == 0 && next <= end) {
>> + set_pmd(pmd, __pmd(0));
>> + pages++;
>> + continue;
>> + }
>> +
>> + /*
>> + * We use 2M page, but we need to remove part of them,
>> + * so split 2M page to 4K page.
>> + */
>> + pte = alloc_low_page(&pte_phys);
>> + __split_large_page((pte_t *)pmd, addr, pte);
>> +
>> + spin_lock(&init_mm.page_table_lock);
>> + pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
>> + spin_unlock(&init_mm.page_table_lock);
>> + }
>> +
>> + spin_lock(&init_mm.page_table_lock);
>> + pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
>> + phys_pte_remove(pte, addr, end);
>> + unmap_low_page(pte);
>> + spin_unlock(&init_mm.page_table_lock);
>> + }
>> + update_page_count(PG_LEVEL_2M, -pages);
>> +}
>> +
>> +static void __meminit
>> +phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
>> +{
>> + unsigned long pages = 0, next;
>> + int i = pud_index(addr);
>> +
>> + for (; i < PTRS_PER_PUD; i++, addr = next) {
>> + unsigned long pmd_phys;
>> + pud_t *pud = pud_page + pud_index(addr);
>> + pmd_t *pmd;
>> +
>> + if (addr >= end)
>> + break;
>> +
>> + next = (addr & PUD_MASK) + PUD_SIZE;
>> +
>> + if (!pud_present(*pud))
>> + continue;
>> +
>> + if (pud_large(*pud)) {
>> + if ((addr & ~PUD_MASK) == 0 && next <= end) {
>> + set_pud(pud, __pud(0));
>> + pages++;
>> + continue;
>> + }
>> +
>> + /*
>> + * We use 1G page, but we need to remove part of them,
>> + * so split 1G page to 2M page.
>> + */
>> + pmd = alloc_low_page(&pmd_phys);
>> + __split_large_page((pte_t *)pud, addr, (pte_t *)pmd);
>> +
>> + spin_lock(&init_mm.page_table_lock);
>> + pud_populate(&init_mm, pud, __va(pmd_phys));
>> + spin_unlock(&init_mm.page_table_lock);
>> + }
>> +
>> + pmd = map_low_page(pmd_offset(pud, 0));
>> + phys_pmd_remove(pmd, addr, end);
>> + unmap_low_page(pmd);
>> + __flush_tlb_all();
>> + }
>> + __flush_tlb_all();
>> +
>> + update_page_count(PG_LEVEL_1G, -pages);
>> +}
>> +
>> +void __meminit
>> +kernel_physical_mapping_remove(unsigned long start, unsigned long end)
>> +{
>> + unsigned long next;
>> +
>> + start = (unsigned long)__va(start);
>> + end = (unsigned long)__va(end);
>> +
>> + for (; start < end; start = next) {
>> + pgd_t *pgd = pgd_offset_k(start);
>> + pud_t *pud;
>> +
>> + next = (start + PGDIR_SIZE) & PGDIR_MASK;
>> + if (next > end)
>> + next = end;
>> +
>> + if (!pgd_present(*pgd))
>> + continue;
>> +
>> + pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
>> + phys_pud_remove(pud, __pa(start), __pa(end));
>> + unmap_low_page(pud);
>> + }
>> +
>> + __flush_tlb_all();
>> +}
>> +
>> #ifdef CONFIG_MEMORY_HOTREMOVE
>> int __ref arch_remove_memory(u64 start, u64 size)
>> {
>> @@ -687,6 +832,8 @@ int __ref arch_remove_memory(u64 start,
>> ret = __remove_pages(zone, start_pfn, nr_pages);
>> WARN_ON_ONCE(ret);
>>
>> + kernel_physical_mapping_remove(start, start + size);
>> +
>> return ret;
>> }
>> #endif
>> Index: linux-3.6/arch/x86/include/asm/pgtable_types.h
>> ===================================================================
>> --- linux-3.6.orig/arch/x86/include/asm/pgtable_types.h 2012-10-04 18:26:51.925486954 +0900
>> +++ linux-3.6/arch/x86/include/asm/pgtable_types.h 2012-10-04 18:30:27.322704656 +0900
>> @@ -334,6 +334,7 @@ static inline void update_page_count(int
>> * as a pte too.
>> */
>> extern pte_t *lookup_address(unsigned long address, unsigned int *level);
>> +extern int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase);
>>
>> #endif /* !__ASSEMBLY__ */
>>
>> Index: linux-3.6/arch/x86/mm/pageattr.c
>> ===================================================================
>> --- linux-3.6.orig/arch/x86/mm/pageattr.c 2012-10-04 18:26:51.923486952 +0900
>> +++ linux-3.6/arch/x86/mm/pageattr.c 2012-10-04 18:30:27.328704662 +0900
>> @@ -501,21 +501,13 @@ out_unlock:
>> return do_split;
>> }
>>
>> -static int split_large_page(pte_t *kpte, unsigned long address)
>> +int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase)
>> {
>> unsigned long pfn, pfninc = 1;
>> unsigned int i, level;
>> - pte_t *pbase, *tmp;
>> + pte_t *tmp;
>> pgprot_t ref_prot;
>> - struct page *base;
>> -
>> - if (!debug_pagealloc)
>> - spin_unlock(&cpa_lock);
>> - base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
>> - if (!debug_pagealloc)
>> - spin_lock(&cpa_lock);
>> - if (!base)
>> - return -ENOMEM;
>> + struct page *base = virt_to_page(pbase);
>>
>> spin_lock(&pgd_lock);
>> /*
>> @@ -523,10 +515,11 @@ static int split_large_page(pte_t *kpte,
>> * up for us already:
>> */
>> tmp = lookup_address(address, &level);
>> - if (tmp != kpte)
>> - goto out_unlock;
>> + if (tmp != kpte) {
>> + spin_unlock(&pgd_lock);
>> + return 1;
>> + }
>>
>> - pbase = (pte_t *)page_address(base);
>> paravirt_alloc_pte(&init_mm, page_to_pfn(base));
>> ref_prot = pte_pgprot(pte_clrhuge(*kpte));
>> /*
>> @@ -579,17 +572,27 @@ static int split_large_page(pte_t *kpte,
>> * going on.
>> */
>> __flush_tlb_all();
>> + spin_unlock(&pgd_lock);
>>
>> - base = NULL;
>> + return 0;
>> +}
>>
>> -out_unlock:
>> - /*
>> - * If we dropped out via the lookup_address check under
>> - * pgd_lock then stick the page back into the pool:
>> - */
>> - if (base)
>> +static int split_large_page(pte_t *kpte, unsigned long address)
>> +{
>> + pte_t *pbase;
>> + struct page *base;
>> +
>> + if (!debug_pagealloc)
>> + spin_unlock(&cpa_lock);
>> + base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
>> + if (!debug_pagealloc)
>> + spin_lock(&cpa_lock);
>> + if (!base)
>> + return -ENOMEM;
>> +
>> + pbase = (pte_t *)page_address(base);
>> + if (__split_large_page(kpte, address, pbase))
>> __free_page(base);
>> - spin_unlock(&pgd_lock);
>>
>> return 0;
>> }
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>
>
>
^ permalink raw reply
* Re: [RFC PATCH powerpc] Fix a lazy irq related WARING in arch_local_irq_restore()
From: Li Zhong @ 2012-10-22 7:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Paul Mackerras, Paul E. McKenney, PowerPC email list
In-Reply-To: <1350518056.4678.113.camel@pasglop>
On Thu, 2012-10-18 at 10:54 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2012-09-26 at 18:10 +0800, Li Zhong wrote:
>
> ../...
>
> Sorry got distracted, got back on this patch today:
>
> > > We might need to "sanitize" the enable state in the PACA before we
> > > actually enter NAP or in the return from NAP code, like we do for normal
> > > idle code...
> >
> > Hi Ben,
> >
> > After some further reading of the code, I updated the code as following,
> > but I'm not very sure, and guess it most probably has some issues ...
> > Could you please help to review and give your comments?
>
> I think it's still not right, see below
>
> > In extended_cede_processor(), if there is still lazy irq pending, I used
> > local_irq_enable() to make sure the irq replayed and flags cleared, but
> > I'm not sure whether it is a proper way.
>
> Right but that will break things for idle. In idle, if you have
> re-enabled interrupts, you need to return and essentially abort the
> attempt at going to nap mode, because the interrupt might have set need
> resched. That's why we normally just check if something's pending and
> return, letting the upper levels re-enable interrupts and do all the
> dirty work for us.
>
> Now, hotplug might differ here in what it needs, but in any case,
> extended_cede_processor doesn't seem to be the right place to handle it,
> at best that function should return if it thinks there's something that
> needs to be done and let the upper layers deal with it appropriately.
>
> > In pseries_mach_cpu_die(), I added local_irq_disable() after cede, and
> > prepare for the start_secondary_resume(), but I'm not sure whether we
> > also need a hard_irq_disable() here.
>
> You probably do if it's going to go back to the start secondary path. It
> shouldn't hurt in any case as long as start_secondary_resume()
> eventually does a local_irq_enable().
>
> > I'm still a little confused by the meaning of PACA_IRQ_HARD_DIS in
> > irq_happened. From the checking at the warning point, it seems only
> > irq_happened equaling 0x1(PACA_IRQ_HARD_DIS) means hard irqs are
> > disabled.
Hi Ben,
Below are my current understandings and a few more questions, please
correct me if there are any misunderstandings. Thank you.
> No. They are disabled if any bit in there corresponding to a "level"
> interrupt is set as well. Only the "edge" interrupts (and decrementer
> which we treat as edge and reset to a high value when it kicks) are
> ignored for the sake of HW irq state.
>From the code, it seems that the hardware_interrupt/external_input
(corresponding to PACA_IRQ_EE) are "level" interrupts. For "level"
interrupts, is it because we will see it again, so we need to hard
disable? or else we might enter into an infinite loop?
> The reason we have this IRQ_HARD_DIS bit is to indicate that a 'manual'
> hard disabling occurred (by opposition to one happening as a result of
> an external interrupt).
The external interrupt causing the hard disabling, is done in the code
of masked_##_H##interrupt: for book3s, or masked_interrupt_book3e
PACA_IRQ_EE 1 for book3e ?
> We need that so we can avoid doing an mfspr() in local_irq_enable() and
> entirely rely on the content of irq_happened to know whether interrupts
> are hard enabled or hard disabled.
Is this about the code in arch_local_irq_restore()? so if (!
irq_happended), we could return directly as we know it is hard enabled.
And here
if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
__hard_irq_disable();
We don't need to hard disable if (irq_happened == PACA_IRQ_HARD_DIS), as
we know it is hard disabled ('manual').
Then here, can we save a few more mtmsrd by also checking PACA_IRQ_EE
bit? like following:
- if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
+ if (unlikely(!(irq_happened & (PACA_IRQ_HARD_DIS | PACA_IRQ_EE))))
> We do that because mfspr is a fairly expensive instruction. But that
> means that we need to make sure we always have a consistent content in
> irq_happened. That's also why I've added all those sanity checks if you
> enable IRQ tracing.
See.
> > Is it possible to set this bit at anyplace the hard irqs are disabled,
> > so then we could check whether this bit is set to know whether hard irqs
> > are disabled? Then it seems that in MASKED_INTERRUPT, we need set this
> > bit where MSR_EE is cleared for something other than decrementer. Maybe
> > I missed too much things?
>
> Either that bit or PACA_IRQ_EE. Both indicate that interrupts are hard
> disabled.
>
> There are some rare cases where do do change MSR:EE without touching
> those bits, only when we're going to restore it shortly afterward in the
> kernel asm exception entry/exit path for example.
I don't get it very clearly here. I might need some more time to read
and understand all the related asm codes.
Currently, it seems to me in EXCEPTION_COMMON, SOFT_DISABLE_INTS is
called to set PACA_IRQ_HARD_DIS, and other bits might be set when
__SOFTEN_TEST (or masked_interrupt_book3e) is called. And in the
exception exit path, something like
SOFT_DISABLE_INTS, .restore_interrupts, restore_check_irq_replay, etc
are called to handle the irq_happened bits.
Thanks, Zhong
> Cheers,
> Ben.
>
> > Thanks, Zhong
> >
> > ===================
> > diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > index 64c97d8..b5f7597 100644
> > --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > @@ -130,6 +130,8 @@ static void pseries_mach_cpu_die(void)
> > extended_cede_processor(cede_latency_hint);
> > }
> >
> > + local_irq_disable();
> > +
> > if (!get_lppaca()->shared_proc)
> > get_lppaca()->donate_dedicated_cpu = 0;
> > get_lppaca()->idle = 0;
> > diff --git a/arch/powerpc/platforms/pseries/plpar_wrappers.h b/arch/powerpc/platforms/pseries/plpar_wrappers.h
> > index 13e8cc4..07560d8 100644
> > --- a/arch/powerpc/platforms/pseries/plpar_wrappers.h
> > +++ b/arch/powerpc/platforms/pseries/plpar_wrappers.h
> > @@ -2,6 +2,7 @@
> > #define _PSERIES_PLPAR_WRAPPERS_H
> >
> > #include <linux/string.h>
> > +#include <linux/irqflags.h>
> >
> > #include <asm/hvcall.h>
> > #include <asm/paca.h>
> > @@ -41,7 +42,19 @@ static inline long extended_cede_processor(unsigned long latency_hint)
> > u8 old_latency_hint = get_cede_latency_hint();
> >
> > set_cede_latency_hint(latency_hint);
> > +
> > + while (!prep_irq_for_idle()) {
> > + local_irq_enable();
> > + local_irq_disable();
> > + }
> > +
> > rc = cede_processor();
> > +#ifdef CONFIG_TRACE_IRQFLAGS
> > + /* Ensure that H_CEDE returns with IRQs on */
> > + if (WARN_ON(!(mfmsr() & MSR_EE)))
> > + __hard_irq_enable();
> > +#endif
> > +
> > set_cede_latency_hint(old_latency_hint);
> >
> > return rc;
> > ===================
> >
> >
> > >
> > > Cheers,
> > > Ben.
> > >
> > > > [ 56.618846] WARNING: at arch/powerpc/kernel/irq.c:240
> > > > [ 56.618851] Modules linked in: rcutorture ipv6 dm_mod ext3 jbd mbcache sg sd_mod crc_t10dif ibmvscsic scsi_transport_srp scsi_tgt ibmveth
> > > > [ 56.618883] NIP: c00000000000ff94 LR: c00000000067a5e0 CTR: 0000000000000001
> > > > [ 56.618889] REGS: c0000001fef6bbe0 TRAP: 0700 Not tainted (3.6.0-rc1-autokern1)
> > > > [ 56.618894] MSR: 8000000000029032 <SF,EE,ME,IR,DR,RI> CR: 42000082 XER: 20000000
> > > > [ 56.618913] SOFTE: 1
> > > > [ 56.618916] CFAR: c00000000067a5dc
> > > > [ 56.618920] TASK = c0000001feed79a0[0] 'swapper/5' THREAD: c0000001fef68000 CPU: 5
> > > > GPR00: 0000000000000001 c0000001fef6be60 c000000000f9ca08 0000000000000001
> > > > GPR04: 0000000000000001 0000000000000008 0000000000000001 0000000000000000
> > > > GPR08: 0000000000000000 c0000001feed79a0 0008a80000000000 0000000000000000
> > > > GPR12: 0000000022000082 c00000000f330f00 c0000001fef6bf90 000000000f394b4c
> > > > GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > > GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > > GPR24: c000000000fe8f80 0000000000000008 0000000000000028 0000000000000000
> > > > GPR28: 0000000000000000 0000000000000020 c000000000f1ab40 0000000000000001
> > > > [ 56.619014] NIP [c00000000000ff94] .arch_local_irq_restore+0x34/0xa0
> > > > [ 56.619020] LR [c00000000067a5e0] .start_secondary+0x368/0x37c
> > > > [ 56.619025] Call Trace:
> > > > [ 56.619030] [c0000001fef6be60] [c000000001ba0500] 0xc000000001ba0500 (unreliable)
> > > > [ 56.619038] [c0000001fef6bed0] [c00000000067a5e0] .start_secondary+0x368/0x37c
> > > > [ 56.619046] [c0000001fef6bf90] [c000000000009380] .start_secondary_resume+0x10/0x14
> > > > [ 56.619052] Instruction dump:
> > > > [ 56.619056] f8010010 f821ff91 986d022a 2fa30000 419e0054 880d022b 78000621 41820048
> > > > [ 56.619071] 2f800001 40de0064 7c0000a6 78008fe2 <0b000000> 2fa00000 40de0050 38000000
> > > > [ 56.619088] ---[ end trace 0199c0d783d7f9ba ]---
> > > >
> > > > Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> > > > ---
> > > > arch/powerpc/platforms/pseries/hotplug-cpu.c | 2 ++
> > > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > > > index 64c97d8..8de539a 100644
> > > > --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > > > +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > > > @@ -137,6 +137,8 @@ static void pseries_mach_cpu_die(void)
> > > > if (get_preferred_offline_state(cpu) == CPU_STATE_ONLINE) {
> > > > unregister_slb_shadow(hwcpu);
> > > >
> > > > + __hard_irq_disable();
> > > > +
> > > > /*
> > > > * Call to start_secondary_resume() will not return.
> > > > * Kernel stack will be reset and start_secondary()
> > >
> > >
> >
>
>
^ permalink raw reply
* [PATCH] TTY: hvcs: fix missing unlock on error in hvcs_initialize()
From: Wei Yongjun @ 2012-10-22 4:42 UTC (permalink / raw)
To: gregkh; +Cc: yongjun_wei, linuxppc-dev, linux-kernel
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Add the missing unlock on the error handling path in function
hvcs_initialize().
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
drivers/tty/hvc/hvcs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index cab5c7a..744c3b8 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -1496,8 +1496,10 @@ static int __devinit hvcs_initialize(void)
num_ttys_to_alloc = hvcs_parm_num_devs;
hvcs_tty_driver = alloc_tty_driver(num_ttys_to_alloc);
- if (!hvcs_tty_driver)
+ if (!hvcs_tty_driver) {
+ mutex_unlock(&hvcs_init_mutex);
return -ENOMEM;
+ }
if (hvcs_alloc_index_list(num_ttys_to_alloc)) {
rc = -ENOMEM;
^ permalink raw reply related
* RE: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests for a disabled USB endpoint on Freescale hardware
From: Li Yang-R58472 @ 2012-10-22 3:33 UTC (permalink / raw)
To: balbi@ti.com, Simon Haggett
Cc: Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
Laurent Pinchart
In-Reply-To: <20121019173718.GA2738@arwen.pp.htv.fi>
> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Saturday, October 20, 2012 1:37 AM
> To: Simon Haggett
> Cc: Li Yang-R58472; Felipe Balbi; Greg Kroah-Hartman; linux-
> usb@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; Laurent Pinchart
> Subject: Re: [PATCH 1/1] usb: gadget: Don't attempt to dequeue requests
> for a disabled USB endpoint on Freescale hardware
>=20
> Hi,
>=20
> On Fri, Oct 19, 2012 at 06:19:26PM +0100, Simon Haggett wrote:
> > Some gadget drivers may attempt to dequeue requests for an endpoint
> > that has already been disabled. For example, in the UVC gadget driver,
> > uvc_function_set_alt() will call usb_ep_disable() when alt setting 0
> > is selected. When the userspace application subsequently issues the
> > VIDIOC_STREAMOFF ioctl, uvc_video_enable() invokes usb_ep_dequeue() to
> ensure that all requests have been cancelled.
>=20
> bug is on uvc gadget, then. Laurent ?
>=20
> Also, fsl should be removed from the tree, I'm trying to persuade iMX
> folks to use drivers/usb/chipidea instead.
Besides the iMX usage, the driver is also being used by many Freescale Powe=
rPC/Coldfire SoCs. I agree that it's ideal to move to a common driver. Bu=
t it is a large task to make the chipidea driver works for all the hardware=
that fsl_udc had supported and been tested on.
>=20
> > For the Freescale High Speed Dual-Role USB controller,
> > fsl_ep_dequeue() provides the implementation of usb_ep_dequeue(). If
> > this is called for a disabled endpoint, a kernel oops will occur when
> the ep->ep.desc field is dereferenced (by ep_index()).
> > fsl_ep_disable() sets this field to NULL, as well as deleting all
> > pending requests for the endpoint.
> >
> > This patch adds an additional check to fsl_ep_dequeue() to ensure that
> > the endpoint has not already been disabled before attempting to dequeue
> requests.
> >
> > Signed-off-by: Simon Haggett <simon.haggett@realvnc.com>
> > ---
> > drivers/usb/gadget/fsl_udc_core.c | 5 ++++-
> > 1 files changed, 4 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/fsl_udc_core.c
> > b/drivers/usb/gadget/fsl_udc_core.c
> > index 6ae70cb..acd513b 100644
> > --- a/drivers/usb/gadget/fsl_udc_core.c
> > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > @@ -955,7 +955,10 @@ static int fsl_ep_dequeue(struct usb_ep *_ep,
> struct usb_request *_req)
> > int ep_num, stopped, ret =3D 0;
> > u32 epctrl;
> >
> > - if (!_ep || !_req)
> > + /* Ensure that the ep and request are valid, and the ep is not
> > + * disabled
> > + */
> > + if (!_ep || !_req || !ep->ep.desc)
> > return -EINVAL;
> >
> > spin_lock_irqsave(&ep->udc->lock, flags);
> > --
> > 1.7.4.1
> >
>=20
> --
> balbi
^ permalink raw reply
* [PATCH] of/fdt: Don't copy garbage after "/" in root node path
From: Benjamin Herrenschmidt @ 2012-10-22 0:32 UTC (permalink / raw)
To: devicetree-discuss; +Cc: linuxppc-dev
The root node path must be internally converted to "/", or various
pieces of code looking for it that way will fail. The code to do
that however had a bug where we might incorrectly append pieces
of the original path from the fdt to the "/".
We should probably add a proper dedicated accessor for the root node
but in the meantime this patch should fix it.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
drivers/of/fdt.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 91a375f..c2b08dc 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -186,6 +186,7 @@ static unsigned long unflatten_dt_node(struct boot_param_header *blob,
*/
fpsize = 1;
allocl = 2;
+ l = 0;
} else {
/* account for '/' and path size minus terminal 0
* already in 'l'
^ permalink raw reply related
* [PATCH] powerpc/powernv: Fix OPAL debug entry
From: Benjamin Herrenschmidt @ 2012-10-22 0:30 UTC (permalink / raw)
To: linuxppc-dev
OPAL provides the firmware base/entry in registers at boot time
for debugging purposes. We had a bug in the code trying to stash
these into the appropriate kernel globals (a line of code was
probably dropped by accident back when this was merged)
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/head_64.S | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index 58bddee..694e3fa 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -703,6 +703,7 @@ _INIT_STATIC(start_here_multiplatform)
#ifdef CONFIG_PPC_EARLY_DEBUG_OPAL
/* Setup OPAL entry */
+ LOAD_REG_ADDR(r11, opal)
std r28,0(r11);
std r29,8(r11);
#endif
^ permalink raw reply related
* Re: ELDK 4.2/kilauea/3.5+ kernel broken
From: Benjamin Herrenschmidt @ 2012-10-21 20:01 UTC (permalink / raw)
To: Robert Berger; +Cc: linuxppc-dev, wd, mla
In-Reply-To: <50841653.4050301@reliableembeddedsystems.com>
On Sun, 2012-10-21 at 18:35 +0300, Robert Berger wrote:
> Hi,
>
> On 10/18/2012 10:03 PM, Benjamin Herrenschmidt wrote:
> >
> > Which looks correct. So this might be something specific to ELDK ?
>
> I just tried with the ELDK 5.2.1 and have exactly the same behavior as
> with ELDK 4.2, so I guess there is something not correct in
> arch/powerpc/sysdev/ppc4xx_msi.c.
>
> Just to exclude the case that I'm doing something stupid which only
> affects kernel versions 3.5+ and not 3.4 can someone else give it a try
> with a kilauea board or something similar?
Remind me what is the symptom ? A specific device isn't working ? Or the
whole kernel goes toast ? My feeling is that those patches make MSIs
work (well that's what they are supposed to do) and for some reason that
doesn't agree with whatever you have connected to the PCIe slot...
Cheers,
Ben.
> Regards,
>
> Robert
>
> >
> > Cheers,
> > Ben.
> >
>
> ..."Serious programming is systems programming or anything beyond
> writing factorial(n) as a student exercise." -- Brian W. Kernighan
>
> My public pgp key is available,at:
> http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x90320BF1
^ permalink raw reply
* Re: ELDK 4.2/kilauea/3.5+ kernel broken
From: Robert Berger @ 2012-10-21 15:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, wd, mla
In-Reply-To: <1350587006.2476.12.camel__24873.815305955$1350587085$gmane$org@pasglop>
Hi,
On 10/18/2012 10:03 PM, Benjamin Herrenschmidt wrote:
>
> Which looks correct. So this might be something specific to ELDK ?
I just tried with the ELDK 5.2.1 and have exactly the same behavior as
with ELDK 4.2, so I guess there is something not correct in
arch/powerpc/sysdev/ppc4xx_msi.c.
Just to exclude the case that I'm doing something stupid which only
affects kernel versions 3.5+ and not 3.4 can someone else give it a try
with a kilauea board or something similar?
Regards,
Robert
>
> Cheers,
> Ben.
>
..."Serious programming is systems programming or anything beyond
writing factorial(n) as a student exercise." -- Brian W. Kernighan
My public pgp key is available,at:
http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x90320BF1
^ permalink raw reply
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