| 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:
<MW4PR18MB5244844411A57790287068E5A64E2@MW4PR18MB5244.namprd18.prod.outlook.com>
Date: Wed, 14 Feb 2024 13:33:34 +0000
From: Vamsi Krishna Attunuru <vattunuru@...vell.com>
To: Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman
<gregkh@...uxfoundation.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH 1/1] misc: mrvl-dpi: add octeontx3 dpi driver
> -----Original Message-----
> From: Arnd Bergmann <arnd@...db.de>
> Sent: Wednesday, February 14, 2024 4:53 PM
> To: Vamsi Krishna Attunuru <vattunuru@...vell.com>; Greg Kroah-Hartman
> <gregkh@...uxfoundation.org>
> Cc: linux-kernel@...r.kernel.org
> Subject: [EXT] Re: [PATCH 1/1] misc: mrvl-dpi: add octeontx3 dpi driver
>
> External Email
>
> ----------------------------------------------------------------------
> On Wed, Feb 14, 2024, at 04:55, Vamsi Attunuru wrote:
> > Adds PCIe PF driver for OcteonTx3 DPI PF device which initializes DPI
> > DMA hardware's global configuration and enables PF-VF mbox channels
> > which can be used by it's VF devices. This DPI PF driver handles only
> > the resource configuration requests from VFs and it does not have any
> > data movement functionality.
> >
> > Signed-off-by: Vamsi Attunuru <vattunuru@...vell.com>
>
> This looks incomplete, as there is no apparent interface to actually use the
> driver from either userspace or kernel. I understand that you want to merge
> this one step at a time, but please try to at least point out how this is
> intended to be used, or post it together with an (in-kernel) user if you plan to
> upstream that.
>
Sure, I will address this in next version. Thanks for the feedback.
> Is this used for anything other than networking? If not, maybe it should be
> part of drivers/net/ instead of drivers/misc.
>
It's DMA offload hardware, not used for networking. The DPI PF function is a simple management interface for global & per VF configurations.
> A few more things that Greg hasn't already commented on:
>
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
> > 4fb291f0bf7c..3142fdb1b4c0 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -574,6 +574,16 @@ config NSM
> > To compile this driver as a module, choose M here.
> > The module will be called nsm.
> >
> > +config MARVELL_OCTEONTX3_DPI
> > + tristate "OcteonTX3 DPI driver"
>
> Is OcteonTX3 an actual product name? I thought the follow-up to OcteonTX2
> (cn9[268]xx) was the OCTEON 10 line. Or is this a follow-up to the Marvell
> Armada (cn91xx) line?
>
Yes, it's OCTEON10/OcteonTX3.
> > +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 < DPI_MAX_VFS; vf++) {
> > + if (!(reg & (0x1UL << vf)))
> > + continue;
> > +
> > + if (!dpi->mbox[vf]) {
> > + dev_err(&dpi->pdev->dev, "bad mbox vf
> %d\n", vf);
> > + continue;
> > + }
> > +
> > + schedule_work(&dpi->mbox[vf]->wk.work);
> > + }
> > +
> > + if (reg)
> > + dpi_reg_write(dpi, DPI_MBOX_VF_PF_INT, reg);
> > + }
> > +}
> > +
> > +static irqreturn_t dpi_mbox_intr_handler(int irq, void *data) {
> > + struct dpipf *dpi = data;
> > +
> > + dpi_poll_pfvf_mbox(dpi);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> Have you considered using the drivers/mailbox framework for the mailbox
> portion?
>
DPI HW mailbox might not fit fully into the drivers/mailbox framework. I will double check once.
>
> > +static void dpi_pfvf_mbox_work(struct work_struct *work) {
> > + struct dpi_pfvf_mbox_wk *wk = container_of(work, struct
> > dpi_pfvf_mbox_wk, work);
> > + union dpi_mbox_message_t msg = { 0 };
> > + struct dpi_mbox *mbox = NULL;
> > + struct dpipf_vf *dpivf;
> > + struct dpipf *dpi;
> > + int vf_id;
> > +
> > + mbox = (struct dpi_mbox *)wk->ctxptr;
> > + dpi = (struct dpipf *)mbox->pf;
>
> Can these pointers be strictly typed instead of casting from a void*?
>
Yes
> > +static int dpi_pfvf_mbox_setup(struct dpipf *dpi) {
> > + int vf;
> > +
> > + for (vf = 0; vf < DPI_MAX_VFS; vf++) {
> > + dpi->mbox[vf] = vzalloc(sizeof(*dpi->mbox[vf]));
> > +
>
> dpi->mbox[vf] does not look excessively large, so I think
> kzalloc() is better than vzalloc() here.
>
ack
> > +module_init(dpi_init_module);
> > +module_exit(dpi_cleanup_module);
> > +MODULE_DEVICE_TABLE(pci, dpi_id_table); MODULE_AUTHOR("Marvell
> > +International Ltd."); MODULE_DESCRIPTION(DPI_DRV_STRING);
> > +MODULE_LICENSE("GPL");
> > +MODULE_VERSION(DPI_DRV_VERSION);
>
> Please remove the DPI_DRV_STRING and DPI_DRV_VERSION macros, they
> prevent grepping for the strings.
>
ack
> > diff --git a/drivers/misc/mrvl-dpi/dpi.h b/drivers/misc/mrvl-dpi/dpi.h
> > new file mode 100644 index 000000000000..99ebe6bbe577
> > --- /dev/null
> > +++ b/drivers/misc/mrvl-dpi/dpi.h
> > @@ -0,0 +1,232 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Marvell OcteonTx3 DPI driver
> > + *
> > + * Copyright (C) 2024 Marvell International Ltd.
> > + */
> > +
> > +#ifndef __DPI_H__
> > +#define __DPI_H__
>
> I see no need for a separate header file if there is no other driver including it,
> so just merge this all into the .c file.
>
Sure, will merge into .c file.
> > +union dpi_mbox_message_t {
> > + uint64_t u[2];
> > + struct dpi_mbox_message_s {
> > + /* VF ID to configure */
> > + uint64_t vfid :8;
> > + /* Command code */
> > + uint64_t cmd :4;
> > + /* Command buffer size in 8-byte words */
> > + uint64_t csize :14;
> > + /* aura of the command buffer */
> > + uint64_t aura :20;
> > + /* SSO PF function */
> > + uint64_t sso_pf_func :16;
> > + /* NPA PF function */
> > + uint64_t npa_pf_func :16;
> > + /* Work queue completion status enable */
> > + uint64_t wqecs :1;
> > + /* Work queue completion status byte offset */
> > + uint64_t wqecsoff :7;
> > + } s __packed;
> > +};
>
> Is this a hardware structure? If it is, you probably don't want to use bit fields
> here, even in the best case that is a bug that prevents you from using the
> driver in big-endian mode.
>
> I also see that there are only 86 bits defined, and one field crosses a 64-bit
> boundary, which feels odd.
It's a software structure only, will fix the bugs.
Thanks Arnd for the review comments.
>
> Arnd
Powered by blists - more mailing lists