Hi Heinrich, On Fri, 1 Sept 2023 at 00:16, Heinrich Schuchardt <xypron.g...@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 <s...@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]; > > #endif > > }; > > > > 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