| Message ID | 20241208105223.2821049-8-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup the LMB subsystem | expand |
Am 8. Dezember 2024 11:52:10 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: >After all the cleanups of the previous patches there's not point anymore >to define lmb_alloc_addr_flags() and _lmb_alloc_addr_flags(). > >Fix the incocistency of the flags type, which one function was >definining as uintn and the other as an enum lmb_flags and rename >_lmb_alloc_addr_flags() to lmb_alloc_addr_flags() > >Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> >--- > include/lmb.h | 2 +- > lib/lmb.c | 34 ++++++++++++++-------------------- > 2 files changed, 15 insertions(+), 21 deletions(-) > >diff --git a/include/lmb.h b/include/lmb.h >index a35a69d69f77..4f3c861b15a6 100644 >--- a/include/lmb.h >+++ b/include/lmb.h >@@ -111,7 +111,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, > * 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); >+ enum lmb_flags flags); This change implies that flags IS NOT a bitfield. Flags may only hold a SINGLE value from the enum. This looks wrong looking at the definition of the constants via BIT(). Defining the constants of a bitfield in an enum was wrong (or at best misleading) from the beginning. Best regards Heinrich > > /** > * lmb_is_reserved_flags() - test if address is in reserved region with flag bits set >diff --git a/lib/lmb.c b/lib/lmb.c >index c09f1a1277ad..67bde60b74be 100644 >--- a/lib/lmb.c >+++ b/lib/lmb.c >@@ -777,8 +777,20 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, > return alloc; > } > >-static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, >- enum lmb_flags flags) >+/** >+ * 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, >+ enum lmb_flags flags) > { > long rgn; > struct lmb_region *lmb_memory = lmb.available_mem.data; >@@ -802,24 +814,6 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, > return 0; > } > >-/** >- * 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) >-{ >- return _lmb_alloc_addr(base, size, flags); >-} >- > /* Return number of bytes from a given address that are free */ > phys_size_t lmb_get_free_size(phys_addr_t addr) > {
Hi Heinrich On Sun, 8 Dec 2024 at 14:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > Am 8. Dezember 2024 11:52:10 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > >After all the cleanups of the previous patches there's not point anymore > >to define lmb_alloc_addr_flags() and _lmb_alloc_addr_flags(). > > > >Fix the incocistency of the flags type, which one function was > >definining as uintn and the other as an enum lmb_flags and rename > >_lmb_alloc_addr_flags() to lmb_alloc_addr_flags() > > > >Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > >--- > > include/lmb.h | 2 +- > > lib/lmb.c | 34 ++++++++++++++-------------------- > > 2 files changed, 15 insertions(+), 21 deletions(-) > > > >diff --git a/include/lmb.h b/include/lmb.h > >index a35a69d69f77..4f3c861b15a6 100644 > >--- a/include/lmb.h > >+++ b/include/lmb.h > >@@ -111,7 +111,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, > > * 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); > >+ enum lmb_flags flags); > > This change implies that flags IS NOT a bitfield. Flags may only hold a SINGLE value from the enum. > > This looks wrong looking at the definition of the constants via BIT(). > > Defining the constants of a bitfield in an enum was wrong (or at best misleading) from the beginning. Ah yes correct, I'll just leave it to uint and change the function prototype Thanks /Ilias > > Best regards > > Heinrich > > > > > /** > > * lmb_is_reserved_flags() - test if address is in reserved region with flag bits set > >diff --git a/lib/lmb.c b/lib/lmb.c > >index c09f1a1277ad..67bde60b74be 100644 > >--- a/lib/lmb.c > >+++ b/lib/lmb.c > >@@ -777,8 +777,20 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, > > return alloc; > > } > > > >-static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, > >- enum lmb_flags flags) > >+/** > >+ * 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, > >+ enum lmb_flags flags) > > { > > long rgn; > > struct lmb_region *lmb_memory = lmb.available_mem.data; > >@@ -802,24 +814,6 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, > > return 0; > > } > > > >-/** > >- * 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) > >-{ > >- return _lmb_alloc_addr(base, size, flags); > >-} > >- > > /* Return number of bytes from a given address that are free */ > > phys_size_t lmb_get_free_size(phys_addr_t addr) > > { >
On Sun, 8 Dec 2024 at 14:12, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Heinrich > > On Sun, 8 Dec 2024 at 14:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 8. Dezember 2024 11:52:10 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > >After all the cleanups of the previous patches there's not point anymore > > >to define lmb_alloc_addr_flags() and _lmb_alloc_addr_flags(). > > > > > >Fix the incocistency of the flags type, which one function was > > >definining as uintn and the other as an enum lmb_flags and rename > > >_lmb_alloc_addr_flags() to lmb_alloc_addr_flags() > > > > > >Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > >--- > > > include/lmb.h | 2 +- > > > lib/lmb.c | 34 ++++++++++++++-------------------- > > > 2 files changed, 15 insertions(+), 21 deletions(-) > > > > > >diff --git a/include/lmb.h b/include/lmb.h > > >index a35a69d69f77..4f3c861b15a6 100644 > > >--- a/include/lmb.h > > >+++ b/include/lmb.h > > >@@ -111,7 +111,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, > > > * 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); > > >+ enum lmb_flags flags); > > > > This change implies that flags IS NOT a bitfield. Flags may only hold a SINGLE value from the enum. > > > > This looks wrong looking at the definition of the constants via BIT(). > > > > Defining the constants of a bitfield in an enum was wrong (or at best misleading) from the beginning. > > Ah yes correct, I'll just leave it to uint and change the function prototype Hmm thinking about it again. Those flags are never used as bitfield. They are always used as discrete values. So I think we should keep it as is Thanks /Ilias > > Thanks > /Ilias > > > > Best regards > > > > Heinrich > > > > > > > > /** > > > * lmb_is_reserved_flags() - test if address is in reserved region with flag bits set > > >diff --git a/lib/lmb.c b/lib/lmb.c > > >index c09f1a1277ad..67bde60b74be 100644 > > >--- a/lib/lmb.c > > >+++ b/lib/lmb.c > > >@@ -777,8 +777,20 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, > > > return alloc; > > > } > > > > > >-static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, > > >- enum lmb_flags flags) > > >+/** > > >+ * 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, > > >+ enum lmb_flags flags) > > > { > > > long rgn; > > > struct lmb_region *lmb_memory = lmb.available_mem.data; > > >@@ -802,24 +814,6 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, > > > return 0; > > > } > > > > > >-/** > > >- * 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) > > >-{ > > >- return _lmb_alloc_addr(base, size, flags); > > >-} > > >- > > > /* Return number of bytes from a given address that are free */ > > > phys_size_t lmb_get_free_size(phys_addr_t addr) > > > { > >
On Sun, 8 Dec 2024 at 14:17, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Sun, 8 Dec 2024 at 14:12, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Heinrich > > > > On Sun, 8 Dec 2024 at 14:05, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > Am 8. Dezember 2024 11:52:10 MEZ schrieb Ilias Apalodimas <ilias.apalodimas@linaro.org>: > > > >After all the cleanups of the previous patches there's not point anymore > > > >to define lmb_alloc_addr_flags() and _lmb_alloc_addr_flags(). > > > > > > > >Fix the incocistency of the flags type, which one function was > > > >definining as uintn and the other as an enum lmb_flags and rename > > > >_lmb_alloc_addr_flags() to lmb_alloc_addr_flags() > > > > > > > >Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > >--- > > > > include/lmb.h | 2 +- > > > > lib/lmb.c | 34 ++++++++++++++-------------------- > > > > 2 files changed, 15 insertions(+), 21 deletions(-) > > > > > > > >diff --git a/include/lmb.h b/include/lmb.h > > > >index a35a69d69f77..4f3c861b15a6 100644 > > > >--- a/include/lmb.h > > > >+++ b/include/lmb.h > > > >@@ -111,7 +111,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, > > > > * 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); > > > >+ enum lmb_flags flags); > > > > > > This change implies that flags IS NOT a bitfield. Flags may only hold a SINGLE value from the enum. > > > > > > This looks wrong looking at the definition of the constants via BIT(). > > > > > > Defining the constants of a bitfield in an enum was wrong (or at best misleading) from the beginning. > > > > Ah yes correct, I'll just leave it to uint and change the function prototype > > Hmm thinking about it again. Those flags are never used as bitfield. > They are always used as discrete values. So I think we should keep it > as is Ok, the above is not correct, we do indeed them use as a bitmask. So I'll just change the type to unit Thanks /Ilias > > Thanks > /Ilias > > > > Thanks > > /Ilias > > > > > > Best regards > > > > > > Heinrich > > > > > > > > > > > /** > > > > * lmb_is_reserved_flags() - test if address is in reserved region with flag bits set > > > >diff --git a/lib/lmb.c b/lib/lmb.c > > > >index c09f1a1277ad..67bde60b74be 100644 > > > >--- a/lib/lmb.c > > > >+++ b/lib/lmb.c > > > >@@ -777,8 +777,20 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, > > > > return alloc; > > > > } > > > > > > > >-static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, > > > >- enum lmb_flags flags) > > > >+/** > > > >+ * 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, > > > >+ enum lmb_flags flags) > > > > { > > > > long rgn; > > > > struct lmb_region *lmb_memory = lmb.available_mem.data; > > > >@@ -802,24 +814,6 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, > > > > return 0; > > > > } > > > > > > > >-/** > > > >- * 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) > > > >-{ > > > >- return _lmb_alloc_addr(base, size, flags); > > > >-} > > > >- > > > > /* Return number of bytes from a given address that are free */ > > > > phys_size_t lmb_get_free_size(phys_addr_t addr) > > > > { > > >
diff --git a/include/lmb.h b/include/lmb.h index a35a69d69f77..4f3c861b15a6 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -111,7 +111,7 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, * 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); + enum lmb_flags flags); /** * lmb_is_reserved_flags() - test if address is in reserved region with flag bits set diff --git a/lib/lmb.c b/lib/lmb.c index c09f1a1277ad..67bde60b74be 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -777,8 +777,20 @@ phys_addr_t lmb_alloc_base_flags(phys_size_t size, ulong align, return alloc; } -static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, - enum lmb_flags flags) +/** + * 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, + enum lmb_flags flags) { long rgn; struct lmb_region *lmb_memory = lmb.available_mem.data; @@ -802,24 +814,6 @@ static phys_addr_t _lmb_alloc_addr(phys_addr_t base, phys_size_t size, return 0; } -/** - * 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) -{ - return _lmb_alloc_addr(base, size, flags); -} - /* Return number of bytes from a given address that are free */ phys_size_t lmb_get_free_size(phys_addr_t addr) {
After all the cleanups of the previous patches there's not point anymore to define lmb_alloc_addr_flags() and _lmb_alloc_addr_flags(). Fix the incocistency of the flags type, which one function was definining as uintn and the other as an enum lmb_flags and rename _lmb_alloc_addr_flags() to lmb_alloc_addr_flags() Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- include/lmb.h | 2 +- lib/lmb.c | 34 ++++++++++++++-------------------- 2 files changed, 15 insertions(+), 21 deletions(-)