[PATCHv2] crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n

[PATCHv2] crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n

[Pascal van Leeuwen]

This patch fixes an unused variable warning from the compiler when the
driver is being compiled without PCI support in the kernel.

changes since v1:
- capture the platform_register_driver error code as well
- actually return the (last) error code
- swapped registration to do PCI first as that's just for development
  boards anyway, so in case both are done we want the platform error
  or no error at all if that passes
- also fixes some indentation issue in the affected code

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/inside-secure/safexcel.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index e12a2a3..2331b31 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -1505,29 +1505,29 @@ static int __init safexcel_init(void)
 {
 	int rc;
 
-#if IS_ENABLED(CONFIG_OF)
-		/* Register platform driver */
-		platform_driver_register(&crypto_safexcel);
+#if IS_ENABLED(CONFIG_PCI)
+	/* Register PCI driver */
+	rc = pci_register_driver(&safexcel_pci_driver);
 #endif
 
-#if IS_ENABLED(CONFIG_PCI)
-		/* Register PCI driver */
-		rc = pci_register_driver(&safexcel_pci_driver);
+#if IS_ENABLED(CONFIG_OF)
+	/* Register platform driver */
+	rc = platform_driver_register(&crypto_safexcel);
 #endif
 
-	return 0;
+	return rc;
 }
 
 static void __exit safexcel_exit(void)
 {
 #if IS_ENABLED(CONFIG_OF)
-		/* Unregister platform driver */
-		platform_driver_unregister(&crypto_safexcel);
+	/* Unregister platform driver */
+	platform_driver_unregister(&crypto_safexcel);
 #endif
 
 #if IS_ENABLED(CONFIG_PCI)
-		/* Unregister PCI driver if successfully registered before */
-		pci_unregister_driver(&safexcel_pci_driver);
+	/* Unregister PCI driver if successfully registered before */
+	pci_unregister_driver(&safexcel_pci_driver);
 #endif
 }
 
-- 
1.8.3.1

Re: [PATCHv2] crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n

[Herbert Xu]

On Fri, Sep 06, 2019 at 10:07:23AM +0200, Pascal van Leeuwen wrote:
>
> diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
> index e12a2a3..2331b31 100644
> --- a/drivers/crypto/inside-secure/safexcel.c
> +++ b/drivers/crypto/inside-secure/safexcel.c
> @@ -1505,29 +1505,29 @@ static int __init safexcel_init(void)
>  {
>  	int rc;
>  
> -#if IS_ENABLED(CONFIG_OF)
> -		/* Register platform driver */
> -		platform_driver_register(&crypto_safexcel);
> +#if IS_ENABLED(CONFIG_PCI)
> +	/* Register PCI driver */
> +	rc = pci_register_driver(&safexcel_pci_driver);
>  #endif
>  
> -#if IS_ENABLED(CONFIG_PCI)
> -		/* Register PCI driver */
> -		rc = pci_register_driver(&safexcel_pci_driver);
> +#if IS_ENABLED(CONFIG_OF)
> +	/* Register platform driver */
> +	rc = platform_driver_register(&crypto_safexcel);
>  #endif
>  
> -	return 0;
> +	return rc;
>  }

According to the Kconfig it is theoretically possible for both
PCI and OF to be off (with COMPILE_TEST enabled).  So you should
add an rc = 0 at the top.

You also need to check rc after each registration and abort if
an error is detected.  After the second step, aborting would
also require unwinding the first step.

So something like:

	int rc = 0;

#if IS_ENABLED(CONFIG_PCI)
	/* Register PCI driver */
	rc = pci_register_driver(&safexcel_pci_driver);
#endif
	if (rc)
		goto out;

#if IS_ENABLED(CONFIG_OF)
	/* Register platform driver */
	rc = platform_driver_register(&crypto_safexcel);
#endif
	if (rc)
		goto undo_pci;

undo_pci:
#if IS_ENABLED(CONFIG_PCI)
	pci_unregister_driver(&safexcel_pci_driver);
#endif
out:
	return rc;

As you can see, these ifdefs get out-of-control pretty quickly.
In fact, we can remove all the CONFIG_PCI ifdefs by adding just
one more stub function in pci.h for pcim_enable_device.

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: [PATCHv2] crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n

[Pascal Van Leeuwen]

> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf
> Of Herbert Xu
> Sent: Friday, September 6, 2019 2:19 PM
> To: Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; antoine.tenart@bootlin.com; davem@davemloft.net;
> Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>; Bjorn Helgaas <helgaas@kernel.org>
> Subject: Re: [PATCHv2] crypto: inside-secure - Fix unused variable warning when
> CONFIG_PCI=n
> 
> On Fri, Sep 06, 2019 at 10:07:23AM +0200, Pascal van Leeuwen wrote:
> >
> > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-
> secure/safexcel.c
> > index e12a2a3..2331b31 100644
> > --- a/drivers/crypto/inside-secure/safexcel.c
> > +++ b/drivers/crypto/inside-secure/safexcel.c
> > @@ -1505,29 +1505,29 @@ static int __init safexcel_init(void)
> >  {
> >  	int rc;
> >
> > -#if IS_ENABLED(CONFIG_OF)
> > -		/* Register platform driver */
> > -		platform_driver_register(&crypto_safexcel);
> > +#if IS_ENABLED(CONFIG_PCI)
> > +	/* Register PCI driver */
> > +	rc = pci_register_driver(&safexcel_pci_driver);
> >  #endif
> >
> > -#if IS_ENABLED(CONFIG_PCI)
> > -		/* Register PCI driver */
> > -		rc = pci_register_driver(&safexcel_pci_driver);
> > +#if IS_ENABLED(CONFIG_OF)
> > +	/* Register platform driver */
> > +	rc = platform_driver_register(&crypto_safexcel);
> >  #endif
> >
> > -	return 0;
> > +	return rc;
> >  }
> 
> According to the Kconfig it is theoretically possible for both
> PCI and OF to be off (with COMPILE_TEST enabled).  So you should
> add an rc = 0 at the top.
> 
Ok

> You also need to check rc after each registration and abort if
> an error is detected.  After the second step, aborting would
> also require unwinding the first step.
> 
I explicitly DON'T want to abort if the PCI registration fails,
since that may be irrelevant if the OF registration passes AND
the device actually happens to be Device Tree.
So not checking the result value is on purpose here.

> So something like:
> 
> 	int rc = 0;
> 
> #if IS_ENABLED(CONFIG_PCI)
> 	/* Register PCI driver */
> 	rc = pci_register_driver(&safexcel_pci_driver);
> #endif
> 	if (rc)
> 		goto out;
> 
> #if IS_ENABLED(CONFIG_OF)
> 	/* Register platform driver */
> 	rc = platform_driver_register(&crypto_safexcel);
> #endif
> 	if (rc)
> 		goto undo_pci;
> 
> undo_pci:
> #if IS_ENABLED(CONFIG_PCI)
> 	pci_unregister_driver(&safexcel_pci_driver);
> #endif
> out:
> 	return rc;
> 
> As you can see, these ifdefs get out-of-control pretty quickly.
> In fact, we can remove all the CONFIG_PCI ifdefs by adding just
> one more stub function in pci.h for pcim_enable_device.
> 
I'm fine with that, too. I did not want these ifdef's in the first
place, I was asked to put them in.

> 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

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com

Re: [PATCHv2] crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n

[Herbert Xu]

On Fri, Sep 06, 2019 at 01:01:19PM +0000, Pascal Van Leeuwen wrote:
>
> I explicitly DON'T want to abort if the PCI registration fails,
> since that may be irrelevant if the OF registration passes AND
> the device actually happens to be Device Tree.
> So not checking the result value is on purpose here.

Well if you want to support that you'll need to remember whether
PCI registration succeeded or not before unregistering it in the
exit function.

Cheers,
-- 
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: [PATCHv2] crypto: inside-secure - Fix unused variable warning when CONFIG_PCI=n

[Pascal Van Leeuwen]

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Friday, September 6, 2019 3:05 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Pascal van Leeuwen <pascalvanl@gmail.com>; linux-crypto@vger.kernel.org;
> antoine.tenart@bootlin.com; davem@davemloft.net; Bjorn Helgaas <helgaas@kernel.org>
> Subject: Re: [PATCHv2] crypto: inside-secure - Fix unused variable warning when
> CONFIG_PCI=n
> 
> On Fri, Sep 06, 2019 at 01:01:19PM +0000, Pascal Van Leeuwen wrote:
> >
> > I explicitly DON'T want to abort if the PCI registration fails,
> > since that may be irrelevant if the OF registration passes AND
> > the device actually happens to be Device Tree.
> > So not checking the result value is on purpose here.
> 
> Well if you want to support that you'll need to remember whether
> PCI registration succeeded or not before unregistering it in the
> exit function.
> 
Hmmm, actually I was assuming those unregister calls to be effective
nops if no matching register succeeded previously. Like free() is a 
NOP if you pass it a null pointr.

If that is not the case, then I indeed need to remember which call(s)
passed, such that I only unregister those. But since the exit()
function does not take any parameters, I don't see any way of doing
that without using some global variable ...

> Cheers,
> --
> 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


Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com