| 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:
<MW4PR18MB5244A08DC9D2CF3FAF14314FA6192@MW4PR18MB5244.namprd18.prod.outlook.com>
Date: Wed, 1 May 2024 07:46:55 +0000
From: Vamsi Krishna Attunuru <vattunuru@...vell.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: "arnd@...db.de" <arnd@...db.de>, Jerin Jacob <jerinj@...vell.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXTERNAL] Re: [PATCH v6 1/1] misc: mrvl-cn10k-dpi: add Octeon
CN10K DPI administrative driver
> -----Original Message-----
> From: Greg KH <gregkh@...uxfoundation.org>
> Sent: Monday, April 29, 2024 2:44 PM
> To: Vamsi Krishna Attunuru <vattunuru@...vell.com>
> Cc: arnd@...db.de; Jerin Jacob <jerinj@...vell.com>; linux-
> kernel@...r.kernel.org
> Subject: [EXTERNAL] Re: [PATCH v6 1/1] misc: mrvl-cn10k-dpi: add Octeon
> CN10K DPI administrative driver
>
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
>
> ----------------------------------------------------------------------
> On Sun, Apr 28, 2024 at 10:50:27PM -0700, Vamsi Attunuru wrote:
> > Adds a misc driver for Marvell CN10K DPI(DMA Engine) device's physical
> > function which initializes DPI DMA hardware's global configuration and
> > enables hardware mailbox channels between physical function (PF) and
> > it's virtual functions (VF). VF device drivers (User space drivers)
> > use this hw mailbox to communicate any required device configuration
> > on it's respective VF device. Accordingly, this DPI PF driver
> > provisions the VF device resources.
> >
> > At the hardware level, the DPI physical function (PF) acts as a
> > management interface to setup the VF device resources, VF devices are
> > only provisioned to handle or control the actual DMA Engine's data transfer
> capabilities.
> >
> > Signed-off-by: Vamsi Attunuru <vattunuru@...vell.com>
>
> No one else at Marvell has reviewed this before you submitted this?
It was reviewed until V3, sorry for the inconvenience. I will get the next version
thoroughly reviewed and will send with reviewed-by sign off.
>
> > ---
> > Changes V5 -> V6:
> > - Updated documentation
> > - Fixed data types in uapi file
> >
> > Changes V4 -> V5:
> > - Fixed license and data types in uapi file
> >
> > Changes V3 -> V4:
> > - Moved ioctl definations to .h file
> > - Fixed structure alignements which are passed in ioctl
> >
> > Changes V2 -> V3:
> > - Added ioctl operation to the fops
> > - Used managed version of kzalloc & request_irq
> > - Addressed miscellaneous comments
> >
> > Changes V1 -> V2:
> > - Fixed return values and busy-wait loops
> > - Merged .h file into .c file
> > - Fixed directory structure
> > - Removed module params
> > - Registered the device as misc device
> >
> > Documentation/misc-devices/index.rst | 1 +
> > Documentation/misc-devices/mrvl_cn10k_dpi.rst | 57 ++
> > .../userspace-api/ioctl/ioctl-number.rst | 1 +
> > MAINTAINERS | 5 +
> > drivers/misc/Kconfig | 13 +
> > drivers/misc/Makefile | 2 +
> > drivers/misc/mrvl_cn10k_dpi.c | 685 ++++++++++++++++++
> > include/uapi/misc/mrvl_cn10k_dpi.h | 37 +
> > 8 files changed, 801 insertions(+)
> >
> > diff --git a/Documentation/misc-devices/index.rst
> > b/Documentation/misc-devices/index.rst
> > index 2d0ce9138588..10f2e0f74e45 100644
> > --- a/Documentation/misc-devices/index.rst
> > +++ b/Documentation/misc-devices/index.rst
> > @@ -20,6 +20,7 @@ fit into other categories.
> > ics932s401
> > isl29003
> > lis3lv02d
> > + mrvl_cn10k_dpi
> > max6875
> > oxsemi-tornado
> > pci-endpoint-test
>
> Why not in sorted order?
Ack, will fix it.
>
> > diff --git a/Documentation/misc-devices/mrvl_cn10k_dpi.rst
> > b/Documentation/misc-devices/mrvl_cn10k_dpi.rst
> > new file mode 100644
> > index 000000000000..cce202f114b7
> > --- /dev/null
> > +++ b/Documentation/misc-devices/mrvl_cn10k_dpi.rst
> > @@ -0,0 +1,57 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +===============================================
> > +Marvell CN10K DMA packet interface (DPI) driver
> > +===============================================
> > +
> > +Overview
> > +========
> > +
> > +DPI is a DMA packet interface hardware block in Marvell's CN10K silicon.
> > +DPI hardware comprises a physical function (PF), its virtual
> > +functions, mailbox logic, and a set of DMA engines & DMA command
> queues.
> > +
> > +DPI PF function is an administrative function which services the
> > +mailbox requests from its VF functions and provisions DMA engine
> > +resources to it's VF functions.
> > +
> > +mrvl_cn10k_dpi.ko misc driver loads on DPI PF device and services the
> > +mailbox commands submitted by the VF devices and accordingly
> > +initializes the DMA engines and VF device's DMA command queues. Also,
> > +driver creates /dev/mrvl-cn10k-dpi node to set DMA engine and PEM
> > +(PCIe interface) port attributes like fifo length, molr, mps & mrrs.
> > +
> > +DPI PF driver is just an administrative driver to setup its VF
> > +device's queues and provisions the hardware resources, it can not
> > +initiate any DMA operations. Only VF devices are provisioned with DMA
> capabilities.
> > +
> > +Driver location
> > +===============
> > +
> > +drivers/misc/mrvl_cn10k_dpi.c
> > +
> > +Driver IOCTLs
> > +=============
> > +
> > +:c:macro::`DPI_MPS_MRRS_CFG`
> > +ioctl that sets max payload size & max read request size parameters
> > +of a pem port to which DMA engines are wired.
> > +
> > +
> > +:c:macro::`DPI_ENGINE_CFG`
> > +ioctl that sets DMA engine's fifo sizes & max outstanding load
> > +request thresholds.
> > +
> > +Userspace code example
> > +----------------------
> > +
> > +DPI VF devices are managed by user space drivers, below is a
> > +reference code to the user space driver's mailbox command exchange
> > +with DPI PF driver through hardware mailbox.
> > +
> > +https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_Vamsi
> > +KrishnaA99_dpi-2Ddma_blob_main_driver_roc-
> 5Fdpi.c&d=DwIBAg&c=nKjWec2b
> >
> +6R0mOyPaz7xtfQ&r=WllrYaumVkxaWjgKto6E_rtDQshhIhik2jkvzFyRhW8&m
> =7PlVgM
> > +7n0EhJ17QxPm3mXq6PT0CQzsOn2YFnRB8RMsIQdFAsZ5UKdTFmkcChUrf-
> &s=8wLYfXsc
> > +lLz7Fuvz9FfyEehC5xTSiIa88PJzturPF4U&e=
> > +
> > +Below is a sample application that uses driver IOCTLs to setup DMA
> > +engine and PEM port attributes over `/dev/mrvl-cn10k-dpi` node.
> > +
> > +https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_Vamsi
> > +KrishnaA99_dpi-
> 2Ddma_blob_main_application_main.c&d=DwIBAg&c=nKjWec2b
> >
> +6R0mOyPaz7xtfQ&r=WllrYaumVkxaWjgKto6E_rtDQshhIhik2jkvzFyRhW8&m
> =7PlVgM
> > +7n0EhJ17QxPm3mXq6PT0CQzsOn2YFnRB8RMsIQdFAsZ5UKdTFmkcChUrf-
> &s=BMRvLLAx
> > +cnlsyZlVCgOoUXju_jKom8zbKQ9GxlxlApg&e=
>
> I appreciate the code being open, but this is on a personal account that was
> created for only this one package, and it doesn't even have a README. Yet it
> is fully owned/copyrighted by Marvell and the code has been around since
> 2021? Why isn't this on a Marvell-controlled page to show that this really is
> supported and is the proper interface to use this driver that it provides to
> their customers?
>
User space driver is already part of open-source code base(github.com/DPDK/dpdk.git).
The mailbox related changes in user driver (roc_dpi.c file) are not yet upstreamed
which has the dependency with this kernel driver. So, I put the latest user driver
in personal account and shared so that it's helpful for reviewing kernel driver.
I will work internally and get it moved to Marvell-controlled page, will host at
https://github.com/MarvellEmbeddedProcessors and update the same in
Documentation page in next version.
>
>
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 457e16f06e04..e6fd0c386b59 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -358,6 +358,7 @@ Code Seq# Include File
> Comments
> > 0xB6 all linux/fpga-dfl.h
> > 0xB7 all uapi/linux/remoteproc_cdev.h <mailto:linux-
> remoteproc@...r.kernel.org>
> > 0xB7 all uapi/linux/nsfs.h <mailto:Andrei Vagin
> <avagin@...nvz.org>>
> > +0xB8 01-02 uapi/misc/mrvl_cn10k_dpi.h Marvell CN10K DPI
> driver
> > 0xC0 00-0F linux/usb/iowarrior.h
> > 0xCA 00-0F uapi/misc/cxl.h
> > 0xCA 10-2F uapi/misc/ocxl.h
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 960512bec428..ab77232d583e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13104,6 +13104,11 @@ S: Supported
> > F: Documentation/devicetree/bindings/mmc/marvell,xenon-
> sdhci.yaml
> > F: drivers/mmc/host/sdhci-xenon*
> >
> > +MARVELL OCTEON CN10K DPI DRIVER
> > +M: Vamsi Attunuru <vattunuru@...vell.com>
> > +S: Maintained
>
> So this is not part of your job to support this code? Why isn't Marvell allowing
> that to happen?
No, we are happy to support it further, will fix this.
>
> > +F: drivers/misc/mrvl_cn10k_dpi.c
> > +
> > MATROX FRAMEBUFFER DRIVER
> > L: linux-fbdev@...r.kernel.org
> > S: Orphan
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
> > 4fb291f0bf7c..78470ef2538f 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -574,6 +574,19 @@ config NSM
> > To compile this driver as a module, choose M here.
> > The module will be called nsm.
> >
> > +config MARVELL_CN10K_DPI
> > + tristate "Octeon CN10K DPI driver"
> > + depends on ARM64 && PCI
> > + help
> > + Enables Octeon CN10K DMA packet interface (DPI) driver which
> intializes
> > + DPI hardware's physical function (PF) device's global configuration
> and
> > + its virtual function's (VFs) resource configuration to enable DMA
> transfers.
> > + DPI PF device does not have any data movement functionality, it
> only serves
> > + VF's resource configuration requests.
>
> Nit, please wrap at 72 columns, didn't checkpatch.pl complain about this?
>
Yes, I did not see any complains, not sure if I am missing any options. I will
double check and ensure clean patch in next version.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called mrvl_cn10k_dpi.
> > +
> > source "drivers/misc/c2port/Kconfig"
> > source "drivers/misc/eeprom/Kconfig"
> > source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
> > ea6ea5bbbc9c..5106bf96ea5c 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -68,3 +68,5 @@ obj-$(CONFIG_TMR_INJECT) += xilinx_tmr_inject.o
> > obj-$(CONFIG_TPS6594_ESM) += tps6594-esm.o
> > obj-$(CONFIG_TPS6594_PFSM) += tps6594-pfsm.o
> > obj-$(CONFIG_NSM) += nsm.o
> > +obj-$(CONFIG_MARVELL_CN10K_DPI) += mrvl_cn10k_dpi.o
> > +obj-y += mrvl_cn10k_dpi.o
>
> That is odd, why are you saying obj-y here? You want everyone to always
> build this code into the kernel image no matter what
>
Sorry, it's my bad, this change(2nd line) was mistakenly added, will remove it.
> > diff --git a/drivers/misc/mrvl_cn10k_dpi.c
> > b/drivers/misc/mrvl_cn10k_dpi.c new file mode 100644 index
> > 000000000000..bd99583994f9
> > --- /dev/null
> > +++ b/drivers/misc/mrvl_cn10k_dpi.c
> > @@ -0,0 +1,685 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell Octeon CN10K DPI driver
> > + *
> > + * Copyright (C) 2024 Marvell.
> > + *
> > + */
> > +
> > +#include <linux/compat.h>
> > +#include <linux/delay.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > +
> > +#include <uapi/misc/mrvl_cn10k_dpi.h>
> > +
> > +#define DPI_DRV_NAME "mrvl-cn10k-dpi"
>
> KBUILD_MODNAME please.
ack
>
> > +
> > +/* PCI device IDs */
> > +#define PCI_DEVID_MRVL_CN10K_DPI_PF 0xA080
> > +#define PCI_SUBDEVID_MRVL_CN10K_DPI_PF 0xB900
> > +
> > +/* PCI BAR nos */
>
> What is "nos"?
It's numbers, will add full wording.
>
> > +#define PCI_DPI_CFG_BAR 0
> > +
> > +/* MSI-X interrupts */
> > +#define DPI_MAX_REQQ_INT 32
> > +#define DPI_MAX_CC_INT 64
> > +
> > +/* MBOX MSI-X interrupt vector index */ #define
> > +DPI_MBOX_PF_VF_INT_IDX 0x75
> > +
> > +#define DPI_MAX_IRQS (DPI_MBOX_PF_VF_INT_IDX + 1)
> > +
> > +#define DPI_MAX_VFS 32
> > +
> > +#define DPI_ENGINE_MASK GENMASK(2, 0)
> > +
> > +#define DPI_DMA_IDS_DMA_NPA_PF_FUNC(x) (((x) &
> GENMASK_ULL(15, 0)) << 16)
> > +#define DPI_DMA_IDS_INST_STRM(x) (((x) &
> GENMASK_ULL(7, 0)) << 40)
> > +#define DPI_DMA_IDS_DMA_STRM(x) (((x) &
> GENMASK_ULL(7, 0)) << 32)
> > +#define DPI_DMA_ENG_EN_MOLR(x) (((x) &
> GENMASK_ULL(9, 0)) << 32)
> > +#define DPI_EBUS_PORTX_CFG_MPS(x) (((x) & GENMASK(2,
> 0)) << 4)
> > +#define DPI_DMA_IDS_DMA_SSO_PF_FUNC(x) ((x) &
> GENMASK(15, 0))
> > +#define DPI_DMA_IDS2_INST_AURA(x) ((x) & GENMASK(19,
> 0))
> > +#define DPI_DMA_IBUFF_CSIZE_CSIZE(x) ((x) & GENMASK(13,
> 0))
> > +#define DPI_EBUS_PORTX_CFG_MRRS(x) ((x) & GENMASK(2,
> 0))
> > +#define DPI_ENG_BUF_BLKS(x) ((x) & GENMASK(5,
> 0))
> > +#define DPI_DMA_CONTROL_DMA_ENB
> GENMASK_ULL(53, 48)
> > +
> > +#define DPI_DMA_CONTROL_O_MODE BIT_ULL(14)
> > +#define DPI_DMA_CONTROL_LDWB BIT_ULL(32)
> > +#define DPI_DMA_CONTROL_WQECSMODE1 BIT_ULL(37)
> > +#define DPI_DMA_CONTROL_ZBWCSEN BIT_ULL(39)
> > +#define DPI_DMA_CONTROL_WQECSOFF(offset) (((u64)offset) << 40)
> > +#define DPI_DMA_CONTROL_WQECSDIS BIT_ULL(47)
> > +#define DPI_DMA_CONTROL_PKT_EN BIT_ULL(56)
> > +#define DPI_DMA_IBUFF_CSIZE_NPA_FREE BIT(16)
> > +
> > +#define DPI_CTL_EN BIT_ULL(0)
> > +#define DPI_DMA_CC_INT BIT_ULL(0)
> > +#define DPI_DMA_QRST BIT_ULL(0)
> > +
> > +#define DPI_REQQ_INT_INSTRFLT BIT_ULL(0)
> > +#define DPI_REQQ_INT_RDFLT BIT_ULL(1)
> > +#define DPI_REQQ_INT_WRFLT BIT_ULL(2)
> > +#define DPI_REQQ_INT_CSFLT BIT_ULL(3)
> > +#define DPI_REQQ_INT_INST_DBO BIT_ULL(4)
> > +#define DPI_REQQ_INT_INST_ADDR_NULL BIT_ULL(5)
> > +#define DPI_REQQ_INT_INST_FILL_INVAL BIT_ULL(6)
> > +#define DPI_REQQ_INT_INSTR_PSN BIT_ULL(7)
> > +
> > +#define DPI_REQQ_INT \
> > + (DPI_REQQ_INT_INSTRFLT | \
> > + DPI_REQQ_INT_RDFLT | \
> > + DPI_REQQ_INT_WRFLT | \
> > + DPI_REQQ_INT_CSFLT | \
> > + DPI_REQQ_INT_INST_DBO | \
> > + DPI_REQQ_INT_INST_ADDR_NULL | \
> > + DPI_REQQ_INT_INST_FILL_INVAL | \
> > + DPI_REQQ_INT_INSTR_PSN)
> > +
> > +#define DPI_PF_RAS_EBI_DAT_PSN BIT_ULL(0)
> > +#define DPI_PF_RAS_NCB_DAT_PSN BIT_ULL(1)
> > +#define DPI_PF_RAS_NCB_CMD_PSN BIT_ULL(2)
> > +
> > +#define DPI_PF_RAS_INT \
> > + (DPI_PF_RAS_EBI_DAT_PSN | \
> > + DPI_PF_RAS_NCB_DAT_PSN | \
> > + DPI_PF_RAS_NCB_CMD_PSN)
> > +
> > +#define DPI_DMAX_IBUFF_CSIZE(x) (0x0ULL | ((x) << 11))
> > +#define DPI_DMAX_IDS(x) (0x18ULL | ((x) << 11))
> > +#define DPI_DMAX_IDS2(x) (0x20ULL | ((x) << 11))
> > +#define DPI_DMAX_QRST(x) (0x30ULL | ((x) << 11))
> > +
> > +#define DPI_CTL 0x10010ULL
> > +#define DPI_DMA_CONTROL 0x10018ULL
> > +#define DPI_DMA_ENGX_EN(x) (0x10040ULL | ((x) << 3))
> > +#define DPI_ENGX_BUF(x) (0x100C0ULL | ((x) << 3))
> > +
> > +#define DPI_EBUS_PORTX_CFG(x) (0x10100ULL | ((x) << 3))
> > +
> > +#define DPI_PF_RAS 0x10308ULL
> > +#define DPI_PF_RAS_ENA_W1C 0x10318ULL
> > +
> > +#define DPI_DMA_CCX_INT(x) (0x11000ULL | ((x) << 3)) #define
> > +DPI_DMA_CCX_INT_ENA_W1C(x) (0x11800ULL | ((x) << 3))
> > +
> > +#define DPI_REQQX_INT(x) (0x12C00ULL | ((x) << 5)) #define
> > +DPI_REQQX_INT_ENA_W1C(x) (0x13800ULL | ((x) << 5))
> > +
> > +#define DPI_MBOX_PF_VF_DATA0(x) (0x16000ULL | ((x) << 4)) #define
> > +DPI_MBOX_PF_VF_DATA1(x) (0x16008ULL | ((x) << 4))
> > +
> > +#define DPI_MBOX_VF_PF_INT 0x16300ULL #define
> DPI_MBOX_VF_PF_INT_W1S
> > +0x16308ULL #define DPI_MBOX_VF_PF_INT_ENA_W1C 0x16310ULL
> #define
> > +DPI_MBOX_VF_PF_INT_ENA_W1S 0x16318ULL
> > +
> > +#define DPI_WCTL_FIF_THR 0x17008ULL
> > +
> > +#define DPI_EBUS_MAX_PORTS 2
> > +
> > +#define DPI_EBUS_MRRS_MIN 128
> > +#define DPI_EBUS_MRRS_MAX 1024
> > +#define DPI_EBUS_MPS_MIN 128
> > +#define DPI_EBUS_MPS_MAX 1024
> > +#define DPI_WCTL_FIFO_THRESHOLD 0x30
> > +
> > +#define DPI_QUEUE_OPEN 0x1
> > +#define DPI_QUEUE_CLOSE 0x2
> > +#define DPI_REG_DUMP 0x3
> > +#define DPI_GET_REG_CFG 0x4
> > +#define DPI_QUEUE_OPEN_V2 0x5
> > +
> > +enum dpi_mbox_rsp_type {
> > + DPI_MBOX_TYPE_CMD,
> > + DPI_MBOX_TYPE_RSP_ACK,
> > + DPI_MBOX_TYPE_RSP_NACK,
> > +};
> > +
> > +struct dpivf_config {
> > + u16 csize;
> > + u32 aura;
> > + u16 sso_pf_func;
> > + u16 npa_pf_func;
>
> Do you intend to have unaligned accesses to the fields in this structure?
>
No, I will fix it.
> > +};
> > +
> > +struct dpipf_vf {
> > + u8 this_vfid;
> > + bool setup_done;
> > + struct dpivf_config vf_config;
> > +};
> > +
> > +/* DPI device mailbox */
> > +struct dpi_mbox {
> > + struct work_struct work;
> > + /* lock to serialize mbox requests */
> > + struct mutex lock;
> > + struct dpipf *pf;
> > + u8 __iomem *pf_vf_data_reg;
> > + u8 __iomem *vf_pf_data_reg;
> > +};
> > +
> > +struct dpipf {
> > + struct miscdevice miscdev;
> > + void __iomem *reg_base;
> > + struct pci_dev *pdev;
> > + struct dpipf_vf vf[DPI_MAX_VFS];
> > + /* Mailbox to talk to VFs */
> > + struct dpi_mbox *mbox[DPI_MAX_VFS];
> > +};
> > +
> > +union dpi_mbox_message_t {
>
> Didn't checkpatch complain about the "_t" here?
>
No, I will double check and fix it.
> > + u64 u[2];
>
> What is "u"?
>
> > + struct dpi_mbox_message_s {
> > + /* VF ID to configure */
> > + u64 vfid :8;
> > + /* Command code */
> > + u64 cmd :4;
> > + /* Command buffer size in 8-byte words */
> > + u64 csize :14;
> > + /* Aura of the command buffer */
> > + u64 aura :20;
> > + /* SSO PF function */
> > + u64 sso_pf_func :16;
> > + /* NPA PF function */
> > + u64 npa_pf_func :16;
> > + /* Work queue completion status enable */
> > + u64 wqecs :1;
> > + /* Work queue completion status byte offset */
> > + u64 wqecsoff :7;
> > + /* Reserved */
> > + u64 rsvd :42;
>
> reserved for what?
>
> If you are reading this from hardware, the bit fields you created here will
> NOT work or be portable at all. Please do this properly.
Sure, I will fix it properly.
>
> Also you have a mix of tabs and spaces for some reason in this structure,
> again, checkpatch should have caught that.
>
> > + } s __packed;
>
> "s"?
>
> What is "u" and "s" here?
>
>
> > +};
> > +
> > +static inline void dpi_reg_write(struct dpipf *dpi, u64 offset, u64
> > +val) {
> > + writeq(val, dpi->reg_base + offset);
>
> No need to read to ensure that the write succeeded? Or are you doing that
> in individual places where you want to make sure it happened? If so, I didn't
> see that in the use of this.
Yes, writes are guaranteed as it's an onboard pcie device and register writes
are uncached memory accesses.
>
> > +}
> > +
> > +static inline u64 dpi_reg_read(struct dpipf *dpi, u64 offset) {
> > + return readq(dpi->reg_base + offset); }
> > +
> > +static void dpi_wqe_cs_offset(struct dpipf *dpi, u8 offset) {
> > + u64 reg;
> > +
> > + reg = dpi_reg_read(dpi, DPI_DMA_CONTROL);
> > + reg &= ~DPI_DMA_CONTROL_WQECSDIS;
> > + reg |= DPI_DMA_CONTROL_ZBWCSEN |
> DPI_DMA_CONTROL_WQECSMODE1;
> > + reg |= DPI_DMA_CONTROL_WQECSOFF(offset);
> > + dpi_reg_write(dpi, DPI_DMA_CONTROL, reg); }
> > +
> > +static int dpi_queue_init(struct dpipf *dpi, struct dpipf_vf *dpivf,
> > +u8 vf) {
> > + u16 sso_pf_func = dpivf->vf_config.sso_pf_func;
> > + u16 npa_pf_func = dpivf->vf_config.npa_pf_func;
> > + u16 csize = dpivf->vf_config.csize;
> > + u32 aura = dpivf->vf_config.aura;
> > + unsigned long timeout;
> > + u64 reg;
> > +
> > + dpi_reg_write(dpi, DPI_DMAX_QRST(vf), DPI_DMA_QRST);
> > +
> > + /* Wait for a maximum of 3 sec */
> > + timeout = jiffies + msecs_to_jiffies(3000);
> > + while (!time_after(jiffies, timeout)) {
> > + reg = dpi_reg_read(dpi, DPI_DMAX_QRST(vf));
> > + if (!(reg & DPI_DMA_QRST))
> > + break;
> > +
> > + usleep_range(500, 1000);
>
> Why sleep this value? Please document.
ack
>
> > + }
> > +
> > + if (reg & DPI_DMA_QRST) {
> > + dev_err(&dpi->pdev->dev, "Queue reset failed\n");
> > + return -EBUSY;
> > + }
> > +
> > + dpi_reg_write(dpi, DPI_DMAX_IDS2(vf), 0);
> > + dpi_reg_write(dpi, DPI_DMAX_IDS(vf), 0);
> > +
> > + reg = DPI_DMA_IBUFF_CSIZE_CSIZE(csize) |
> DPI_DMA_IBUFF_CSIZE_NPA_FREE;
> > + dpi_reg_write(dpi, DPI_DMAX_IBUFF_CSIZE(vf), reg);
> > +
> > + reg = dpi_reg_read(dpi, DPI_DMAX_IDS2(vf));
> > + reg |= DPI_DMA_IDS2_INST_AURA(aura);
> > + dpi_reg_write(dpi, DPI_DMAX_IDS2(vf), reg);
> > +
> > + reg = dpi_reg_read(dpi, DPI_DMAX_IDS(vf));
> > + reg |= DPI_DMA_IDS_DMA_NPA_PF_FUNC(npa_pf_func);
> > + reg |= DPI_DMA_IDS_DMA_SSO_PF_FUNC(sso_pf_func);
> > + reg |= DPI_DMA_IDS_DMA_STRM(vf + 1);
> > + reg |= DPI_DMA_IDS_INST_STRM(vf + 1);
> > + dpi_reg_write(dpi, DPI_DMAX_IDS(vf), reg);
> > +
> > + return 0;
> > +}
> > +
> > +static void dpi_queue_fini(struct dpipf *dpi, struct dpipf_vf *dpivf,
> > +u8 vf) {
> > + dpi_reg_write(dpi, DPI_DMAX_QRST(vf), DPI_DMA_QRST);
> > +
> > + /* Reset IDS and IDS2 registers */
> > + dpi_reg_write(dpi, DPI_DMAX_IDS2(vf), 0);
> > + dpi_reg_write(dpi, DPI_DMAX_IDS(vf), 0); }
> > +
> > +static void dpi_poll_pfvf_mbox(struct dpipf *dpi) {
> > + u64 reg;
> > + u32 vf;
> > +
> > + reg = dpi_reg_read(dpi, DPI_MBOX_VF_PF_INT);
> > + if (reg) {
> > + for (vf = 0; vf < pci_num_vf(dpi->pdev); vf++) {
> > + if (reg & BIT_ULL(vf))
> > + schedule_work(&dpi->mbox[vf]->work);
> > + }
> > + dpi_reg_write(dpi, DPI_MBOX_VF_PF_INT, reg);
> > + }
>
> No error if reg was not read properly?
Yes, ideally no errors as it's an on board pcie device.
>
> > +}
> > +
> > +static irqreturn_t dpi_mbox_intr_handler(int irq, void *data) {
> > + struct dpipf *dpi = data;
> > +
> > + dpi_poll_pfvf_mbox(dpi);
> > +
> > + return IRQ_HANDLED;
>
> So this can always succeed?
Yes. It's msix interrupt and not shared, I will check and fix if any thing
need to be validated.
>
> > +}
> > +
> > +static int queue_config(struct dpipf *dpi, struct dpipf_vf *dpivf,
> > +union dpi_mbox_message_t *msg) {
> > + int ret = 0;
> > +
> > + switch (msg->s.cmd) {
> > + case DPI_QUEUE_OPEN:
> > + case DPI_QUEUE_OPEN_V2:
> > + dpivf->vf_config.aura = msg->s.aura;
> > + dpivf->vf_config.csize = msg->s.cmd == DPI_QUEUE_OPEN ?
> msg->s.csize / 8 :
> > + msg->s.csize;
> > + dpivf->vf_config.sso_pf_func = msg->s.sso_pf_func;
> > + dpivf->vf_config.npa_pf_func = msg->s.npa_pf_func;
> > + ret = dpi_queue_init(dpi, dpivf, msg->s.vfid);
> > + if (!ret) {
> > + if (msg->s.wqecs)
> > + dpi_wqe_cs_offset(dpi, msg->s.wqecsoff);
> > + dpivf->setup_done = true;
> > + }
> > + break;
> > + case DPI_QUEUE_CLOSE:
> > + dpivf->vf_config.aura = 0;
> > + dpivf->vf_config.csize = 0;
> > + dpivf->vf_config.sso_pf_func = 0;
> > + dpivf->vf_config.npa_pf_func = 0;
> > + dpi_queue_fini(dpi, dpivf, msg->s.vfid);
> > + dpivf->setup_done = false;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void dpi_pfvf_mbox_work(struct work_struct *work) {
> > + struct dpi_mbox *mbox = container_of(work, struct dpi_mbox,
> work);
> > + union dpi_mbox_message_t msg = { 0 };
>
> Are you sure this can be on the stack?
No, it's not guaranteed, will use memset. Thanks.
>
> > + struct dpipf_vf *dpivf;
> > + struct dpipf *dpi;
> > + int ret;
> > +
> > + dpi = mbox->pf;
> > +
> > + mutex_lock(&mbox->lock);
> > + msg.u[0] = readq(mbox->vf_pf_data_reg);
> > + if (unlikely(msg.u[0] == (u64)-1))
> > + goto exit;
>
> Only use likely/unlikely if you can prove with a benchmark that it makes a
> difference. Otherwise let the CPU and compiler choose for you, as it knows
> better than you do (and will know better over time.)
Ack.
>
> If you want to use unlikely, you have to document it as to how it matters.
>
> > +
> > + if (unlikely(msg.s.vfid >= pci_num_vf(dpi->pdev))) {
> > + dev_err(&dpi->pdev->dev, "Invalid vfid:%d\n", msg.s.vfid);
>
> You send this to the kernel log if a device is broken?
Ok, ideally, it's not case, will remove log.
>
> > + goto exit;
> > + }
> > +
> > + dpivf = &dpi->vf[msg.s.vfid];
> > + msg.u[1] = readq(mbox->pf_vf_data_reg);
> > +
> > + ret = queue_config(dpi, dpivf, &msg);
> > + if (ret < 0)
> > + writeq(DPI_MBOX_TYPE_RSP_NACK, mbox-
> >pf_vf_data_reg);
> > + else
> > + writeq(DPI_MBOX_TYPE_RSP_ACK, mbox->pf_vf_data_reg);
> > +exit:
> > + mutex_unlock(&mbox->lock);
> > +}
> > +
> > +/* Setup registers for a PF mailbox */ static void
> > +dpi_setup_mbox_regs(struct dpipf *dpi, int vf) {
> > + struct dpi_mbox *mbox = dpi->mbox[vf];
> > +
> > + mbox->pf_vf_data_reg = dpi->reg_base +
> DPI_MBOX_PF_VF_DATA0(vf);
> > + mbox->vf_pf_data_reg = dpi->reg_base +
> DPI_MBOX_PF_VF_DATA1(vf); }
> > +
> > +static int dpi_pfvf_mbox_setup(struct dpipf *dpi) {
> > + int vf;
> > +
> > + for (vf = 0; vf < DPI_MAX_VFS; vf++) {
> > + dpi->mbox[vf] = devm_kzalloc(&dpi->pdev->dev,
> > +sizeof(*dpi->mbox[vf]), GFP_KERNEL);
> > +
> > + if (!dpi->mbox[vf])
> > + return -ENOMEM;
> > +
> > + mutex_init(&dpi->mbox[vf]->lock);
> > + INIT_WORK(&dpi->mbox[vf]->work, dpi_pfvf_mbox_work);
> > + dpi->mbox[vf]->pf = dpi;
> > + dpi_setup_mbox_regs(dpi, vf);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void dpi_pfvf_mbox_destroy(struct dpipf *dpi) {
> > + unsigned int vf;
> > +
> > + for (vf = 0; vf < DPI_MAX_VFS; vf++) {
> > + if (work_pending(&dpi->mbox[vf]->work))
> > + cancel_work_sync(&dpi->mbox[vf]->work);
> > +
> > + dpi->mbox[vf] = NULL;
> > + }
> > +}
> > +
> > +static void dpi_init(struct dpipf *dpi) {
> > + unsigned int engine, port;
> > + u8 mrrs_val, mps_val;
> > + u64 reg;
> > +
> > + for (engine = 0; engine < DPI_MAX_ENGINES; engine++) {
> > + if (engine == 4 || engine == 5)
> > + reg = DPI_ENG_BUF_BLKS(16);
> > + else
> > + reg = DPI_ENG_BUF_BLKS(8);
> > +
> > + dpi_reg_write(dpi, DPI_ENGX_BUF(engine), reg);
> > + }
> > +
> > + reg = DPI_DMA_CONTROL_ZBWCSEN |
> DPI_DMA_CONTROL_PKT_EN | DPI_DMA_CONTROL_LDWB |
> > + DPI_DMA_CONTROL_O_MODE |
> DPI_DMA_CONTROL_DMA_ENB;
> > +
> > + dpi_reg_write(dpi, DPI_DMA_CONTROL, reg);
> > + dpi_reg_write(dpi, DPI_CTL, DPI_CTL_EN);
> > +
> > + mrrs_val = 2; /* 512B */
> > + mps_val = 1; /* 256B */
> > +
> > + for (port = 0; port < DPI_EBUS_MAX_PORTS; port++) {
> > + reg = dpi_reg_read(dpi, DPI_EBUS_PORTX_CFG(port));
> > + reg &= ~(DPI_EBUS_PORTX_CFG_MRRS(7) |
> DPI_EBUS_PORTX_CFG_MPS(7));
> > + reg |= DPI_EBUS_PORTX_CFG_MPS(mps_val) |
> DPI_EBUS_PORTX_CFG_MRRS(mrrs_val);
> > + dpi_reg_write(dpi, DPI_EBUS_PORTX_CFG(port), reg);
> > + }
> > +
> > + dpi_reg_write(dpi, DPI_WCTL_FIF_THR,
> DPI_WCTL_FIFO_THRESHOLD); }
> > +
> > +static void dpi_fini(struct dpipf *dpi) {
> > + unsigned int engine;
> > +
> > + for (engine = 0; engine < DPI_MAX_ENGINES; engine++)
> > + dpi_reg_write(dpi, DPI_ENGX_BUF(engine), 0);
> > +
> > + dpi_reg_write(dpi, DPI_DMA_CONTROL, 0);
> > + dpi_reg_write(dpi, DPI_CTL, 0);
> > +}
> > +
> > +static void dpi_free_irq_vectors(void *pdev) {
> > + pci_free_irq_vectors((struct pci_dev *)pdev); }
> > +
> > +static int dpi_irq_init(struct dpipf *dpi) {
> > + struct pci_dev *pdev = dpi->pdev;
> > + struct device *dev = &pdev->dev;
> > + int i, ret;
> > +
> > + /* Clear all RAS interrupts */
> > + dpi_reg_write(dpi, DPI_PF_RAS, DPI_PF_RAS_INT);
> > +
> > + /* Clear all RAS interrupt enable bits */
> > + dpi_reg_write(dpi, DPI_PF_RAS_ENA_W1C, DPI_PF_RAS_INT);
> > +
> > + for (i = 0; i < DPI_MAX_REQQ_INT; i++) {
> > + dpi_reg_write(dpi, DPI_REQQX_INT(i), DPI_REQQ_INT);
> > + dpi_reg_write(dpi, DPI_REQQX_INT_ENA_W1C(i),
> DPI_REQQ_INT);
> > + }
> > +
> > + for (i = 0; i < DPI_MAX_CC_INT; i++) {
> > + dpi_reg_write(dpi, DPI_DMA_CCX_INT(i),
> DPI_DMA_CC_INT);
> > + dpi_reg_write(dpi, DPI_DMA_CCX_INT_ENA_W1C(i),
> DPI_DMA_CC_INT);
> > + }
> > +
> > + ret = pci_alloc_irq_vectors(pdev, DPI_MAX_IRQS, DPI_MAX_IRQS,
> PCI_IRQ_MSIX);
> > + if (ret != DPI_MAX_IRQS) {
> > + dev_err(dev, "DPI: Failed to alloc %d msix irqs\n",
> DPI_MAX_IRQS);
> > + return ret;
> > + }
> > +
> > + ret = devm_add_action_or_reset(dev, dpi_free_irq_vectors, pdev);
> > + if (ret) {
> > + dev_err(dev, "DPI: Failed to add irq free action\n");
> > + return ret;
> > + }
> > +
> > + ret = devm_request_irq(dev, pci_irq_vector(pdev,
> DPI_MBOX_PF_VF_INT_IDX),
> > + dpi_mbox_intr_handler, 0, "dpi-mbox", dpi);
> > + if (ret) {
> > + dev_err(dev, "DPI: request_irq failed for mbox; err=%d\n",
> ret);
> > + return ret;
> > + }
> > +
> > + dpi_reg_write(dpi, DPI_MBOX_VF_PF_INT_ENA_W1S,
> GENMASK_ULL(31, 0));
> > +
> > + return 0;
> > +}
> > +
> > +static int dpi_mps_mrrs_config(struct dpipf *dpi, void __user *arg) {
> > + struct dpi_mps_mrrs_cfg cfg;
> > + u8 mrrs_val, mps_val;
> > + u64 reg;
> > +
> > + if (copy_from_user(&cfg, arg, sizeof(struct dpi_mps_mrrs_cfg)))
> > + return -EFAULT;
> > +
> > + if (cfg.max_read_req_sz < DPI_EBUS_MRRS_MIN ||
> cfg.max_read_req_sz > DPI_EBUS_MRRS_MAX ||
> > + !is_power_of_2(cfg.max_read_req_sz)) {
> > + dev_err(&dpi->pdev->dev, "Invalid MRRS size:%u\n",
> > +cfg.max_read_req_sz);
>
> You are allowing userspace to spam the kernel log with messages by sending
> the driver invalid data. THat is a denial of service, please never do that.
Sure, will fix it.
>
> > + return -EINVAL;
> > + }
> > +
> > + if (cfg.max_payload_sz < DPI_EBUS_MPS_MIN ||
> cfg.max_payload_sz > DPI_EBUS_MPS_MAX ||
> > + !is_power_of_2(cfg.max_payload_sz)) {
> > + dev_err(&dpi->pdev->dev, "Invalid MPS size:%u\n",
> cfg.max_payload_sz);
> > + return -EINVAL;
> > + }
> > +
> > + if (cfg.port >= DPI_EBUS_MAX_PORTS) {
> > + dev_err(&dpi->pdev->dev, "Invalid EBUS port:%u\n",
> cfg.port);
> > + return -EINVAL;
> > + }
> > +
> > + mrrs_val = fls(cfg.max_read_req_sz >> 8);
> > + mps_val = fls(cfg.max_payload_sz >> 8);
> > +
> > + reg = dpi_reg_read(dpi, DPI_EBUS_PORTX_CFG(cfg.port));
> > + reg &= ~(DPI_EBUS_PORTX_CFG_MRRS(0x7) |
> DPI_EBUS_PORTX_CFG_MPS(0x7));
> > + reg |= DPI_EBUS_PORTX_CFG_MPS(mps_val) |
> DPI_EBUS_PORTX_CFG_MRRS(mrrs_val);
> > + dpi_reg_write(dpi, DPI_EBUS_PORTX_CFG(cfg.port), reg);
> > +
> > + return 0;
> > +}
> > +
> > +static int dpi_engine_config(struct dpipf *dpi, void __user *arg) {
> > + struct dpi_engine_cfg cfg;
> > + unsigned int engine;
> > + u8 *eng_buf;
> > + u64 reg;
> > +
> > + if (copy_from_user(&cfg, arg, sizeof(struct dpi_engine_cfg)))
> > + return -EFAULT;
>
> No need to ever validate any information in that structure from userspace?
> You always blindly copy it on to the device? Userspace can never get this
> wrong?
>
Yes, couple of range checks are needed, will address in next patch.
> > +
> > + eng_buf = (u8 *)&cfg.fifo_mask;
> > +
> > + for (engine = 0; engine < DPI_MAX_ENGINES; engine++) {
> > + reg = DPI_ENG_BUF_BLKS(eng_buf[engine &
> DPI_ENGINE_MASK]);
> > + dpi_reg_write(dpi, DPI_ENGX_BUF(engine), reg);
> > +
> > + if (cfg.update_molr) {
> > + reg = DPI_DMA_ENG_EN_MOLR(cfg.molr[engine &
> DPI_ENGINE_MASK]);
> > + dpi_reg_write(dpi, DPI_DMA_ENGX_EN(engine),
> reg);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static long dpi_dev_ioctl(struct file *fptr, unsigned int cmd,
> > +unsigned long data) {
> > + void __user *arg = (void __user *)data;
> > + struct dpipf *dpi;
> > + int ret = -EINVAL;
>
> Wrong error code for an invalid ioctl command :(
ack
>
> > +
> > + dpi = container_of(fptr->private_data, struct dpipf, miscdev);
> > +
> > + switch (cmd) {
> > + case DPI_MPS_MRRS_CFG:
> > + ret = dpi_mps_mrrs_config(dpi, arg);
> > + break;
> > + case DPI_ENGINE_CFG:
> > + ret = dpi_engine_config(dpi, arg);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static const struct file_operations dpi_device_fops = {
> > + .owner = THIS_MODULE,
> > + .unlocked_ioctl = dpi_dev_ioctl,
> > + .compat_ioctl = compat_ptr_ioctl,
> > +};
> > +
> > +static int dpi_probe(struct pci_dev *pdev, const struct pci_device_id
> > +*id) {
> > + struct device *dev = &pdev->dev;
> > + struct dpipf *dpi;
> > + int ret;
> > +
> > + dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL);
> > + if (!dpi)
> > + return -ENOMEM;
> > +
> > + dpi->pdev = pdev;
> > +
> > + ret = pcim_enable_device(pdev);
> > + if (ret) {
> > + dev_err(dev, "DPI: Failed to enable PCI device\n");
> > + return ret;
> > + }
> > +
> > + ret = pcim_iomap_regions(pdev, BIT(0) | BIT(4), DPI_DRV_NAME);
> > + if (ret) {
> > + dev_err(dev, "DPI: Failed to request MMIO region\n");
> > + return ret;
> > + }
> > +
> > + dpi->reg_base = pcim_iomap_table(pdev)[PCI_DPI_CFG_BAR];
> > +
> > + /* Initialize global PF registers */
> > + dpi_init(dpi);
> > +
> > + /* Setup PF-VF mailbox */
> > + ret = dpi_pfvf_mbox_setup(dpi);
> > + if (ret) {
> > + dev_err(dev, "DPI: Failed to setup pf-vf mbox\n");
> > + goto err_dpi_fini;
> > + }
> > +
> > + /* Register interrupts */
> > + ret = dpi_irq_init(dpi);
> > + if (ret) {
> > + dev_err(dev, "DPI: Failed to initialize irq vectors\n");
> > + goto err_dpi_mbox_free;
> > + }
> > +
> > + pci_set_drvdata(pdev, dpi);
> > + dpi->miscdev.minor = MISC_DYNAMIC_MINOR;
> > + dpi->miscdev.name = DPI_DRV_NAME;
> > + dpi->miscdev.fops = &dpi_device_fops;
> > + dpi->miscdev.parent = dev;
> > +
> > + ret = misc_register(&dpi->miscdev);
> > + if (ret) {
> > + dev_err(dev, "DPI: Failed to register misc device\n");
> > + goto err_dpi_mbox_free;
> > + }
> > +
> > + return 0;
> > +
> > +err_dpi_mbox_free:
> > + dpi_pfvf_mbox_destroy(dpi);
> > +err_dpi_fini:
> > + dpi_fini(dpi);
> > + return ret;
> > +}
> > +
> > +static void dpi_remove(struct pci_dev *pdev) {
> > + struct dpipf *dpi = pci_get_drvdata(pdev);
> > +
> > + misc_deregister(&dpi->miscdev);
> > + pci_sriov_configure_simple(pdev, 0);
> > + dpi_pfvf_mbox_destroy(dpi);
> > + dpi_fini(dpi);
> > + pci_set_drvdata(pdev, NULL);
> > +}
> > +
> > +static const struct pci_device_id dpi_id_table[] = {
> > + { PCI_DEVICE_SUB(PCI_VENDOR_ID_CAVIUM,
> PCI_DEVID_MRVL_CN10K_DPI_PF,
> > + PCI_VENDOR_ID_CAVIUM,
> PCI_SUBDEVID_MRVL_CN10K_DPI_PF) },
> > + { 0, } /* end of table */
> > +};
> > +
> > +static struct pci_driver dpi_driver = {
> > + .name = DPI_DRV_NAME,
> > + .id_table = dpi_id_table,
> > + .probe = dpi_probe,
> > + .remove = dpi_remove,
> > + .sriov_configure = pci_sriov_configure_simple, };
> > +
> > +module_pci_driver(dpi_driver);
> > +MODULE_DEVICE_TABLE(pci, dpi_id_table);
> MODULE_AUTHOR("Marvell.");
> > +MODULE_DESCRIPTION("Marvell Octeon CN10K DPI Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/uapi/misc/mrvl_cn10k_dpi.h
> > b/include/uapi/misc/mrvl_cn10k_dpi.h
> > new file mode 100644
> > index 000000000000..a1951644448a
> > --- /dev/null
> > +++ b/include/uapi/misc/mrvl_cn10k_dpi.h
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
>
> Why is this file "GPL-2.0+" but your driver is "GPL-2.0"? Is that what your
> lawyers want to have happen (sorry, I have to ask.)
No specific reason, I think I need to use " GPL-2.0-or-later" instead of
"GPL-2.0+", will fix it.
Thanks
Vamsi
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists