Re: [PATCH v3 07/21] s390/zcrypt: Rework zcrypt function zcrypt_device_status_mask_ext

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

On 2025-04-09 10:56, Holger Dengler wrote:
> On 27/03/2025 15:39, Harald Freudenberger wrote:
>> Rework the existing function zcrypt_device_status_mask_ext():
>> Add two new parameters to provide upper limits for
>> cards and queues. The existing implementation needed an
>> array of 256 * 256 * 4 = 256 KB which is really huge. The
>> reworked function is more flexible in the sense that the
>> caller can decide the upper limit for cards and domains to
>> be stored into the status array. So for example a caller may
>> decide to only query for cards 0...127 and queues 0...127
>> and thus only an array of size 128 * 128 * 4 = 64 KB is needed.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> 
> Looks good now. With the minor change mentioned below
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
> 
>> ---
>>  drivers/s390/crypto/zcrypt_api.c      | 14 +++++++++++---
>>  drivers/s390/crypto/zcrypt_api.h      |  3 ++-
>>  drivers/s390/crypto/zcrypt_ccamisc.c  | 17 ++++++++++++-----
>>  drivers/s390/crypto/zcrypt_ep11misc.c | 12 ++++++++----
>>  4 files changed, 33 insertions(+), 13 deletions(-)
>> 
> [...]
>> diff --git a/drivers/s390/crypto/zcrypt_ep11misc.c 
>> b/drivers/s390/crypto/zcrypt_ep11misc.c
>> index 12efcf1b54fc..504064c27a8b 100644
>> --- a/drivers/s390/crypto/zcrypt_ep11misc.c
>> +++ b/drivers/s390/crypto/zcrypt_ep11misc.c
>> @@ -1599,7 +1599,7 @@ int ep11_findcard2(u32 **apqns, u32 *nr_apqns, 
>> u16 cardnr, u16 domain,
>>  {
>>  	struct zcrypt_device_status_ext *device_status;
>>  	u32 *_apqns = NULL, _nr_apqns = 0;
>> -	int i, card, dom, rc = -ENOMEM;
>> +	int i, card, dom, rc;
> 
> If you initialize rc with -ENOMEM here ....
> 

You are totally right. However, I will not take this
as the following patches anyway rework this code and
the end result is that there is no alloc done within
this function and the rc variable is not needed any more.

>>  	struct ep11_domain_info edi;
>>  	struct ep11_card_info eci;
>> 
>> @@ -1609,13 +1609,16 @@ int ep11_findcard2(u32 **apqns, u32 *nr_apqns, 
>> u16 cardnr, u16 domain,
>>  				 GFP_KERNEL);
>>  	if (!device_status)
>>  		return -ENOMEM;
>> -	zcrypt_device_status_mask_ext(device_status);
>> +
>> +	zcrypt_device_status_mask_ext(device_status,
>> +				      MAX_ZDEV_CARDIDS_EXT,
>> +				      MAX_ZDEV_DOMAINS_EXT);
>> 
>>  	/* allocate 1k space for up to 256 apqns */
>>  	_apqns = kmalloc_array(256, sizeof(u32), GFP_KERNEL);
>>  	if (!_apqns) {
>> -		kvfree(device_status);
>> -		return -ENOMEM;
>> +		rc = -ENOMEM;
>> +		goto out;
> 
> ... you do not need to set it here. Just jump to out.
> 
> if (!_apqns)
> 	goto out;
> 
>>  	}
>> 
>>  	/* walk through all the crypto apqnss */
> [...]