Re: [PATCH v5 17/17] crypto/nx: Add sysfs interface to export NX capabilities

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Haren Myneni's message of June 13, 2021 9:05 pm:
> 
> Changes to export the following NXGZIP capabilities through sysfs:
> 
> /sys/devices/vio/ibm,compression-v1/nx_gzip_caps:
> min_compress_len  /*Recommended minimum compress length in bytes*/
> min_decompress_len /*Recommended minimum decompress length in bytes*/
> req_max_processed_len /* Maximum number of bytes processed in one
> 			request */
> 
> NX will return RMA_Reject if the request buffer size is greater
> than req_max_processed_len.

Similar for the previous patch, can userspace sanely use the API without 
these capabilities? If not, reorder so the final enable comes last.

I would put those comments in the code, and make the changelog a little
higher level, including a reference or example of how userspace uses it,
which should come with any new userspace ABI.

"Export NX-GZIP capabilities to usrespace in sysfs /sys/devices/vio/...
directory. These are queried by userspace accelerator libraries to set
minimum length heuristics and maximum limits on request sizes."

Something like that. Otherwise looks okay to me.

Thanks,
Nick

> 
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>  drivers/crypto/nx/nx-common-pseries.c | 43 +++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/crypto/nx/nx-common-pseries.c b/drivers/crypto/nx/nx-common-pseries.c
> index 60b5049ec9f7..db28a84a826c 100644
> --- a/drivers/crypto/nx/nx-common-pseries.c
> +++ b/drivers/crypto/nx/nx-common-pseries.c
> @@ -967,6 +967,36 @@ static struct attribute_group nx842_attribute_group = {
>  	.attrs = nx842_sysfs_entries,
>  };
>  
> +#define	nxct_caps_read(_name)						\
> +static ssize_t nxct_##_name##_show(struct device *dev,			\
> +			struct device_attribute *attr, char *buf)	\
> +{									\
> +	return sprintf(buf, "%lld\n", nx_ct_caps._name);		\
> +}
> +
> +#define NXCT_ATTR_RO(_name)						\
> +	nxct_caps_read(_name);						\
> +	static struct device_attribute dev_attr_##_name = __ATTR(_name,	\
> +						0444,			\
> +						nxct_##_name##_show,	\
> +						NULL);
> +
> +NXCT_ATTR_RO(req_max_processed_len);
> +NXCT_ATTR_RO(min_compress_len);
> +NXCT_ATTR_RO(min_decompress_len);
> +
> +static struct attribute *nxct_caps_sysfs_entries[] = {
> +	&dev_attr_req_max_processed_len.attr,
> +	&dev_attr_min_compress_len.attr,
> +	&dev_attr_min_decompress_len.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group nxct_caps_attr_group = {
> +	.name	=	"nx_gzip_caps",
> +	.attrs	=	nxct_caps_sysfs_entries,
> +};
> +
>  static struct nx842_driver nx842_pseries_driver = {
>  	.name =		KBUILD_MODNAME,
>  	.owner =	THIS_MODULE,
> @@ -1056,6 +1086,16 @@ static int nx842_probe(struct vio_dev *viodev,
>  		goto error;
>  	}
>  
> +	if (caps_feat) {
> +		if (sysfs_create_group(&viodev->dev.kobj,
> +					&nxct_caps_attr_group)) {
> +			dev_err(&viodev->dev,
> +				"Could not create sysfs NX capability entries\n");
> +			ret = -1;
> +			goto error;
> +		}
> +	}
> +
>  	return 0;
>  
>  error_unlock:
> @@ -1075,6 +1115,9 @@ static void nx842_remove(struct vio_dev *viodev)
>  	pr_info("Removing IBM Power 842 compression device\n");
>  	sysfs_remove_group(&viodev->dev.kobj, &nx842_attribute_group);
>  
> +	if (caps_feat)
> +		sysfs_remove_group(&viodev->dev.kobj, &nxct_caps_attr_group);
> +
>  	crypto_unregister_alg(&nx842_pseries_alg);
>  
>  	spin_lock_irqsave(&devdata_mutex, flags);
> -- 
> 2.18.2
> 
> 
>