Hi Heinrich,
On Fri, 1 Sept 2023 at 00:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 9/1/23 03:13, Simon Glass wrote:
The lmb data structures use the word 'region' to describe the region in which the allocations happen, as well as the individual allocations in that region. The interior structure is called lmb_property but is described as a region.
This is quite confusing. One of the reviewers in v1 asked why we are using a property to describe a region!
We currently have:
struct lmb_region - Description of a set of region. struct lmb_property - Description of one region.
The word 'region' implies that it is contiguous, while 'region set' implies that its members might not be contiguous.
From what I can tell, the areas inside a region are contiguous. The
code is so badly commented that it is hard to know what the intent was.
Actually I just looked it up and found it came from Linux, so I suppose that explains the lack of comments. There it has been renamed memblock.
Could you take a look and see if we could adopt the same naming?
Calling a set region is what starts the confusion.
Your patch is creating new confusion by calling a member of "a set of regions" "area".
Please, rename as follows:
lmb_region -> lmb_region_set
That sounds like it sets a region
lmb_property -> lmb_region
The comments below are only valid if you stick with area which I strongly discourage.
Fair enough, I don't like it much either.
It seems better to adopt the word 'area' for the internal pieces, since an area is smaller than a region.
As a first step to improve this, rename struct lmb_property to lmb_area.
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
include/lmb.h | 12 ++++++------ test/lib/lmb.c | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 231b68b27d91..4b7664f22c1d 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -23,13 +23,13 @@ enum lmb_flags { };
/**
- struct lmb_property - Description of one region.
- struct lmb_area - Description of one region.
%s/region/memory area/
For struct lmb_area we need a long text explaining what it describes.
- @base: Base address of the region.
- @size: Size of the region
- @flags: memory region attributes
*/ -struct lmb_property { +struct lmb_area { phys_addr_t base; phys_size_t size; enum lmb_flags flags; @@ -64,9 +64,9 @@ struct lmb_region { unsigned long cnt; unsigned long max; #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
%s/MAX_REGIONS/MAX_AREAS/
struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
struct lmb_area region[CONFIG_LMB_MAX_REGIONS];This is really confusing: An area called region and indexed by something related to regions not areas.
#else
struct lmb_property *region;
struct lmb_area *region;ditto
Best regards
Heinrich
#endif };
@@ -87,8 +87,8 @@ struct lmb { struct lmb_region memory; struct lmb_region reserved; #if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
struct lmb_area memory_regions[CONFIG_LMB_MEMORY_REGIONS];struct lmb_area reserved_regions[CONFIG_LMB_RESERVED_REGIONS];diff --git a/test/lib/lmb.c b/test/lib/lmb.c index 162887503588..59b140fde4ce 100644 --- a/test/lib/lmb.c +++ b/test/lib/lmb.c @@ -12,7 +12,7 @@ #include <test/test.h> #include <test/ut.h>
-static inline bool lmb_is_nomap(struct lmb_property *m) +static inline bool lmb_is_nomap(struct lmb_area *m) { return m->flags & LMB_NOMAP; }
Regards, Simon