A previous fixup avoided an unused variable warning but replaced
it with a slightly scarier warning:
drivers/crypto/inside-secure/safexcel.c:1100:6: error: variable 'irq' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
This is harmless as it is impossible to get into this case, but
the compiler has no way of knowing that. Add an explicit error
handling case to make it obvious to both compilers and humans
reading the source.
Fixes: 212ef6f29e5b ("crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/crypto/inside-secure/safexcel.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index 4ab1bde8dd9b..311bf60df39f 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -1120,6 +1120,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid,
irq_name, irq);
return irq;
}
+ } else {
+ return -ENXIO;
}
ret = devm_request_threaded_irq(dev, irq, handler,
--
2.20.0
safexcel_aead_setkey() contains three large stack variables, totalling
slightly more than the 1024 byte warning limit:
drivers/crypto/inside-secure/safexcel_cipher.c:303:12: error: stack frame size of 1032 bytes in function 'safexcel_aead_setkey' [-Werror,-Wframe-larger-than=]
The function already contains a couple of dynamic allocations, so it is
likely not performance critical and it can only be called in a context
that allows sleeping, so the easiest workaround is to add change it
to use dynamic allocations. Combining istate and ostate into a single
variable simplifies the allocation at the cost of making it slightly
less readable.
Alternatively, it should be possible to shrink these allocations
as the extra buffers appear to be largely unnecessary, but doing
this would be a much more invasive change.
Fixes: 0e17e3621a28 ("crypto: inside-secure - add support for authenc(hmac(sha*),rfc3686(ctr(aes))) suites")
Signed-off-by: Arnd Bergmann <[email protected]>
---
.../crypto/inside-secure/safexcel_cipher.c | 53 ++++++++++++-------
1 file changed, 35 insertions(+), 18 deletions(-)
diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c
index ef51f8c2b473..51a4112aa9bc 100644
--- a/drivers/crypto/inside-secure/safexcel_cipher.c
+++ b/drivers/crypto/inside-secure/safexcel_cipher.c
@@ -305,10 +305,10 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
{
struct crypto_tfm *tfm = crypto_aead_tfm(ctfm);
struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
- struct safexcel_ahash_export_state istate, ostate;
+ struct safexcel_ahash_export_state *state;
struct safexcel_crypto_priv *priv = ctx->priv;
+ struct crypto_aes_ctx *aes;
struct crypto_authenc_keys keys;
- struct crypto_aes_ctx aes;
int err = -EINVAL;
if (crypto_authenc_extractkeys(&keys, key, len) != 0)
@@ -334,7 +334,14 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
goto badkey_expflags;
break;
case SAFEXCEL_AES:
- err = aes_expandkey(&aes, keys.enckey, keys.enckeylen);
+ aes = kzalloc(sizeof(*aes), GFP_KERNEL);
+ if (!aes) {
+ err = -ENOMEM;
+ goto badkey;
+ }
+
+ err = aes_expandkey(aes, keys.enckey, keys.enckeylen);
+ kfree(aes);
if (unlikely(err))
goto badkey;
break;
@@ -347,56 +354,66 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
memcmp(ctx->key, keys.enckey, keys.enckeylen))
ctx->base.needs_inv = true;
+ state = kzalloc(sizeof(struct safexcel_ahash_export_state) * 2, GFP_KERNEL);
+ if (!state) {
+ err = -ENOMEM;
+ goto badkey;
+ }
+
/* Auth key */
switch (ctx->hash_alg) {
case CONTEXT_CONTROL_CRYPTO_ALG_SHA1:
if (safexcel_hmac_setkey("safexcel-sha1", keys.authkey,
- keys.authkeylen, &istate, &ostate))
- goto badkey;
+ keys.authkeylen, &state[0], &state[1]))
+ goto badkey_free;
break;
case CONTEXT_CONTROL_CRYPTO_ALG_SHA224:
if (safexcel_hmac_setkey("safexcel-sha224", keys.authkey,
- keys.authkeylen, &istate, &ostate))
- goto badkey;
+ keys.authkeylen, &state[0], &state[1]))
+ goto badkey_free;
break;
case CONTEXT_CONTROL_CRYPTO_ALG_SHA256:
if (safexcel_hmac_setkey("safexcel-sha256", keys.authkey,
- keys.authkeylen, &istate, &ostate))
- goto badkey;
+ keys.authkeylen, &state[0], &state[1]))
+ goto badkey_free;
break;
case CONTEXT_CONTROL_CRYPTO_ALG_SHA384:
if (safexcel_hmac_setkey("safexcel-sha384", keys.authkey,
- keys.authkeylen, &istate, &ostate))
- goto badkey;
+ keys.authkeylen, &state[0], &state[1]))
+ goto badkey_free;
break;
case CONTEXT_CONTROL_CRYPTO_ALG_SHA512:
if (safexcel_hmac_setkey("safexcel-sha512", keys.authkey,
- keys.authkeylen, &istate, &ostate))
- goto badkey;
+ keys.authkeylen, &state[0], &state[1]))
+ goto badkey_free;
break;
default:
dev_err(priv->dev, "aead: unsupported hash algorithm\n");
- goto badkey;
+ goto badkey_free;
}
crypto_aead_set_flags(ctfm, crypto_aead_get_flags(ctfm) &
CRYPTO_TFM_RES_MASK);
if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma &&
- (memcmp(ctx->ipad, istate.state, ctx->state_sz) ||
- memcmp(ctx->opad, ostate.state, ctx->state_sz)))
+ (memcmp(ctx->ipad, &state[0].state, ctx->state_sz) ||
+ memcmp(ctx->opad, &state[1].state, ctx->state_sz)))
ctx->base.needs_inv = true;
/* Now copy the keys into the context */
memcpy(ctx->key, keys.enckey, keys.enckeylen);
ctx->key_len = keys.enckeylen;
- memcpy(ctx->ipad, &istate.state, ctx->state_sz);
- memcpy(ctx->opad, &ostate.state, ctx->state_sz);
+ memcpy(ctx->ipad, &state[0].state, ctx->state_sz);
+ memcpy(ctx->opad, &state[1].state, ctx->state_sz);
memzero_explicit(&keys, sizeof(keys));
+ kfree(state);
+
return 0;
+badkey_free:
+ kfree(state);
badkey:
crypto_aead_set_flags(ctfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
badkey_expflags:
--
2.20.0
When both PCI and OF are disabled, no drivers are registered, and
we get some unused-function warnings:
drivers/crypto/inside-secure/safexcel.c:1221:13: error: unused function 'safexcel_unregister_algorithms' [-Werror,-Wunused-function]
static void safexcel_unregister_algorithms(struct safexcel_crypto_priv *priv)
drivers/crypto/inside-secure/safexcel.c:1307:12: error: unused function 'safexcel_probe_generic' [-Werror,-Wunused-function]
static int safexcel_probe_generic(void *pdev,
drivers/crypto/inside-secure/safexcel.c:1531:13: error: unused function 'safexcel_hw_reset_rings' [-Werror,-Wunused-function]
static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
It's better to make the compiler see what is going on and remove
such ifdef checks completely. In case of PCI, this is trivial since
pci_register_driver() is defined to an empty function that makes the
compiler subsequently drop all unused code silently.
The global pcireg_rc/ofreg_rc variables are not actually needed here
since the driver registration does not fail in ways that would make
it helpful.
For CONFIG_OF, an IS_ENABLED() check is still required, since platform
drivers can exist both with and without it.
A little change to linux/pci.h is needed to ensure that
pcim_enable_device() is visible to the driver. Moving the declaration
outside of ifdef would be sufficient here, but for consistency with the
rest of the file, adding an inline helper is probably best.
Fixes: 212ef6f29e5b ("crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/crypto/inside-secure/safexcel.c | 49 ++++++-------------------
include/linux/pci.h | 1 +
2 files changed, 13 insertions(+), 37 deletions(-)
diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index 311bf60df39f..c4e8fd27314c 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -1547,7 +1547,6 @@ static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
}
}
-#if IS_ENABLED(CONFIG_OF)
/* for Device Tree platform driver */
static int safexcel_probe(struct platform_device *pdev)
@@ -1666,9 +1665,7 @@ static struct platform_driver crypto_safexcel = {
.of_match_table = safexcel_of_match_table,
},
};
-#endif
-#if IS_ENABLED(CONFIG_PCI)
/* PCIE devices - i.e. Inside Secure development boards */
static int safexcel_pci_probe(struct pci_dev *pdev,
@@ -1789,54 +1786,32 @@ static struct pci_driver safexcel_pci_driver = {
.probe = safexcel_pci_probe,
.remove = safexcel_pci_remove,
};
-#endif
-
-/* Unfortunately, we have to resort to global variables here */
-#if IS_ENABLED(CONFIG_PCI)
-int pcireg_rc = -EINVAL; /* Default safe value */
-#endif
-#if IS_ENABLED(CONFIG_OF)
-int ofreg_rc = -EINVAL; /* Default safe value */
-#endif
static int __init safexcel_init(void)
{
-#if IS_ENABLED(CONFIG_PCI)
+ int ret;
+
/* Register PCI driver */
- pcireg_rc = pci_register_driver(&safexcel_pci_driver);
-#endif
+ ret = pci_register_driver(&safexcel_pci_driver);
-#if IS_ENABLED(CONFIG_OF)
/* Register platform driver */
- ofreg_rc = platform_driver_register(&crypto_safexcel);
- #if IS_ENABLED(CONFIG_PCI)
- /* Return success if either PCI or OF registered OK */
- return pcireg_rc ? ofreg_rc : 0;
- #else
- return ofreg_rc;
- #endif
-#else
- #if IS_ENABLED(CONFIG_PCI)
- return pcireg_rc;
- #else
- return -EINVAL;
- #endif
-#endif
+ if (IS_ENABLED(CONFIG_OF) && !ret) {
+ ret = platform_driver_register(&crypto_safexcel);
+ if (ret)
+ pci_unregister_driver(&safexcel_pci_driver);
+ }
+
+ return ret;
}
static void __exit safexcel_exit(void)
{
-#if IS_ENABLED(CONFIG_OF)
/* Unregister platform driver */
- if (!ofreg_rc)
+ if (IS_ENABLED(CONFIG_OF))
platform_driver_unregister(&crypto_safexcel);
-#endif
-#if IS_ENABLED(CONFIG_PCI)
/* Unregister PCI driver if successfully registered before */
- if (!pcireg_rc)
- pci_unregister_driver(&safexcel_pci_driver);
-#endif
+ pci_unregister_driver(&safexcel_pci_driver);
}
module_init(safexcel_init);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index f9088c89a534..1a6cf19eac2d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1686,6 +1686,7 @@ static inline struct pci_dev *pci_get_class(unsigned int class,
static inline void pci_set_master(struct pci_dev *dev) { }
static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
static inline void pci_disable_device(struct pci_dev *dev) { }
+static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
static inline int pci_assign_resource(struct pci_dev *dev, int i)
{ return -EBUSY; }
static inline int __pci_register_driver(struct pci_driver *drv,
--
2.20.0
> -----Original Message-----
> From: Arnd Bergmann <[email protected]>
> Sent: Monday, September 30, 2019 2:15 PM
> To: Antoine Tenart <[email protected]>; Herbert Xu <[email protected]>;
> David S. Miller <[email protected]>
> Cc: Arnd Bergmann <[email protected]>; Pascal Van Leeuwen <[email protected]>; Pascal
> van Leeuwen <[email protected]>; Ard Biesheuvel <[email protected]>; Eric Biggers
> <[email protected]>; [email protected]; [email protected]
> Subject: [PATCH 2/3] crypto: inside-secure - Reduce stack usage
>
> safexcel_aead_setkey() contains three large stack variables, totalling
> slightly more than the 1024 byte warning limit:
>
> drivers/crypto/inside-secure/safexcel_cipher.c:303:12: error: stack frame size of 1032 bytes
> in function 'safexcel_aead_setkey' [-Werror,-Wframe-larger-than=]
>
Ok, I did not realise that, so thanks for pointing that out to me.
> The function already contains a couple of dynamic allocations, so it is
> likely not performance critical and it can only be called in a context
> that allows sleeping, so the easiest workaround is to add change it
> to use dynamic allocations. Combining istate and ostate into a single
> variable simplifies the allocation at the cost of making it slightly
> less readable.
>
Hmmm... I wouldn't exactly consider it to be not performance critical - it
can be under certain circumstanced, but I guess it's already wasting lots
of cycles on allocations and key precomputes in safexcel_hmac_setkey, so
for now dynamically allocating the state is fine.
> Alternatively, it should be possible to shrink these allocations
> as the extra buffers appear to be largely unnecessary, but doing
> this would be a much more invasive change.
>
Actually, for HMAC-SHA512 you DO need all that buffer space.
You could shrink it to 2 * ctx->state_sz but then your simple indexing
is no longer going to fly. Not sure if that would be worth the effort.
I don't like the part where you dynamically allocate the cryto_aes_ctx
though, I think that was not necessary considering its a lot smaller.
And it conflicts with another change I have waiting that gets rid of
aes_expandkey and that struct alltogether (since it was really just
abused to do a key size check, which was very wasteful since the
function actually generates all roundkeys we don't need at all ...)
So I'll get rid of that struct anyway and doing it in this patch just
complicates applying your patch to my code or rebasing my stuff later.
> Fixes: 0e17e3621a28 ("crypto: inside-secure - add support for
> authenc(hmac(sha*),rfc3686(ctr(aes))) suites")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> .../crypto/inside-secure/safexcel_cipher.c | 53 ++++++++++++-------
> 1 file changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-
> secure/safexcel_cipher.c
> index ef51f8c2b473..51a4112aa9bc 100644
> --- a/drivers/crypto/inside-secure/safexcel_cipher.c
> +++ b/drivers/crypto/inside-secure/safexcel_cipher.c
> @@ -305,10 +305,10 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8
> *key,
> {
> struct crypto_tfm *tfm = crypto_aead_tfm(ctfm);
> struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm);
> - struct safexcel_ahash_export_state istate, ostate;
> + struct safexcel_ahash_export_state *state;
> struct safexcel_crypto_priv *priv = ctx->priv;
> + struct crypto_aes_ctx *aes;
> struct crypto_authenc_keys keys;
> - struct crypto_aes_ctx aes;
> int err = -EINVAL;
>
> if (crypto_authenc_extractkeys(&keys, key, len) != 0)
> @@ -334,7 +334,14 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8 *key,
> goto badkey_expflags;
> break;
> case SAFEXCEL_AES:
> - err = aes_expandkey(&aes, keys.enckey, keys.enckeylen);
> + aes = kzalloc(sizeof(*aes), GFP_KERNEL);
> + if (!aes) {
> + err = -ENOMEM;
> + goto badkey;
> + }
> +
> + err = aes_expandkey(aes, keys.enckey, keys.enckeylen);
> + kfree(aes);
> if (unlikely(err))
> goto badkey;
> break;
> @@ -347,56 +354,66 @@ static int safexcel_aead_setkey(struct crypto_aead *ctfm, const u8
> *key,
> memcmp(ctx->key, keys.enckey, keys.enckeylen))
> ctx->base.needs_inv = true;
>
> + state = kzalloc(sizeof(struct safexcel_ahash_export_state) * 2, GFP_KERNEL);
> + if (!state) {
> + err = -ENOMEM;
> + goto badkey;
> + }
> +
> /* Auth key */
> switch (ctx->hash_alg) {
> case CONTEXT_CONTROL_CRYPTO_ALG_SHA1:
> if (safexcel_hmac_setkey("safexcel-sha1", keys.authkey,
> - keys.authkeylen, &istate, &ostate))
> - goto badkey;
> + keys.authkeylen, &state[0], &state[1]))
> + goto badkey_free;
> break;
> case CONTEXT_CONTROL_CRYPTO_ALG_SHA224:
> if (safexcel_hmac_setkey("safexcel-sha224", keys.authkey,
> - keys.authkeylen, &istate, &ostate))
> - goto badkey;
> + keys.authkeylen, &state[0], &state[1]))
> + goto badkey_free;
> break;
> case CONTEXT_CONTROL_CRYPTO_ALG_SHA256:
> if (safexcel_hmac_setkey("safexcel-sha256", keys.authkey,
> - keys.authkeylen, &istate, &ostate))
> - goto badkey;
> + keys.authkeylen, &state[0], &state[1]))
> + goto badkey_free;
> break;
> case CONTEXT_CONTROL_CRYPTO_ALG_SHA384:
> if (safexcel_hmac_setkey("safexcel-sha384", keys.authkey,
> - keys.authkeylen, &istate, &ostate))
> - goto badkey;
> + keys.authkeylen, &state[0], &state[1]))
> + goto badkey_free;
> break;
> case CONTEXT_CONTROL_CRYPTO_ALG_SHA512:
> if (safexcel_hmac_setkey("safexcel-sha512", keys.authkey,
> - keys.authkeylen, &istate, &ostate))
> - goto badkey;
> + keys.authkeylen, &state[0], &state[1]))
> + goto badkey_free;
> break;
> default:
> dev_err(priv->dev, "aead: unsupported hash algorithm\n");
> - goto badkey;
> + goto badkey_free;
> }
>
> crypto_aead_set_flags(ctfm, crypto_aead_get_flags(ctfm) &
> CRYPTO_TFM_RES_MASK);
>
> if (priv->flags & EIP197_TRC_CACHE && ctx->base.ctxr_dma &&
> - (memcmp(ctx->ipad, istate.state, ctx->state_sz) ||
> - memcmp(ctx->opad, ostate.state, ctx->state_sz)))
> + (memcmp(ctx->ipad, &state[0].state, ctx->state_sz) ||
> + memcmp(ctx->opad, &state[1].state, ctx->state_sz)))
> ctx->base.needs_inv = true;
>
> /* Now copy the keys into the context */
> memcpy(ctx->key, keys.enckey, keys.enckeylen);
> ctx->key_len = keys.enckeylen;
>
> - memcpy(ctx->ipad, &istate.state, ctx->state_sz);
> - memcpy(ctx->opad, &ostate.state, ctx->state_sz);
> + memcpy(ctx->ipad, &state[0].state, ctx->state_sz);
> + memcpy(ctx->opad, &state[1].state, ctx->state_sz);
>
> memzero_explicit(&keys, sizeof(keys));
> + kfree(state);
> +
> return 0;
>
> +badkey_free:
> + kfree(state);
> badkey:
> crypto_aead_set_flags(ctfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
> badkey_expflags:
> --
> 2.20.0
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com
> -----Original Message-----
> From: Arnd Bergmann <[email protected]>
> Sent: Monday, September 30, 2019 10:12 PM
> To: Pascal Van Leeuwen <[email protected]>
> Cc: Antoine Tenart <[email protected]>; Herbert Xu <[email protected]>;
> David S. Miller <[email protected]>; Pascal van Leeuwen <[email protected]>; Ard
> Biesheuvel <[email protected]>; Eric Biggers <[email protected]>; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/3] crypto: inside-secure - Reduce stack usage
>
> On Mon, Sep 30, 2019 at 9:04 PM Pascal Van Leeuwen
> <[email protected]> wrote:
>
> > > Alternatively, it should be possible to shrink these allocations
> > > as the extra buffers appear to be largely unnecessary, but doing
> > > this would be a much more invasive change.
> > >
> > Actually, for HMAC-SHA512 you DO need all that buffer space.
> > You could shrink it to 2 * ctx->state_sz but then your simple indexing
> > is no longer going to fly. Not sure if that would be worth the effort.
>
> Stack allocations can no longer be dynamically sized in the kernel,
> so that would not work.
>
I was actually referring to your kzalloc, not to the original stack
based implementation ...
> What I noticed though is that the largest part of safexcel_ahash_export_state
> is used in the 'cache' member, and this is apparently only referenced inside of
> safexcel_hmac_init_iv, which you call twice. If that cache can be allocated
> only once, you save SHA512_BLOCK_SIZE bytes in one of the two paths.
>
Well ... hmmm ... "my"... This is not originally "my" code. And until you
brought this up, I did not fully realise it was using this export_state
struct to store those digests. Seems to have something to do with directly
taking the crypto_ahash_export state output, which is much more than that,
in case you need to continue the hash (which you don't here).
I guess you need to "catch" that output somewhere, so probably you could
save a bit of memory but I doubt it would be a significant improvement.
> > I don't like the part where you dynamically allocate the cryto_aes_ctx
> > though, I think that was not necessary considering its a lot smaller.
>
> I count 484 bytes for it, which is really large.
>
Maybe I should've counted myself ... totally unexpectedly huge. Why??
Anyway, glad I got rid of it already then.
> > And it conflicts with another change I have waiting that gets rid of
> > aes_expandkey and that struct alltogether (since it was really just
> > abused to do a key size check, which was very wasteful since the
> > function actually generates all roundkeys we don't need at all ...)
>
> Right, this is what I noticed there. With 480 of the 484 bytes gone,
> you are well below the warning limit even without the other change.
>
And by "other change" you mean the safexcel_ahash_export_state?
Ok, good to known, although I do like to improve that one as well,
but preferably by not exporting the cache so I don't need the full
struct.
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com
On Mon, Sep 30, 2019 at 02:14:35PM +0200, Arnd Bergmann wrote:
> When both PCI and OF are disabled, no drivers are registered, and
> we get some unused-function warnings:
>
> drivers/crypto/inside-secure/safexcel.c:1221:13: error: unused function 'safexcel_unregister_algorithms' [-Werror,-Wunused-function]
> static void safexcel_unregister_algorithms(struct safexcel_crypto_priv *priv)
> drivers/crypto/inside-secure/safexcel.c:1307:12: error: unused function 'safexcel_probe_generic' [-Werror,-Wunused-function]
> static int safexcel_probe_generic(void *pdev,
> drivers/crypto/inside-secure/safexcel.c:1531:13: error: unused function 'safexcel_hw_reset_rings' [-Werror,-Wunused-function]
> static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
>
> It's better to make the compiler see what is going on and remove
> such ifdef checks completely. In case of PCI, this is trivial since
> pci_register_driver() is defined to an empty function that makes the
> compiler subsequently drop all unused code silently.
>
> The global pcireg_rc/ofreg_rc variables are not actually needed here
> since the driver registration does not fail in ways that would make
> it helpful.
>
> For CONFIG_OF, an IS_ENABLED() check is still required, since platform
> drivers can exist both with and without it.
>
> A little change to linux/pci.h is needed to ensure that
> pcim_enable_device() is visible to the driver. Moving the declaration
> outside of ifdef would be sufficient here, but for consistency with the
> rest of the file, adding an inline helper is probably best.
>
> Fixes: 212ef6f29e5b ("crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/crypto/inside-secure/safexcel.c | 49 ++++++-------------------
> include/linux/pci.h | 1 +
> 2 files changed, 13 insertions(+), 37 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi Arnd,
Sorry for not responding earlier, but I've been very busy lately.
So I'm looking at this now for the first time.
> -----Original Message-----
> From: Arnd Bergmann <[email protected]>
> Sent: Monday, September 30, 2019 2:15 PM
> To: Antoine Tenart <[email protected]>; Herbert Xu <[email protected]>;
> David S. Miller <[email protected]>; Bjorn Helgaas <[email protected]>
> Cc: Arnd Bergmann <[email protected]>; Pascal Van Leeuwen <[email protected]>; Pascal van
> Leeuwen <[email protected]>; Kelsey Skunberg <[email protected]>; linux-
> [email protected]; [email protected]; [email protected]
> Subject: [PATCH 3/3] crypto: inside-secure - Remove #ifdef checks
>
> When both PCI and OF are disabled, no drivers are registered, and
> we get some unused-function warnings:
>
> drivers/crypto/inside-secure/safexcel.c:1221:13: error: unused function
> 'safexcel_unregister_algorithms' [-Werror,-Wunused-function]
> static void safexcel_unregister_algorithms(struct safexcel_crypto_priv *priv)
> drivers/crypto/inside-secure/safexcel.c:1307:12: error: unused function
> 'safexcel_probe_generic' [-Werror,-Wunused-function]
> static int safexcel_probe_generic(void *pdev,
> drivers/crypto/inside-secure/safexcel.c:1531:13: error: unused function
> 'safexcel_hw_reset_rings' [-Werror,-Wunused-function]
> static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
>
> It's better to make the compiler see what is going on and remove
> such ifdef checks completely. In case of PCI, this is trivial since
> pci_register_driver() is defined to an empty function that makes the
> compiler subsequently drop all unused code silently.
>
> The global pcireg_rc/ofreg_rc variables are not actually needed here
> since the driver registration does not fail in ways that would make
> it helpful.
>
> For CONFIG_OF, an IS_ENABLED() check is still required, since platform
> drivers can exist both with and without it.
>
> A little change to linux/pci.h is needed to ensure that
> pcim_enable_device() is visible to the driver. Moving the declaration
> outside of ifdef would be sufficient here, but for consistency with the
> rest of the file, adding an inline helper is probably best.
>
> Fixes: 212ef6f29e5b ("crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/crypto/inside-secure/safexcel.c | 49 ++++++-------------------
> include/linux/pci.h | 1 +
> 2 files changed, 13 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
> index 311bf60df39f..c4e8fd27314c 100644
> --- a/drivers/crypto/inside-secure/safexcel.c
> +++ b/drivers/crypto/inside-secure/safexcel.c
> @@ -1547,7 +1547,6 @@ static void safexcel_hw_reset_rings(struct safexcel_crypto_priv *priv)
> }
> }
>
> -#if IS_ENABLED(CONFIG_OF)
> /* for Device Tree platform driver */
>
> static int safexcel_probe(struct platform_device *pdev)
> @@ -1666,9 +1665,7 @@ static struct platform_driver crypto_safexcel = {
> .of_match_table = safexcel_of_match_table,
> },
> };
> -#endif
>
> -#if IS_ENABLED(CONFIG_PCI)
> /* PCIE devices - i.e. Inside Secure development boards */
>
> static int safexcel_pci_probe(struct pci_dev *pdev,
> @@ -1789,54 +1786,32 @@ static struct pci_driver safexcel_pci_driver = {
> .probe = safexcel_pci_probe,
> .remove = safexcel_pci_remove,
> };
> -#endif
> -
> -/* Unfortunately, we have to resort to global variables here */
> -#if IS_ENABLED(CONFIG_PCI)
> -int pcireg_rc = -EINVAL; /* Default safe value */
> -#endif
> -#if IS_ENABLED(CONFIG_OF)
> -int ofreg_rc = -EINVAL; /* Default safe value */
> -#endif
>
> static int __init safexcel_init(void)
> {
> -#if IS_ENABLED(CONFIG_PCI)
> + int ret;
> +
> /* Register PCI driver */
> - pcireg_rc = pci_register_driver(&safexcel_pci_driver);
> -#endif
> + ret = pci_register_driver(&safexcel_pci_driver);
>
> -#if IS_ENABLED(CONFIG_OF)
> /* Register platform driver */
> - ofreg_rc = platform_driver_register(&crypto_safexcel);
> - #if IS_ENABLED(CONFIG_PCI)
> - /* Return success if either PCI or OF registered OK */
> - return pcireg_rc ? ofreg_rc : 0;
> - #else
> - return ofreg_rc;
> - #endif
> -#else
> - #if IS_ENABLED(CONFIG_PCI)
> - return pcireg_rc;
> - #else
> - return -EINVAL;
> - #endif
> -#endif
> + if (IS_ENABLED(CONFIG_OF) && !ret) {
>
Hmm ... this would make it skip the OF registration if the PCIE
registration failed. Note that the typical embedded system will
have a PCIE subsystem (e.g. Marvell A7K/A8K does) but will have
the EIP embedded on the SoC as an OF device.
So the question is: is it possible somehow that PCIE registration
fails while OF registration does pass? Because in that case, this
code would be wrong ...
Other than that, I don't care much how this code is implemented
as long as it works for both my use cases, being an OF embedded
device (on a SoC _with_ or _without_ PCIE support) and a device
on a PCIE board in a PCI (which has both PCIE and OF support).
> + ret = platform_driver_register(&crypto_safexcel);
> + if (ret)
> + pci_unregister_driver(&safexcel_pci_driver);
> + }
> +
> + return ret;
> }
>
> static void __exit safexcel_exit(void)
> {
> -#if IS_ENABLED(CONFIG_OF)
> /* Unregister platform driver */
> - if (!ofreg_rc)
> + if (IS_ENABLED(CONFIG_OF))
> platform_driver_unregister(&crypto_safexcel);
> -#endif
>
> -#if IS_ENABLED(CONFIG_PCI)
> /* Unregister PCI driver if successfully registered before */
> - if (!pcireg_rc)
> - pci_unregister_driver(&safexcel_pci_driver);
> -#endif
> + pci_unregister_driver(&safexcel_pci_driver);
> }
>
> module_init(safexcel_init);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f9088c89a534..1a6cf19eac2d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1686,6 +1686,7 @@ static inline struct pci_dev *pci_get_class(unsigned int class,
> static inline void pci_set_master(struct pci_dev *dev) { }
> static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
> static inline void pci_disable_device(struct pci_dev *dev) { }
> +static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
> static inline int pci_assign_resource(struct pci_dev *dev, int i)
> { return -EBUSY; }
> static inline int __pci_register_driver(struct pci_driver *drv,
> --
> 2.20.0
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com
On Thu, Oct 17, 2019 at 3:26 PM Pascal Van Leeuwen
<[email protected]> wrote:
> > /* Register PCI driver */
> > - pcireg_rc = pci_register_driver(&safexcel_pci_driver);
> > -#endif
> > + ret = pci_register_driver(&safexcel_pci_driver);
> >
> > -#if IS_ENABLED(CONFIG_OF)
> > /* Register platform driver */
> > - ofreg_rc = platform_driver_register(&crypto_safexcel);
> > - #if IS_ENABLED(CONFIG_PCI)
> > - /* Return success if either PCI or OF registered OK */
> > - return pcireg_rc ? ofreg_rc : 0;
> > - #else
> > - return ofreg_rc;
> > - #endif
> > -#else
> > - #if IS_ENABLED(CONFIG_PCI)
> > - return pcireg_rc;
> > - #else
> > - return -EINVAL;
> > - #endif
> > -#endif
> > + if (IS_ENABLED(CONFIG_OF) && !ret) {
> >
> Hmm ... this would make it skip the OF registration if the PCIE
> registration failed. Note that the typical embedded system will
> have a PCIE subsystem (e.g. Marvell A7K/A8K does) but will have
> the EIP embedded on the SoC as an OF device.
>
> So the question is: is it possible somehow that PCIE registration
> fails while OF registration does pass? Because in that case, this
> code would be wrong ...
I don't see how it would fail. When CONFIG_PCI is disabled,
pci_register_driver() does nothing, and the pci_driver as well
as everything referenced from it will be silently dropped from
the object file.
If CONFIG_PCI is enabled, then the driver will be registered
to the PCI subsystem, waiting for a device to show up, but
the driver registration does not care about whether there is
such a device.
> Other than that, I don't care much how this code is implemented
> as long as it works for both my use cases, being an OF embedded
> device (on a SoC _with_ or _without_ PCIE support) and a device
> on a PCIE board in a PCI (which has both PCIE and OF support).
Yes, that should be fine. There are a lot of drivers that support
multiple bus interfaces, and this is the normal way to handle them.
Arnd
> -----Original Message-----
> From: Arnd Bergmann <[email protected]>
> Sent: Thursday, October 17, 2019 3:48 PM
> To: Pascal Van Leeuwen <[email protected]>
> Cc: Antoine Tenart <[email protected]>; Herbert Xu <[email protected]>;
> David S. Miller <[email protected]>; Bjorn Helgaas <[email protected]>; Pascal van Leeuwen
> <[email protected]>; Kelsey Skunberg <[email protected]>; linux-
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH 3/3] crypto: inside-secure - Remove #ifdef checks
>
> On Thu, Oct 17, 2019 at 3:26 PM Pascal Van Leeuwen
> <[email protected]> wrote:
>
> > > /* Register PCI driver */
> > > - pcireg_rc = pci_register_driver(&safexcel_pci_driver);
> > > -#endif
> > > + ret = pci_register_driver(&safexcel_pci_driver);
> > >
> > > -#if IS_ENABLED(CONFIG_OF)
> > > /* Register platform driver */
> > > - ofreg_rc = platform_driver_register(&crypto_safexcel);
> > > - #if IS_ENABLED(CONFIG_PCI)
> > > - /* Return success if either PCI or OF registered OK */
> > > - return pcireg_rc ? ofreg_rc : 0;
> > > - #else
> > > - return ofreg_rc;
> > > - #endif
> > > -#else
> > > - #if IS_ENABLED(CONFIG_PCI)
> > > - return pcireg_rc;
> > > - #else
> > > - return -EINVAL;
> > > - #endif
> > > -#endif
> > > + if (IS_ENABLED(CONFIG_OF) && !ret) {
> > >
> > Hmm ... this would make it skip the OF registration if the PCIE
> > registration failed. Note that the typical embedded system will
> > have a PCIE subsystem (e.g. Marvell A7K/A8K does) but will have
> > the EIP embedded on the SoC as an OF device.
> >
> > So the question is: is it possible somehow that PCIE registration
> > fails while OF registration does pass? Because in that case, this
> > code would be wrong ...
>
> I don't see how it would fail. When CONFIG_PCI is disabled,
> pci_register_driver() does nothing, and the pci_driver as well
> as everything referenced from it will be silently dropped from
> the object file.
> If CONFIG_PCI is enabled, then the driver will be registered
> to the PCI subsystem, waiting for a device to show up, but
> the driver registration does not care about whether there is
> such a device.
>
I know it does not care about the device being present or not.
I was just worried some issue with the PCIE subsystem would propagate
to (unrelated) OF device use this way. But I have no idea on the exact
ways PCIE registration may fail. If it is because of lack of memory,
I assume that subsequent OF device registration would fail as well.
So maybe I'm worried about an issue that doesn't really exist.
> > Other than that, I don't care much how this code is implemented
> > as long as it works for both my use cases, being an OF embedded
> > device (on a SoC _with_ or _without_ PCIE support) and a device
> > on a PCIE board in a PCI (which has both PCIE and OF support).
>
> Yes, that should be fine. There are a lot of drivers that support
> multiple bus interfaces, and this is the normal way to handle them.
>
Ok, if this is the "normal way to handle this" and a lot of other
drivers do it the same way, then I'm OK with that ...
I already verified it works correctly for my specific use cases.
Thanks.
> Arnd
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
http://www.insidesecure.com