Re: [PATCH v2 02/14] lmb: Rename interior piece to area

[Heinrich Schuchardt]

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.

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
lmb_property -> lmb_region

The comments below are only valid if you stick with area which I
strongly discourage.

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;
  }