Hi Antoine,
Thanks for reviewing.
> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx>
> Sent: Wednesday, June 19, 2019 2:15 PM
> To: Pascal van Leeuwen <pascalvanl@xxxxxxxxx>
> Cc: linux-crypto@xxxxxxxxxxxxxxx; antoine.tenart@xxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; Pascal Van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH 2/3] crypto: inside-secure - add support for PCI based FPGA
> development board
>
> Hi Pascal,
>
> Thanks for the patch :)
>
> 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.
Also, looking at my log, there's other drivers that are similarly
(or even more) verbose.
If it's a really considered a problem, I could make it dev_dbg?
Downside of that is I will need to educate my customers into
building debug kernels just to get some basic support feedback ...
> 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 ...
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.
Note that you can only get this message if the EIP(1)97 hardware has
indeed been found. You won't get it if the hardware does not exist.
> > - if (priv->version == EIP197B || priv->version == EIP197D) {
> > + if ((priv->version & EIP197B) || (priv->version & EIP197D)) {
>
> You could also use "version & (EIP197B | EIP197D)" (there are other
> examples of this).
>
Yes, I will fix that. I just lazily changed the == into & :-)
> > -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?
> > irq_handler_t handler,
> > irq_handler_t threaded_handler,
> > struct safexcel_ring_irq_data *ring_irq_priv)
> > {
>
> > + dev_err(dev, "unable to get IRQ '%s'\n (err %d)",
> > + irq_name, irq);
>
> I think there's a typo here, the \n should be at the end of the string.
>
Yes, will fix, thanks.
> > +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*.
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 ...)
> > + /*
> > + * 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)
> > + 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?
> > + if (ret) {
> > + dev_err(dev, "Failed to initialize rings");
>
> Also here, and probably in other places :)
>
> > - snprintf(irq_name, 6, "ring%d", i);
> > - irq = safexcel_request_ring_irq(pdev, irq_name, safexcel_irq_ring,
> > + irq = safexcel_request_ring_irq(pdev, i + is_pci_dev,
>
> This 'i + is_pci_dev' is hard to read and understand. I would suggest to
> use a define, or an helper to get the real irq given if a device is
> probed using PCI or not. Something like "safexcel_irq_number(i, priv)"
>
OK, I can do that.
> > + dev_err(dev, "Failed to create work queue for ring %d",
> > + i);
> > + return -ENOMEM;
> > }
> > -
>
> Why removing this one blank line?
>
Don't know, I can add it back
> > priv->ring[i].requests = 0;
> > priv->ring[i].busy = false;
> > -
>
> Ditto.
>
Same
> > + dev_err(dev, "EIP(1)97 h/w init failed (%d)", ret);
>
> Adding a version information in the log means all the outputs will have
> to be updated if we ever support an h/w with a different name.
>
Fair enough, then I'll remove all EIP references
> > +/*
> > + *
> > + * for Device Tree platform driver
> > + *
> > + */
>
> Please follow the comment styles (you can remove set in on a single line
> here, or at least remove the blank lines). (There are other examples).
>
Ok, I'll fix that
> > @@ -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.
>
> > +static int crypto_is_pci_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *ent)
>
> The alignment is wrong here.
>
Will fix
> > +{
> > + if (IS_ENABLED(CONFIG_PCI)) {
>
> Can this be called with CONFIG_PCI disabled?
>
I would expect not, but I suppose I was fearing it wouldn't compile
with CONFIG_PCI disabled. Since I only have PCI based systems, I
can't really try this.
> > + dev_info(dev, "Probing PCIE device: vendor %04x, device %04x, subv %04x, subdev
> %04x, ctxt %lx",
> > + ent->vendor, ent->device, ent->subvendor,
> > + ent->subdevice, ent->driver_data);
>
> Please drop this new message. If the userspace needs it, I believe there
> are clean ways to get it.
>
It's not for userspace ...
This is just information to see whether the HW is correctly
detected. Very useful for debugging and providing support.
Again, I can make it a dev_dbg, but I really DO need this.
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv) {
> > + dev_err(dev, "Failed to allocate memory");
> > + return -ENOMEM;
>
> No need to have a debug message when returning ENOMEM.
>
OK, will remove
>
> > + /* 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?
> > + if (priv->version & XILINX_PCIE) {
> > + dev_info(dev, "Device identified as FPGA based development board -
> applying HW reset");
> > +
> > + rc = pcim_iomap_regions(pdev, 4, "crypto_safexcel");
> > + if (rc) {
> > + dev_err(dev, "pcim_iomap_regions() failed for BAR4");
> > + return rc;
> > + }
> > +
> > + pciebase = pcim_iomap_table(pdev)[2];
> > + val = readl(pciebase + XILINX_IRQ_BLOCK_ID);
> > + if ((val >> 16) == 0x1fc2) {
>
> Try not to use magic values, use defines with a meaningful name.
>
I can make it a define
> > + dev_info(dev, "Detected Xilinx PCIE IRQ block version %d, multiple
> MSI support enabled",
> > + (val & 0xff));
>
> Same comments as the other dev_info().
>
And same response from my side. When working with lots of different
HW, some of which only half-functional, such logging is really crucial.
> > +
> > + /* Setup MSI identity map mapping */
> > + writel(0x03020100,
> > + pciebase + XILINX_USER_VECT_LUT0);
> > + writel(0x07060504,
> > + pciebase + XILINX_USER_VECT_LUT1);
> > + writel(0x0b0a0908,
> > + pciebase + XILINX_USER_VECT_LUT2);
> > + writel(0x0f0e0d0c,
> > + pciebase + XILINX_USER_VECT_LUT3);
>
> Use defines here.
>
OK
> > + /* 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.
> > +void crypto_is_pci_remove(struct pci_dev *pdev)
> > +{
> > + if (IS_ENABLED(CONFIG_PCI)) {
>
> Can this be accessed with CONFIG_PCI disabled?
>
Again, probably not, but will it compile without this?
> > +static const struct pci_device_id crypto_is_pci_ids[] = {
> > + {
> > + .vendor = 0x10ee,
> > + .device = 0x9038,
> > + .subvendor = 0x16ae,
> > + .subdevice = 0xc522,
> > + .class = 0x00000000,
> > + .class_mask = 0x00000000,
>
> You should use proper defines here, and define the vendor ID in a
> generic way in the kernel. You can look for the use of PCI_DEVICE and
> PCI_DEVICE_DATA as a starting point.
>
Ok, I will look into that
> > + // assume EIP197B for now
> > + .driver_data = XILINX_PCIE | DEVICE_IS_PCI | EIP197B,
>
> We do use the data (here and for dt compatibles) to store a flag about
> the version. I think XILINX_PCIE and DEVICE_IS_PCI should be flags, and
> you should be able to set them using pci_dev->vendor_id (or other ids).
>
But why would I want to do that as this is more convenient?
Basically, the "version" (wrong name, if you ask me) field was used
to identify the device type, so I just extended that to more types.
> Also, you should prefix XILINX_PCIE and DEVICE_IS_PCI with an "unique"
> name, which should be driver-specific to avoid colliding with other
> possible global definitions. In this driver we do use EIP197_ and
> SAFEXCEL_ a lot for historical reasons, you can pick one :)
>
Ok, will do
> > +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).
> > +MODULE_AUTHOR("Pascal van Leeuwen <pvanleeuwen@xxxxxxxxxxxxxxxx>");
>
> I think this is usually done in a separate patch, when one has made
> numerous contributions to a driver.
>
OK, I can remove that
> I tested it on my hardware and the boot tests passed nicely, so it seems
> there were no regressions (I'll make more tests though).
>
> Thanks!
> Antoine
>
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
www.insidesecure.com
