| Message ID | 20190903014518.20880-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2,-next] crypto: inside-secure - Fix build error without CONFIG_PCI | expand |
> -----Original Message----- > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of > YueHaibing > Sent: Tuesday, September 3, 2019 3:45 AM > To: antoine.tenart@bootlin.com; herbert@gondor.apana.org.au; davem@davemloft.net; > pvanleeuwen@insidesecure.com > Cc: linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org; YueHaibing > <yuehaibing@huawei.com> > Subject: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI > > If CONFIG_PCI is not set, building fails: > > rivers/crypto/inside-secure/safexcel.c: In function safexcel_request_ring_irq: > drivers/crypto/inside-secure/safexcel.c:944:9: error: implicit declaration of function > pci_irq_vector; > did you mean rcu_irq_enter? [-Werror=implicit-function-declaration] > irq = pci_irq_vector(pci_pdev, irqid); > ^~~~~~~~~~~~~~ > > Use #ifdef block to guard this. > Actually, this is interesting. My *original* implementation was using straight #ifdefs, but then I got review feedback stating that I should not do that, as it's not compile testable, suggesting to use regular C if's instead. Then there was quite some back-and-forth on the actual implementation and I ended up with this. So now it turns out that doesn't work and I'm suggested to go full-circle back to straight #ifdef's? Or is there some other way to make this work? Because I don't know where to go from here ... > Fixes: 625f269a5a7a ("crypto: inside-secure - add support for PCI based FPGA development > board") > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > v2: use 'ifdef' instead of 'IS_ENABLED' > --- > drivers/crypto/inside-secure/safexcel.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside- > secure/safexcel.c > index e12a2a3..5253900 100644 > --- a/drivers/crypto/inside-secure/safexcel.c > +++ b/drivers/crypto/inside-secure/safexcel.c > @@ -937,7 +937,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > int ret, irq; > struct device *dev; > > - if (IS_ENABLED(CONFIG_PCI) && is_pci_dev) { > +#ifdef CONFIG_PCI > + if (is_pci_dev) { > struct pci_dev *pci_pdev = pdev; > > dev = &pci_pdev->dev; > @@ -947,7 +948,10 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > irqid, irq); > return irq; > } > - } else if (IS_ENABLED(CONFIG_OF)) { > + } else > +#endif > + { > +#ifdef CONFIG_OF > struct platform_device *plf_pdev = pdev; > char irq_name[6] = {0}; /* "ringX\0" */ > > @@ -960,6 +964,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > irq_name, irq); > return irq; > } > +#endif > } > > ret = devm_request_threaded_irq(dev, irq, handler, > @@ -1137,7 +1142,8 @@ static int safexcel_probe_generic(void *pdev, > > safexcel_configure(priv); > > - if (IS_ENABLED(CONFIG_PCI) && priv->version == EIP197_DEVBRD) { > +#ifdef CONFIG_PCI > + if (priv->version == EIP197_DEVBRD) { > /* > * Request MSI vectors for global + 1 per ring - > * or just 1 for older dev images > @@ -1153,6 +1159,7 @@ static int safexcel_probe_generic(void *pdev, > return ret; > } > } > +#endif > > /* Register the ring IRQ handlers and configure the rings */ > priv->ring = devm_kcalloc(dev, priv->config.rings, > -- > 2.7.4 > Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
On Wed, 4 Sep 2019 at 04:57, Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> wrote: > > > > -----Original Message----- > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of > > YueHaibing > > Sent: Tuesday, September 3, 2019 3:45 AM > > To: antoine.tenart@bootlin.com; herbert@gondor.apana.org.au; davem@davemloft.net; > > pvanleeuwen@insidesecure.com > > Cc: linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org; YueHaibing > > <yuehaibing@huawei.com> > > Subject: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI > > > > If CONFIG_PCI is not set, building fails: > > > > rivers/crypto/inside-secure/safexcel.c: In function safexcel_request_ring_irq: > > drivers/crypto/inside-secure/safexcel.c:944:9: error: implicit declaration of function > > pci_irq_vector; > > did you mean rcu_irq_enter? [-Werror=implicit-function-declaration] > > irq = pci_irq_vector(pci_pdev, irqid); > > ^~~~~~~~~~~~~~ > > > > Use #ifdef block to guard this. > > > Actually, this is interesting. My *original* implementation was using > straight #ifdefs, but then I got review feedback stating that I should not > do that, as it's not compile testable, suggesting to use regular C if's > instead. Then there was quite some back-and-forth on the actual > implementation and I ended up with this. > > So now it turns out that doesn't work and I'm suggested to go full-circle > back to straight #ifdef's? Or is there some other way to make this work? > Because I don't know where to go from here ... > C conditionals are preferred over preprocessor conditional, but if the conditional code refers to symbols that are not declared when the Kconfig symbol is not defined, preprocessor conditionals are the only option. This is the reason we have so many empty static inline functions in header files - it ensures that the symbols are declared even if the only invocations are from dead code. > > Fixes: 625f269a5a7a ("crypto: inside-secure - add support for PCI based FPGA development > > board") > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > --- > > v2: use 'ifdef' instead of 'IS_ENABLED' > > --- > > drivers/crypto/inside-secure/safexcel.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside- > > secure/safexcel.c > > index e12a2a3..5253900 100644 > > --- a/drivers/crypto/inside-secure/safexcel.c > > +++ b/drivers/crypto/inside-secure/safexcel.c > > @@ -937,7 +937,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > int ret, irq; > > struct device *dev; > > > > - if (IS_ENABLED(CONFIG_PCI) && is_pci_dev) { > > +#ifdef CONFIG_PCI > > + if (is_pci_dev) { > > struct pci_dev *pci_pdev = pdev; > > > > dev = &pci_pdev->dev; > > @@ -947,7 +948,10 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > irqid, irq); > > return irq; > > } > > - } else if (IS_ENABLED(CONFIG_OF)) { > > + } else > > +#endif > > + { > > +#ifdef CONFIG_OF > > struct platform_device *plf_pdev = pdev; > > char irq_name[6] = {0}; /* "ringX\0" */ > > > > @@ -960,6 +964,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > irq_name, irq); > > return irq; > > } > > +#endif > > } > > > > ret = devm_request_threaded_irq(dev, irq, handler, > > @@ -1137,7 +1142,8 @@ static int safexcel_probe_generic(void *pdev, > > > > safexcel_configure(priv); > > > > - if (IS_ENABLED(CONFIG_PCI) && priv->version == EIP197_DEVBRD) { > > +#ifdef CONFIG_PCI > > + if (priv->version == EIP197_DEVBRD) { > > /* > > * Request MSI vectors for global + 1 per ring - > > * or just 1 for older dev images > > @@ -1153,6 +1159,7 @@ static int safexcel_probe_generic(void *pdev, > > return ret; > > } > > } > > +#endif > > > > /* Register the ring IRQ handlers and configure the rings */ > > priv->ring = devm_kcalloc(dev, priv->config.rings, > > -- > > 2.7.4 > > > > Regards, > Pascal van Leeuwen > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > www.insidesecure.com
> -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Sent: Wednesday, September 4, 2019 2:11 PM > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> > Cc: YueHaibing <yuehaibing@huawei.com>; antoine.tenart@bootlin.com; > herbert@gondor.apana.org.au; davem@davemloft.net; pvanleeuwen@insidesecure.com; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI > > On Wed, 4 Sep 2019 at 04:57, Pascal Van Leeuwen > <pvanleeuwen@verimatrix.com> wrote: > > > > > > > -----Original Message----- > > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On > Behalf Of > > > YueHaibing > > > Sent: Tuesday, September 3, 2019 3:45 AM > > > To: antoine.tenart@bootlin.com; herbert@gondor.apana.org.au; davem@davemloft.net; > > > pvanleeuwen@insidesecure.com > > > Cc: linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org; YueHaibing > > > <yuehaibing@huawei.com> > > > Subject: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI > > > > > > If CONFIG_PCI is not set, building fails: > > > > > > rivers/crypto/inside-secure/safexcel.c: In function safexcel_request_ring_irq: > > > drivers/crypto/inside-secure/safexcel.c:944:9: error: implicit declaration of function > > > pci_irq_vector; > > > did you mean rcu_irq_enter? [-Werror=implicit-function-declaration] > > > irq = pci_irq_vector(pci_pdev, irqid); > > > ^~~~~~~~~~~~~~ > > > > > > Use #ifdef block to guard this. > > > > > Actually, this is interesting. My *original* implementation was using > > straight #ifdefs, but then I got review feedback stating that I should not > > do that, as it's not compile testable, suggesting to use regular C if's > > instead. Then there was quite some back-and-forth on the actual > > implementation and I ended up with this. > > > > So now it turns out that doesn't work and I'm suggested to go full-circle > > back to straight #ifdef's? Or is there some other way to make this work? > > Because I don't know where to go from here ... > > > > > C conditionals are preferred over preprocessor conditional, but if the > conditional code refers to symbols that are not declared when the > Kconfig symbol is not defined, preprocessor conditionals are the only > option. > Sure, I get that. But I *had* the #ifdef's and then other people told me to get rid of them. How is one supposed to know when which symbols are declared exactly? Moreover, I feel that if #ifdef's are sometimes the only way, then you should be careful providing feedback on the subject. > This is the reason we have so many empty static inline functions in > header files - it ensures that the symbols are declared even if the > only invocations are from dead code. > This ties back into my previous question: how am I supposed to know whether stuff is nicely covered by these empty static inlines or not? If this happens to be a hit-and-miss affair. Note that I tested the code with the 2 platforms at my disposal - actually the only 2 relevant platforms for this driver, if you ask me - and they both compiled just fine, so I had no way of finding this "problem" myself. > > > > Fixes: 625f269a5a7a ("crypto: inside-secure - add support for PCI based FPGA > development > > > board") > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > --- > > > v2: use 'ifdef' instead of 'IS_ENABLED' > > > --- > > > drivers/crypto/inside-secure/safexcel.c | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside- > > > secure/safexcel.c > > > index e12a2a3..5253900 100644 > > > --- a/drivers/crypto/inside-secure/safexcel.c > > > +++ b/drivers/crypto/inside-secure/safexcel.c > > > @@ -937,7 +937,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > int ret, irq; > > > struct device *dev; > > > > > > - if (IS_ENABLED(CONFIG_PCI) && is_pci_dev) { > > > +#ifdef CONFIG_PCI > > > + if (is_pci_dev) { > > > struct pci_dev *pci_pdev = pdev; > > > > > > dev = &pci_pdev->dev; > > > @@ -947,7 +948,10 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > irqid, irq); > > > return irq; > > > } > > > - } else if (IS_ENABLED(CONFIG_OF)) { > > > + } else > > > +#endif > > > + { > > > +#ifdef CONFIG_OF > > > struct platform_device *plf_pdev = pdev; > > > char irq_name[6] = {0}; /* "ringX\0" */ > > > > > > @@ -960,6 +964,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > irq_name, irq); > > > return irq; > > > } > > > +#endif > > > } > > > > > > ret = devm_request_threaded_irq(dev, irq, handler, > > > @@ -1137,7 +1142,8 @@ static int safexcel_probe_generic(void *pdev, > > > > > > safexcel_configure(priv); > > > > > > - if (IS_ENABLED(CONFIG_PCI) && priv->version == EIP197_DEVBRD) { > > > +#ifdef CONFIG_PCI > > > + if (priv->version == EIP197_DEVBRD) { > > > /* > > > * Request MSI vectors for global + 1 per ring - > > > * or just 1 for older dev images > > > @@ -1153,6 +1159,7 @@ static int safexcel_probe_generic(void *pdev, > > > return ret; > > > } > > > } > > > +#endif > > > > > > /* Register the ring IRQ handlers and configure the rings */ > > > priv->ring = devm_kcalloc(dev, priv->config.rings, > > > -- > > > 2.7.4 > > > > > > > Regards, > > Pascal van Leeuwen > > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > > www.insidesecure.com Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
On Wed, 4 Sep 2019 at 05:25, Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> wrote: > > > -----Original Message----- > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Sent: Wednesday, September 4, 2019 2:11 PM > > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> > > Cc: YueHaibing <yuehaibing@huawei.com>; antoine.tenart@bootlin.com; > > herbert@gondor.apana.org.au; davem@davemloft.net; pvanleeuwen@insidesecure.com; linux- > > crypto@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI > > > > On Wed, 4 Sep 2019 at 04:57, Pascal Van Leeuwen > > <pvanleeuwen@verimatrix.com> wrote: > > > > > > > > > > -----Original Message----- > > > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On > > Behalf Of > > > > YueHaibing > > > > Sent: Tuesday, September 3, 2019 3:45 AM > > > > To: antoine.tenart@bootlin.com; herbert@gondor.apana.org.au; davem@davemloft.net; > > > > pvanleeuwen@insidesecure.com > > > > Cc: linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org; YueHaibing > > > > <yuehaibing@huawei.com> > > > > Subject: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI > > > > > > > > If CONFIG_PCI is not set, building fails: > > > > > > > > rivers/crypto/inside-secure/safexcel.c: In function safexcel_request_ring_irq: > > > > drivers/crypto/inside-secure/safexcel.c:944:9: error: implicit declaration of function > > > > pci_irq_vector; > > > > did you mean rcu_irq_enter? [-Werror=implicit-function-declaration] > > > > irq = pci_irq_vector(pci_pdev, irqid); > > > > ^~~~~~~~~~~~~~ > > > > > > > > Use #ifdef block to guard this. > > > > > > > Actually, this is interesting. My *original* implementation was using > > > straight #ifdefs, but then I got review feedback stating that I should not > > > do that, as it's not compile testable, suggesting to use regular C if's > > > instead. Then there was quite some back-and-forth on the actual > > > implementation and I ended up with this. > > > > > > So now it turns out that doesn't work and I'm suggested to go full-circle > > > back to straight #ifdef's? Or is there some other way to make this work? > > > Because I don't know where to go from here ... > > > > > > > > > C conditionals are preferred over preprocessor conditional, but if the > > conditional code refers to symbols that are not declared when the > > Kconfig symbol is not defined, preprocessor conditionals are the only > > option. > > > Sure, I get that. But I *had* the #ifdef's and then other people told me > to get rid of them. How is one supposed to know when which symbols are > declared exactly? Moreover, I feel that if #ifdef's are sometimes the > only way, then you should be careful providing feedback on the subject. > If you compile your code with and without the Kconfig symbol defined, the compiler will tell you if there is a problem or not. > > This is the reason we have so many empty static inline functions in > > header files - it ensures that the symbols are declared even if the > > only invocations are from dead code. > > > This ties back into my previous question: how am I supposed to know whether > stuff is nicely covered by these empty static inlines or not? If this > happens to be a hit-and-miss affair. > Indeed. > Note that I tested the code with the 2 platforms at my disposal - actually > the only 2 relevant platforms for this driver, if you ask me - and they > both compiled just fine, so I had no way of finding this "problem" myself. > Did you try disabling CONFIG_PCI? > > > > > > Fixes: 625f269a5a7a ("crypto: inside-secure - add support for PCI based FPGA > > development > > > > board") > > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > > --- > > > > v2: use 'ifdef' instead of 'IS_ENABLED' > > > > --- > > > > drivers/crypto/inside-secure/safexcel.c | 13 ++++++++++--- > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside- > > > > secure/safexcel.c > > > > index e12a2a3..5253900 100644 > > > > --- a/drivers/crypto/inside-secure/safexcel.c > > > > +++ b/drivers/crypto/inside-secure/safexcel.c > > > > @@ -937,7 +937,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > > int ret, irq; > > > > struct device *dev; > > > > > > > > - if (IS_ENABLED(CONFIG_PCI) && is_pci_dev) { > > > > +#ifdef CONFIG_PCI > > > > + if (is_pci_dev) { > > > > struct pci_dev *pci_pdev = pdev; > > > > > > > > dev = &pci_pdev->dev; > > > > @@ -947,7 +948,10 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > > irqid, irq); > > > > return irq; > > > > } > > > > - } else if (IS_ENABLED(CONFIG_OF)) { > > > > + } else > > > > +#endif > > > > + { > > > > +#ifdef CONFIG_OF > > > > struct platform_device *plf_pdev = pdev; > > > > char irq_name[6] = {0}; /* "ringX\0" */ > > > > > > > > @@ -960,6 +964,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > > irq_name, irq); > > > > return irq; > > > > } > > > > +#endif > > > > } > > > > > > > > ret = devm_request_threaded_irq(dev, irq, handler, > > > > @@ -1137,7 +1142,8 @@ static int safexcel_probe_generic(void *pdev, > > > > > > > > safexcel_configure(priv); > > > > > > > > - if (IS_ENABLED(CONFIG_PCI) && priv->version == EIP197_DEVBRD) { > > > > +#ifdef CONFIG_PCI > > > > + if (priv->version == EIP197_DEVBRD) { > > > > /* > > > > * Request MSI vectors for global + 1 per ring - > > > > * or just 1 for older dev images > > > > @@ -1153,6 +1159,7 @@ static int safexcel_probe_generic(void *pdev, > > > > return ret; > > > > } > > > > } > > > > +#endif > > > > > > > > /* Register the ring IRQ handlers and configure the rings */ > > > > priv->ring = devm_kcalloc(dev, priv->config.rings, > > > > -- > > > > 2.7.4 > > > > > > > > > > Regards, > > > Pascal van Leeuwen > > > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > > > www.insidesecure.com > > Regards, > Pascal van Leeuwen > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > www.insidesecure.com >
On Wed, Sep 04, 2019 at 05:27:19AM -0700, Ard Biesheuvel wrote: > > Did you try disabling CONFIG_PCI? Indeed. Even with my patch if you compile with CONFIG_PCI you still get a warning: CC [M] drivers/crypto/inside-secure/safexcel.o ../drivers/crypto/inside-secure/safexcel.c: In function \u2018safexcel_init\u2019: ../drivers/crypto/inside-secure/safexcel.c:1506:6: warning: unused variable \u2018rc\u2019 [-Wunused-variable] int rc; ^~ We should fix that in inside-secure. Thanks,
> -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Sent: Wednesday, September 4, 2019 2:27 PM > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> > Cc: YueHaibing <yuehaibing@huawei.com>; antoine.tenart@bootlin.com; > herbert@gondor.apana.org.au; davem@davemloft.net; pvanleeuwen@insidesecure.com; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI > > On Wed, 4 Sep 2019 at 05:25, Pascal Van Leeuwen > <pvanleeuwen@verimatrix.com> wrote: > > > > > -----Original Message----- > > > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > Sent: Wednesday, September 4, 2019 2:11 PM > > > To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com> > > > Cc: YueHaibing <yuehaibing@huawei.com>; antoine.tenart@bootlin.com; > > > herbert@gondor.apana.org.au; davem@davemloft.net; pvanleeuwen@insidesecure.com; linux- > > > crypto@vger.kernel.org; linux-kernel@vger.kernel.org > > > Subject: Re: [PATCH v2 -next] crypto: inside-secure - Fix build error without > CONFIG_PCI > > > > > > On Wed, 4 Sep 2019 at 04:57, Pascal Van Leeuwen > > > <pvanleeuwen@verimatrix.com> wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On > > > Behalf Of > > > > > YueHaibing > > > > > Sent: Tuesday, September 3, 2019 3:45 AM > > > > > To: antoine.tenart@bootlin.com; herbert@gondor.apana.org.au; davem@davemloft.net; > > > > > pvanleeuwen@insidesecure.com > > > > > Cc: linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org; YueHaibing > > > > > <yuehaibing@huawei.com> > > > > > Subject: [PATCH v2 -next] crypto: inside-secure - Fix build error without > CONFIG_PCI > > > > > > > > > > If CONFIG_PCI is not set, building fails: > > > > > > > > > > rivers/crypto/inside-secure/safexcel.c: In function safexcel_request_ring_irq: > > > > > drivers/crypto/inside-secure/safexcel.c:944:9: error: implicit declaration of > function > > > > > pci_irq_vector; > > > > > did you mean rcu_irq_enter? [-Werror=implicit-function-declaration] > > > > > irq = pci_irq_vector(pci_pdev, irqid); > > > > > ^~~~~~~~~~~~~~ > > > > > > > > > > Use #ifdef block to guard this. > > > > > > > > > Actually, this is interesting. My *original* implementation was using > > > > straight #ifdefs, but then I got review feedback stating that I should not > > > > do that, as it's not compile testable, suggesting to use regular C if's > > > > instead. Then there was quite some back-and-forth on the actual > > > > implementation and I ended up with this. > > > > > > > > So now it turns out that doesn't work and I'm suggested to go full-circle > > > > back to straight #ifdef's? Or is there some other way to make this work? > > > > Because I don't know where to go from here ... > > > > > > > > > > > > > C conditionals are preferred over preprocessor conditional, but if the > > > conditional code refers to symbols that are not declared when the > > > Kconfig symbol is not defined, preprocessor conditionals are the only > > > option. > > > > > Sure, I get that. But I *had* the #ifdef's and then other people told me > > to get rid of them. How is one supposed to know when which symbols are > > declared exactly? Moreover, I feel that if #ifdef's are sometimes the > > only way, then you should be careful providing feedback on the subject. > > > > If you compile your code with and without the Kconfig symbol defined, > the compiler will tell you if there is a problem or not. > Probably. Maybe. I don't know much about configuring Linux kernels (I'm just happy with working configs provided to me) so I don't know what problems that might give (beyond those in my own code). Actually, I assumed the Macchiatobin config did not have PCI (as Antoine asked me to ifdef that stuff out IIRC), so there was no incentive for me to try explictly. But it turns out the Macchiatobin config has PCI after all. Which makes me wonder what the point of #ifdef'ing out the PCI stuff is in the first place, considering there is no use case for this driver that I know of that does not have PCI support in the kernel. But I guess in that case I would make it depend on PCI in the Kconfig instead. And Antoine may still have had a good reason for his request. > > > This is the reason we have so many empty static inline functions in > > > header files - it ensures that the symbols are declared even if the > > > only invocations are from dead code. > > > > > This ties back into my previous question: how am I supposed to know whether > > stuff is nicely covered by these empty static inlines or not? If this > > happens to be a hit-and-miss affair. > > > > Indeed. > > > Note that I tested the code with the 2 platforms at my disposal - actually > > the only 2 relevant platforms for this driver, if you ask me - and they > > both compiled just fine, so I had no way of finding this "problem" myself. > > > > Did you try disabling CONFIG_PCI? > No, I'm afraid I assumed the Macchiatobin config covered that already. (yeah ... I know ... assumptions ...) > > > > > > > > Fixes: 625f269a5a7a ("crypto: inside-secure - add support for PCI based FPGA > > > development > > > > > board") > > > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > > > --- > > > > > v2: use 'ifdef' instead of 'IS_ENABLED' > > > > > --- > > > > > drivers/crypto/inside-secure/safexcel.c | 13 ++++++++++--- > > > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside- > > > > > secure/safexcel.c > > > > > index e12a2a3..5253900 100644 > > > > > --- a/drivers/crypto/inside-secure/safexcel.c > > > > > +++ b/drivers/crypto/inside-secure/safexcel.c > > > > > @@ -937,7 +937,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > > > int ret, irq; > > > > > struct device *dev; > > > > > > > > > > - if (IS_ENABLED(CONFIG_PCI) && is_pci_dev) { > > > > > +#ifdef CONFIG_PCI > > > > > + if (is_pci_dev) { > > > > > struct pci_dev *pci_pdev = pdev; > > > > > > > > > > dev = &pci_pdev->dev; > > > > > @@ -947,7 +948,10 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > > > irqid, irq); > > > > > return irq; > > > > > } > > > > > - } else if (IS_ENABLED(CONFIG_OF)) { > > > > > + } else > > > > > +#endif > > > > > + { > > > > > +#ifdef CONFIG_OF > > > > > struct platform_device *plf_pdev = pdev; > > > > > char irq_name[6] = {0}; /* "ringX\0" */ > > > > > > > > > > @@ -960,6 +964,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, > > > > > irq_name, irq); > > > > > return irq; > > > > > } > > > > > +#endif > > > > > } > > > > > > > > > > ret = devm_request_threaded_irq(dev, irq, handler, > > > > > @@ -1137,7 +1142,8 @@ static int safexcel_probe_generic(void *pdev, > > > > > > > > > > safexcel_configure(priv); > > > > > > > > > > - if (IS_ENABLED(CONFIG_PCI) && priv->version == EIP197_DEVBRD) { > > > > > +#ifdef CONFIG_PCI > > > > > + if (priv->version == EIP197_DEVBRD) { > > > > > /* > > > > > * Request MSI vectors for global + 1 per ring - > > > > > * or just 1 for older dev images > > > > > @@ -1153,6 +1159,7 @@ static int safexcel_probe_generic(void *pdev, > > > > > return ret; > > > > > } > > > > > } > > > > > +#endif > > > > > > > > > > /* Register the ring IRQ handlers and configure the rings */ > > > > > priv->ring = devm_kcalloc(dev, priv->config.rings, > > > > > -- > > > > > 2.7.4 > > > > > > > > > > > > > Regards, > > > > Pascal van Leeuwen > > > > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > > > > www.insidesecure.com > > > > Regards, > > Pascal van Leeuwen > > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix > > www.insidesecure.com > > Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com
So, with that patch you ONLY get a warning on that unused int rc, right? I do understand that one, that should have been inside an #ifdef as well. Everybody happy if I just fix that and leave the rest as is? Regards, Pascal van Leeuwen Silicon IP Architect, Multi-Protocol Engines @ Verimatrix www.insidesecure.com > -----Original Message----- > From: Herbert Xu <herbert@gondor.apana.org.au> > Sent: Wednesday, September 4, 2019 2:32 PM > To: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>; YueHaibing <yuehaibing@huawei.com>; > antoine.tenart@bootlin.com; davem@davemloft.net; pvanleeuwen@insidesecure.com; linux- > crypto@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI > > On Wed, Sep 04, 2019 at 05:27:19AM -0700, Ard Biesheuvel wrote: > > > > Did you try disabling CONFIG_PCI? > > Indeed. Even with my patch if you compile with CONFIG_PCI you still > get a warning: > > CC [M] drivers/crypto/inside-secure/safexcel.o > ../drivers/crypto/inside-secure/safexcel.c: In function \u2018safexcel_init\u2019: > ../drivers/crypto/inside-secure/safexcel.c:1506:6: warning: unused variable \u2018rc\u2019 > [-Wunused-variable] > int rc; > ^~ > > We should fix that in inside-secure. > > 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
On Wed, Sep 04, 2019 at 12:45:00PM +0000, Pascal Van Leeuwen wrote: > So, with that patch you ONLY get a warning on that unused int rc, right? > > I do understand that one, that should have been inside an #ifdef as well. > Everybody happy if I just fix that and leave the rest as is? Yes please send a patch to fix the unused variable warning. Thanks,
diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c index e12a2a3..5253900 100644 --- a/drivers/crypto/inside-secure/safexcel.c +++ b/drivers/crypto/inside-secure/safexcel.c @@ -937,7 +937,8 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, int ret, irq; struct device *dev; - if (IS_ENABLED(CONFIG_PCI) && is_pci_dev) { +#ifdef CONFIG_PCI + if (is_pci_dev) { struct pci_dev *pci_pdev = pdev; dev = &pci_pdev->dev; @@ -947,7 +948,10 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, irqid, irq); return irq; } - } else if (IS_ENABLED(CONFIG_OF)) { + } else +#endif + { +#ifdef CONFIG_OF struct platform_device *plf_pdev = pdev; char irq_name[6] = {0}; /* "ringX\0" */ @@ -960,6 +964,7 @@ static int safexcel_request_ring_irq(void *pdev, int irqid, irq_name, irq); return irq; } +#endif } ret = devm_request_threaded_irq(dev, irq, handler, @@ -1137,7 +1142,8 @@ static int safexcel_probe_generic(void *pdev, safexcel_configure(priv); - if (IS_ENABLED(CONFIG_PCI) && priv->version == EIP197_DEVBRD) { +#ifdef CONFIG_PCI + if (priv->version == EIP197_DEVBRD) { /* * Request MSI vectors for global + 1 per ring - * or just 1 for older dev images @@ -1153,6 +1159,7 @@ static int safexcel_probe_generic(void *pdev, return ret; } } +#endif /* Register the ring IRQ handlers and configure the rings */ priv->ring = devm_kcalloc(dev, priv->config.rings,
If CONFIG_PCI is not set, building fails: rivers/crypto/inside-secure/safexcel.c: In function safexcel_request_ring_irq: drivers/crypto/inside-secure/safexcel.c:944:9: error: implicit declaration of function pci_irq_vector; did you mean rcu_irq_enter? [-Werror=implicit-function-declaration] irq = pci_irq_vector(pci_pdev, irqid); ^~~~~~~~~~~~~~ Use #ifdef block to guard this. Fixes: 625f269a5a7a ("crypto: inside-secure - add support for PCI based FPGA development board") Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- v2: use 'ifdef' instead of 'IS_ENABLED' --- drivers/crypto/inside-secure/safexcel.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)