diff mbox

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

Message ID 20170608114414.8787-1-jerin.jacob@caviumnetworks.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Jerin Jacob June 8, 2017, 11:44 a.m. UTC 
  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>
---
v2:
Introduced RTE_PCI_DRV_KEEP_MAPPED_RES flag scheme(Ferruh),
Based on the discussion in the following thread.
http://dpdk.org/ml/archives/dev/2017-June/067091.htmlRTE_PCI_DRV_KEEP_MAPPED_RES
---
 lib/librte_eal/common/eal_common_pci.c  | 19 +++++++++++++++----
 lib/librte_eal/common/include/rte_pci.h |  2 ++
 2 files changed, 17 insertions(+), 4 deletions(-)

Comments

Ferruh Yigit June 8, 2017, 2:40 p.m. UTC | #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?

>  		/* unmap resources for devices that use igb_uio */
>  		rte_pci_unmap_device(dev);
>  

<...>
Jerin Jacob June 8, 2017, 5:15 p.m. UTC | #2 
-----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.

> 
> >  		/* unmap resources for devices that use igb_uio */
> >  		rte_pci_unmap_device(dev);
> >  
> 
> <...>
>
Ferruh Yigit June 8, 2017, 7:44 p.m. UTC | #3 
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?


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?

> 
>>
>>>  		/* unmap resources for devices that use igb_uio */
>>>  		rte_pci_unmap_device(dev);
>>>  
>>
>> <...>
>>
Jerin Jacob June 9, 2017, 4:35 a.m. UTC | #4 
-----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.

> 
> > 
> >>
> >>>  		/* unmap resources for devices that use igb_uio */
> >>>  		rte_pci_unmap_device(dev);
> >>>  
> >>
> >> <...>
> >>
>
Ferruh Yigit June 9, 2017, 9:13 a.m. UTC | #5 
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.

Thanks,
ferruh

> 
>>
>>>
>>>>
>>>>>  		/* unmap resources for devices that use igb_uio */
>>>>>  		rte_pci_unmap_device(dev);
>>>>>  
>>>>
>>>> <...>
>>>>
>>
Jerin Jacob June 9, 2017, 9:27 a.m. UTC | #6 
-----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().
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 5ae520186..0d4e4a9f1 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -221,7 +221,12 @@  rte_pci_probe_one_driver(struct rte_pci_driver *dr,
 	ret = dr->probe(dr, dev);
 	if (ret) {
 		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 device is unsupported and
+			 * driver needs mapped resources.
+			 */
+			!(ret > 0 &&
+				(dr->drv_flags & RTE_PCI_DRV_KEEP_MAPPED_RES)))
 			rte_pci_unmap_device(dev);
 	}
 
@@ -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)))
 		/* unmap resources for devices that use igb_uio */
 		rte_pci_unmap_device(dev);
 
diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index b82ab9e79..0284a6208 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -212,6 +212,8 @@  struct rte_pci_bus {
 #define RTE_PCI_DRV_INTR_LSC	0x0008
 /** Device driver supports device removal interrupt */
 #define RTE_PCI_DRV_INTR_RMV 0x0010
+/** Device driver needs to keep mapped resources if unsupported dev detected */
+#define RTE_PCI_DRV_KEEP_MAPPED_RES 0x0020
 
 /**
  * A structure describing a PCI mapping.