[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 <sjg at 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;
>   }