lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <PH0PR22MB3809DE8446E7EAEFB0F6103FE55C2@PH0PR22MB3809.namprd22.prod.outlook.com>
Date: Thu, 7 Nov 2024 18:42:59 +0000
From: Robert Joslyn <robert_joslyn@...inc.com>
To: Lee Jones <lee@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [RFC PATCH 1/2] mfd: Add SEL PCI Virtual Multifunction (PVMF)
 support

> -----Original Message-----
> From: Lee Jones <lee@...nel.org>
> Sent: Tuesday, November 5, 2024 9:31 AM
> To: Robert Joslyn <robert_joslyn@...inc.com>
> Cc: linux-kernel@...r.kernel.org; netdev@...r.kernel.org
> Subject: Re: [RFC PATCH 1/2] mfd: Add SEL PCI Virtual Multifunction (PVMF)
> support
> 
> [Caution - External]
> 
> On Mon, 28 Oct 2024, Robert Joslyn wrote:
> 
> > Add support for SEL FPGA based PCIe devices. These expose a single PCI
> > BAR with multiple IP cores for various functions, such as serial
> > ports, ethernet, and time (PTP/IRIG). This initial driver supports
> > Ethernet on the SEL-3350 mainboard, SEL-3390E4 ethernet card, and
> > SEL-3390T ethernet and time card.
> >
> > Signed-off-by: Robert Joslyn <robert_joslyn@...inc.com>
> > ---
> >  MAINTAINERS                |   8 +
> >  drivers/mfd/Kconfig        |  16 ++
> >  drivers/mfd/Makefile       |   3 +
> >  drivers/mfd/selpvmf-core.c | 482
> > +++++++++++++++++++++++++++++++++++++
> >  drivers/mfd/selpvmf-cvp.c  | 431
> +++++++++++++++++++++++++++++++++
> > drivers/mfd/selpvmf-cvp.h  |  18 ++
> >  6 files changed, 958 insertions(+)
> >  create mode 100644 drivers/mfd/selpvmf-core.c  create mode 100644
> > drivers/mfd/selpvmf-cvp.c  create mode 100644
> > drivers/mfd/selpvmf-cvp.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > a27407950242..4a24b3be8aa5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -20730,6 +20730,14 @@ F:   tools/testing/selftests/lsm/
> >  X:   security/selinux/
> >  K:   \bsecurity_[a-z_0-9]\+\b
> >
> > +SEL DRIVERS
> > +M:   Robert Joslyn <robert_joslyn@...inc.com>
> > +S:   Supported
> > +B:   mailto:opensource@...inc.com
> > +F:   drivers/mfd/selpvmf-*
> > +F:   drivers/platform/x86/sel3350-platform.c
> > +F:   include/linux/mfd/selpvmf.h
> > +
> >  SELINUX SECURITY MODULE
> >  M:   Paul Moore <paul@...l-moore.com>
> >  M:   Stephen Smalley <stephen.smalley.work@...il.com>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > f9325bcce1b9..77e2ce3db505 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2402,5 +2402,21 @@ config MFD_RSMU_SPI
> >         Additional drivers must be enabled in order to use the functionality
> >         of the device.
> >
> > +config MFD_SELPVMF
> > +     tristate "SEL PVMF Support"
> > +     help
> > +       Support for SEL PCI virtual multifunction (PVMF) devcies utilizing
> > +       an FPGA based PCIe interface, including:
> > +         * SEL-3350
> > +         * SEL-3390E4
> > +         * SEL-3390T
> > +
> > +       This driver provides common support for accessing the device.
> > +       Additional drivers must be enabled in order to use the functionality
> > +       of the device.
> > +
> > +       This driver can be built as a module. If built as a module it will be
> > +       called "selpvmf".
> > +
> >  endmenu
> >  endif
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > 2a9f91e81af8..198e43ef7a51 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -289,3 +289,6 @@ obj-$(CONFIG_MFD_ATC260X_I2C)     += atc260x-
> i2c.o
> >
> >  obj-$(CONFIG_MFD_RSMU_I2C)   += rsmu_i2c.o rsmu_core.o
> >  obj-$(CONFIG_MFD_RSMU_SPI)   += rsmu_spi.o rsmu_core.o
> > +
> > +selpvmf-objs                 := selpvmf-core.o selpvmf-cvp.o
> > +obj-$(CONFIG_MFD_SELPVMF)    += selpvmf.o
> > diff --git a/drivers/mfd/selpvmf-core.c b/drivers/mfd/selpvmf-core.c
> > new file mode 100644 index 000000000000..ec3e65fe8064
> > --- /dev/null
> > +++ b/drivers/mfd/selpvmf-core.c
> > @@ -0,0 +1,482 @@
> > +// SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause
> > +/*
> > + * Copyright 2017 Schweitzer Engineering Laboratories, Inc.
> 
> No changes since 2017?
> 
> Why upstream this now?

The driver has been continually maintained out-of-tree since then. I can update the copyright notices to 2017-2024 to be more accurate. The hardware this enables is still actively maintained and sold.

> > + * 2350 NE Hopkins Court, Pullman, WA 99163 USA
> > + * SEL Open Source <opensource@...inc.com> */
> > +
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/property.h>
> > +#include <linux/mfd/core.h>
> > +
> > +#include "selpvmf-cvp.h"
> > +
> > +#define PCI_VENDOR_ID_SEL    0x1aa9
> > +#define PCI_DEVICE_ID_B1190  0x0014 /* SEL-3390T */ #define
> > +PCI_DEVICE_ID_B2093  0x0015 /* SEL-3350 mainboard */ #define
> > +PCI_DEVICE_ID_B2077  0x0018 /* SEL-3390E4 */ #define
> > +PCI_DEVICE_ID_B2091  0x0019 /* SEL-2241-2 */
> > +
> > +#define B1190_NUM_ETHERNET 2
> > +#define B2077_NUM_ETHERNET 4
> > +#define B2091_NUM_ETHERNET 5
> > +#define B2093_NUM_ETHERNET 5
> > +#define SELETH_NUM_RESOURCES 4
> > +
> > +#define SELETH_LEN   0x1000
> > +
> > +static bool skipcvp;
> > +module_param(skipcvp, bool, 0644);
> > +MODULE_PARM_DESC(skipcvp, "Skip firmware load via CvP.");
> > +
> > +/**
> > + * sel_create_cell() - Create an MFD cell and add it to the device.
> > + * @pdev: The PCI device to operate on
> > + * @name: MFD cell name
> > + * @start: Start address of memory resource
> > + * @length: Length of memory resource
> > + * @msix_start: MSIX vector number to start from.
> > + * @resources: Pointer to resources to add to the cell.
> > + * @num_resources: Number of resources. The first resource is assumed to
> > + *                 be memory, all other resources are IRQs.
> > + */
> > +static int sel_create_cell(struct pci_dev *pdev,
> 
> This is misleading.  It should be sel_register_device.

I'll rename the function.

> 
> > +                        const char *name,
> 
> This never changes.  Why supply it as a variable?
> 
> > +                        unsigned int start,
> > +                        unsigned int length,
> 
> This never changes.  Why supply it as a variable?
> 
> > +                        unsigned int msix_start,
> 
> This never changes.  Why supply it as a variable?
> 
> > +                        struct resource *resources,
> > +                        unsigned int num_resources)
> 
> This never changes.  Why supply it as a variable?

This patch only enables ethernet, but we have additional functionality that we will enable in subsequent patches, such as serial ports, a time component, and a firmware upgrade mechanism. Those components have different names, lengths, interrupt counts, etc. If you prefer, I can remove this abstraction until we have another cell type to add.

> 
> > +{
> > +     struct mfd_cell cell;
> > +     unsigned int msix;
> > +     int rc;
> > +     int i;
> > +
> > +     if (!resources || num_resources < 1)
> 
> How would this be possible?

It would be a bug in the code. I can remove this.

> 
> > +             return -EINVAL;
> > +
> > +     /* If MSI-X is enabled, assume we're using it and start at the
> 
> Malformed multi-line comment.

Is the style here to have an empty line with the leading /*?

/*
 * like this?
 */

I'll change the style.

> 
> > +      * provided vector number and iterate through the interrupts
> > +      * when adding IRQ resources. Otherwise leave the interrupt
> > +      * index at zero for legacy IRQs.
> > +      */
> > +     if (pdev->msix_enabled)
> > +             msix = msix_start;
> > +     else
> > +             msix = 0;
> 
> Preinitialise then drop the else.

I'll make this change.

> 
> > +     /* First resource is always a memory resource */
> > +     resources[0].start = start;
> > +     resources[0].end = start + length - 1;
> > +     resources[0].flags = IORESOURCE_MEM;
> > +
> > +     /* Any additional resources are IRQs */
> > +     for (i = 1; i < num_resources; i++) {
> > +             resources[i].start = pci_irq_vector(pdev, msix);
> > +             resources[i].end = pci_irq_vector(pdev, msix);
> 
> Does pci_irq_vector(pdev, msix) change between calls?

It does not, this can be simplified to one call to pci_irq_vector.

> 
> > +             resources[i].flags = IORESOURCE_IRQ;
> > +             if (pdev->msix_enabled)
> > +                     msix++;
> > +     }
> > +
> > +     cell.name = name;
> > +     cell.num_resources = num_resources;
> > +     cell.resources = resources;
> > +
> > +     rc = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO, &cell,
> > +                          1, NULL, 0, NULL);
> > +
> > +     return rc;
> > +}
> > +
> > +/**
> > + * create_eth_cell - Create and add an MFD cell for an selnet
> > +component
> > + *
> > + * @pdev: PCI device
> > + * @offset: Offset from BAR 0 of the component
> > + * @msix_num: Starting MSI-X vector number  */ static int
> > +create_eth_cell(struct pci_dev *pdev,
> > +                        unsigned int offset,
> > +                        unsigned int msix_num) {
> > +     struct resource resources[SELETH_NUM_RESOURCES] = {0};
> > +
> > +     return sel_create_cell(pdev,
> 
> This is only called from here, thus bring all of the sel_create_cell() stuff into
> here instead.

As before, this is an abstraction to facilitate creation of multiple cell types. I can remove it until we have that if preferred.

> 
> > +                            "selpcimac",
> > +                            pdev->resource[0].start + offset,
> > +                            SELETH_LEN,
> > +                            msix_num,
> > +                            resources,
> > +                            SELETH_NUM_RESOURCES); }
> > +
> > +/**
> > + * b1190_alloc_mfd_cells - Allocate and initialize MFD cell
> > +descriptions
> > + *
> > + * @pdev: PCI device for the b1190
> > + *
> > + * +----------------------------------------+
> > + * |          | Resources |     Address     |
> > + * |  Device  | Mem | IRQ | Start  | Length |
> > + * |----------+-----+-----+--------+--------|
> > + * | Ethernet |  1  |  3  | 0x0000 | 0x1000 |
> > + * | Ethernet |  1  |  3  | 0x1000 | 0x1000 |
> > + * | seltime  |  1  |  1  | 0x3000 | 0x1000 |
> > + * | upgrades |  1  |  0  | 0x4000 | 0x4000 |
> > + * +----------------------------------------+
> > + */
> > +static int b1190_alloc_mfd_cells(struct pci_dev *pdev) {
> > +     unsigned int offset;
> > +     int rc;
> 
> 'ret' is more common.

I'll rename the variable.

> 
> > +     int i;
> 
> Use for (int i = 0 ... instead.
> 
> > +     /* Ethernet */
> > +     offset = 0x0000;
> 
> Initialise this during declaration.

I can move it. I did this because once there is a second cell type, such as seltime, there will be a pattern of assigning the offset, then creating the cells. So when we have the seltime driver ready, there will be a block of code right after this that will set the offset to 0x3000 and then create the time cell.

> 
> > +     for (i = 0; i < B1190_NUM_ETHERNET; i++) {
> > +             rc = create_eth_cell(pdev, offset, i * 3);
> > +             if (rc)
> > +                     goto out_fail;
> > +             offset += SELETH_LEN;
> > +     }
> > +
> > +out_fail:
> > +     return rc;
> > +}
> 
> These functions are always identical.  Create a single function and pass the
> number of ports into it and remove the rest.

As above, the abstraction is to accommodate future additions. I can remove the abstraction if that is preferred at this time.

> 
> > +
> > +/**
> > + * b2077_alloc_mfd_cells - Allocate and initialize MFD cell
> > +descriptions
> > + *
> > + * @pdev: PCI device for the b2077
> > + *
> > + * +----------------------------------------+
> > + * |          | Resources |     Address     |
> > + * |  Device  | Mem | IRQ | Start  | Length |
> > + * |----------+-----+-----+--------+--------|
> > + * | Ethernet |  1  |  3  | 0x0000 | 0x1000 |
> > + * | Ethernet |  1  |  3  | 0x1000 | 0x1000 |
> > + * | Ethernet |  1  |  3  | 0x2000 | 0x1000 |
> > + * | Ethernet |  1  |  3  | 0x3000 | 0x1000 |
> > + * | upgrades |  1  |  0  | 0x8000 | 0x4000 |
> > + * | seltime  |  1  |  1  | 0xc000 | 0x1000 |
> > + * +----------------------------------------+
> > + */
> > +static int b2077_alloc_mfd_cells(struct pci_dev *pdev) {
> > +     unsigned int offset;
> > +     int rc;
> > +     int i;
> > +
> > +     /* Ethernet */
> > +     offset = 0x0000;
> > +     for (i = 0; i < B2077_NUM_ETHERNET; i++) {
> > +             rc = create_eth_cell(pdev, offset, i * 3);
> > +             if (rc)
> > +                     goto out_fail;
> > +             offset += SELETH_LEN;
> > +     }
> > +
> > +out_fail:
> > +     return rc;
> > +}
> > +
> > +/*
> > + * b2091_alloc_mfd_cells - Allocate and initialize MFD cell
> > +descriptions
> > + *
> > + * @pdev: PCI device for the b2091
> > + *
> > + * +------------------------------------------+
> > + * |            | Resources |     Address     |
> > + * |   Device   | Mem | IRQ | Start  | Length |
> > + * |------------+-----+-----+--------+--------|
> > + * | Ethernet   |  1  |  3  | 0x0000 | 0x1000 |
> > + * | Ethernet   |  1  |  3  | 0x1000 | 0x1000 |
> > + * | Ethernet   |  1  |  3  | 0x2000 | 0x1000 |
> > + * | Ethernet   |  1  |  3  | 0x3000 | 0x1000 |
> > + * | Ethernet   |  1  |  3  | 0x4000 | 0x1000 |
> > + * | upgrades   |  1  |  0  | 0x8000 | 0x4000 |
> > + * | seltime    |  1  |  1  | 0xc000 | 0x1000 |
> > + * | multiuart  |  1  |  1  | 0xf000 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf100 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf200 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf300 | 0x0100 |
> > + * +------------------------------------------+
> > + */
> > +static int b2091_alloc_mfd_cells(struct pci_dev *pdev) {
> > +     unsigned int offset;
> > +     int rc;
> > +     int i;
> > +
> > +     /* Ethernet */
> > +     offset = 0x0000;
> > +     for (i = 0; i < B2091_NUM_ETHERNET; i++) {
> > +             rc = create_eth_cell(pdev, offset, i * 3);
> > +             if (rc)
> > +                     goto out_fail;
> > +             offset += SELETH_LEN;
> > +     }
> > +
> > +out_fail:
> > +     return rc;
> > +}
> > +
> > +/**
> > + * b2093_alloc_mfd_cells - Allocate and initialize MFD cell
> > +descriptions
> > + *
> > + * @pdev: PCI device for the b2093
> > + *
> > + * +------------------------------------------+
> > + * |            | Resources |     Address     |
> > + * |   Device   | Mem | IRQ | Start  | Length |
> > + * |------------+-----+-----+--------+--------|
> > + * | Ethernet   |  1  |  3  | 0x0000 | 0x1000 |
> > + * | Ethernet   |  1  |  3  | 0x1000 | 0x1000 |
> > + * | Ethernet   |  1  |  3  | 0x2000 | 0x1000 |
> > + * | Ethernet   |  1  |  3  | 0x3000 | 0x1000 |
> > + * | Ethernet   |  1  |  3  | 0x4000 | 0x1000 |
> > + * | upgrades   |  1  |  0  | 0x8000 | 0x4000 |
> > + * | seltime    |  1  |  1  | 0xc000 | 0x1000 |
> > + * | contact IO |  1  |  0  | 0xe000 | 0x1000 |
> > + * | multiuart  |  1  |  1  | 0xf000 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf100 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf200 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf300 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf400 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf500 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf600 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf700 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf800 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xf900 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xfa00 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xfb00 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xfc00 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xfd00 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xfe00 | 0x0100 |
> > + * | multiuart  |  1  |  1  | 0xff00 | 0x0100 |
> > + * +------------------------------------------+
> > + */
> > +static int b2093_alloc_mfd_cells(struct pci_dev *pdev) {
> > +     unsigned int offset;
> > +     int rc;
> > +     int i;
> > +
> > +     /* Ethernet */
> > +     offset = 0x0000;
> > +     for (i = 0; i < B2093_NUM_ETHERNET; i++) {
> > +             rc = create_eth_cell(pdev, offset, i * 3);
> > +             if (rc)
> > +                     goto out_fail;
> > +             offset += SELETH_LEN;
> > +     }
> > +
> > +out_fail:
> > +     return rc;
> > +}
> > +
> > +/*
> > + * selpvmf_probe() - Probe function for the device.
> 
> None of these headers are really necessary.  This is not an external API.

I'll remove internal function headers.

> 
> > + *
> > + * @pdev: PCI device being probed.
> > + * @ent: Entry in the pci_tbl for the device.
> > + *
> > + * Return: 0 for success, otherwise the appropriate negative error
> > +code  */ static int selpvmf_probe(struct pci_dev *pdev, struct
> > +pci_device_id const *ent) {
> > +     typedef int (*alloc_fn_t)(struct pci_dev *pdev);
> > +     alloc_fn_t alloc_fn = (alloc_fn_t)ent->driver_data;
> > +     u16 cvp_capable;
> > +     int num_msix;
> > +     int rc = 0;
> > +
> > +     rc = pcim_enable_device(pdev);
> 
> Could it be that the device isn't ready?
> 
> No use for -EPROBE_DEFER?

I'll change this to return -EPROBE_DEFER.

> 
> > +     if (rc != 0)
> > +             return rc;
> > +
> > +     /* Perform CvP program if necessary */
> > +     cvp_capable = pci_find_ext_capability(pdev, CVP_CAPABILITY_ID);
> > +     if (!skipcvp && cvp_capable) {
> 
> The '_' inconsistency is bothering me.
> 
> As is the placement of "cvp".

Would you prefer to see skipcvp called skip_cvp? Or cvp_skip? I'm happy to change it.

> 
> > +             rc = cvp_start(pdev);
> > +             if (rc != 0) {
> > +                     dev_err(&pdev->dev, "Error: CvP failed: 0x%x\n",
> > + rc);
> 
> This is a user facing error message and I still don't know what CvP is.
> 
> Please be more forthcoming.

CvP is Configuration via Protocol. It's a method for loading firmware into Altera FPGAs. If CvP fails, it means we're unable to load firmware into the FPGA and the card is basically dead. I can improve the error message. Maybe something like, "Loading FPGA firmware failed"?

> 
> > +                     goto out_disable;
> > +             }
> > +             dev_info(&pdev->dev, "CvP Successful!\n");
> 
> This serves no purpose.  Please keep the log as clean as possible.

I'll remove this.

> 
> > +     }
> > +
> > +     pci_set_master(pdev);
> > +
> > +     /* Only MSI-X and legacy interrupts are supported. Try to get
> 
> Malformed multi-line comment.
> 
> > +      * MSI-X, if we don't get them all, fall back to legacy.
> 
> Full stop after MSI-X.
> 
> > +      */
> > +     num_msix = pci_msix_vec_count(pdev);
> 
> '\n' here.

I'll fix these.

> 
> > +     rc = pci_alloc_irq_vectors(pdev, num_msix, num_msix, PCI_IRQ_MSIX);
> > +     if (rc != num_msix) {
> > +             dev_warn(&pdev->dev,
> > +                      "Failed to get MSI-X, falling back to
> > + legacy\n");
> 
> Who is going to care about that?
> 
> '\n' here.

Falling back to legacy interrupts has a performance impact, so this is mostly just a debugging aid. It can be removed.

> 
> > +             rc = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_INTX);
> > +             if (rc != 1) {
> > +                     rc = -ENOSPC;
> 
> I think you should be returning rc as-is here.

Agreed, I'll change this.

> 
> > +                     goto out_disable;
> > +             }
> > +     }
> > +
> > +     rc = alloc_fn(pdev);
> 
> These call-backs are horrible.  Please don't do this.

Ok, we'll do something else. Would you prefer to pass in a constant and use a switch statement to select which function to call?

> 
> > +     if (rc < 0) {
> > +             dev_err(&pdev->dev, "ERROR: Failed to add child
> > + cells\n");
> 
> "Failed to register child devices"
> 
> And it should come after the call to mfd_add_devices() not here.

I'll update the message.

> 
> > +             goto out_free_irq;
> > +     }
> > +
> > +     dev_info(&pdev->dev, "Successfully added new devices\n");
> 
> Remove this please.

I'll remove this.

> 
> > +     return 0;
> > +
> > +out_free_irq:
> > +     pci_free_irq_vectors(pdev);
> > +out_disable:
> > +     pci_clear_master(pdev);
> > +     return rc;
> > +}
> > +
> > +/*
> > + * selpvmf_remove - Remove a selpvmf enabled PCI device from the
> > +system
> > + *
> > + * @pdev: PCI device to be removed
> > + */
> > +static void selpvmf_remove(struct pci_dev *pdev) {
> > +     mfd_remove_devices(&pdev->dev);
> 
> Use devm_* instead,

Didn't realize there was a devm version, I'll use it.

> 
> > +     pci_free_irq_vectors(pdev);
> > +     pci_clear_master(pdev);
> > +}
> > +
> > +/*
> > + * selpvmf_shutdown() - Tears down a selpvmf card.
> > + *
> > + * @pdev: Pointer to the device structure describing the port.
> > + */
> > +static void selpvmf_shutdown(struct pci_dev *pdev) {
> > +     mfd_remove_devices(&pdev->dev);
> 
> As above.
> 
> > +}
> > +
> > +/*
> > + * selpvmf_suspend - Suspend the device in preparation for entering a
> > + * a low power state.
> > + *
> > + * @dev: Pointer to the device structure describing the port.
> > + *
> > + * Return: This function always indicates success.
> > + *
> > + * Most of the work should be handled by the child devices of this
> > + * driver. There is no specific action necessary by the card, so when
> > + * the child devices are done suspending, just do a generic PCI
> > +saving
> > + * of state.
> > + */
> > +static int selpvmf_suspend(struct device *dev) {
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > +     pci_save_state(pdev);
> > +
> > +     return 0;
> > +}
> > +
> > +/*
> > + * selpvmf_resume() - Resume a selpvmf device that has previously
> > +been
> > + * suspended
> > + *
> > + * @dev: Pointer to the device structure describing the port.
> > + *
> > + * Return: This function always indicates success.
> > + */
> > +static int selpvmf_resume(struct device *dev) {
> > +     struct pci_dev *pdev = to_pci_dev(dev);
> > +     u16 cvp_capable;
> > +     int rc;
> > +
> > +     /* Turn the card on by requesting a transition to D0 before
> > +      * restoring the PCI device state and enabling the UARTs
> > +      */
> > +     pci_set_power_state(pdev, PCI_D0);
> > +
> > +     /* Restore so we can do IOMEM based CvP */
> > +     pci_restore_state(pdev);
> > +
> > +     rc = pcim_enable_device(pdev);
> > +     if (rc != 0) {
> > +             /* We are not really supposed to return an error from this
> > +              * function, so if the enable fails, just print some info
> > +              * to aid in troubleshooting.
> > +              */
> 
> What?  It returns an int for a reason, please use it.

I'll update this.

> 
> > +             dev_err(&pdev->dev,
> > +                     "Unable to enable the device. Continuing anyway\n");
> > +     }
> > +
> > +     /* Perform CvP program if necessary */
> > +     cvp_capable = pci_find_ext_capability(pdev, CVP_CAPABILITY_ID);
> > +     if (!skipcvp && cvp_capable) {
> > +             pci_save_state(pdev);
> > +
> > +             rc = cvp_start(pdev);
> > +             if (rc != 0) {
> > +                     dev_err(&pdev->dev,
> > +                             "Error: CvP failed (resume): 0x%x\n",
> > + rc);
> 
> You can use up to 100-chars to prevent this kind of wrapping.

I'll update this.

> 
> > +             }
> > +             dev_info(&pdev->dev, "CvP done (resume): 0x%x\n", rc);
> 
> Remove this please.

I'll remove the message.

> 
> > +
> > +             pci_restore_state(pdev);
> > +     }
> > +
> > +     pci_set_master(pdev);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dev_pm_ops selpvmf_pm_ops = {
> > +     SYSTEM_SLEEP_PM_OPS(selpvmf_suspend, selpvmf_resume) };
> > +
> > +static struct pci_device_id selpvmf_pci_tbl[] = {
> > +     {
> > +             PCI_DEVICE(PCI_VENDOR_ID_SEL, PCI_DEVICE_ID_B1190),
> > +             .driver_data = (kernel_ulong_t)b1190_alloc_mfd_cells,
> 
> Pass an identifier here instead of call-back functions.

Do you have a preference for the type of construct we use? Just a #define integer with a switch statement in probe? Something else?

> 
> > +     },
> > +     {
> > +             PCI_DEVICE(PCI_VENDOR_ID_SEL, PCI_DEVICE_ID_B2077),
> > +             .driver_data = (kernel_ulong_t)b2077_alloc_mfd_cells,
> > +     },
> > +     {
> > +             PCI_DEVICE(PCI_VENDOR_ID_SEL, PCI_DEVICE_ID_B2091),
> > +             .driver_data = (kernel_ulong_t)b2091_alloc_mfd_cells,
> > +     },
> > +     {
> > +             PCI_DEVICE(PCI_VENDOR_ID_SEL, PCI_DEVICE_ID_B2093),
> > +             .driver_data = (kernel_ulong_t)b2093_alloc_mfd_cells,
> > +     },
> > +     {0,}
> 
> What is this?  Where have you seen that before?

The {0,} is how to signal the end of this array. I don't think the comma is required though. Random example: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/lpc_sch.c?h=v6.12-rc6#n66

> 
> > +};
> > +MODULE_DEVICE_TABLE(pci, selpvmf_pci_tbl);
> > +
> > +static struct pci_driver selpvmf_driver = {
> > +     .name           = "selpvmf",
> > +     .id_table       = selpvmf_pci_tbl,
> > +     .probe          = selpvmf_probe,
> > +     .remove         = selpvmf_remove,
> > +     .shutdown       = selpvmf_shutdown,
> > +     .driver.pm      = pm_ptr(&selpvmf_pm_ops),
> > +};
> > +module_pci_driver(selpvmf_driver);
> > +
> > +MODULE_LICENSE("Dual BSD/GPL");
> > +MODULE_AUTHOR("Schweitzer Engineering Laboratories, Inc.");
> > +MODULE_DESCRIPTION("Multi function driver for enumerating multiple
> > +device drivers for a single PCI resource"); MODULE_DEVICE_TABLE(pci,
> > +selpvmf_pci_tbl); MODULE_FIRMWARE("sel/1AA9_0014_01.cvp");
> > +MODULE_FIRMWARE("sel/1AA9_0015_01.cvp");
> > +MODULE_FIRMWARE("sel/1AA9_0018_01.cvp");
> > diff --git a/drivers/mfd/selpvmf-cvp.c b/drivers/mfd/selpvmf-cvp.c
> 
> I'm not sure what all of this is, but I suggest that it doesn't live here.
> drivers/firmware or drivers/fpga perhaps?

I'll start a discussion with the FPGA manager maintainers to see what makes most sense. We can probably make this a library driver there.

> 
> > new file mode 100644
> > index 000000000000..e119cce83d7c
> > --- /dev/null
> > +++ b/drivers/mfd/selpvmf-cvp.c
> > @@ -0,0 +1,431 @@
> > +// SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause
> > +/*
> > + * Copyright 2017 Schweitzer Engineering Laboratories, Inc.
> > + * 2350 NE Hopkins Court, Pullman, WA 99163 USA
> > + * SEL Open Source <opensource@...inc.com>
> 
> Do you really need the whole postal address here?
> 
> I suggest this is legacy of a time well past.

We can probably remove this.

> 
> > + *
> > + * Support for Configuration via Protocol (CvP) on SEL devices using
> > + * Altera FPGAs.
> > + */
> > +
> > +#include <linux/firmware.h>
> > +#include <linux/pci.h>
> > +#include <linux/sched.h>
> > +
> > +#include "selpvmf-cvp.h"
> > +
> > +#define MIN_WAIT 2 /* number of jiffies for unit wait period */
> 
> Comments should start with an upper case char.
> 
> > +#define US_PER_JIFFY  (1000000 / HZ) /* number of microseconds per
> > +jiffy */
> 
> Please tab all of these out.

I'll update the formatting and grammar.

> 
> > +#define CVP_WAIT_TIMEOUT 1000000 /* Wait timeout in microseconds */
> > +#define BYTES_PER_DWORD      4
> > +#define CORE_FIRMWARE_FILE_BASEDIR "sel/"
> > +#define CORE_FIRMWARE_FILE_EXT ".cvp"
> > +
> > +#define CVP_STATUS_OFFSET    0x0000001C
> > +#define CVP_CONFIG_READY     BIT(18)
> > +#define CVP_CONFIG_ERROR     BIT(19)
> > +#define CVP_EN                       BIT(20)
> > +#define USERMODE             BIT(21)
> > +#define PLD_CLK_IN_USE               BIT(24)
> > +
> > +#define CVP_MODE_CONTROL_OFFSET              0x00000020
> > +#define CVP_MODE                     BIT(0)
> > +#define HIP_CLK_SEL                  BIT(1)
> > +#define CVP_NUMCLKS_MASK             GENMASK(15, 8)
> > +#define CVP_NUMCLKS_UNCMPRSSD_UNENCRYPT      BIT(8)
> > +#define CVP_NUMCLKS_COMPRESSED               BIT(11)
> > +#define CVP_NUMCLKS_ENCRYPTED                BIT(10)
> > +
> > +#define CVP_DATA_OFFSET      0x00000028
> > +
> > +#define CVP_PROGRAMMING_CTRL_OFFSET  0x0000002C
> > +#define CVP_CONFIG                   BIT(0)
> > +#define START_XFER                   BIT(1)
> > +
> > +#define CVP_UNCORRECTABLE_IE_STATUS_OFFSET   0x00000034
> > +#define CVP_CONFIG_ERROR_LATCHED             BIT(5)
> 
> I'm going to stop here.  Please address the issues before resubmitting.
> 
> --
> Lee Jones [李琼斯]

Thank you for the review! I'll make these changes and start a discussion regarding the best path forward to integrate loading the FPGA firmware.

Thanks,
Robert

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ