Previously, the initialization state of the `hmm_bo_device` was tracked
in two places: a global `hmm_initialized` boolean in `hmm.c`, and a local
integer `flag` in the `hmm_bo_device` struct. This was redundant and could
lead to inconsistent state checks.

- Removes the global `hmm_initialized` variable and all checks against it.
- Replaces the `int flag` in `struct hmm_bo_device` with a strongly-typed 
 `enum hmm_bo_device_init_flag flag` (values: UNINITED = 0, INITED = 1).
- Initializes `flag` to `HMM_BO_DEVICE_UNINITED` at declaration to 
  ensure a well-defined starting state.
- Removes a redundant `hmm_init()` call inside `__hmm_alloc()` since its
  always called after hmm_init()

This change improves type safety, consistency, and readability when
handling the HMM initialization state.

Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
---
 .../staging/media/atomisp/include/hmm/hmm_bo.h   |  9 +++++++--
 drivers/staging/media/atomisp/pci/hmm/hmm.c      | 16 ++++------------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
index e09ac29ac43d..155f9d89b365 100644
--- a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
+++ b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
@@ -58,7 +58,10 @@
 #define	ISP_VM_SIZE	(0x7FFFFFFF)	/* 2G address space */
 #define	ISP_PTR_NULL	NULL
 
-#define	HMM_BO_DEVICE_INITED	0x1
+enum hmm_bo_device_init_flag {
+	HMM_BO_DEVICE_INITED	= 0x1,
+	HMM_BO_DEVICE_UNINITED	= 0x2,
+};
 
 enum hmm_bo_type {
 	HMM_BO_PRIVATE,
@@ -86,7 +89,9 @@ struct hmm_bo_device {
 
 	/* list lock is used to protect the entire_bo_list */
 	spinlock_t	list_lock;
-	int flag;
+
+	/* flag to indicate whether the bo device is inited or not */
+	enum hmm_bo_device_init_flag flag;
 
 	/* linked list for entire buffer object */
 	struct list_head entire_bo_list;
diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
index c2ee9d2ec0d5..767a3a24f8e5 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
@@ -24,9 +24,10 @@
 #include "mmu/isp_mmu.h"
 #include "mmu/sh_mmu_mrfld.h"
 
-struct hmm_bo_device bo_device;
+struct hmm_bo_device bo_device = {
+	.flag = HMM_BO_DEVICE_UNINITED,
+};
 static ia_css_ptr dummy_ptr = mmgr_EXCEPTION;
-static bool hmm_initialized;
 
 int hmm_init(void)
 {
@@ -38,8 +39,6 @@ int hmm_init(void)
 		dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
 		return ret;
 
-	hmm_initialized = true;
-
 	/*
 	 * As hmm use NULL to indicate invalid ISP virtual address,
 	 * and ISP_VM_START is defined to 0 too, so we allocate
@@ -62,7 +61,7 @@ void hmm_cleanup(void)
 	dummy_ptr = 0;
 
 	hmm_bo_device_exit(&bo_device);
-	hmm_initialized = false;
+	bo_device.flag = HMM_BO_DEVICE_UNINITED;
 }
 
 static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type,
@@ -72,13 +71,6 @@ static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type,
 	struct hmm_buffer_object *bo;
 	int ret;
 
-	/*
-	 * Check if we are initialized. In the ideal world we wouldn't need
-	 * this but we can tackle it once the driver is a lot cleaner
-	 */
-
-	if (!hmm_initialized)
-		hmm_init();
 	/* Get page number from size */
 	pgnr = size_to_pgnr_ceil(bytes);
 
-- 
2.25.1

Hi Abdelrahman,

On 7-Jul-25 16:09, Abdelrahman Fekry wrote:
> Previously, the initialization state of the `hmm_bo_device` was tracked
> in two places: a global `hmm_initialized` boolean in `hmm.c`, and a local
> integer `flag` in the `hmm_bo_device` struct. This was redundant and could
> lead to inconsistent state checks.
> 
> - Removes the global `hmm_initialized` variable and all checks against it.
> - Replaces the `int flag` in `struct hmm_bo_device` with a strongly-typed 
>  `enum hmm_bo_device_init_flag flag` (values: UNINITED = 0, INITED = 1).
> - Initializes `flag` to `HMM_BO_DEVICE_UNINITED` at declaration to 
>   ensure a well-defined starting state.
> - Removes a redundant `hmm_init()` call inside `__hmm_alloc()` since its
>   always called after hmm_init()
> 
> This change improves type safety, consistency, and readability when
> handling the HMM initialization state.
> 
> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
> ---
>  .../staging/media/atomisp/include/hmm/hmm_bo.h   |  9 +++++++--
>  drivers/staging/media/atomisp/pci/hmm/hmm.c      | 16 ++++------------
>  2 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> index e09ac29ac43d..155f9d89b365 100644
> --- a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> +++ b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> @@ -58,7 +58,10 @@
>  #define	ISP_VM_SIZE	(0x7FFFFFFF)	/* 2G address space */
>  #define	ISP_PTR_NULL	NULL
>  
> -#define	HMM_BO_DEVICE_INITED	0x1
> +enum hmm_bo_device_init_flag {
> +	HMM_BO_DEVICE_INITED	= 0x1,
> +	HMM_BO_DEVICE_UNINITED	= 0x2,
> +};
>  
>  enum hmm_bo_type {
>  	HMM_BO_PRIVATE,
> @@ -86,7 +89,9 @@ struct hmm_bo_device {
>  
>  	/* list lock is used to protect the entire_bo_list */
>  	spinlock_t	list_lock;
> -	int flag;
> +
> +	/* flag to indicate whether the bo device is inited or not */
> +	enum hmm_bo_device_init_flag flag;

Please just replace this with a "bool initialized"; data
member taking `true` and `false as values instead of
introducing a new type for this.

>  
>  	/* linked list for entire buffer object */
>  	struct list_head entire_bo_list;
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> index c2ee9d2ec0d5..767a3a24f8e5 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> @@ -24,9 +24,10 @@
>  #include "mmu/isp_mmu.h"
>  #include "mmu/sh_mmu_mrfld.h"
>  
> -struct hmm_bo_device bo_device;
> +struct hmm_bo_device bo_device = {
> +	.flag = HMM_BO_DEVICE_UNINITED,
> +};
>  static ia_css_ptr dummy_ptr = mmgr_EXCEPTION;
> -static bool hmm_initialized;
>  
>  int hmm_init(void)
>  {
> @@ -38,8 +39,6 @@ int hmm_init(void)
>  		dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
>  		return ret;
>  
> -	hmm_initialized = true;
> -
>  	/*
>  	 * As hmm use NULL to indicate invalid ISP virtual address,
>  	 * and ISP_VM_START is defined to 0 too, so we allocate
> @@ -62,7 +61,7 @@ void hmm_cleanup(void)
>  	dummy_ptr = 0;
>  
>  	hmm_bo_device_exit(&bo_device);
> -	hmm_initialized = false;
> +	bo_device.flag = HMM_BO_DEVICE_UNINITED;

This clearing of the flag / setting `initialized = false` belongs inside bo_exit()
not here.


>  }
>  
>  static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type,
> @@ -72,13 +71,6 @@ static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type,
>  	struct hmm_buffer_object *bo;
>  	int ret;
>  
> -	/*
> -	 * Check if we are initialized. In the ideal world we wouldn't need
> -	 * this but we can tackle it once the driver is a lot cleaner
> -	 */
> -
> -	if (!hmm_initialized)
> -		hmm_init();
>  	/* Get page number from size */
>  	pgnr = size_to_pgnr_ceil(bytes);
>  


Regards,

Hans

Hi Hans,
Thanks for the feedback!

On Mon, Jul 7, 2025 at 5:14 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Abdelrahman,
>
> On 7-Jul-25 16:09, Abdelrahman Fekry wrote:
> > Previously, the initialization state of the `hmm_bo_device` was tracked
> > in two places: a global `hmm_initialized` boolean in `hmm.c`, and a local
> > integer `flag` in the `hmm_bo_device` struct. This was redundant and could
> > lead to inconsistent state checks.
> >
> > - Removes the global `hmm_initialized` variable and all checks against it.
> > - Replaces the `int flag` in `struct hmm_bo_device` with a strongly-typed
> >  `enum hmm_bo_device_init_flag flag` (values: UNINITED = 0, INITED = 1).
> > - Initializes `flag` to `HMM_BO_DEVICE_UNINITED` at declaration to
> >   ensure a well-defined starting state.
> > - Removes a redundant `hmm_init()` call inside `__hmm_alloc()` since its
> >   always called after hmm_init()
> >
> > This change improves type safety, consistency, and readability when
> > handling the HMM initialization state.
> >
> > Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
> > ---
> >  .../staging/media/atomisp/include/hmm/hmm_bo.h   |  9 +++++++--
> >  drivers/staging/media/atomisp/pci/hmm/hmm.c      | 16 ++++------------
> >  2 files changed, 11 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> > index e09ac29ac43d..155f9d89b365 100644
> > --- a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> > +++ b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
> > @@ -58,7 +58,10 @@
> >  #define      ISP_VM_SIZE     (0x7FFFFFFF)    /* 2G address space */
> >  #define      ISP_PTR_NULL    NULL
> >
> > -#define      HMM_BO_DEVICE_INITED    0x1
> > +enum hmm_bo_device_init_flag {
> > +     HMM_BO_DEVICE_INITED    = 0x1,
> > +     HMM_BO_DEVICE_UNINITED  = 0x2,
> > +};
> >
> >  enum hmm_bo_type {
> >       HMM_BO_PRIVATE,
> > @@ -86,7 +89,9 @@ struct hmm_bo_device {
> >
> >       /* list lock is used to protect the entire_bo_list */
> >       spinlock_t      list_lock;
> > -     int flag;
> > +
> > +     /* flag to indicate whether the bo device is inited or not */
> > +     enum hmm_bo_device_init_flag flag;
>
> Please just replace this with a "bool initialized"; data
> member taking `true` and `false as values instead of
> introducing a new type for this.
>
Okay , will do this.

> >
> >       /* linked list for entire buffer object */
> >       struct list_head entire_bo_list;
> > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > index c2ee9d2ec0d5..767a3a24f8e5 100644
> > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > @@ -24,9 +24,10 @@
> >  #include "mmu/isp_mmu.h"
> >  #include "mmu/sh_mmu_mrfld.h"
> >
> > -struct hmm_bo_device bo_device;
> > +struct hmm_bo_device bo_device = {
> > +     .flag = HMM_BO_DEVICE_UNINITED,
> > +};
> >  static ia_css_ptr dummy_ptr = mmgr_EXCEPTION;
> > -static bool hmm_initialized;
> >
> >  int hmm_init(void)
> >  {
> > @@ -38,8 +39,6 @@ int hmm_init(void)
> >               dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
> >               return ret;
> >
> > -     hmm_initialized = true;
> > -
> >       /*
> >        * As hmm use NULL to indicate invalid ISP virtual address,
> >        * and ISP_VM_START is defined to 0 too, so we allocate
> > @@ -62,7 +61,7 @@ void hmm_cleanup(void)
> >       dummy_ptr = 0;
> >
> >       hmm_bo_device_exit(&bo_device);
> > -     hmm_initialized = false;
> > +     bo_device.flag = HMM_BO_DEVICE_UNINITED;
>
> This clearing of the flag / setting `initialized = false` belongs inside bo_exit()
> not here.
>
okay, moving it in v2
>
> >  }
> >
> >  static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type,
> > @@ -72,13 +71,6 @@ static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type,
> >       struct hmm_buffer_object *bo;
> >       int ret;
> >
> > -     /*
> > -      * Check if we are initialized. In the ideal world we wouldn't need
> > -      * this but we can tackle it once the driver is a lot cleaner
> > -      */
> > -
> > -     if (!hmm_initialized)
> > -             hmm_init();
> >       /* Get page number from size */
> >       pgnr = size_to_pgnr_ceil(bytes);
> >
>
>
> Regards,
>
> Hans
>
>

Hi,

On 7-Jul-25 4:14 PM, Hans de Goede wrote:
> Hi Abdelrahman,
> 
> On 7-Jul-25 16:09, Abdelrahman Fekry wrote:
>> Previously, the initialization state of the `hmm_bo_device` was tracked
>> in two places: a global `hmm_initialized` boolean in `hmm.c`, and a local
>> integer `flag` in the `hmm_bo_device` struct. This was redundant and could
>> lead to inconsistent state checks.
>>
>> - Removes the global `hmm_initialized` variable and all checks against it.
>> - Replaces the `int flag` in `struct hmm_bo_device` with a strongly-typed 
>>  `enum hmm_bo_device_init_flag flag` (values: UNINITED = 0, INITED = 1).
>> - Initializes `flag` to `HMM_BO_DEVICE_UNINITED` at declaration to 
>>   ensure a well-defined starting state.
>> - Removes a redundant `hmm_init()` call inside `__hmm_alloc()` since its
>>   always called after hmm_init()
>>
>> This change improves type safety, consistency, and readability when
>> handling the HMM initialization state.
>>
>> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
>> ---
>>  .../staging/media/atomisp/include/hmm/hmm_bo.h   |  9 +++++++--
>>  drivers/staging/media/atomisp/pci/hmm/hmm.c      | 16 ++++------------
>>  2 files changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
>> index e09ac29ac43d..155f9d89b365 100644
>> --- a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
>> +++ b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
>> @@ -58,7 +58,10 @@
>>  #define	ISP_VM_SIZE	(0x7FFFFFFF)	/* 2G address space */
>>  #define	ISP_PTR_NULL	NULL
>>  
>> -#define	HMM_BO_DEVICE_INITED	0x1
>> +enum hmm_bo_device_init_flag {
>> +	HMM_BO_DEVICE_INITED	= 0x1,
>> +	HMM_BO_DEVICE_UNINITED	= 0x2,
>> +};
>>  
>>  enum hmm_bo_type {
>>  	HMM_BO_PRIVATE,
>> @@ -86,7 +89,9 @@ struct hmm_bo_device {
>>  
>>  	/* list lock is used to protect the entire_bo_list */
>>  	spinlock_t	list_lock;
>> -	int flag;
>> +
>> +	/* flag to indicate whether the bo device is inited or not */
>> +	enum hmm_bo_device_init_flag flag;
> 
> Please just replace this with a "bool initialized"; data
> member taking `true` and `false as values instead of
> introducing a new type for this.
> 
>>  
>>  	/* linked list for entire buffer object */
>>  	struct list_head entire_bo_list;
>> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
>> index c2ee9d2ec0d5..767a3a24f8e5 100644
>> --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
>> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
>> @@ -24,9 +24,10 @@
>>  #include "mmu/isp_mmu.h"
>>  #include "mmu/sh_mmu_mrfld.h"
>>  
>> -struct hmm_bo_device bo_device;
>> +struct hmm_bo_device bo_device = {
>> +	.flag = HMM_BO_DEVICE_UNINITED,
>> +};
>>  static ia_css_ptr dummy_ptr = mmgr_EXCEPTION;
>> -static bool hmm_initialized;
>>  
>>  int hmm_init(void)
>>  {
>> @@ -38,8 +39,6 @@ int hmm_init(void)
>>  		dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
>>  		return ret;
>>  
>> -	hmm_initialized = true;
>> -
>>  	/*
>>  	 * As hmm use NULL to indicate invalid ISP virtual address,
>>  	 * and ISP_VM_START is defined to 0 too, so we allocate
>> @@ -62,7 +61,7 @@ void hmm_cleanup(void)
>>  	dummy_ptr = 0;
>>  
>>  	hmm_bo_device_exit(&bo_device);
>> -	hmm_initialized = false;
>> +	bo_device.flag = HMM_BO_DEVICE_UNINITED;
> 
> This clearing of the flag / setting `initialized = false` belongs inside bo_exit()
> not here.

To be clear I meant this belongs inside hmm_bo_device_exit().

Regards,

Hans