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

Hans de Goede
2025-07-07T14:14:50Z

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