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

Harald Freudenberger
2025-03-25T10:51:48Z

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);
>> }