> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> Sent: Thursday, June 20, 2019 3:06 PM
> To: Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxxxx>
> Cc: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>; Pascal van Leeuwen <pascalvanl@xxxxxxxxx>; linux-crypto@xxxxxxxxxxxxxxx;
> herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx
> Subject: Re: [PATCH 2/3] crypto: inside-secure - add support for PCI based FPGA development board
>
> Hi Pascal,
>
> On Wed, Jun 19, 2019 at 02:22:19PM +0000, Pascal Van Leeuwen wrote:
> > > From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> > > On Tue, Jun 18, 2019 at 07:56:23AM +0200, Pascal van Leeuwen wrote:
> > > >
> > > > /* Fallback to the old firmware location for the
> > > > @@ -294,6 +291,9 @@ static int safexcel_hw_init(struct safexcel_crypto_priv *priv)
> > > >
> > > > + dev_info(priv->dev, "EIP(1)97 HW init: burst size %d beats, using %d pipe(s) and %d
> > > ring(s)",
> > > > + 16, priv->config.pes, priv->config.rings);
> > >
> > > Adding custom messages in the kernel log has to be done carefully.
> > > Although it's not considered stable it could be difficult to rework
> > > later on. Also, if all driver were to print custom messages the log
> > > would be very hard to read. But you can also argue that a single message
> > > when probing a driver is also done in other drivers.
> > >
> > Hmm ... don't know what the rules for logging are exactly, but from my
> > perspective, I'm dealing with a zillion different HW configurations so
> > some feedback whether the driver detected the *correct* HW parameters -
> > or actually, whether I stuffed the correct image into my FPGA :o) - is
> > very convenient to have. And not just for my local development, but also
> > to debug deployments in the field at customer sites.
>
> I understand it can be convenient, it's just a matter of having a
> logging message for you that will end up in many builds for many users.
> They do not necessarily have the same needs. So it's a matter of
> compromise, one or two messages at boot can be OK, more is likely to
> become an issue.
>
OK, got it. So I have to stuff all my logging into one or two very long lines :-P
(just kidding)
> > Also, looking at my log, there's other drivers that are similarly
> > (or even more) verbose.
>
> There are *always* other examples in the kernel :)
>
> > If it's a really considered a problem, I could make it dev_dbg?
>
> Sure. I would say adding one message at boot like this one seems OK. But
> the others would need to be debugging messages.
>
OK, then that's what I'll really do.
> > > For this one particularly, the probe could fail later on. So if we were
> > > to add this output, it should be done at the very end of the probe.
> > >
> > I'm in doubt about this one. I understand that you want to reduce the
> > logging in that case, but at the same time that message can convey
> > information as to WHY the probing fails later on ...
>
> If the drivers fails to probe, there will be other messages. In that
> case is this one really needed? I'm not sure.
>
> > i.e. if it detects, say, 4 pipes on a device that, in fact, only has
> > 2, then that may be the very reason for the FW init to fail later on.
>
> In case of failure you'll need anyway to debug and understand what's
> going on. By adding new prints, or enabling debugging messages.
>
If it fails for me locally, I can do that. If it somehow fails "in the field",
I think most people won't be able to recompile their own Linux
kernel with debug messages let alone add their own debug messages.
Anyway, I'll just make everything dev_dbg to avoid further discussion.
> > > > -static int safexcel_request_ring_irq(struct platform_device *pdev, const char *name,
> > > > +static int safexcel_request_ring_irq(void *pdev, int irqid,
> > > > + int is_pci_dev,
> > >
> > > You could probably use the DEVICE_IS_PCI flag instead.
> > >
> > I'm not currently passing priv to that function, so I can't access
> > priv->version directly. I could pass a priv pointer instead and use
> > the flag, but passing a bool constant seemed to be more efficient?
>
> That right, whatever you prefer then.
>
Ok, then I'll leave it as it is.
> > > > +static int safexcel_probe_generic(void *pdev,
> > > > + struct safexcel_crypto_priv *priv,
> > > > + int is_pci_dev)
> > > > {
> > >
> > > > + if (IS_ENABLED(CONFIG_PCI) && (priv->version & DEVICE_IS_PCI)) {
> > >
> > > DEVICE_IS_PCI should be set in ->flags, not ->version.
> > >
> > As far as I could tell from the existing code, "version" was
> > a highlevel indication of the device *type* (EIP97/EIP197B/D)
> > while flags was more about low-level *features*.
>
> Using the same argument I would say the way the device is accessed is
> very run-time dependent, and it has nothing to do with the version of
> the h/w (the same engine could be accessed through different ways given
> the platform).
>
> Maybe the name 'feature' is too specific, I agree with you.
>
> > So in my humble opinion, version was the correct location, it
> > is just a confusing name. (i.e. you can have many *versions*
> > of an EIP197B, for instance ...)
>
> That would be an issue with the driver. We named the 'version' given the
> knowledge we had of the h/w, it might not be specific enough. Or maybe
> you can think of this as being a "family of engine versions". The idea
> is the version is what the h/w is capable of, not how it's being
> wired/accessed.
>
Well ... I want to avoid the whole discussion about the naming of the
variable (which can be trivially changed) and what the intention may
have been, if you allow me.
Fact is ... this variable is what receives .data / .driver_data from the
OF or PCI match table. So it is a means of conveying a value that is
specific to the table entry that was matched. No more, no less.
In "your" device tree case you want to distinguish between
Armada 39x, Armada 7K/8K and Armada 9K. In "my" PCI case I
want to potentially distinguish multiple FPGA boards/images.
It wouldn't make much sense to me to do the vendor/subvendor/
device/subdevice decoding all over again in my probe routine.
So what exactly is so very wrong with the way I'm doing this?
> > > > + /*
> > > > + * Request MSI vectors for global + 1 per ring -
> > > > + * or just 1 for older dev images
> > > > + */
> > > > + struct pci_dev *pci_pdev = pdev;
> > > > +
> > > > + ret = pci_alloc_irq_vectors(pci_pdev,
> > > > + priv->config.rings + 1,
> > > > + priv->config.rings + 1,
> > > > + PCI_IRQ_MSI|PCI_IRQ_MSIX);
> > >
> > > You need a space before and after the | here.
> > >
> > Ok, will add (checkpatch did not complain though)
>
> It may complain with --strict on this one. (Well, I'm sure many --strict
> tests won't pass on the existing upstream code, no worries :)).
>
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "Failed to allocate PCI MSI interrupts");
> > >
> > > Do not forget the \n at the end of the string.
> > >
> > Actually, I removed all "\n"'s from my messages as I
> > understood they are not needed? If they are really needed,
> > I can add them to all log strings again ... anyone else?
>
> The simple answer is: everybody set the \n, so at least for consistency
> we should have them.
>
I did a bit of side-investigation to conclude the same. So I already added them.
> > > > @@ -1189,13 +1249,12 @@ static int safexcel_remove(struct platform_device *pdev)
> > > > .compatible = "inside-secure,safexcel-eip197d",
> > > > .data = (void *)EIP197D,
> > > > },
> > > > + /* For backward compatibiliry and intended for generic use */
> > > > {
> > > > - /* Deprecated. Kept for backward compatibility. */
> > > > .compatible = "inside-secure,safexcel-eip97",
> > > > .data = (void *)EIP97IES,
> > > > },
> > > > {
> > > > - /* Deprecated. Kept for backward compatibility. */
> > > > .compatible = "inside-secure,safexcel-eip197",
> > > > .data = (void *)EIP197B,
> > > > },
> > >
> > > I'm not sure about this. The compatible should describe what the
> > > hardware is, and the driver can then decide if it has special things to
> > > do or not. It is not used to configure the driver to be used with a
> > > generic use or not.
> > >
> > > Do you have a practical reason to do this?
> >
> > I have to admit I don't fully understand how these compatible
> > strings work. All I wanted to achieve is provide some generic
> > device tree entry to point to this driver, to be used for
> > devices other than Marvell. No need to convey b/d that way
> > (or even eip97/197, for that matter) as that can all be probed.
>
> Compatibles are used in device trees, which intend to be a description
> of the hardware (not the configuration of how the hardware should be
> used). So we can't have a compatible being a restricted configuration
> use of a given hardware. But I think here you're right, and there is
> room for a more generic eip197 compatible: the b/d versions only have
> few differences and are part of the same family, so we can have a very
> specific compatible plus a "family" one. Something like:
>
> compatible = "inside-secure,safexcel-eip197d", "inside-secure,safexcel-eip197";
>
> This would need to be in a separated patch, and this should be
> documented in:
> Documentation/devicetree/bindings/crypto/inside-secure-safexcel.txt
>
Ok, then I'll leave that part untouched for now.
(I only changed the comments anyway ...)
> > > > + /* enable the device */
> > > > + rc = pcim_enable_device(pdev);
> > > > + if (rc) {
> > > > + dev_err(dev, "pci_enable_device() failed");
> > >
> > > Please use error messages describing the issue rather than printing the
> > > function names. (There are other examples).
> > >
> > But ... that IS the issue, isn't it? Well, apart from the actual
> > return code. What message text would you suggest?
>
> Yes, it is the issue. We generally use a sentence instead of function
> names (something alongside s/_/ / :)).
>
Fine, will do.
> > > > + /* HW reset FPGA dev board */
> > > > + // assert reset
> > > > + writel(1, priv->base + XILINX_GPIO_BASE);
> > > > + wmb(); /* maintain strict ordering for accesses here */
> > > > + // deassert reset
> > > > + writel(0, priv->base + XILINX_GPIO_BASE);
> > > > + wmb(); /* maintain strict ordering for accesses here */
> > >
> > > It seems the driver here access to a GPIO controller, to assert and
> > > de-assert reset, which is outside the crypto engine i/o range. If true,
> > > this is not acceptable in the upstream kernel: those GPIO or reset
> > > should be accessed through the dedicated in-kernel API and be
> > > implemented in separate drivers (maybe a reset driver here).
> > >
> > Hmmm ... this is some *local* GPIO controller *inside*
> > the FPGA device (i.e. doesn't even leave the silicon die).
> > It's inside the range of the (single!) PCIE device, it's not
> > a seperate device and thus cannot be managed as such ...
> >
> > Effectively, it's just an MMIO mapped register no different
> > from any other slave accessible register.
> > If I had given it a less explicit name, nobody would've cared.
>
> OK, that makes sense. If the register accessed is within the register
> bank of the crypto engine you can keep this.
>
> > > > +static struct pci_driver crypto_is_pci_driver = {
> > > > + .name = "crypto-safexcel",
> > > > + .id_table = crypto_is_pci_ids,
> > > > + .probe = crypto_is_pci_probe,
> > > > + .remove = crypto_is_pci_remove,
> > > > +};
> > >
> > > More generally, you should protect all the PCI specific functions and
> > > definitions between #ifdef.
> > >
> > I asked the mailing list and the answer was I should NOT use #ifdef,
> > but instead use IS_ENABLED to *only* remove relevant function bodies.
> > Which is exactly what I did (or tried to do, anyway).
>
> My bad, I realise there's a mistake in my comment. That should have
> been: you should protect all the PCI specific functions and definitions
> with #if IS_ENABLED(...). When part of a function should be excluded
> you can use if(IS_ENABLED(...)), but if the entire function can be left
> out, #if is the way to go.
>
Ok #if instead of if or #ifdef,that makes sense.
So can I just put all the PCI stuff into one big #if then?
Thanks,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
www.insidesecure.com
