Problems is fdt_init_node (fdt_generic_util.c)

[cmcoldwell]

Testing the variable compat_len before it is initialized.
Leaking memory (device_type = g_strdup_printf("device_type:%s", device_type);

My gawd: this is bush league stuff.

I'm working on a patch.

[cmcoldwell]

I have a set of patches to propose.
0001-Use-a-standard-linked-list-idiom.txt
0002-Straighten-out-the-logic-in-fdt_init_node.txt
0003-Fix-segfault-in-xilinx_intc_fdt_auto_parent.txt
0004-Put-sysbus-MMIO-in-system-memory-map.txt

[edgarigl]

Thanks for contributing!

A few comments:

Unfortunately the patches do not pass checkpatch, you can try on your own by doing:
git show | ./scripts/checkpatch.pl -

It would also be great if you could prefix the summary of the commit message with something.
E.g fdt-generic: Some description
You can see examples in the history for a specific file or submodule with git log -- file.c

Also, if the patch is non-obvious, it would be great if you could provide some context in the commit-message, e.g for patches 2 & 4.

I've fixed up and applied patch 1 & 3.

Could you please fix up and resubmit patch 2 & 4, perhaps as a pull request on github?

Thanks again for contributing!

Cheers,
Edgar

[cmcoldwell]

I went over the other two patches and expanded on my justification in the commit logs, and made sure they pass they scripts/checkpatch.pl test. I should warn that it is possible that 634b8af introduces a deadlock if there is ever a CPU waiting for the interrupt controller to initialize (while the interrupt controller is waiting for the CPU to initialize). I don't think the situation arise in practice, but I would feel better if there were some kind of regression test. It does, however, work for me and fixes a segfault. My last patch "Put sysbus MMIO in system-memory map" is potentially controversial also, if there are places where the sysbus registers shouldn't go into the system-memory (global) memory map.

…

On Tue, Jan 7, 2020 at 8:21 PM Edgar E. Iglesias ***@***.***> wrote: Thanks for contributing! A few comments: Unfortunately the patches do not pass checkpatch, you can try on your own by doing: git show | ./scripts/checkpatch.pl - It would also be great if you could prefix the summary of the commit message with something. E.g fdt-generic: Some description You can see examples in the history for a specific file or submodule with git log -- file.c Also, if the patch is non-obvious, it would be great if you could provide some context in the commit-message, e.g for patches 2 & 4. I've fixed up and applied patch 1 & 3. Could you please fix up and resubmit patch 2 & 4, perhaps as a pull request on github? Thanks again for contributing! Cheers, Edgar — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#39?email_source=notifications&email_token=AOFKTQT6GJXDDTRUBMVXGZLQ4UTA3A5CNFSM4KCDSFQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIK3POA#issuecomment-571848632>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOFKTQWRB4JRMZMBYY4V6K3Q4UTA3ANCNFSM4KCDSFQA> .

-- Charles M. Coldwell, W1CMC Belmont, Massachusetts, New England "Turn on, log in, tune out"

[cmcoldwell]

Forgot to add the patches.

…

On Wed, Jan 8, 2020 at 8:54 AM Charles Coldwell ***@***.***> wrote: I went over the other two patches and expanded on my justification in the commit logs, and made sure they pass they scripts/checkpatch.pl test. I should warn that it is possible that 634b8af introduces a deadlock if there is ever a CPU waiting for the interrupt controller to initialize (while the interrupt controller is waiting for the CPU to initialize). I don't think the situation arise in practice, but I would feel better if there were some kind of regression test. It does, however, work for me and fixes a segfault. My last patch "Put sysbus MMIO in system-memory map" is potentially controversial also, if there are places where the sysbus registers shouldn't go into the system-memory (global) memory map. On Tue, Jan 7, 2020 at 8:21 PM Edgar E. Iglesias ***@***.***> wrote: > Thanks for contributing! > > A few comments: > > Unfortunately the patches do not pass checkpatch, you can try on your own > by doing: > git show | ./scripts/checkpatch.pl - > > It would also be great if you could prefix the summary of the commit > message with something. > E.g fdt-generic: Some description > You can see examples in the history for a specific file or submodule with > git log -- file.c > > Also, if the patch is non-obvious, it would be great if you could provide > some context in the commit-message, e.g for patches 2 & 4. > > I've fixed up and applied patch 1 & 3. > > Could you please fix up and resubmit patch 2 & 4, perhaps as a pull > request on github? > > Thanks again for contributing! > > Cheers, > Edgar > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#39?email_source=notifications&email_token=AOFKTQT6GJXDDTRUBMVXGZLQ4UTA3A5CNFSM4KCDSFQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIK3POA#issuecomment-571848632>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AOFKTQWRB4JRMZMBYY4V6K3Q4UTA3ANCNFSM4KCDSFQA> > . > -- Charles M. Coldwell, W1CMC Belmont, Massachusetts, New England "Turn on, log in, tune out"

-- Charles M. Coldwell, W1CMC Belmont, Massachusetts, New England "Turn on, log in, tune out"

[cmcoldwell]

0001-Straighten-out-the-logic-in-fdt_init_node.txt
0002-Put-sysbus-MMIO-in-system-memory-map.txt

[edgarigl]

Hi,

I've applied patch #1.

Patch number 2 breaks our system though. We rely quite heavily on attaching devices to the actual parent bus. I'm not sure how we'd work around this for device-tree's that don't match this. Perhaps you could find a way to specifically catch the orphaned case and apply your logic only in that case.

Did you publish the device-tree causing problems somewhere?
Is that device-tree auto-generated or are we in a position to hand-edit it?

Thanks,
Edgar

[cmcoldwell]

The device tree that causes the problem was auto-generated by the Xilinx HSI utility (although I hand edited system-top.dts to add the memory@0 node). I'm attaching the files.

I'll take another look and see if I can find a better way. To be honest, this method felt pretty heavy-handed when I did it.

dts.zip

[cmcoldwell]

OK, here's another swing at the problem. I'll admit my understanding of QOM and QDev (and the relationship between the two) is limited, but it looks like when MMIO regions (e.g. "amba_pl") are "orphaned", they get added as "child" type properties to the root node of the object tree. This code finds those properties and adds them to the system bus memory map. Passes checkpatch.pl.

0001-Put-orphaned-MMIO-into-system-memory-map.txt

[cmcoldwell]

Any feedback on this patch? #39 (comment)

…

On Wed, Jan 8, 2020 at 1:04 PM Edgar E. Iglesias ***@***.***> wrote: Hi, I've applied patch #1 <#1>. Patch number 2 breaks our system though. We rely quite heavily on attaching devices to the actual parent bus. I'm not sure how we'd work around this for device-tree's that don't match this. Perhaps you could find a way to specifically catch the orphaned case and apply your logic only in that case. Did you publish the device-tree causing problems somewhere? Is that device-tree auto-generated or are we in a position to hand-edit it? Thanks, Edgar — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#39?email_source=notifications&email_token=AOFKTQRKINMMQI5C2LKVQPLQ4YITXA5CNFSM4KCDSFQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEINOF3I#issuecomment-572187373>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOFKTQRKA2Q2P7HF2NUXK53Q4YITXANCNFSM4KCDSFQA> .

-- Charles M. Coldwell, W1CMC Belmont, Massachusetts, New England "Turn on, log in, tune out"