[PATCH v2 0/4] lmb: Improve style

[PATCH v2 0/4] lmb: Improve style

[Sam Protsenko]

This series is intended to be picked up for -next.

Cleanup the LMB code a bit, after fixing the false positive error
messages. No functional change. This series depends on [1] (which is
"lmb: Fix reserving the same region multiple times").

Changes in v2:
  - The original series was split by actual functional change patches
    [1] and cleanup patches (this series)
  - Collected review tags

[1] https://lists.denx.de/pipermail/u-boot/2024-December/574606.html

Sam Protsenko (4):
  lmb: Fix flags data type in lmb_add_region_flags()
  lmb: Make const flag_str[] in lmb_print_region_flags() more const
  lmb: Improve coding style
  lmb: Improve kernel-doc comments

 include/lmb.h | 125 ++++++++++++++++++++++++++++++--------------------
 lib/lmb.c     |  87 ++++++++---------------------------
 2 files changed, 94 insertions(+), 118 deletions(-)

-- 
2.39.5

[PATCH v2 1/4] lmb: Fix flags data type in lmb_add_region_flags()

[Sam Protsenko]

rgnflags variable in lmb_add_region_flags() has incorrect type: it's
declared as phys_size_t when it should be enum lmb_flags. That
copy-paste mistake was firstly introduced in commit 59c0ea5df33f ("lmb:
Add support of flags for no-map properties"), and then copied further
into commit ed17a33fed29 ("lmb: make LMB memory map persistent and
global"). Fix it by using the correct type to match struct lmb_region
field.

No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Acked-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes in v2:
  - Added R-b tag from Ilias
  - Added Acked-by tag from Sughosh
  - Removed Fixes tags, as suggested by Sughosh

 lib/lmb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index a695edf70dfa..1d57f48bff67 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -202,7 +202,7 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
 	for (i = 0; i < lmb_rgn_lst->count; i++) {
 		phys_addr_t rgnbase = rgn[i].base;
 		phys_size_t rgnsize = rgn[i].size;
-		phys_size_t rgnflags = rgn[i].flags;
+		enum lmb_flags rgnflags = rgn[i].flags;
 
 		ret = lmb_addrs_adjacent(base, size, rgnbase, rgnsize);
 		if (ret > 0) {
-- 
2.39.5

[PATCH v2 2/4] lmb: Make const flag_str[] in lmb_print_region_flags() more const

[Sam Protsenko]

flag_str[] is a pointer to const. Make it also a const pointer. Improve
a style a bit while a it, to make this line fit 80 characters limit.

No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes in v2:
  - Added R-b tag from Ilias

 lib/lmb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index 1d57f48bff67..0f9de26b64af 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -472,7 +472,8 @@ static int lmb_map_update_notify(phys_addr_t addr, phys_size_t size, u8 op,
 
 static void lmb_print_region_flags(enum lmb_flags flags)
 {
-	const char *flag_str[] = { "none", "no-map", "no-overwrite", "no-notify" };
+	const char * const flag_str[] = { "none", "no-map", "no-overwrite",
+					  "no-notify" };
 	unsigned int pflags = flags &
 			      (LMB_NOMAP | LMB_NOOVERWRITE | LMB_NONOTIFY);
 
-- 
2.39.5

[PATCH v2 3/4] lmb: Improve coding style

[Sam Protsenko]

Fix checkpatch warnings. No functional change.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes in v2:
  - Added Acked-by tag from Ilias

 lib/lmb.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index 0f9de26b64af..40f03151929c 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -57,7 +57,6 @@ static long lmb_regions_overlap(struct alist *lmb_rgn_lst, unsigned long r1,
 				unsigned long r2)
 {
 	struct lmb_region *rgn = lmb_rgn_lst->data;
-
 	phys_addr_t base1 = rgn[r1].base;
 	phys_size_t size1 = rgn[r1].size;
 	phys_addr_t base2 = rgn[r2].base;
@@ -70,11 +69,11 @@ static long lmb_regions_adjacent(struct alist *lmb_rgn_lst, unsigned long r1,
 				 unsigned long r2)
 {
 	struct lmb_region *rgn = lmb_rgn_lst->data;
-
 	phys_addr_t base1 = rgn[r1].base;
 	phys_size_t size1 = rgn[r1].size;
 	phys_addr_t base2 = rgn[r2].base;
 	phys_size_t size2 = rgn[r2].size;
+
 	return lmb_addrs_adjacent(base1, size1, base2, size2);
 }
 
@@ -228,6 +227,8 @@ static long lmb_add_region_flags(struct alist *lmb_rgn_lst, phys_addr_t base,
 
 			coalesced++;
 			break;
+
+			return -1;
 		}
 	}
 
@@ -278,14 +279,17 @@ static long _lmb_free(struct alist *lmb_rgn_lst, phys_addr_t base,
 	phys_addr_t end = base + size - 1;
 	int i;
 
-	rgnbegin = rgnend = 0; /* supress gcc warnings */
+	/* Suppress GCC warnings */
+	rgnbegin = 0;
+	rgnend = 0;
+
 	rgn = lmb_rgn_lst->data;
 	/* Find the region where (base, size) belongs to */
 	for (i = 0; i < lmb_rgn_lst->count; i++) {
 		rgnbegin = rgn[i].base;
 		rgnend = rgnbegin + rgn[i].size - 1;
 
-		if ((rgnbegin <= base) && (end <= rgnend))
+		if (rgnbegin <= base && end <= rgnend)
 			break;
 	}
 
@@ -294,7 +298,7 @@ static long _lmb_free(struct alist *lmb_rgn_lst, phys_addr_t base,
 		return -1;
 
 	/* Check to see if we are removing entire region */
-	if ((rgnbegin == base) && (rgnend == end)) {
+	if (rgnbegin == base && rgnend == end) {
 		lmb_remove_region(lmb_rgn_lst, i);
 		return 0;
 	}
@@ -330,6 +334,7 @@ static long lmb_overlaps_region(struct alist *lmb_rgn_lst, phys_addr_t base,
 	for (i = 0; i < lmb_rgn_lst->count; i++) {
 		phys_addr_t rgnbase = rgn[i].base;
 		phys_size_t rgnsize = rgn[i].size;
+
 		if (lmb_addrs_overlap(base, size, rgnbase, rgnsize))
 			break;
 	}
@@ -705,7 +710,7 @@ long lmb_reserve(phys_addr_t base, phys_size_t size)
 }
 
 static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
-				    phys_addr_t max_addr, enum lmb_flags flags)
+				   phys_addr_t max_addr, enum lmb_flags flags)
 {
 	int ret;
 	long i, rgn;
@@ -720,16 +725,18 @@ static phys_addr_t _lmb_alloc_base(phys_size_t size, ulong align,
 
 		if (lmbsize < size)
 			continue;
-		if (max_addr == LMB_ALLOC_ANYWHERE)
+
+		if (max_addr == LMB_ALLOC_ANYWHERE) {
 			base = lmb_align_down(lmbbase + lmbsize - size, align);
-		else if (lmbbase < max_addr) {
+		} else if (lmbbase < max_addr) {
 			base = lmbbase + lmbsize;
 			if (base < lmbbase)
 				base = -1;
 			base = min(base, max_addr);
 			base = lmb_align_down(base - size, align);
-		} else
+		} else {
 			continue;
+		}
 
 		while (base && lmbbase <= base) {
 			rgn = lmb_overlaps_region(&lmb.used_mem, base, size);
@@ -803,7 +810,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
 }
 
 static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size,
-				    enum lmb_flags flags)
+				   enum lmb_flags flags)
 {
 	long rgn;
 	struct lmb_region *lmb_memory = lmb.free_mem.data;
-- 
2.39.5

[PATCH v2 4/4] lmb: Improve kernel-doc comments

[Sam Protsenko]

Fix warnings from kernel-doc script. Improve and unify overall style of
kernel-doc comments in lmb source files. Move all kernel-doc comments
for public functions into the header, as recommended in U-Boot
documentation [1]:

    Non-trivial functions should have a comment which describes what
    they do. If it is an exported function, put the comment in the
    header file so the API is in one place. If it is a static function,
    put it in the C file.

This also takes care of existing duplication. While at it, do a bit of
cosmetic cleanups as well.

No functional change.

[1] doc/develop/codingstyle.rst

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes in v2:
  - Added Acked-by tag from Ilias
  - Provided the excerpt from U-Boot doc about comments for public API

 include/lmb.h | 125 ++++++++++++++++++++++++++++++--------------------
 lib/lmb.c     |  55 ----------------------
 2 files changed, 74 insertions(+), 106 deletions(-)

diff --git a/include/lmb.h b/include/lmb.h
index f221f0cce8f7..03d5fac6aa79 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -1,6 +1,13 @@
 /* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Logical memory blocks.
+ *
+ * Copyright (C) 2001 Peter Bergner, IBM Corp.
+ */
+
 #ifndef _LINUX_LMB_H
 #define _LINUX_LMB_H
+
 #ifdef __KERNEL__
 
 #include <alist.h>
@@ -8,21 +15,15 @@
 #include <asm/u-boot.h>
 #include <linux/bitops.h>
 
-/*
- * Logical memory blocks.
- *
- * Copyright (C) 2001 Peter Bergner, IBM Corp.
- */
-
-#define LMB_ALLOC_ANYWHERE      0
-#define LMB_ALIST_INITIAL_SIZE  4
+#define LMB_ALLOC_ANYWHERE	0
+#define LMB_ALIST_INITIAL_SIZE	4
 
 /**
- * enum lmb_flags - definition of memory region attributes
- * @LMB_NONE: no special request
- * @LMB_NOMAP: don't add to mmu configuration
- * @LMB_NOOVERWRITE: the memory region cannot be overwritten/re-reserved
- * @LMB_NONOTIFY: do not notify other modules of changes to this memory region
+ * enum lmb_flags - Definition of memory region attributes
+ * @LMB_NONE: No special request
+ * @LMB_NOMAP: Don't add to MMU configuration
+ * @LMB_NOOVERWRITE: The memory region cannot be overwritten/re-reserved
+ * @LMB_NONOTIFY: Do not notify other modules of changes to this memory region
  */
 enum lmb_flags {
 	LMB_NONE		= 0,
@@ -32,11 +33,10 @@ enum lmb_flags {
 };
 
 /**
- * struct lmb_region - Description of one region.
- *
- * @base:	Base address of the region.
- * @size:	Size of the region
- * @flags:	memory region attributes
+ * struct lmb_region - Description of one region
+ * @base: Base address of the region
+ * @size: Size of the region
+ * @flags: Memory region attributes
  */
 struct lmb_region {
 	phys_addr_t base;
@@ -46,10 +46,9 @@ struct lmb_region {
 
 /**
  * struct lmb - The LMB structure
- *
- * @free_mem:	List of free memory regions
- * @used_mem:	List of used/reserved memory regions
- * @test:	Is structure being used for LMB tests
+ * @free_mem: List of free memory regions
+ * @used_mem: List of used/reserved memory regions
+ * @test: Is structure being used for LMB tests
  */
 struct lmb {
 	struct alist free_mem;
@@ -58,51 +57,77 @@ struct lmb {
 };
 
 /**
- * lmb_init() - Initialise the LMB module
+ * lmb_init() - Initialise the LMB module.
+ *
+ * Return: 0 on success, negative error code on failure.
  *
  * Initialise the LMB lists needed for keeping the memory map. There
- * are two lists, in form of alloced list data structure. One for the
+ * are two lists, in form of allocated list data structure. One for the
  * available memory, and one for the used memory. Initialise the two
  * lists as part of board init. Add memory to the available memory
  * list and reserve common areas by adding them to the used memory
  * list.
- *
- * Return: 0 on success, -ve on error
  */
 int lmb_init(void);
 
 /**
- * lmb_add_memory() - Add memory range for LMB allocations
+ * lmb_add_memory() - Add memory range for LMB allocations.
  *
  * Add the entire available memory range to the pool of memory that
  * can be used by the LMB module for allocations.
- *
- * Return: None
  */
 void lmb_add_memory(void);
 
 long lmb_add(phys_addr_t base, phys_size_t size);
-long lmb_reserve(phys_addr_t base, phys_size_t size);
+
 /**
- * lmb_reserve_flags - Reserve one region with a specific flags bitfield.
+ * lmb_reserve() - Reserve a memory region (with no special flags)
+ * @base: Base address of the memory region
+ * @size: Size of the memory region
  *
- * @base:	base address of the memory region
- * @size:	size of the memory region
- * @flags:	flags for the memory region
- * Return:	0 if OK, > 0 for coalesced region or a negative error code.
+ * Return: 0 on success, negative error code on failure.
+ */
+long lmb_reserve(phys_addr_t base, phys_size_t size);
+
+/**
+ * lmb_reserve_flags() - Reserve one region with a specific flags bitfield
+ * @base: Base address of the memory region
+ * @size: Size of the memory region
+ * @flags: Flags for the memory region
+ *
+ * Return:
+ * * %0		- Added successfully, or it's already added (only if LMB_NONE)
+ * * %-EEXIST	- The region is already added, and flags != LMB_NONE
+ * * %-1	- Failure
  */
 long lmb_reserve_flags(phys_addr_t base, phys_size_t size,
 		       enum lmb_flags flags);
+
 phys_addr_t lmb_alloc(phys_size_t size, ulong align);
 phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr);
 phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size);
 phys_size_t lmb_get_free_size(phys_addr_t addr);
 
+/**
+ * lmb_alloc_base_flags() - Allocate specified memory region with specified
+ *			    attributes
+ * @size: Size of the region requested
+ * @align: Alignment of the memory region requested
+ * @max_addr: Maximum address of the requested region
+ * @flags: Memory region attributes to be set
+ *
+ * Allocate a region of memory with the attributes specified through the
+ * parameter. The max_addr parameter is used to specify the maximum address
+ * below which the requested region should be allocated.
+ *
+ * Return: Base address on success, 0 on error.
+ */
 phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
 				 phys_addr_t max_addr, uint flags);
 
 /**
- * lmb_alloc_addr_flags() - Allocate specified memory address with specified attributes
+ * lmb_alloc_addr_flags() - Allocate specified memory address with specified
+ *			    attributes
  * @base: Base Address requested
  * @size: Size of the region requested
  * @flags: Memory region attributes to be set
@@ -111,20 +136,21 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
  * parameter. The base parameter is used to specify the base address
  * of the requested region.
  *
- * Return: base address on success, 0 on error
+ * Return: Base address on success, 0 on error.
  */
 phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size,
 				 uint flags);
 
 /**
- * lmb_is_reserved_flags() - test if address is in reserved region with flag bits set
+ * lmb_is_reserved_flags() - Test if address is in reserved region with flag
+ *			     bits set
+ * @addr: Address to be tested
+ * @flags: Bitmap with bits to be tested
  *
  * The function checks if a reserved region comprising @addr exists which has
  * all flag bits set which are set in @flags.
  *
- * @addr:	address to be tested
- * @flags:	bitmap with bits to be tested
- * Return:	1 if matching reservation exists, 0 otherwise
+ * Return: 1 if matching reservation exists, 0 otherwise.
  */
 int lmb_is_reserved_flags(phys_addr_t addr, int flags);
 
@@ -134,9 +160,7 @@ int lmb_is_reserved_flags(phys_addr_t addr, int flags);
  * @size: Size of the region to be freed
  * @flags: Memory region attributes
  *
- * Free up a region of memory.
- *
- * Return: 0 if successful, -1 on failure
+ * Return: 0 on success, negative error code on failure.
  */
 long lmb_free_flags(phys_addr_t base, phys_size_t size, uint flags);
 
@@ -160,7 +184,7 @@ static inline int lmb_read_check(phys_addr_t addr, phys_size_t len)
  * io_lmb_setup() - Initialize LMB struct
  * @io_lmb: IO LMB to initialize
  *
- * Returns: 0 on success, negative error code on failure
+ * Return: 0 on success, negative error code on failure.
  */
 int io_lmb_setup(struct lmb *io_lmb);
 
@@ -178,12 +202,13 @@ void io_lmb_teardown(struct lmb *io_lmb);
  *
  * Add the IOVA space [base, base + size] to be managed by io_lmb.
  *
- * Returns: 0 if the region addition was successful, -1 on failure
+ * Return: 0 on success, negative error code on failure.
  */
 long io_lmb_add(struct lmb *io_lmb, phys_addr_t base, phys_size_t size);
 
 /**
- * io_lmb_alloc() - Allocate specified IO memory address with specified alignment
+ * io_lmb_alloc() - Allocate specified IO memory address with specified
+ *		    alignment
  * @io_lmb: LMB to alloc from
  * @size: Size of the region requested
  * @align: Required address and size alignment
@@ -191,7 +216,7 @@ long io_lmb_add(struct lmb *io_lmb, phys_addr_t base, phys_size_t size);
  * Allocate a region of IO memory. The base parameter is used to specify the
  * base address of the requested region.
  *
- * Return: base IO address on success, 0 on error
+ * Return: Base IO address on success, 0 on error.
  */
 phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align);
 
@@ -201,9 +226,7 @@ phys_addr_t io_lmb_alloc(struct lmb *io_lmb, phys_size_t size, ulong align);
  * @base: Base Address of region to be freed
  * @size: Size of the region to be freed
  *
- * Free up a region of IOVA space.
- *
- * Return: 0 if successful, -1 on failure
+ * Return: 0 on success, negative error code on failure.
  */
 long io_lmb_free(struct lmb *io_lmb, phys_addr_t base, phys_size_t size);
 
diff --git a/lib/lmb.c b/lib/lmb.c
index 40f03151929c..f9880a8dc62b 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -601,14 +601,6 @@ static __maybe_unused void lmb_reserve_common_spl(void)
 	}
 }
 
-/**
- * lmb_add_memory() - Add memory range for LMB allocations
- *
- * Add the entire available memory range to the pool of memory that
- * can be used by the LMB module for allocations.
- *
- * Return: None
- */
 void lmb_add_memory(void)
 {
 	int i;
@@ -665,16 +657,6 @@ long lmb_add(phys_addr_t base, phys_size_t size)
 	return lmb_map_update_notify(base, size, MAP_OP_ADD, LMB_NONE);
 }
 
-/**
- * lmb_free_flags() - Free up a region of memory
- * @base: Base Address of region to be freed
- * @size: Size of the region to be freed
- * @flags: Memory region attributes
- *
- * Free up a region of memory.
- *
- * Return: 0 if successful, negative error code on failure
- */
 long lmb_free_flags(phys_addr_t base, phys_size_t size,
 		    uint flags)
 {
@@ -782,19 +764,6 @@ phys_addr_t lmb_alloc_base(phys_size_t size, ulong align, phys_addr_t max_addr)
 	return alloc;
 }
 
-/**
- * lmb_alloc_base_flags() - Allocate specified memory region with specified attributes
- * @size: Size of the region requested
- * @align: Alignment of the memory region requested
- * @max_addr: Maximum address of the requested region
- * @flags: Memory region attributes to be set
- *
- * Allocate a region of memory with the attributes specified through the
- * parameter. The max_addr parameter is used to specify the maximum address
- * below which the requested region should be allocated.
- *
- * Return: base address on success, 0 on error
- */
 phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align,
 				 phys_addr_t max_addr, uint flags)
 {
@@ -843,18 +812,6 @@ phys_addr_t lmb_alloc_addr(phys_addr_t base, phys_size_t size)
 	return _lmb_alloc_addr(base, size, LMB_NONE);
 }
 
-/**
- * lmb_alloc_addr_flags() - Allocate specified memory address with specified attributes
- * @base: Base Address requested
- * @size: Size of the region requested
- * @flags: Memory region attributes to be set
- *
- * Allocate a region of memory with the attributes specified through the
- * parameter. The base parameter is used to specify the base address
- * of the requested region.
- *
- * Return: base address on success, 0 on error
- */
 phys_addr_t lmb_alloc_addr_flags(phys_addr_t base, phys_size_t size,
 				 uint flags)
 {
@@ -927,18 +884,6 @@ static int lmb_setup(bool test)
 	return 0;
 }
 
-/**
- * lmb_init() - Initialise the LMB module
- *
- * Initialise the LMB lists needed for keeping the memory map. There
- * are two lists, in form of alloced list data structure. One for the
- * available memory, and one for the used memory. Initialise the two
- * lists as part of board init. Add memory to the available memory
- * list and reserve common areas by adding them to the used memory
- * list.
- *
- * Return: 0 on success, -ve on error
- */
 int lmb_init(void)
 {
 	int ret;
-- 
2.39.5

Re: [PATCH v2 0/4] lmb: Improve style

[Tom Rini]

On Tue, 10 Dec 2024 20:25:46 -0600, Sam Protsenko wrote:

> This series is intended to be picked up for -next.
> 
> Cleanup the LMB code a bit, after fixing the false positive error
> messages. No functional change. This series depends on [1] (which is
> "lmb: Fix reserving the same region multiple times").
> 
> Changes in v2:
>   - The original series was split by actual functional change patches
>     [1] and cleanup patches (this series)
>   - Collected review tags
> 
> [...]

Applied to u-boot/next, thanks!

-- 
Tom