Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Fri, 9 Jun 2017 10:13:56 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: dev@dpdk.org, thomas@monjalon.net
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.1.1
> 
> On 6/9/2017 5:35 AM, Jerin Jacob wrote:
> > -----Original Message-----
> >> Date: Thu, 8 Jun 2017 20:44:17 +0100
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >> CC: dev@dpdk.org, thomas@monjalon.net
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> >> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
> >>  Thunderbird/52.1.1
> >>
> >> On 6/8/2017 6:15 PM, Jerin Jacob wrote:
> >>> -----Original Message-----
> >>>> Date: Thu, 8 Jun 2017 15:40:33 +0100
> >>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
> >>>> CC: thomas@monjalon.net
> >>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
> >>>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
> >>>>  Thunderbird/52.1.1
> >>>>
> >>>> On 6/8/2017 12:44 PM, Jerin Jacob wrote:
> >>>>> Some ethdev devices like nicvf thunderx PMD need special treatment for
> >>>>> Secondary queue set(SQS) PCIe VF devices, where, it expects to not unmap
> >>>>> or free the memory without registering the ethdev subsystem.
> >>>>>
> >>>>> Introducing a new RTE_PCI_DRV_KEEP_MAPPED_RES
> >>>>> PCI driver flag to request PCI subsystem to not unmap the mapped PCI
> >>>>> resources(PCI BAR address) if unsupported device detected.
> >>>>>
> >>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>>>
> >>>> <...>
> >>>>
> >>>>> @@ -235,6 +240,7 @@ rte_pci_probe_one_driver(struct rte_pci_driver *dr,
> >>>>>  static int
> >>>>>  rte_pci_detach_dev(struct rte_pci_device *dev)
> >>>>>  {
> >>>>> +	int ret = 0;
> >>>>>  	struct rte_pci_addr *loc;
> >>>>>  	struct rte_pci_driver *dr;
> >>>>>  
> >>>>> @@ -251,13 +257,18 @@ rte_pci_detach_dev(struct rte_pci_device *dev)
> >>>>>  	RTE_LOG(DEBUG, EAL, "  remove driver: %x:%x %s\n", dev->id.vendor_id,
> >>>>>  			dev->id.device_id, dr->driver.name);
> >>>>>  
> >>>>> -	if (dr->remove && (dr->remove(dev) < 0))
> >>>>> -		return -1;	/* negative value is an error */
> >>>>> +	if (dr->remove) {
> >>>>> +		ret = dr->remove(dev);
> >>>>> +		if (ret < 0)
> >>>>> +			return -1; /* negative value is an error */
> >>>>> +	}
> >>>>>  
> >>>>>  	/* clear driver structure */
> >>>>>  	dev->driver = NULL;
> >>>>>  
> >>>>> -	if (dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING)
> >>>>> +	if ((dr->drv_flags & RTE_PCI_DRV_NEED_MAPPING) &&
> >>>>> +	/* Don't unmap if dev is unsupported and it needs mapped resources */
> >>>>> +		!(ret > 0 && (dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
> >>>>
> >>>> Why it is required to keep mapping during detach?
> >>>
> >>> To keep symmetrical with other(on probe) unmap change. This will
> >>> activated only when PMD returns the positive number on remove() so PMD
> >>> has control over it. The existing use case, We cannot just detach a single
> >>> VF(one SQS VF is _not_ one ethdev port i.e one ethdev port consists of
> >>> multiple VFs) so we need control on when to unmap those BARs.
> >>
> >> For generic eal, there is an explicit request to detach the device, I am
> >> not sure about returning success but not releasing the resources based
> >> on PMD flag. How this will work with hotplug?
> > 
> > Again it is in the control of PMD. If PMD remove() returns 0 or <0 or
> > !RTE_PCI_DRV_KEEP_MAPPED_RES flag it will release the memory. If PMD is
> > keeping the resources it can free on primary(!SQS VF) VF detach.
> > 
> >>
> >>
> >> And specific to your case, -thanks for clarification, since no eth_dev
> >> created for SQS VF, rte_eth_dev_pci_generic_remove() won't be useful but
> >> assuming you have implemented your remove(), can it be possible to
> >> detect SQS VF and act accordingly, or just return error perhaps if you
> >> cannot detach that VF?
> > 
> > nicvf PMD is not advertising RTE_ETH_DEV_DETACHABLE capable and it is in
> > integrated internal bus so PCI hot-plug may not be a use case for this
> > PMD.
> > 
> > if you still think, RTE_PCI_DRV_KEEP_MAPPED_RES check needs to removed from
> > rte_pci_detach_dev(), I can do that send a new version.
> 
> I see it is control of PMD but I think it shouldn't. If it is required
> to unplug a device it should or return an error, not releasing them but
> returning success doesn't sound right.
> 
> For your case it doesn't matter to have this. And the flag is there,
> this can be added later if a special use case emerges. Right now I am
> for removing check from detach.

OK. Yes, As far the current use case concerned, We don't use that flag in
detach as detach is not supported. We will revisit this later.
I will send the next version with the flag removal in detach().