From: Vamsi Krishna Attunuru <vattunuru@marvell.com>
To: Arnd Bergmann <arnd@arndb.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [EXT] Re: [PATCH 1/1] misc: mrvl-dpi: add octeontx3 dpi driver
Date: Wed, 14 Feb 2024 13:33:34 +0000 [thread overview]
Message-ID: <MW4PR18MB5244844411A57790287068E5A64E2@MW4PR18MB5244.namprd18.prod.outlook.com> (raw)
In-Reply-To: <165cec24-680e-4d3e-883e-56fccbb4d7d9@app.fastmail.com>
> -----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
next prev parent reply other threads:[~2024-02-14 13:33 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-14 3:55 [PATCH 1/1] misc: mrvl-dpi: add octeontx3 dpi driver Vamsi Attunuru
2024-02-14 10:50 ` Greg KH
2024-02-14 11:40 ` [EXT] " Vamsi Krishna Attunuru
2024-02-14 11:22 ` Arnd Bergmann
2024-02-14 13:33 ` Vamsi Krishna Attunuru [this message]
2024-02-16 10:32 ` [PATCH v2 1/1] misc: mrvl-cn10k-dpi: add Octeon CN10K " Vamsi Attunuru
2024-02-17 8:13 ` Greg KH
2024-02-19 5:03 ` [EXT] " Vamsi Krishna Attunuru
2024-02-19 6:18 ` Greg KH
2024-02-19 7:03 ` Vamsi Krishna Attunuru
2024-02-28 16:21 ` [PATCH v3 1/1] misc: mrvl-cn10k-dpi: add Octeon CN10K DPI administrative driver Vamsi Attunuru
2024-03-07 21:55 ` Greg KH
2024-03-08 11:36 ` [EXTERNAL] " Vamsi Krishna Attunuru
2024-03-12 10:56 ` [PATCH v4 " Vamsi Attunuru
2024-04-11 13:02 ` Greg KH
2024-04-12 6:34 ` [EXTERNAL] " Vamsi Krishna Attunuru
2024-04-12 12:10 ` [PATCH v5 " Vamsi Attunuru
2024-04-12 12:26 ` Greg KH
2024-04-12 13:56 ` [EXTERNAL] " Vamsi Krishna Attunuru
2024-04-12 15:34 ` Greg KH
2024-04-12 16:19 ` Vamsi Krishna Attunuru
2024-04-13 5:47 ` Greg KH
2024-04-13 10:58 ` Vamsi Krishna Attunuru
2024-04-13 11:25 ` Greg KH
2024-04-13 16:17 ` Vamsi Krishna Attunuru
2024-04-13 19:11 ` Arnd Bergmann
2024-04-14 9:33 ` Vamsi Krishna Attunuru
2024-04-14 9:46 ` Greg Kroah-Hartman
2024-04-14 12:32 ` Vamsi Krishna Attunuru
2024-04-25 13:36 ` Vamsi Krishna Attunuru
2024-04-26 1:04 ` Greg KH
2024-04-26 18:20 ` [PATCH v6 " Vamsi Attunuru
2024-04-27 11:06 ` Greg KH
2024-04-27 11:59 ` [EXTERNAL] " Vamsi Krishna Attunuru
2024-04-29 5:50 ` Vamsi Attunuru
2024-04-29 9:13 ` Greg KH
2024-05-01 7:46 ` [EXTERNAL] " Vamsi Krishna Attunuru
2024-05-01 8:02 ` Greg KH
2024-05-20 11:06 ` [PATCH v7 " Vamsi Attunuru
2024-06-04 15:52 ` Greg KH
2024-06-04 16:21 ` [EXTERNAL] " Vamsi Krishna Attunuru
2024-06-05 11:57 ` Arnd Bergmann
2024-06-06 8:58 ` Vamsi Krishna Attunuru
2024-06-06 16:42 ` Vamsi Krishna Attunuru
2024-06-08 8:42 ` Arnd Bergmann
2024-04-30 14:00 ` [PATCH v6 " kernel test robot
2024-04-30 14:22 ` kernel test robot
2024-04-30 15:36 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MW4PR18MB5244844411A57790287068E5A64E2@MW4PR18MB5244.namprd18.prod.outlook.com \
--to=vattunuru@marvell.com \
--cc=arnd@arndb.de \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.