RE: [EXTERNAL] Re: [PATCH v5 1/1] misc: mrvl-cn10k-dpi: add Octeon CN10K DPI administrative driver

[Vamsi Krishna Attunuru]

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Friday, April 12, 2024 5:57 PM
> To: Vamsi Krishna Attunuru <vattunuru@xxxxxxxxxxx>
> Cc: arnd@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH v5 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 Fri, Apr 12, 2024 at 05:10:05AM -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.
> 
> No pointer to the userspace code that uses this?  Why not?  How are we
> supposed to be able to review this?

Userspace code will use two functionalities (mailbox & ioctl) from this driver. DPDK DMA driver uses the mailbox and
the dpdk application uses the ioctl to setup the device attributes. We are waiting for this kernel driver get merged
 to update the corresponding support in DPDK driver and applications. I will provide the pointers to both the use
cases in userspace code. 
Meanwhile below is the current dpdk dma driver that uses sysfs based scheme to convey mbox requests to
the kernel DPI driver which gets replaced with hardware mailbox scheme once mrvl-cn10k-dpi kernel driver is merged.
https://github.com/DPDK/dpdk/blob/main/drivers/common/cnxk/roc_dpi.c


> 
> > +config MARVELL_CN10K_DPI
> > +	tristate "Octeon CN10K DPI driver"
> > +	depends on ARM64 && PCI
> > +	help
> > +	  Enables Octeon CN10K DPI driver which intializes DPI PF device's
> global configuration
> > +	  and its 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.
> 
> Did this pass checkpatch?  Please wrap your help text at the proper boundry.

Yes, I did not find any failure, will double check and wrap the text as suggested.
> 
> And what is "DPI"?  What is "PF"?  What is "VF"?  These are all terms that
> need to be documented somewhere, right?

Yes, I will describe in full form or document it somewhere.

> 
> > --- /dev/null
> > +++ b/include/uapi/misc/mrvl_cn10k_dpi.h
> > @@ -0,0 +1,36 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> > +/*
> > + * Marvell Octeon CN10K DPI driver
> > + *
> > + * Copyright (C) 2024 Marvell.
> > + *
> > + */
> > +
> > +#ifndef __MRVL_CN10K_DPI_H__
> > +#define __MRVL_CN10K_DPI_H__
> > +
> > +#include <linux/types.h>
> > +
> > +#define DPI_MAX_ENGINES 6
> > +
> > +struct dpi_mps_mrrs_cfg {
> > +	__u64 mrrs; /* Max read request size */
> > +	__u64 mps;  /* Max packet size */
> 
> You can spell out variables with more characters :)
ack
> 
> > +	__u64 port; /* Ebus port */
> > +};
> > +
> > +struct dpi_engine_cfg {
> > +	__u64 fifo_mask; /* FIFO size mask in KBytes */
> > +	__u64 molr[DPI_MAX_ENGINES];
> 
> What is a "molr"?
Sorry, I will add required full forms. It's maximum outstanding load requests.
> 
> > +	__u64 update_molr; /* '1' to update engine MOLR */
> 
> You "burn" a whole 64 for 1 bit?  That feels wrong, who on your end
> reviewed this api to be correct?

Just for the explicit alignment, defined all variables as __u64. I will shrink it to minimal sizes
considering any reserved fields if required.

> 
> > +#define DPI_MAGIC_NUM	0xB8
> 
> Did you document this api somewhere?

No, I followed as other MAGIC NUM api definations, will document it as required.
> 
> > +
> > +/* Set MPS & MRRS parameters */
> > +#define DPI_MPS_MRRS_CFG _IOW(DPI_MAGIC_NUM, 1, struct
> > +dpi_mps_mrrs_cfg)
> > +
> > +/* Set Engine FIFO configuration */
> > +#define DPI_ENGINE_CFG   _IOW(DPI_MAGIC_NUM, 2, struct
> dpi_engine_cfg)
> > +
> > +#endif /* __MRVL_CN10K_DPI_H__ */
> > --
> > 2.25.1
> >