Attention is currently required from: Arthur Heymans, Cliff Huang, David Milosevic, Lance Zhao, Martin L Roth, Maximilian Brune, Patrick Rudolph, Tim Wawrzynczak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78071?usp=email )
Change subject: acpi: Add PPTT support ......................................................................
Patch Set 19:
(2 comments)
File src/acpi/acpi_pptt.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/9e687eaf_ff06df5c : PS17, Line 12: typedef
Removing typedefs would elongate the signatures even more, so I would like to keep them, if possible […]
IMO, it's highly subjective. Upstream, code is more often read than worked on. The average reader probably wouldn't know about local conventions like using typedefs for internal things. It's true that we made an exception for ACPI tables. Maybe that's a bad idea. All the discussions around it could be avoided if we would clean up that code for good.
https://review.coreboot.org/c/coreboot/+/78071/comment/d35abff8_a85bf2c5 : PS17, Line 131: cache_reference_t cache_refs[CONFIG_ACPI_PPTT_MAX_CACHES];
Maybe we could add a small note for the user, that the PPTT code is sensitive to the pointers you pass in.
Sounds good. Actually, after some sleep, I'm not convinced anymore if the deep comparison I proposed would work. There are cases where I'm not sure what the ACPI spec would want us to do. So leaving this to the platform code might be the only option anyway.
Is this not more appropriate to declare this in setup_topology so that all siblings can reuse it? Now you create these references for each call of new_pptt_cpu. return reference_for_cache in pptt_cache if !0?
I have moved the cache list to setup_topology, so this is done. Keeping the discussion open though, in case someone wants reply. Otherwise feel free to mark as done.
But doesn't this suffer from the huge list size like you described above? (I find this hard to discuss btw. when numbers are hidden in undisclosed patches.)