RE: [PATCH v2 -next] crypto: inside-secure - Fix build error without CONFIG_PCI

[Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>]

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