Re: [PATCH v4 2/5] firmware: stratix10-svc: Implement ID pool management for asynchronous operations

[Mahesh Rao <mahesh.rao@altera.com>]

Hi Mathew,

On 10-06-2025 09:51 pm, Matthew Gerlach wrote:
> 
> On 6/10/25 8:37 AM, Mahesh Rao via B4 Relay wrote:
>> From: Mahesh Rao <mahesh.rao@altera.com>
>>
>> Implement ID pool management API's which will be
>> used for Stratix10 Asynchronous communication with
>> Secure Device Manager. These API's will be used
>> in subsequent patches for ID management in
>> asynchronous operations.
>>
>> Signed-off-by: Mahesh Rao <mahesh.rao@altera.com>
>> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
>> ---
>>   drivers/firmware/stratix10-svc.c | 195 +++++++++++++++++++++++++++++ 
>> ++++++++++
>>   1 file changed, 195 insertions(+)
>>
>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/ 
>> stratix10-svc.c
>> index 
>> 955468555738b2031dfcf6dc4db7dbf11ccc482c..6d21f0301c3457c1b1bed52e39ee03d14294943d 100644
>> --- a/drivers/firmware/stratix10-svc.c
>> +++ b/drivers/firmware/stratix10-svc.c
>> @@ -170,6 +170,21 @@ struct stratix10_svc_chan {
>>       spinlock_t lock;
>>   };
>> +/**
>> + * struct stratix10_sip_id_pool - Structure representing a pool of 
>> IDs for
>> + *                                asynchronous operations.
>> + * @head:         The head index of the ID pool.
>> + * @size:         The total size of the ID pool.
>> + * @id_mask:      Pointer to an array representing the mask of 
>> allocated IDs.
>> + * @lock:         Mutex lock to protect access to the ID pool.
>> + */
>> +struct stratix10_sip_id_pool {
>> +    unsigned long head;
>> +    unsigned long size;
>> +    unsigned long *id_mask;
>> +    struct mutex lock;
>> +};
>> +
>>   static LIST_HEAD(svc_ctrl);
>>   static LIST_HEAD(svc_data_mem);
>>   /* svc_mem_lock protects access to the svc_data_mem list for
>> @@ -177,6 +192,186 @@ static LIST_HEAD(svc_data_mem);
>>    */
>>   static DEFINE_MUTEX(svc_mem_lock);
>> +/**
>> + * stratix10_id_pool_create - Create a new ID pool for Stratix10
>> + * async operation
>> + * @size: The size of the ID pool to create
>> + *
>> + * This function allocates and initializes a new ID pool structure
>> + * for Stratix10 async operations. It allocates memory for the ID
>> + * pool structure and the associated bitmaps for ID management.
>> + *
>> + * Return: Pointer to the newly created ID pool structure, or NULL
>> + * on failure.
>> + */
>> +static struct stratix10_sip_id_pool 
>> *stratix10_id_pool_create(unsigned long size)
>> +{
>> +    struct stratix10_sip_id_pool *id_pool = NULL;
>> +
>> +    if (size == 0)
>> +        return NULL;
>> +
>> +    id_pool = kzalloc(sizeof(*id_pool), GFP_KERNEL);
>> +    if (!id_pool)
>> +        return NULL;
>> +
>> +    id_pool->size = size;
>> +
>> +    id_pool->id_mask = bitmap_zalloc(size, GFP_KERNEL);
>> +    if (!id_pool->id_mask) {
>> +        kfree(id_pool);
>> +        return NULL;
>> +    }
>> +
>> +    id_pool->head = 0;
> The above is not necessary because you used kzalloc() above.

Ok will remove it.
>> +
>> +    mutex_init(&id_pool->lock);
>> +
>> +    return id_pool;
>> +}
>> +
>> +/**
>> + * stratix10_id_pool_destroy - Destroy an ID pool for Stratix10 async 
>> operation
>> + * @id_pool: Pointer to the ID pool structure
>> + *
>> + * This function destroys an ID pool for Stratix10 async operations. 
>> It first
>> + * checks if the ID pool is valid, then frees the associated bitmap 


>> +/**
>> + * stratix10_deallocate_id - Deallocate an ID in the ID pool
>> + * @id_pool: Pointer to the ID pool structure
>> + * @id: The ID to be deallocated
>> + *
>> + * This function deallocates an ID in the given ID pool. It first
>> + * checks if the ID pool is valid and if the ID is within the valid
>> + * range.
>> + *
>> + * Return:
>> + * 0 on success,
>> + * -EINVAL if the ID pool is invalid, the ID is out of range, or the
>> + * ID is not set.
>> + */
>> +static int stratix10_deallocate_id(struct stratix10_sip_id_pool 
>> *id_pool, unsigned long id)
>> +{
>> +    if (!id_pool)
>> +        return -EINVAL;
>> +
>> +    if (id >= id_pool->size)
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&id_pool->lock);
>> +    if (!test_bit(id, id_pool->id_mask)) {
>> +        mutex_unlock(&id_pool->lock);
>> +        return -EINVAL;
>> +    }
>> +    clear_bit(id, id_pool->id_mask);
>> +    mutex_unlock(&id_pool->lock);
>> +
>> +    return 0;
> 
> Inverting the if statement would allow for a single exit path:
> 
>      int ret = -EINVAL;
> 
> ...
> 
>      if (test_bit(id, id_pool->id_mask)) {
> 
>          clear_bit(id, id_pool->id_mask);
> 
>          ret = 0;
> 
>      }
> 
>      mutex_unlock(&id_pool->lock);
> 
>      return ret;
> 

Will add the change.

>> +}
>> +
>>   /**
>>    * svc_pa_to_va() - translate physical address to virtual address
>>    * @addr: to be translated physical address
>>