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

[Hans de Goede <hansg@kernel.org>]

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