Messages in this thread | | | From | Vamsi Krishna Attunuru <> | Subject | RE: [EXT] Re: [PATCH 1/1] misc: mrvl-dpi: add octeontx3 dpi driver | Date | Wed, 14 Feb 2024 13:33:34 +0000 |
| |
> -----Original Message----- > From: Arnd Bergmann <arnd@arndb.de> > Sent: Wednesday, February 14, 2024 4:53 PM > To: Vamsi Krishna Attunuru <vattunuru@marvell.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org> > Cc: linux-kernel@vger.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@marvell.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
| |