[PATCH 0/2] staging: media: atomisp: clean up and unify HMM initialization logic

[PATCH 0/2] staging: media: atomisp: clean up and unify HMM initialization logic

[Abdelrahman Fekry]

Continuing the series of cleaning up the atomisp driver as discussed in
[1], This patch series improves the consistency and reliability of the
initialization logic in the AtomISP driver's Hybrid Memory Manager (HMM)
subsystem.

These modifications were tested by building the AtomISP driver

Link: https://lore.kernel.org/all/fbfbd0e5-2c27-4f32-a3d7-9cf57fde5098@kernel.org/ [1]
Abdelrahman Fekry (2):
  staging: media: atomisp: return early on hmm_bo_device_init() failure
  staging: media: atomisp: unify initialization flag usage in HMM

 .../media/atomisp/include/hmm/hmm_bo.h        |  9 +++++++--
 drivers/staging/media/atomisp/pci/hmm/hmm.c   | 19 ++++++-------------
 2 files changed, 13 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH 1/2] staging: media: atomisp: return early on hmm_bo_device_init() failure

[Abdelrahman Fekry]

hmm_init() would continue execution even if hmm_bo_device_init() failed,
potentially leading to bad behaviour when calling hmm_alloc().

- returns the error immediately if hmm_bo_device_init() fails.

Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
---
 drivers/staging/media/atomisp/pci/hmm/hmm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
index f998b57f90c4..c2ee9d2ec0d5 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
@@ -36,6 +36,7 @@ int hmm_init(void)
 				 ISP_VM_START, ISP_VM_SIZE);
 	if (ret)
 		dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
+		return ret;
 
 	hmm_initialized = true;
 
@@ -48,7 +49,7 @@ int hmm_init(void)
 	 */
 	dummy_ptr = hmm_alloc(1);
 
-	return ret;
+	return 0;
 }
 
 void hmm_cleanup(void)
-- 
2.25.1

[PATCH 2/2] staging: media: atomisp: unify initialization flag usage in HMM

[Abdelrahman Fekry]

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

Re: [PATCH 1/2] staging: media: atomisp: return early on hmm_bo_device_init() failure

[Hans de Goede]

Hi Abdelrahman,

On 7-Jul-25 16:09, Abdelrahman Fekry wrote:
> hmm_init() would continue execution even if hmm_bo_device_init() failed,
> potentially leading to bad behaviour when calling hmm_alloc().
> 
> - returns the error immediately if hmm_bo_device_init() fails.
> 
> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
> ---
>  drivers/staging/media/atomisp/pci/hmm/hmm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> index f998b57f90c4..c2ee9d2ec0d5 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> @@ -36,6 +36,7 @@ int hmm_init(void)
>  				 ISP_VM_START, ISP_VM_SIZE);
>  	if (ret)
>  		dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
> +		return ret;

You need to add { } here otherwise the "return ret;" will
always get executed since it is not part of the code block
guarded by the if (despite the indentation).

Regards,

Hans



>  
>  	hmm_initialized = true;
>  
> @@ -48,7 +49,7 @@ int hmm_init(void)
>  	 */
>  	dummy_ptr = hmm_alloc(1);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  void hmm_cleanup(void)

Re: [PATCH 2/2] staging: media: atomisp: unify initialization flag usage in HMM

[Hans de Goede]

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

Re: [PATCH 1/2] staging: media: atomisp: return early on hmm_bo_device_init() failure

[Abdelrahman Fekry]

Hi Hans.
On Mon, Jul 7, 2025 at 5:12 PM Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Abdelrahman,
>
> On 7-Jul-25 16:09, Abdelrahman Fekry wrote:
> > hmm_init() would continue execution even if hmm_bo_device_init() failed,
> > potentially leading to bad behaviour when calling hmm_alloc().
> >
> > - returns the error immediately if hmm_bo_device_init() fails.
> >
> > Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
> > ---
> >  drivers/staging/media/atomisp/pci/hmm/hmm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > index f998b57f90c4..c2ee9d2ec0d5 100644
> > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > @@ -36,6 +36,7 @@ int hmm_init(void)
> >                                ISP_VM_START, ISP_VM_SIZE);
> >       if (ret)
> >               dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
> > +             return ret;
>
> You need to add { } here otherwise the "return ret;" will
> always get executed since it is not part of the code block
> guarded by the if (despite the indentation).
>
Yes , sorry for this dumb mistake. I will send v2.

> Regards,
>
> Hans
>
>
>
> >
> >       hmm_initialized = true;
> >
> > @@ -48,7 +49,7 @@ int hmm_init(void)
> >        */
> >       dummy_ptr = hmm_alloc(1);
> >
> > -     return ret;
> > +     return 0;
> >  }
> >
> >  void hmm_cleanup(void)
>

Re: [PATCH 2/2] staging: media: atomisp: unify initialization flag usage in HMM

[Hans de Goede]

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

Re: [PATCH 2/2] staging: media: atomisp: unify initialization flag usage in HMM

[Abdelrahman Fekry]

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

Re: [PATCH 1/2] staging: media: atomisp: return early on hmm_bo_device_init() failure

[Dan Carpenter]

On Mon, Jul 07, 2025 at 05:15:20PM +0300, Abdelrahman Fekry wrote:
> Hi Hans.
> On Mon, Jul 7, 2025 at 5:12 PM Hans de Goede <hansg@kernel.org> wrote:
> >
> > Hi Abdelrahman,
> >
> > On 7-Jul-25 16:09, Abdelrahman Fekry wrote:
> > > hmm_init() would continue execution even if hmm_bo_device_init() failed,
> > > potentially leading to bad behaviour when calling hmm_alloc().
> > >
> > > - returns the error immediately if hmm_bo_device_init() fails.
> > >
> > > Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@gmail.com>
> > > ---
> > >  drivers/staging/media/atomisp/pci/hmm/hmm.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > > index f998b57f90c4..c2ee9d2ec0d5 100644
> > > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > > @@ -36,6 +36,7 @@ int hmm_init(void)
> > >                                ISP_VM_START, ISP_VM_SIZE);
> > >       if (ret)
> > >               dev_err(atomisp_dev, "hmm_bo_device_init failed.\n");
> > > +             return ret;
> >
> > You need to add { } here otherwise the "return ret;" will
> > always get executed since it is not part of the code block
> > guarded by the if (despite the indentation).
> >
> Yes , sorry for this dumb mistake. I will send v2.
> 

Smatch has a check for this.

~/smatch/smatch_scripts/kchecker drivers/staging/media/atomisp/pci/hmm/hmm.c

regards,
dan carpenter

Re: [PATCH 1/2] staging: media: atomisp: return early on hmm_bo_device_init() failure

[Abdelrahman Fekry]

Hi Dan,
On Mon, Jul 14, 2025 at 10:13 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Smatch has a check for this.
>
> ~/smatch/smatch_scripts/kchecker drivers/staging/media/atomisp/pci/hmm/hmm.c
>
I have sent a new patch series that this was fixed in and added other patches
here is the link :
https://lore.kernel.org/all/20250712191325.132666-1-abdelrahmanfekry375@gmail.com/
> regards,
> dan carpenter
>