Re: [PATCH v2 07/20] s390/zcrypt: Rework zcrypt function zcrypt_device_status_mask_ext

Harald Freudenberger
2025-03-25T09:24:32Z

On 2025-03-19 12:03, Holger Dengler wrote:
> On 04/03/2025 18:21, 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.
>> * Instead of void the reworked function now returns an int.
>> The currently only way to have the function return != 0
>> is by providing card or domains limits beyond 256.
>
> I would prefer to stay with a void function and limit the card and
> domain values to the current maximum. Details below.
>
>>
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> ---
>> drivers/s390/crypto/zcrypt_api.c | 20 +++++++++++++++-----
>> drivers/s390/crypto/zcrypt_api.h | 3 ++-
>> drivers/s390/crypto/zcrypt_ccamisc.c | 22 +++++++++++++++++-----
>> drivers/s390/crypto/zcrypt_ep11misc.c | 14 ++++++++++----
>> 4 files changed, 44 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/zcrypt_api.c
>> b/drivers/s390/crypto/zcrypt_api.c
>> index 62cc05881b13..bd2738e3792a 100644
>> --- a/drivers/s390/crypto/zcrypt_api.c
>> +++ b/drivers/s390/crypto/zcrypt_api.c
>> @@ -1317,19 +1317,25 @@ static void zcrypt_device_status_mask(struct
>> zcrypt_device_status *devstatus)
>> spin_unlock(&zcrypt_list_lock);
>> }
>>
>> -void zcrypt_device_status_mask_ext(struct zcrypt_device_status_ext
>> *devstatus)
>> +int zcrypt_device_status_mask_ext(struct zcrypt_device_status_ext
>> *devstatus,
>> + int maxcard, int maxqueue)
>
> Keep void and ...
>
>> {
>> struct zcrypt_card *zc;
>> struct zcrypt_queue *zq;
>> struct zcrypt_device_status_ext *stat;
>> int card, queue;
>>
>> + if (maxcard > MAX_ZDEV_CARDIDS_EXT || maxqueue >
>> MAX_ZDEV_DOMAINS_EXT)
>> + return -EINVAL;
>> +
>
> ... limit maxcard/maxqueue to the maximum supported values. In my
> opinion, it does not make any sense to call this function with higher
> values than the maximum.
>
> maxcard = MIN(maxcard, MAX_ZDEV_CARDIDS_EXT);
> maxqueue = MIN(maxqueue, MAX_ZDEV_DOMAINS_EXT);
>
> As a side effect, it keeps the caller code much simpler.
>
>> spin_lock(&zcrypt_list_lock);
>> for_each_zcrypt_card(zc) {
>> for_each_zcrypt_queue(zq, zc) {
>> card = AP_QID_CARD(zq->queue->qid);
>> queue = AP_QID_QUEUE(zq->queue->qid);
>> - stat = &devstatus[card * AP_DOMAINS + queue];
>> + if (card >= maxcard || queue >= maxqueue)
>> + continue;
>> + stat = &devstatus[card * maxqueue + queue];
>> stat->hwtype = zc->card->ap_dev.device_type;
>> stat->functions = zc->card->hwinfo.fac >> 26;
>> stat->qid = zq->queue->qid;
> [...]
>> @@ -1635,9 +1643,11 @@ static long zcrypt_unlocked_ioctl(struct file
>> *filp, unsigned int cmd,
>> GFP_KERNEL);
>> if (!device_status)
>> return -ENOMEM;
>> - zcrypt_device_status_mask_ext(device_status);
>> - if (copy_to_user((char __user *)arg, device_status,
>> - total_size))
>> + rc = zcrypt_device_status_mask_ext(device_status,
>> + MAX_ZDEV_CARDIDS_EXT,
>> + MAX_ZDEV_DOMAINS_EXT);
>> + if (!rc && copy_to_user((char __user *)arg, device_status,
>> + total_size))
>
> With the change above, you can stay with the current error handling.
> Only the addition parameters for zcrypt_device_status_mask_ext() need
> to be added.
>
>> rc = -EFAULT;
>> kvfree(device_status);
>> return rc;
> [...]

Done as suggested - only I used typed min (min_t()) instead of MIN.
-> v3