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 9:05 PM
> To: Vamsi Krishna Attunuru <vattunuru@xxxxxxxxxxx>
> Cc: arnd@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [EXTERNAL] Re: [PATCH v5 1/1] misc: mrvl-cn10k-dpi: add
> Octeon CN10K DPI administrative driver
> 
> On Fri, Apr 12, 2024 at 01:56:36PM +0000, Vamsi Krishna Attunuru wrote:
> >
> >
> > > -----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://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_DPDK_d
> > pdk_blob_main_drivers_common_cnxk_roc-
> 5Fdpi.c&d=DwIBAg&c=nKjWec2b6R0mO
> >
> yPaz7xtfQ&r=WllrYaumVkxaWjgKto6E_rtDQshhIhik2jkvzFyRhW8&m=o3EhoL
> s7dsod
> > -YHS438Wl2Pf_MKMBYegGSKteoX3qFTB0HV897ykpCVbTp-
> nmj4e&s=A6TJDFUtPm3ksJh
> > qop89CL8GgKj4sjkJIVi1-RdnUr8&e=
> 
> So this is a DPDK thing?  Ugh, do the networking people know about this?
> If not, why aren't they reviewing this?

Actually, It's not networking related. Like the Linux kernel, DPDK also supports multiple subsystems like
network, scheduler, DMA, mempool etc. Regarding the usecases, the DPDK Marvell DMA/DPI VF driver
interacts(over hardware mailbox) with the mrvl-cn10k-dpi misc kernel driver(administrative driver) for setting
up the VF device resources.

DPDK is one example that uses this driver, there can be other userspace generic frameworks/applications
where the virtual functions are binded to userspace drivers and interact with physical/administrative function
driver running in the kernel.

> 
> > > > +	__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.
> 
> The alignment is fine, but wasting 63 bits feels odd, make it a bit field at worst
> case, or just make it __u8 if you don't want a bitfield.

ack
> 
> thanks,
> 
> greg k-h