| Message ID | 20200619025752.200662-1-bauerman@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | dt: Set new property length in dt_resize_property() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (fe70fbb78d33abea788a3221bc409a7c50c019c3) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On 6/19/20 8:27 AM, Thiago Jung Bauermann wrote: > All callers of dt_resize_property() need to set the new property length > after calling it. append_chip_id() wasn't doing it, which caused this > assert when booting my machine: > > [ 136.387213258,3] Unable to use memory range 0 from MSAREA 0 > [ 136.387356677,3] Unable to use memory range 0 from MSAREA 2 > [ 136.387408390,3] *********************************************** > [ 136.387454272,3] < assert failed at core/device.c:605 > > [ 136.387493225,3] . > [ 136.387512799,3] . > [ 136.387534056,3] . > [ 136.387550294,3] OO__) > [ 136.387579530,3] <"__/ > [ 136.387605086,3] ^ ^ > [ 136.387719329,3] Fatal TRAP at 0000000030028a18 .dt_property_set_cell+0x34 MSR 9000000000021002 > [ 136.387801707,3] CFAR : 00000000300bfd3c MSR : 9000000000001000 > [ 136.387847032,3] SRR0 : 0000000030028a18 SRR1 : 9000000000021002 > [ 136.387893119,3] HSRR0: 0000000030012524 HSRR1: 9000000000001000 > [ 136.387936830,3] DSISR: 40000000 DAR : 00000002019df000 > [ 136.387983570,3] LR : 00000000300bfd40 CTR : 0000000000000000 > [ 136.388046031,3] CR : 20004202 XER : 00000000 > [ 136.388094553,3] GPR00: 00000000300bfd40 GPR16: 0000000000000001 > [ 136.388139862,3] GPR01: 0000000031e536e0 GPR17: 00000000300ca3c9 > [ 136.388181131,3] GPR02: 0000000030121200 GPR18: 0000000030103e1c > [ 136.388224105,3] GPR03: 000000003053fc60 GPR19: 0000000000000008 > [ 136.388270356,3] GPR04: 0000000000000001 GPR20: 000000003053fba0 > [ 136.388313950,3] GPR05: 0000000000000008 GPR21: 0000000000000001 > [ 136.388363021,3] GPR06: 0000000031e50060 GPR22: 0000000000000001 > [ 136.388416754,3] GPR07: 0000000000000000 GPR23: 0000000000000000 > [ 136.388465729,3] GPR08: 0000000000000000 GPR24: 0000000000000000 > [ 136.388508156,3] GPR09: 0000000000000004 GPR25: 0000000031204060 > [ 136.388556203,3] GPR10: 0000000000000008 GPR26: 000000003120402c > [ 136.388599076,3] GPR11: 0000000000000000 GPR27: 0000000030010000 > [ 136.388642108,3] GPR12: 0000000040004204 GPR28: 0000000000000002 > [ 136.388694064,3] GPR13: 0000000031e50000 GPR29: 0000000031203ee0 > [ 136.388743298,3] GPR14: 00000000300cbf03 GPR30: 0000000031202e80 > [ 136.388797131,3] GPR15: 00000000300cc01c GPR31: 0000000030103a33 > CPU 0048 Backtrace: > S: 0000000031e539e0 R: 0000000030028874 .dt_resize_property+0x28 > S: 0000000031e53a60 R: 00000000300bfd40 .memory_parse+0xd84 > S: 0000000031e53c40 R: 00000000300bc4d8 .parse_hdat+0xed0 > S: 0000000031e53e30 R: 000000003001504c .main_cpu_entry+0x1ac > S: 0000000031e53f00 R: 0000000030002760 boot_entry+0x1b0 > > Avoid further appearances of the unidentified animal of doom by making > dt_resize_property() do the length updating itself, freeing its callers > from that need. > > Suggested-by: Oliver O'Halloran <oohall@gmail.com> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> Looks good. Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> -Vasant
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes: > On 6/19/20 8:27 AM, Thiago Jung Bauermann wrote: >> Suggested-by: Oliver O'Halloran <oohall@gmail.com> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > Looks good. > Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> Thanks for the review!
On Fri, Jun 19, 2020 at 12:58 PM Thiago Jung Bauermann <bauerman@linux.ibm.com> wrote: > > All callers of dt_resize_property() need to set the new property length > after calling it. append_chip_id() wasn't doing it, which caused this > assert when booting my machine: > > *snip* > > Avoid further appearances of the unidentified animal of doom by making > dt_resize_property() do the length updating itself, freeing its callers > from that need. > > Suggested-by: Oliver O'Halloran <oohall@gmail.com> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> Thanks, I'm still surprised we haven't hit this bug until now. Merged as 5021a037a790.
diff --git a/core/cpufeatures.c b/core/cpufeatures.c index c6754abbe..f31850ab6 100644 --- a/core/cpufeatures.c +++ b/core/cpufeatures.c @@ -827,7 +827,6 @@ static void add_cpufeatures_dependencies(struct dt_node *features) deps_names = f->dependencies_names; nr_deps = strcount(deps_names, " ") + 1; dt_resize_property(&deps, nr_deps * sizeof(u32)); - deps->len = nr_deps * sizeof(u32); DBG("feature %s has %d dependencies (%s)\n", f->name, nr_deps, deps_names); /* diff --git a/core/device.c b/core/device.c index 4fd44597b..b102dd973 100644 --- a/core/device.c +++ b/core/device.c @@ -476,6 +476,7 @@ void dt_resize_property(struct dt_property **prop, size_t len) size_t new_len = sizeof(**prop) + len; *prop = realloc(*prop, new_len); + (*prop)->len = len; /* Fix up linked lists in case we moved. (note: not an empty list). */ (*prop)->list.next->prev = &(*prop)->list; diff --git a/hdata/hostservices.c b/hdata/hostservices.c index 9e53a865e..2181ecd8b 100644 --- a/hdata/hostservices.c +++ b/hdata/hostservices.c @@ -36,7 +36,6 @@ static void merge_property(const struct dt_node *src_root, /* Append src to dst. */ dt_resize_property(&dst, dst->len + src->len); memcpy(dst->prop + dst->len, src->prop, src->len); - dst->len += src->len; } static void hservice_parse_dt_tree(const struct dt_node *src) diff --git a/hw/npu.c b/hw/npu.c index 9266f3674..dba7ee50f 100644 --- a/hw/npu.c +++ b/hw/npu.c @@ -416,7 +416,6 @@ static void npu_append_pci_phandle(struct dt_node *dn, u32 phandle) prop_len = pci_npu_phandle_prop->len; prop_len += sizeof(*npu_phandles); dt_resize_property(&pci_npu_phandle_prop, prop_len); - pci_npu_phandle_prop->len = prop_len; npu_phandles = (uint32_t *) pci_npu_phandle_prop->prop; npu_phandles[prop_len/sizeof(*npu_phandles) - 1] = phandle; diff --git a/hw/npu2.c b/hw/npu2.c index 1624d52cf..cf57eeb0c 100644 --- a/hw/npu2.c +++ b/hw/npu2.c @@ -510,7 +510,6 @@ static void npu2_append_phandle(struct dt_node *dn, /* Need to append to the properties */ len = prop->len + sizeof(*npu_phandles); dt_resize_property(&prop, len); - prop->len = len; npu_phandles = (uint32_t *)prop->prop; npu_phandles[len / sizeof(*npu_phandles) - 1] = phandle; diff --git a/hw/npu3-nvlink.c b/hw/npu3-nvlink.c index 42054cfca..920864b32 100644 --- a/hw/npu3-nvlink.c +++ b/hw/npu3-nvlink.c @@ -350,7 +350,6 @@ static void npu3_append_phandle(struct dt_node *dn, const char *name, /* Need to append to the property */ len = prop->len + sizeof(*phandles); dt_resize_property(&prop, len); - prop->len = len; phandles = (uint32_t *)prop->prop; phandles[len / sizeof(*phandles) - 1] = phandle;
All callers of dt_resize_property() need to set the new property length after calling it. append_chip_id() wasn't doing it, which caused this assert when booting my machine: [ 136.387213258,3] Unable to use memory range 0 from MSAREA 0 [ 136.387356677,3] Unable to use memory range 0 from MSAREA 2 [ 136.387408390,3] *********************************************** [ 136.387454272,3] < assert failed at core/device.c:605 > [ 136.387493225,3] . [ 136.387512799,3] . [ 136.387534056,3] . [ 136.387550294,3] OO__) [ 136.387579530,3] <"__/ [ 136.387605086,3] ^ ^ [ 136.387719329,3] Fatal TRAP at 0000000030028a18 .dt_property_set_cell+0x34 MSR 9000000000021002 [ 136.387801707,3] CFAR : 00000000300bfd3c MSR : 9000000000001000 [ 136.387847032,3] SRR0 : 0000000030028a18 SRR1 : 9000000000021002 [ 136.387893119,3] HSRR0: 0000000030012524 HSRR1: 9000000000001000 [ 136.387936830,3] DSISR: 40000000 DAR : 00000002019df000 [ 136.387983570,3] LR : 00000000300bfd40 CTR : 0000000000000000 [ 136.388046031,3] CR : 20004202 XER : 00000000 [ 136.388094553,3] GPR00: 00000000300bfd40 GPR16: 0000000000000001 [ 136.388139862,3] GPR01: 0000000031e536e0 GPR17: 00000000300ca3c9 [ 136.388181131,3] GPR02: 0000000030121200 GPR18: 0000000030103e1c [ 136.388224105,3] GPR03: 000000003053fc60 GPR19: 0000000000000008 [ 136.388270356,3] GPR04: 0000000000000001 GPR20: 000000003053fba0 [ 136.388313950,3] GPR05: 0000000000000008 GPR21: 0000000000000001 [ 136.388363021,3] GPR06: 0000000031e50060 GPR22: 0000000000000001 [ 136.388416754,3] GPR07: 0000000000000000 GPR23: 0000000000000000 [ 136.388465729,3] GPR08: 0000000000000000 GPR24: 0000000000000000 [ 136.388508156,3] GPR09: 0000000000000004 GPR25: 0000000031204060 [ 136.388556203,3] GPR10: 0000000000000008 GPR26: 000000003120402c [ 136.388599076,3] GPR11: 0000000000000000 GPR27: 0000000030010000 [ 136.388642108,3] GPR12: 0000000040004204 GPR28: 0000000000000002 [ 136.388694064,3] GPR13: 0000000031e50000 GPR29: 0000000031203ee0 [ 136.388743298,3] GPR14: 00000000300cbf03 GPR30: 0000000031202e80 [ 136.388797131,3] GPR15: 00000000300cc01c GPR31: 0000000030103a33 CPU 0048 Backtrace: S: 0000000031e539e0 R: 0000000030028874 .dt_resize_property+0x28 S: 0000000031e53a60 R: 00000000300bfd40 .memory_parse+0xd84 S: 0000000031e53c40 R: 00000000300bc4d8 .parse_hdat+0xed0 S: 0000000031e53e30 R: 000000003001504c .main_cpu_entry+0x1ac S: 0000000031e53f00 R: 0000000030002760 boot_entry+0x1b0 Avoid further appearances of the unidentified animal of doom by making dt_resize_property() do the length updating itself, freeing its callers from that need. Suggested-by: Oliver O'Halloran <oohall@gmail.com> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> --- core/cpufeatures.c | 1 - core/device.c | 1 + hdata/hostservices.c | 1 - hw/npu.c | 1 - hw/npu2.c | 1 - hw/npu3-nvlink.c | 1 - 6 files changed, 1 insertion(+), 5 deletions(-)