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

[Greg KH]

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?

> +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.

And what is "DPI"?  What is "PF"?  What is "VF"?  These are all terms
that need to be documented somewhere, right?

> --- /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 :)

> +	__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"?

> +	__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?

> +#define DPI_MAGIC_NUM	0xB8

Did you document this api somewhere?

> +
> +/* 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
>