| 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
| ||
|
Message-ID: <20241112133518.GF8552@google.com>
Date: Tue, 12 Nov 2024 13:35:18 +0000
From: Lee Jones <lee@...nel.org>
To: Robert Joslyn <robert_joslyn@...inc.com>
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
On Thu, 07 Nov 2024, Robert Joslyn wrote:
> > -----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)
These should not end up in the mail's body.
> > 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
[...]
> > > +/**
> > > + * 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.
Please refrain from responding to comments that you agree with, then
trim the rest. It will save multiple people a lot of time.
>
> >
> > > + 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.
I can only review what I see before me.
Add the extra pieces when/if you upstream the additional functionality.
[...]
> > > + 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.
The latter seems more consistent.
[...]
> > > + 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"?
Sounds good.
[...]
> >
> > > + 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?
That sort of thing, yes.
[...]
> > > + + 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?
Probably an enum. Take a look at some other drivers.
> > > + }, + { + 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
{} is fine.
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists