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 <arnd@xxxxxxxx> > Sent: Monday, September 30, 2019 2:15 PM > To: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>; Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>; > David S. Miller <davem@xxxxxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > Cc: Arnd Bergmann <arnd@xxxxxxxx>; Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxx>; Pascal van > Leeuwen <pascalvanl@xxxxxxxxx>; Kelsey Skunberg <skunberg.kelsey@xxxxxxxxx>; linux- > crypto@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx > 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 <arnd@xxxxxxxx> > --- > 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 www.insidesecure.com