[PATCH 0/6] drivers/nx-842: reduce verbosity of logging

[PATCH 0/6] drivers/nx-842: reduce verbosity of logging

[Nishanth Aravamudan]

Currently, on a LPAR with the nx-842 device disabled, the following
messages are emitted:

nx_compress: no nx842 driver found. [1]
Registering IBM Power 842 compression driver
nx_compress_pseries ibm,compression-v1: nx842_OF_upd_status: status 'disabled' is not 'okay'
nx_compress_pseries ibm,compression-v1: nx842_OF_upd: max_sync_size new:4096 old:0 [2]
nx_compress_pseries ibm,compression-v1: nx842_OF_upd: max_sync_sg new:510 old:0
nx_compress_pseries ibm,compression-v1: nx842_OF_upd: max_sg_len new:4080 old:0
nx_compress_powernv: loading [3]
nx_compress_powernv: no coprocessors found
alg: No test for 842 (842-nx) [4]

[1] is the result of an ordering issue when the CONFIG_ options are set =y. [2]
is the result of nx842_OF_upd_status() not returning the correct error code.
[3] is the result of attempting to load the PowerNV platform driver on a
non-PowerNV platform. [4] is the result of there simply not being any test for
842 in the crypto test manager.

After the changes in the series, the same system as above emits:

Registering IBM Power 842 compression driver
nx_compress_pseries ibm,compression-v1: nx842_OF_upd: device disabled

which seems to me, at least, to be far clearer.

Dan, I think there is still a issue in the code. If CONFIG_DEV_NX_COMPRESS=y
and CONFIG_DEV_NX_COMPRESS_PSERIES=m/CONFIG_DEV_NX_COMPRESS_POWERNV=m, it seems
like the request_module() is not working properly and we simply get a "
nx_compress: no nx842 driver found." at boot (even if I ensure the platform
drivers are in the initrd). If I make CONFIG_DEV_NX_COMPRESS=m, though, the
module(s) load successfully. Does it make sense/is it possible to have these
three symbols always be the same (either all =y or all =m)?

[1/6] crypto/nx-842-pseries: nx842_OF_upd_status should return ENODEV if device is not 'okay'
 drivers/crypto/nx/nx-842-pseries.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
[2/6] nx-842-pseries: rename nx842_{init,exit} to nx842_pseries_{init,exit}
 drivers/crypto/nx/nx-842-pseries.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
[3/6] nx-842-pseries: do not emit extra output if status is disabled
 drivers/crypto/nx/nx-842-pseries.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
[4/6] crypto/nx-842-{powerpc,pseries}: only load on the appropriate machine type
 drivers/crypto/nx/nx-842-powernv.c | 6 ++++++
 drivers/crypto/nx/nx-842-pseries.c | 6 ++++++
 drivers/crypto/nx/nx-842.h         | 1 +
 3 files changed, 13 insertions(+)
[5/6] crypto/testmgr: add null test for 842 algorithm
 crypto/testmgr.c | 3 +++
 1 file changed, 3 insertions(+)
[6/6] nx-842-platform: if NX842 platform drivers are not modules, don't try to load them
 drivers/crypto/nx/nx-842-platform.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

[PATCH 1/6] crypto/nx-842-pseries: nx842_OF_upd_status should return ENODEV if device is not 'okay'

[Nishanth Aravamudan]

The current documention mentions explicitly that EINVAL should be
returned if the device is not available, but nx842_OF_upd_status()
always returns 0. However, nx842_probe() specifically checks for
non-ENODEV returns from nx842_of_upd() (which in turn calls
nx842_OF_upd_status()) and emits an extra error in that case. It seems
like the proper return code of a disabled device is ENODEV.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>

---
 drivers/crypto/nx/nx-842-pseries.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/nx/nx-842-pseries.c b/drivers/crypto/nx/nx-842-pseries.c
index 3040a6091bf2..819c23c546e3 100644
--- a/drivers/crypto/nx/nx-842-pseries.c
+++ b/drivers/crypto/nx/nx-842-pseries.c
@@ -556,7 +556,7 @@ static int nx842_OF_set_defaults(struct nx842_devdata *devdata)
  *
  * Returns:
  *  0 - Device is available
- *  -EINVAL - Device is not available
+ *  -ENODEV - Device is not available
  */
 static int nx842_OF_upd_status(struct nx842_devdata *devdata,
 					struct property *prop) {
@@ -569,6 +569,7 @@ static int nx842_OF_upd_status(struct nx842_devdata *devdata,
 		dev_info(devdata->dev, "%s: status '%s' is not 'okay'\n",
 				__func__, status);
 		devdata->status = UNAVAILABLE;
+		ret = -ENODEV;
 	}
 
 	return ret;
-- 
2.1.4

[PATCH 2/6] nx-842-pseries: rename nx842_{init,exit} to nx842_pseries_{init,exit}

[Nishanth Aravamudan]

While there is no technical reason that both nx-842.c and
nx-842-pseries.c can have the same name for the init/exit functions, it
is a bit confusing with initcall_debug. Rename the pseries specific
functions appropriately

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>

---
 drivers/crypto/nx/nx-842-pseries.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/nx/nx-842-pseries.c b/drivers/crypto/nx/nx-842-pseries.c
index 819c23c546e3..e17f4d2e96e0 100644
--- a/drivers/crypto/nx/nx-842-pseries.c
+++ b/drivers/crypto/nx/nx-842-pseries.c
@@ -1080,7 +1080,7 @@ static struct vio_driver nx842_vio_driver = {
 	.id_table = nx842_vio_driver_ids,
 };
 
-static int __init nx842_init(void)
+static int __init nx842_pseries_init(void)
 {
 	struct nx842_devdata *new_devdata;
 	int ret;
@@ -1116,9 +1116,9 @@ static int __init nx842_init(void)
 	return 0;
 }
 
-module_init(nx842_init);
+module_init(nx842_pseries_init);
 
-static void __exit nx842_exit(void)
+static void __exit nx842_pseries_exit(void)
 {
 	struct nx842_devdata *old_devdata;
 	unsigned long flags;
@@ -1137,5 +1137,5 @@ static void __exit nx842_exit(void)
 	vio_unregister_driver(&nx842_vio_driver);
 }
 
-module_exit(nx842_exit);
+module_exit(nx842_pseries_exit);
 
-- 
2.1.4

[PATCH 3/6] nx-842-pseries: do not emit extra output if status is disabled

[Nishanth Aravamudan]

If the device-tree indicates the nx-842 device's status is 'disabled',
we emit two messages:

nx_compress_pseries ibm,compression-v1: nx842_OF_upd_status: status 'disabled' is not 'okay'.
nx_compress_pseries ibm,compression-v1: nx842_OF_upd: device disabled

Given that 'disabled' is a valid state, and we are going to emit that
the device is disabled, only print out a non-'okay' status if it is not
'disabled'.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>

---
 drivers/crypto/nx/nx-842-pseries.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/nx/nx-842-pseries.c b/drivers/crypto/nx/nx-842-pseries.c
index e17f4d2e96e0..b84b0ceeb46e 100644
--- a/drivers/crypto/nx/nx-842-pseries.c
+++ b/drivers/crypto/nx/nx-842-pseries.c
@@ -566,8 +566,14 @@ static int nx842_OF_upd_status(struct nx842_devdata *devdata,
 	if (!strncmp(status, "okay", (size_t)prop->length)) {
 		devdata->status = AVAILABLE;
 	} else {
-		dev_info(devdata->dev, "%s: status '%s' is not 'okay'\n",
+		/*
+		 * Caller will log that the device is disabled, so only
+		 * output if there is an unexpected status.
+		 */
+		if (strncmp(status, "disabled", (size_t)prop->length)) {
+			dev_info(devdata->dev, "%s: status '%s' is not 'okay'\n",
 				__func__, status);
+		}
 		devdata->status = UNAVAILABLE;
 		ret = -ENODEV;
 	}
-- 
2.1.4

[PATCH 4/6] crypto/nx-842-{powerpc,pseries}: only load on the appropriate machine type

[Nishanth Aravamudan]

While we never would successfully load on the wrong machine type, there
is extra output by default regardless of machine type.

For instance, on a PowerVM LPAR, we see the following:

nx_compress_powernv: loading
nx_compress_powernv: no coprocessors found

even though those coprocessors could never be found. Similar pseries
messages are printed on powernv.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>

---
 drivers/crypto/nx/nx-842-powernv.c | 6 ++++++
 drivers/crypto/nx/nx-842-pseries.c | 6 ++++++
 drivers/crypto/nx/nx-842.h         | 1 +
 3 files changed, 13 insertions(+)

diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c
index 33b3b0abf4ae..6b5e5143c95b 100644
--- a/drivers/crypto/nx/nx-842-powernv.c
+++ b/drivers/crypto/nx/nx-842-powernv.c
@@ -594,6 +594,9 @@ static __init int nx842_powernv_init(void)
 	BUILD_BUG_ON(DDE_BUFFER_ALIGN % DDE_BUFFER_SIZE_MULT);
 	BUILD_BUG_ON(DDE_BUFFER_SIZE_MULT % DDE_BUFFER_LAST_MULT);
 
+	if (!machine_is(powernv))
+		return -ENODEV;
+
 	pr_info("loading\n");
 
 	for_each_compatible_node(dn, NULL, "ibm,power-nx")
@@ -625,6 +628,9 @@ static void __exit nx842_powernv_exit(void)
 {
 	struct nx842_coproc *coproc, *n;
 
+	if (!machine_is(powernv))
+		return;
+
 	nx842_platform_driver_unset(&nx842_powernv_driver);
 
 	list_for_each_entry_safe(coproc, n, &nx842_coprocs, list) {
diff --git a/drivers/crypto/nx/nx-842-pseries.c b/drivers/crypto/nx/nx-842-pseries.c
index b84b0ceeb46e..75a7bfdc160e 100644
--- a/drivers/crypto/nx/nx-842-pseries.c
+++ b/drivers/crypto/nx/nx-842-pseries.c
@@ -1091,6 +1091,9 @@ static int __init nx842_pseries_init(void)
 	struct nx842_devdata *new_devdata;
 	int ret;
 
+	if (!machine_is(pseries))
+		return -ENODEV;
+
 	pr_info("Registering IBM Power 842 compression driver\n");
 
 	if (!of_find_compatible_node(NULL, NULL, "ibm,compression"))
@@ -1129,6 +1132,9 @@ static void __exit nx842_pseries_exit(void)
 	struct nx842_devdata *old_devdata;
 	unsigned long flags;
 
+	if (!machine_is(pseries))
+		return;
+
 	pr_info("Exiting IBM Power 842 compression driver\n");
 	nx842_platform_driver_unset(&nx842_pseries_driver);
 	spin_lock_irqsave(&devdata_mutex, flags);
diff --git a/drivers/crypto/nx/nx-842.h b/drivers/crypto/nx/nx-842.h
index ac0ea79d0f8b..ea89c661e476 100644
--- a/drivers/crypto/nx/nx-842.h
+++ b/drivers/crypto/nx/nx-842.h
@@ -10,6 +10,7 @@
 #include <linux/io.h>
 #include <linux/mm.h>
 #include <linux/ratelimit.h>
+#include <asm/machdep.h>
 
 /* Restrictions on Data Descriptor List (DDL) and Entry (DDE) buffers
  *
-- 
2.1.4

[PATCH 5/6] [RFC] crypto/testmgr: add null test for 842 algorithm

[Nishanth Aravamudan]

Currently, when the nx-842-pseries driver loads, the following message
is emitted:

alg: No test for 842 (842-nx)

It seems like the simplest way to fix this message (other than adding a
proper test) is to just insert the null test into the list in the
testmgr.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>

---
 crypto/testmgr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index d0a42bd3aae9..ff0f76e0d0b4 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1982,6 +1982,9 @@ static int alg_test_null(const struct alg_test_desc *desc,
 /* Please keep this list sorted by algorithm name. */
 static const struct alg_test_desc alg_test_descs[] = {
 	{
+		.alg = "842",
+		.test = alg_test_null,
+	}, {
 		.alg = "__cbc-cast5-avx",
 		.test = alg_test_null,
 	}, {
-- 
2.1.4

[PATCH 6/6] nx-842-platform: if NX842 platform drivers are not modules, don't try to load them

[Nishanth Aravamudan]

Based off the CONFIG_SPU_FS_MODULE code, only attempt to load platform
modules if the nx-842 pseries/powernv drivers are built as modules.

Otherwise, if CONFIG_DEV_NX_COMPRESS=y,
CONFIG_DEV_NX_COMPRESS_PSERIES=y, CONFIG_DEV_NX_POWERNV=y, the following
message is emitted at boot:

nx_compress: no nx842 driver found.

even though the drivers successfully loads.

This is because in the =y case, the module_init() calls get converted to
initcalls and the nx842_init() runs before the platform driver
nx842_pseries_init() or nx842_powernv_init() functions, which are what
normally set the static platform driver.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
Cc: Dan Streetman <ddstreet@us.ibm.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
---
 drivers/crypto/nx/nx-842-platform.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/nx/nx-842-platform.c b/drivers/crypto/nx/nx-842-platform.c
index 664f13dd06ed..5363c72593b4 100644
--- a/drivers/crypto/nx/nx-842-platform.c
+++ b/drivers/crypto/nx/nx-842-platform.c
@@ -53,6 +53,7 @@ void nx842_platform_driver_unset(struct nx842_driver *_driver)
 }
 EXPORT_SYMBOL_GPL(nx842_platform_driver_unset);
 
+#if defined(CONFIG_CRYPTO_DEV_NX_COMPRESS_PSERIES_MODULE) || defined(CONFIG_CRYPTO_DEV_NX_COMPRESS_POWERNV_MODULE)
 bool nx842_platform_driver_get(void)
 {
 	bool ret = false;
@@ -66,7 +67,6 @@ bool nx842_platform_driver_get(void)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(nx842_platform_driver_get);
 
 void nx842_platform_driver_put(void)
 {
@@ -77,6 +77,17 @@ void nx842_platform_driver_put(void)
 
 	spin_unlock(&driver_lock);
 }
+#else
+bool nx842_platform_driver_get(void)
+{
+	return true;
+}
+
+void nx842_platform_driver_put(void)
+{
+}
+#endif
+EXPORT_SYMBOL_GPL(nx842_platform_driver_get);
 EXPORT_SYMBOL_GPL(nx842_platform_driver_put);
 
 MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [PATCH 4/6] crypto/nx-842-{powerpc,pseries}: only load on the appropriate machine type

[Michael Ellerman]

On Thu, 2015-07-02 at 15:40 -0700, Nishanth Aravamudan wrote:
> While we never would successfully load on the wrong machine type, there
> is extra output by default regardless of machine type.
> 
> For instance, on a PowerVM LPAR, we see the following:
> 
> nx_compress_powernv: loading
> nx_compress_powernv: no coprocessors found
> 
> even though those coprocessors could never be found. Similar pseries
> messages are printed on powernv.

I know I've been converting init calls to machine_initcalls() to avoid these
sort of issues in platform code. But for a driver it should be trivial for it
to only probe when the hardware is found.

By which I mean I think we shouldn't need these.

> diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c
> index 33b3b0abf4ae..6b5e5143c95b 100644
> --- a/drivers/crypto/nx/nx-842-powernv.c
> +++ b/drivers/crypto/nx/nx-842-powernv.c
> @@ -594,6 +594,9 @@ static __init int nx842_powernv_init(void)
>  	BUILD_BUG_ON(DDE_BUFFER_ALIGN % DDE_BUFFER_SIZE_MULT);
>  	BUILD_BUG_ON(DDE_BUFFER_SIZE_MULT % DDE_BUFFER_LAST_MULT);
>  
> +	if (!machine_is(powernv))
> +		return -ENODEV;
> +
>  	pr_info("loading\n");

This is just too chatty, drop it.

>  	for_each_compatible_node(dn, NULL, "ibm,power-nx")

It shouldn't be printing anything unless it finds some devices in this loop.

And you should drop the print in here:

	if (!nx842_ct) {
		pr_err("no coprocessors found\n");
		return -ENODEV;
	}

And that should mean no output unless hardware is found I think?

> @@ -625,6 +628,9 @@ static void __exit nx842_powernv_exit(void)
>  {
>  	struct nx842_coproc *coproc, *n;
>  
> +	if (!machine_is(powernv))
> +		return;

You shouldn't need to touch the exit paths if the drivers were never loaded?

> diff --git a/drivers/crypto/nx/nx-842-pseries.c b/drivers/crypto/nx/nx-842-pseries.c
> index b84b0ceeb46e..75a7bfdc160e 100644
> --- a/drivers/crypto/nx/nx-842-pseries.c
> +++ b/drivers/crypto/nx/nx-842-pseries.c
> @@ -1091,6 +1091,9 @@ static int __init nx842_pseries_init(void)
>  	struct nx842_devdata *new_devdata;
>  	int ret;
>  
> +	if (!machine_is(pseries))
> +		return -ENODEV;
> +
>  	pr_info("Registering IBM Power 842 compression driver\n");

Again this is too chatty, just remove it.

>  	if (!of_find_compatible_node(NULL, NULL, "ibm,compression"))
		return -ENODEV;

That should do the trick shouldn't it?

cheers

Re: [PATCH 5/6] [RFC] crypto/testmgr: add null test for 842 algorithm

[Stephan Mueller]

Am Donnerstag, 2. Juli 2015, 15:41:19 schrieb Nishanth Aravamudan:

Hi Nishanth,

>Currently, when the nx-842-pseries driver loads, the following message
>is emitted:
>
>alg: No test for 842 (842-nx)
>
>It seems like the simplest way to fix this message (other than adding a
>proper test) is to just insert the null test into the list in the
>testmgr.
>
>Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
>
>---
> crypto/testmgr.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/crypto/testmgr.c b/crypto/testmgr.c
>index d0a42bd3aae9..ff0f76e0d0b4 100644
>--- a/crypto/testmgr.c
>+++ b/crypto/testmgr.c
>@@ -1982,6 +1982,9 @@ static int alg_test_null(const struct alg_test_desc
>*desc, /* Please keep this list sorted by algorithm name. */
> static const struct alg_test_desc alg_test_descs[] = {
> 	{
>+		.alg = "842",
>+		.test = alg_test_null,
>+	}, {
> 		.alg = "__cbc-cast5-avx",
> 		.test = alg_test_null,

As this is a compression algo, it is safe to add fips_allowed = 1 here as 
otherwise the algo is not available in fips=1

> 	}, {


Ciao
Stephan

Re: [PATCH 5/6] [RFC] crypto/testmgr: add null test for 842 algorithm

[Herbert Xu]

On Thu, Jul 02, 2015 at 03:41:19PM -0700, Nishanth Aravamudan wrote:
> Currently, when the nx-842-pseries driver loads, the following message
> is emitted:
> 
> alg: No test for 842 (842-nx)
> 
> It seems like the simplest way to fix this message (other than adding a
> proper test) is to just insert the null test into the list in the
> testmgr.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>

Please add some real test vectors instead.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 6/6] nx-842-platform: if NX842 platform drivers are not modules, don't try to load them

[Herbert Xu]

On Thu, Jul 02, 2015 at 03:42:26PM -0700, Nishanth Aravamudan wrote:
> Based off the CONFIG_SPU_FS_MODULE code, only attempt to load platform
> modules if the nx-842 pseries/powernv drivers are built as modules.
> 
> Otherwise, if CONFIG_DEV_NX_COMPRESS=y,
> CONFIG_DEV_NX_COMPRESS_PSERIES=y, CONFIG_DEV_NX_POWERNV=y, the following
> message is emitted at boot:
> 
> nx_compress: no nx842 driver found.
> 
> even though the drivers successfully loads.
> 
> This is because in the =y case, the module_init() calls get converted to
> initcalls and the nx842_init() runs before the platform driver
> nx842_pseries_init() or nx842_powernv_init() functions, which are what
> normally set the static platform driver.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> Cc: Dan Streetman <ddstreet@us.ibm.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-crypto@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org

Ugh, I think this whole thing is redundant.  The whole point of
the crypto API is to allow the coexistence of multiple underlying
implementations.

Please get rid of nx-842-platform.c completely and move the crypto
registration into the individual platform drivers.  That is, powernv
and pseries should each register their own crypto driver.  They can
of course share a common set of crypto code which can live in its own
module.  There should be no need for mucking with module reference
counts at all.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/6] drivers/nx-842: reduce verbosity of logging

[Herbert Xu]

On Thu, Jul 02, 2015 at 03:38:00PM -0700, Nishanth Aravamudan wrote:
> Currently, on a LPAR with the nx-842 device disabled, the following
> messages are emitted:

Patches 1-3 applied.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v2] crypto/nx-842-{powerpc,pseries}: reduce chattiness of platform drivers

[Nishanth Aravamudan]

On 03.07.2015 [11:30:32 +1000], Michael Ellerman wrote:
> On Thu, 2015-07-02 at 15:40 -0700, Nishanth Aravamudan wrote:
> > While we never would successfully load on the wrong machine type, there
> > is extra output by default regardless of machine type.
> > 
> > For instance, on a PowerVM LPAR, we see the following:
> > 
> > nx_compress_powernv: loading
> > nx_compress_powernv: no coprocessors found
> > 
> > even though those coprocessors could never be found. Similar pseries
> > messages are printed on powernv.
> 
> I know I've been converting init calls to machine_initcalls() to avoid
> these sort of issues in platform code. But for a driver it should be
> trivial for it to only probe when the hardware is found.
> 
> By which I mean I think we shouldn't need these.

Ok.

> > diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c
> > index 33b3b0abf4ae..6b5e5143c95b 100644
> > --- a/drivers/crypto/nx/nx-842-powernv.c
> > +++ b/drivers/crypto/nx/nx-842-powernv.c
> > @@ -594,6 +594,9 @@ static __init int nx842_powernv_init(void)
> >  	BUILD_BUG_ON(DDE_BUFFER_ALIGN % DDE_BUFFER_SIZE_MULT);
> >  	BUILD_BUG_ON(DDE_BUFFER_SIZE_MULT % DDE_BUFFER_LAST_MULT);
> >  
> > +	if (!machine_is(powernv))
> > +		return -ENODEV;
> > +
> >  	pr_info("loading\n");
> 
> This is just too chatty, drop it.
> 
> >  	for_each_compatible_node(dn, NULL, "ibm,power-nx")
> 
> It shouldn't be printing anything unless it finds some devices in this loop.
> 
> And you should drop the print in here:
> 
> 	if (!nx842_ct) {
> 		pr_err("no coprocessors found\n");
> 		return -ENODEV;
> 	}
> 
> And that should mean no output unless hardware is found I think?

Yep, will adjust.

> > @@ -625,6 +628,9 @@ static void __exit nx842_powernv_exit(void)
> >  {
> >  	struct nx842_coproc *coproc, *n;
> >  
> > +	if (!machine_is(powernv))
> > +		return;
> 
> You shouldn't need to touch the exit paths if the drivers were never
> loaded?

Duh, yep.

> > diff --git a/drivers/crypto/nx/nx-842-pseries.c b/drivers/crypto/nx/nx-842-pseries.c
> > index b84b0ceeb46e..75a7bfdc160e 100644
> > --- a/drivers/crypto/nx/nx-842-pseries.c
> > +++ b/drivers/crypto/nx/nx-842-pseries.c
> > @@ -1091,6 +1091,9 @@ static int __init nx842_pseries_init(void)
> >  	struct nx842_devdata *new_devdata;
> >  	int ret;
> >  
> > +	if (!machine_is(pseries))
> > +		return -ENODEV;
> > +
> >  	pr_info("Registering IBM Power 842 compression driver\n");
> 
> Again this is too chatty, just remove it.

Will do.

> >  	if (!of_find_compatible_node(NULL, NULL, "ibm,compression"))
> 		return -ENODEV;
> 
> That should do the trick shouldn't it?

Yep, I think so.

Thanks Michael!


While we never would successfully load on the wrong machine type, there
is extra output by default regardless of machine type.

For instance, on a PowerVM LPAR, we see the following:

nx_compress_powernv: loading
nx_compress_powernv: no coprocessors found

even though those coprocessors could never be found.

Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
Cc: Dan Streetman <ddstreet@us.ibm.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org

---
v2:
  Rather than not loading, just reduce the verbosity

 drivers/crypto/nx/nx-842-powernv.c | 10 +---------
 drivers/crypto/nx/nx-842-pseries.c |  3 ---
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/crypto/nx/nx-842-powernv.c b/drivers/crypto/nx/nx-842-powernv.c
index 33b3b0abf4ae..7e474562058d 100644
--- a/drivers/crypto/nx/nx-842-powernv.c
+++ b/drivers/crypto/nx/nx-842-powernv.c
@@ -594,15 +594,11 @@ static __init int nx842_powernv_init(void)
 	BUILD_BUG_ON(DDE_BUFFER_ALIGN % DDE_BUFFER_SIZE_MULT);
 	BUILD_BUG_ON(DDE_BUFFER_SIZE_MULT % DDE_BUFFER_LAST_MULT);
 
-	pr_info("loading\n");
-
 	for_each_compatible_node(dn, NULL, "ibm,power-nx")
 		nx842_powernv_probe(dn);
 
-	if (!nx842_ct) {
-		pr_err("no coprocessors found\n");
+	if (!nx842_ct)
 		return -ENODEV;
-	}
 
 	if (!nx842_platform_driver_set(&nx842_powernv_driver)) {
 		struct nx842_coproc *coproc, *n;
@@ -615,8 +611,6 @@ static __init int nx842_powernv_init(void)
 		return -EEXIST;
 	}
 
-	pr_info("loaded\n");
-
 	return 0;
 }
 module_init(nx842_powernv_init);
@@ -631,7 +625,5 @@ static void __exit nx842_powernv_exit(void)
 		list_del(&coproc->list);
 		kfree(coproc);
 	}
-
-	pr_info("unloaded\n");
 }
 module_exit(nx842_powernv_exit);
diff --git a/drivers/crypto/nx/nx-842-pseries.c b/drivers/crypto/nx/nx-842-pseries.c
index b84b0ceeb46e..d44524da6589 100644
--- a/drivers/crypto/nx/nx-842-pseries.c
+++ b/drivers/crypto/nx/nx-842-pseries.c
@@ -1091,8 +1091,6 @@ static int __init nx842_pseries_init(void)
 	struct nx842_devdata *new_devdata;
 	int ret;
 
-	pr_info("Registering IBM Power 842 compression driver\n");
-
 	if (!of_find_compatible_node(NULL, NULL, "ibm,compression"))
 		return -ENODEV;
 
@@ -1129,7 +1127,6 @@ static void __exit nx842_pseries_exit(void)
 	struct nx842_devdata *old_devdata;
 	unsigned long flags;
 
-	pr_info("Exiting IBM Power 842 compression driver\n");
 	nx842_platform_driver_unset(&nx842_pseries_driver);
 	spin_lock_irqsave(&devdata_mutex, flags);
 	old_devdata = rcu_dereference_check(devdata,
-- 
2.1.4

Re: [PATCH 6/6] nx-842-platform: if NX842 platform drivers are not modules, don't try to load them

[Nishanth Aravamudan]

On 06.07.2015 [16:13:07 +0800], Herbert Xu wrote:
> On Thu, Jul 02, 2015 at 03:42:26PM -0700, Nishanth Aravamudan wrote:
> > Based off the CONFIG_SPU_FS_MODULE code, only attempt to load platform
> > modules if the nx-842 pseries/powernv drivers are built as modules.
> > 
> > Otherwise, if CONFIG_DEV_NX_COMPRESS=y,
> > CONFIG_DEV_NX_COMPRESS_PSERIES=y, CONFIG_DEV_NX_POWERNV=y, the following
> > message is emitted at boot:
> > 
> > nx_compress: no nx842 driver found.
> > 
> > even though the drivers successfully loads.
> > 
> > This is because in the =y case, the module_init() calls get converted to
> > initcalls and the nx842_init() runs before the platform driver
> > nx842_pseries_init() or nx842_powernv_init() functions, which are what
> > normally set the static platform driver.
> > 
> > Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> > Cc: Dan Streetman <ddstreet@us.ibm.com>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: linux-crypto@vger.kernel.org
> > Cc: linuxppc-dev@lists.ozlabs.org
> 
> Ugh, I think this whole thing is redundant.  The whole point of
> the crypto API is to allow the coexistence of multiple underlying
> implementations.

Sure, that makes sense -- sorry, I was picking this up while Dan was on
vacation. Will provide a better v2.

> Please get rid of nx-842-platform.c completely and move the crypto
> registration into the individual platform drivers.  That is, powernv
> and pseries should each register their own crypto driver.  They can of
> course share a common set of crypto code which can live in its own
> module.  There should be no need for mucking with module reference
> counts at all.

Will do, thanks!

-Nish

Re: [PATCH v2] crypto/nx-842-{powerpc,pseries}: reduce chattiness of platform drivers

[Michael Ellerman]

On Mon, 2015-07-06 at 10:06 -0700, Nishanth Aravamudan wrote:
> On 03.07.2015 [11:30:32 +1000], Michael Ellerman wrote:
> > On Thu, 2015-07-02 at 15:40 -0700, Nishanth Aravamudan wrote:
> > > While we never would successfully load on the wrong machine type, there
> > > is extra output by default regardless of machine type.
> > > 
> 
> Thanks Michael!

Thanks for cleaning it up.


> While we never would successfully load on the wrong machine type, there
> is extra output by default regardless of machine type.
> 
> For instance, on a PowerVM LPAR, we see the following:
> 
> nx_compress_powernv: loading
> nx_compress_powernv: no coprocessors found
> 
> even though those coprocessors could never be found.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> Cc: Dan Streetman <ddstreet@us.ibm.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-crypto@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> 
> ---
> v2:
>   Rather than not loading, just reduce the verbosity

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

I'm assuming Herbert will take this.

cheers

Re: [PATCH v2] crypto/nx-842-{powerpc, pseries}: reduce chattiness of platform drivers

[Herbert Xu]

On Mon, Jul 06, 2015 at 10:06:21AM -0700, Nishanth Aravamudan wrote:
>
> v2:
>   Rather than not loading, just reduce the verbosity

Applied.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 5/6] [RFC] crypto/testmgr: add null test for 842 algorithm

[Nishanth Aravamudan]

On 04.07.2015 [15:24:53 +0800], Herbert Xu wrote:
> On Thu, Jul 02, 2015 at 03:41:19PM -0700, Nishanth Aravamudan wrote:
> > Currently, when the nx-842-pseries driver loads, the following message
> > is emitted:
> > 
> > alg: No test for 842 (842-nx)
> > 
> > It seems like the simplest way to fix this message (other than adding a
> > proper test) is to just insert the null test into the list in the
> > testmgr.
> > 
> > Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> 
> Please add some real test vectors instead.

Re: [PATCH 5/6] [RFC] crypto/testmgr: add null test for 842 algorithm

[Nishanth Aravamudan]

On 13.07.2015 [17:05:36 -0700], Nishanth Aravamudan wrote:
> On 04.07.2015 [15:24:53 +0800], Herbert Xu wrote:
> > On Thu, Jul 02, 2015 at 03:41:19PM -0700, Nishanth Aravamudan wrote:
> > > Currently, when the nx-842-pseries driver loads, the following message
> > > is emitted:
> > > 
> > > alg: No test for 842 (842-nx)
> > > 
> > > It seems like the simplest way to fix this message (other than adding a
> > > proper test) is to just insert the null test into the list in the
> > > testmgr.
> > > 
> > > Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> > 
> > Please add some real test vectors instead.

Apologies, hit send too fast. I'll work with Dan on this when he gets
back from vacation.

-Nish

Re: [PATCH 5/6] [RFC] crypto/testmgr: add null test for 842 algorithm

[Dan Streetman]

On Mon, Jul 13, 2015 at 8:05 PM, Nishanth Aravamudan
<nacc@linux.vnet.ibm.com> wrote:
> On 13.07.2015 [17:05:36 -0700], Nishanth Aravamudan wrote:
>> On 04.07.2015 [15:24:53 +0800], Herbert Xu wrote:
>> > On Thu, Jul 02, 2015 at 03:41:19PM -0700, Nishanth Aravamudan wrote:
>> > > Currently, when the nx-842-pseries driver loads, the following message
>> > > is emitted:
>> > >
>> > > alg: No test for 842 (842-nx)
>> > >
>> > > It seems like the simplest way to fix this message (other than adding a
>> > > proper test) is to just insert the null test into the list in the
>> > > testmgr.
>> > >
>> > > Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
>> >
>> > Please add some real test vectors instead.
>
> Apologies, hit send too fast. I'll work with Dan on this when he gets
> back from vacation.

Back from vacation! :-)

I originally didn't add any test vector for NX 842 because the main
driver was loading before the "platform" (pseries/powernv) drivers,
and the test couldn't run as the platform driver hadn't loaded yet.
That's now fixed so we should be able to add a real test for NX 842
now, I can work on that patch.

>
> -Nish
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] nx-842-platform: if NX842 platform drivers are not modules, don't try to load them

[Dan Streetman]

On Mon, Jul 6, 2015 at 1:07 PM, Nishanth Aravamudan
<nacc@linux.vnet.ibm.com> wrote:
> On 06.07.2015 [16:13:07 +0800], Herbert Xu wrote:
>> On Thu, Jul 02, 2015 at 03:42:26PM -0700, Nishanth Aravamudan wrote:
>> > Based off the CONFIG_SPU_FS_MODULE code, only attempt to load platform
>> > modules if the nx-842 pseries/powernv drivers are built as modules.
>> >
>> > Otherwise, if CONFIG_DEV_NX_COMPRESS=y,
>> > CONFIG_DEV_NX_COMPRESS_PSERIES=y, CONFIG_DEV_NX_POWERNV=y, the following
>> > message is emitted at boot:
>> >
>> > nx_compress: no nx842 driver found.
>> >
>> > even though the drivers successfully loads.
>> >
>> > This is because in the =y case, the module_init() calls get converted to
>> > initcalls and the nx842_init() runs before the platform driver
>> > nx842_pseries_init() or nx842_powernv_init() functions, which are what
>> > normally set the static platform driver.
>> >
>> > Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
>> > Cc: Dan Streetman <ddstreet@us.ibm.com>
>> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> > Cc: "David S. Miller" <davem@davemloft.net>
>> > Cc: linux-crypto@vger.kernel.org
>> > Cc: linuxppc-dev@lists.ozlabs.org
>>
>> Ugh, I think this whole thing is redundant.  The whole point of
>> the crypto API is to allow the coexistence of multiple underlying
>> implementations.
>
> Sure, that makes sense -- sorry, I was picking this up while Dan was on
> vacation. Will provide a better v2.
>
>> Please get rid of nx-842-platform.c completely and move the crypto
>> registration into the individual platform drivers.  That is, powernv
>> and pseries should each register their own crypto driver.  They can of
>> course share a common set of crypto code which can live in its own
>> module.  There should be no need for mucking with module reference
>> counts at all.
>
> Will do, thanks!

Yep, I originally did it this way because I didn't realize crypto
could register different drivers with the same alg name (but different
driver names).  I have some patches already to start doing this but
they weren't ready enough to send before I left for vacation; I'll
finish them up and send them.

>
> -Nish
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html