Re: [dpdk-dev] [PATCH v10 3/3] iFPGA: Add Intel FPGA BUS Rawdev Driver

["Xu, Rosen" <rosen.xu@intel.com>]

Hi Jingjing,

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Thursday, May 10, 2018 17:21
> To: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org; thomas@monjalon.net
> Cc: Xu, Rosen <rosen.xu@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>; shreyansh.jain@nxp.com;
> Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>;
> Liu, Song <song.liu@intel.com>; Wu, Hao <hao.wu@intel.com>;
> gaetan.rivet@6wind.com; Wu, Yanglong <yanglong.wu@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v10 3/3] iFPGA: Add Intel FPGA BUS Rawdev
> Driver
> 
> Hi, Rosen
> 
> Few comments below.

Thanks a lot.

> Thanks
> Jingjing
> 
> [...]
> 
> > +static int
> > +ifpga_rawdev_start(struct rte_rawdev *dev) {
> > +	int ret = 0;
> > +	struct opae_adapter *adapter;
> > +
> > +	IFPGA_RAWDEV_PMD_FUNC_TRACE();
> > +
> > +	RTE_FUNC_PTR_OR_ERR_RET(dev, -EINVAL);
> > +
> > +	adapter = ifpga_rawdev_get_priv(dev);
> > +	if (!adapter)
> > +		return -ENODEV;
> > +
> Set dev->started?

Rawdev lib enable it.

> > +	return ret;
> > +}
> 
> 
> [...]
> 
> > +
> > +static const struct rte_rawdev_ops ifpga_rawdev_ops = {
> > +	.dev_info_get = ifpga_rawdev_info_get,
> > +	.dev_configure = NULL,
> If go the declaration of rte_rawdev_configure, you will see "This function
> must be invoked first before any other function in the API."
> So I think we need to function for it, even it does nothing.

For other Rawdev Drivers, no used function pointer is initialized with NULL.

> 
> [...]
> 
> > +static struct rte_pci_driver rte_ifpga_rawdev_pmd = {
> > +	.id_table  = pci_ifpga_map,
> > +	.drv_flags = RTE_PCI_DRV_NEED_MAPPING |
> RTE_PCI_DRV_INTR_LSC,
> Is RTE_PCI_DRV_INTR_LSC supported?
> 
> [...]
> 
> > +static struct rte_vdev_driver ifpga_cfg_driver = {
> > +	.probe = ifpga_cfg_probe,
> > +	.remove = ifpga_cfg_remove,
> > +};
> > +
> > +RTE_PMD_REGISTER_VDEV(net_ifpga_cfg, ifpga_cfg_driver);
> I think prefix net_ would mean the device is net device (eth_dev)? How about
> to change the prefix to raw_?

I have fixed it both in code and document.

> > +RTE_PMD_REGISTER_ALIAS(net_ifpga_cfg, ifpga_cfg);
> > +RTE_PMD_REGISTER_PARAM_STRING(net_ifpga_cfg,
> > +	"bdf=<string> "
> ifpga=<string>?
> > +	"port=<int> "
> > +	"afu_bts=<path>");
> > +