| Message ID | 20190930121520.1388317-3-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | None | expand |
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 <arnd@arndb.de> > --- > drivers/crypto/inside-secure/safexcel.c | 49 ++++++------------------- > include/linux/pci.h | 1 + > 2 files changed, 13 insertions(+), 37 deletions(-) > ... > 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; } I would have used "dev" here to match surrounding stubs, but either way: Acked-by: Bjorn Helgaas <bhelgaas@google.com> # pci.h > 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 >
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 <arnd@arndb.de> > --- > drivers/crypto/inside-secure/safexcel.c | 49 ++++++------------------- > include/linux/pci.h | 1 + > 2 files changed, 13 insertions(+), 37 deletions(-) Patch applied. Thanks.
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@arndb.de> > Sent: Monday, September 30, 2019 2:15 PM > To: Antoine Tenart <antoine.tenart@bootlin.com>; Herbert Xu <herbert@gondor.apana.org.au>; > David S. Miller <davem@davemloft.net>; Bjorn Helgaas <bhelgaas@google.com> > Cc: Arnd Bergmann <arnd@arndb.de>; Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>; Pascal van > Leeuwen <pascalvanl@gmail.com>; Kelsey Skunberg <skunberg.kelsey@gmail.com>; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org > 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@arndb.de> > --- > 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
On Thu, Oct 17, 2019 at 3:26 PM Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> 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 <arnd@arndb.de> > Sent: Thursday, October 17, 2019 3:48 PM > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> > Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Herbert Xu <herbert@gondor.apana.org.au>; > David S. Miller <davem@davemloft.net>; Bjorn Helgaas <bhelgaas@google.com>; Pascal van Leeuwen > <pascalvanl@gmail.com>; Kelsey Skunberg <skunberg.kelsey@gmail.com>; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org > Subject: Re: [PATCH 3/3] crypto: inside-secure - Remove #ifdef checks > > On Thu, Oct 17, 2019 at 3:26 PM Pascal Van Leeuwen > <pvanleeuwen@verimatrix.com> 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 www.insidesecure.com
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,
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@arndb.de> --- drivers/crypto/inside-secure/safexcel.c | 49 ++++++------------------- include/linux/pci.h | 1 + 2 files changed, 13 insertions(+), 37 deletions(-)