RE: [EXT] Re: [PATCH 1/1] misc: mrvl-dpi: add octeontx3 dpi driver

[Vamsi Krishna Attunuru]

> -----Original Message-----
> From: Arnd Bergmann <arnd@xxxxxxxx>
> Sent: Wednesday, February 14, 2024 4:53 PM
> To: Vamsi Krishna Attunuru <vattunuru@xxxxxxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxx>
> 
> 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