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

[Xu, Rosen]

Hi Jingjing,

After we have aligned this morning, I will reply your email again.

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

Fixed.

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

Yes, the rte_rawdev_configure function should not NULL.
I have fill it.
 
> 
> [...]
> 
> > +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?

Not supported, and I removed it.

> [...]
> 
> > +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_?
> 
> > +RTE_PMD_REGISTER_ALIAS(net_ifpga_cfg, ifpga_cfg);
> > +RTE_PMD_REGISTER_PARAM_STRING(net_ifpga_cfg,
> > +	"bdf=<string> "
> ifpga=<string>?

Fixed.

> > +	"port=<int> "
> > +	"afu_bts=<path>");
> > +