Re: [PATCH v2 08/20] s390/zcrypt: Introduce pre-allocated device status array for cca misc

[Harald Freudenberger <freude@linux.ibm.com>]

On 2025-03-19 15:31, Holger Dengler wrote:
> On 04/03/2025 18:21, Harald Freudenberger wrote:
>> Introduce a pre-allocated device status array memory together with
>> a mutex controlling the occupation to be used by the two findcard()
>> functions. Limit the device status array to max 128 cards and max
> 
> As far as I can see, cca_findcard() (and also the static findcard())
> are no longer used. If this is the case, I would prefer to remove this
> unused variant, either in this patch (with a hint in the commit
> message) or in a separate one (in the order prior to this patch).
> 
>> 128 domains to reduce the size of this pre-allocated memory to 64 KB.
> 
> I'm a bit unhappy about introducing a synchronization with this single
> piece of memory.
> 

Any other ideas ?

>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> ---
>>  drivers/s390/crypto/zcrypt_ccamisc.c | 78 
>> +++++++++++++++++++---------
>>  1 file changed, 53 insertions(+), 25 deletions(-)
>> 
>> diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c 
>> b/drivers/s390/crypto/zcrypt_ccamisc.c
>> index 420895df70f0..65b4cdb9b478 100644
>> --- a/drivers/s390/crypto/zcrypt_ccamisc.c
>> +++ b/drivers/s390/crypto/zcrypt_ccamisc.c
> [...]
>> @@ -1801,21 +1817,20 @@ static int findcard(u64 mkvp, u16 *pcardnr, 
>> u16 *pdomain,
> 
> As mentioned above, please check for users of findcard() and
> cca_findcard(). If there are no users anymore, please remove the
> obsolete code.
> 

Seems you are right - findcard() and cca_findcard() are no longer used.
So I'll introduce another patch before this one which removes the unused
functions. -> v3

> [...]
>> @@ -2023,11 +2041,21 @@ int __init zcrypt_ccamisc_init(void)
>>  		cprb_mempool = NULL;
>>  		return -ENOMEM;
>>  	}
>> +
>> +	/* Pre-allocate one crypto status card struct used in findcard() */
>> +	dev_status_mem = kvmalloc(ZCRYPT_DEV_STATUS_EXT_SIZE, GFP_KERNEL);
> 
> The alloc of dev_status_mem should be protected by the mutex as well.
> 

No, init() is supposed to be executed exactly once and there is nothing
running which may somehow occupy this memory already.

>> +	if (!dev_status_mem) {
>> +		ZCRYPT_DBF_ERR("%s allocation of dev_status_mem failed\n", 
>> __func__);
>> +		mempool_destroy(cprb_mempool);
>> +		return -ENOMEM;
>> +	}
>> +
>>  	return 0;
>>  }
>> 
>>  void zcrypt_ccamisc_exit(void)
>>  {
>>  	mkvp_cache_free();
>> +	kvfree(dev_status_mem);
> 
> Same here: please lock the mutex before releasing dev_status_mem.
> 

Agreed - the free call should be delayed in cases where the memory
is currently occupied by a caller. So I'll lock the mutex before
calling free() -> v3

>>  	mempool_destroy(cprb_mempool);
>>  }