Re: [PATCH v1 07/20] s390/zcrypt: New zcrypt function zcrypt_device_status_mask_ext2

[Holger Dengler <dengler@linux.ibm.com>]

On 23/02/2025 10:54, Harald Freudenberger wrote:
> Introduce a new function zcrypt_device_status_mask_ext2().
> This function gives the possibility to provide upper limits
> for cards and queues. The current implementation of
> zcrypt_device_status_mask_ext() needs an array of
> 256 * 256 * 4 = 256 KB which is huge. The new 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>
> ---
>  drivers/s390/crypto/zcrypt_api.c | 26 ++++++++++++++++++++++++++
>  drivers/s390/crypto/zcrypt_api.h |  2 ++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
> index 3a521e65f3fe..fd9666ae1d26 100644
> --- a/drivers/s390/crypto/zcrypt_api.c
> +++ b/drivers/s390/crypto/zcrypt_api.c
> @@ -1330,6 +1330,32 @@ void zcrypt_device_status_mask_ext(struct zcrypt_device_status_ext *devstatus)
>  }
>  EXPORT_SYMBOL(zcrypt_device_status_mask_ext);
>  
> +void zcrypt_device_status_mask_ext2(struct zcrypt_device_status_ext *devstatus,
> +				    int maxcard, int maxqueue)
> +{
> +	struct zcrypt_card *zc;
> +	struct zcrypt_queue *zq;
> +	struct zcrypt_device_status_ext *stat;
> +	int card, queue;
> +
> +	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);
> +			if (card >= maxcard || queue >= maxqueue)
> +				continue;

What about doing a "break" here? Or are the the cards/queues iterated non-linear?

> +			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;
> +			stat->online = zq->online ? 0x01 : 0x00;
> +		}
> +	}
> +	spin_unlock(&zcrypt_list_lock);
> +}
> +EXPORT_SYMBOL(zcrypt_device_status_mask_ext2);
> +

In my opinion, this is a lot of code duplication for an early loop termination. Either extent the zcrypt_device_status_mask_ext() function with the two additional parameters and change all callers to

zcrypt_device_status_mask_ext(... , MAX_ZDEV_CARDIDS, MAX_ZDEV_DOMAINS);

or just implement zcrypt_device_status_mask_ext() as a wrapper to zcrypt_device_status_mask_ext2().

void zcrypt_device_status_mask_ext(struct zcrypt_device_status_ext *devstatus)
{
	zcrypt_device_status_ext2(devstatus, MAX_ZDEV_CARDIDS, MAX_ZDEV_DOMAINS);
}
EXPORT_SYMBOL(zcrypt_device_status_mask_ext);

>  int zcrypt_device_status_ext(int card, int queue,
>  			     struct zcrypt_device_status_ext *devstat)
>  {
> diff --git a/drivers/s390/crypto/zcrypt_api.h b/drivers/s390/crypto/zcrypt_api.h
> index 06ff697d171c..92027304f0d8 100644
> --- a/drivers/s390/crypto/zcrypt_api.h
> +++ b/drivers/s390/crypto/zcrypt_api.h
> @@ -172,6 +172,8 @@ void zcrypt_api_exit(void);
>  long zcrypt_send_cprb(struct ica_xcRB *xcRB, u32 xflags);
>  long zcrypt_send_ep11_cprb(struct ep11_urb *urb, u32 xflags);
>  void zcrypt_device_status_mask_ext(struct zcrypt_device_status_ext *devstatus);
> +void zcrypt_device_status_mask_ext2(struct zcrypt_device_status_ext *devstatus,
> +				    int maxcard, int maxqueue);
>  int zcrypt_device_status_ext(int card, int queue,
>  			     struct zcrypt_device_status_ext *devstatus);
>  

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com