Re: [V3 PATCH 15/16] crypto/nx: Get NX capabilities for GZIP coprocessor type

[Haren Myneni <haren@linux.ibm.com>]

On Mon, 2021-05-10 at 16:38 +1000, Nicholas Piggin wrote:
> Excerpts from Haren Myneni's message of April 18, 2021 7:12 am:
> > phyp provides NX capabilities which gives recommended minimum
> > compression / decompression length and maximum request buffer size
> > in bytes.
> > 
> > Changes to get NX overall capabilities which points to the specific
> > features phyp supports. Then retrieve NXGZIP specific capabilities.
> > 
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> >  drivers/crypto/nx/nx-common-pseries.c | 83
> > +++++++++++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> > 
> > diff --git a/drivers/crypto/nx/nx-common-pseries.c
> > b/drivers/crypto/nx/nx-common-pseries.c
> > index 9a40fca8a9e6..49224870d05e 100644
> > --- a/drivers/crypto/nx/nx-common-pseries.c
> > +++ b/drivers/crypto/nx/nx-common-pseries.c
> > @@ -9,6 +9,7 @@
> >   */
> >  
> >  #include <asm/vio.h>
> > +#include <asm/hvcall.h>
> >  #include <asm/vas.h>
> >  
> >  #include "nx-842.h"
> > @@ -20,6 +21,24 @@ MODULE_DESCRIPTION("842 H/W Compression driver
> > for IBM Power processors");
> >  MODULE_ALIAS_CRYPTO("842");
> >  MODULE_ALIAS_CRYPTO("842-nx");
> >  
> > +struct nx_ct_capabs_be {
> 
> What does "ct" mean? I've seen it in a few other places too.

ct means coprocessor type such as 842 and GZIP. phyp provides only GZIP
capabilities right now. But this struct may be extended to other types
in future.

> 
> > +	__be64	descriptor;
> > +	__be64	req_max_processed_len;	/* Max bytes in one GZIP
> > request */
> > +	__be64	min_compress_len;	/* Min compression size in
> > bytes */
> > +	__be64	min_decompress_len;	/* Min decompression size
> > in bytes */
> > +} __packed __aligned(0x1000);
> > +
> > +struct nx_ct_capabs {
> > +	char	name[VAS_DESCR_LEN + 1];
> > +	u64	descriptor;
> > +	u64	req_max_processed_len;	/* Max bytes in one GZIP request */
> > +	u64	min_compress_len;	/* Min compression in bytes */
> > +	u64	min_decompress_len;	/* Min decompression in bytes */
> > +};
> > +
> > +u64 capab_feat = 0;
> 
> Why is this here and not a local variable?

capab_feat is used to add / delete sysfs entries.

> 
> > +struct nx_ct_capabs nx_ct_capab;
> 
> It's okay and generally better to use the same name as the struct
> name
> in this situation, i.e.,
> 
> "struct nx_ct_capabs nx_ct_capabs"
> 
> (modulo static / caps / etc)

Sure, will change.

> 
> Thanks,
> Nick