DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org, thomas@monjalon.net
Subject: Re: [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag
Date: Fri, 9 Jun 2017 14:57:07 +0530	[thread overview]
Message-ID: <20170609092707.GA3956@jerin> (raw)
In-Reply-To: <c648c2f8-e5a8-e124-145b-d4216f7eac8a@intel.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().

  reply	other threads:[~2017-06-09  9:28 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-01 13:05 [dpdk-dev] [PATCH] net/thunderx: manage PCI device mapping for SQS VFs Jerin Jacob
2017-06-06 13:36 ` Ferruh Yigit
2017-06-06 14:05   ` Jerin Jacob
2017-06-06 14:54     ` Ferruh Yigit
2017-06-08 11:44 ` [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag Jerin Jacob
2017-06-08 11:44   ` [dpdk-dev] [PATCH v2 2/2] net/thunderx: manage PCI device mapping for SQS VFs Jerin Jacob
2017-06-08 14:40   ` [dpdk-dev] [PATCH v2 1/2] eal/pci: introduce a PCI driver flag Ferruh Yigit
2017-06-08 17:15     ` Jerin Jacob
2017-06-08 19:44       ` Ferruh Yigit
2017-06-09  4:35         ` Jerin Jacob
2017-06-09  9:13           ` Ferruh Yigit
2017-06-09  9:27             ` Jerin Jacob [this message]
2017-06-09 10:27   ` [dpdk-dev] [PATCH v3 " Jerin Jacob
2017-06-09 10:27     ` [dpdk-dev] [PATCH v3 2/2] net/thunderx: manage PCI device mapping for SQS VFs Jerin Jacob
2017-06-09 10:46     ` [dpdk-dev] [PATCH v3 1/2] eal/pci: introduce a PCI driver flag Ferruh Yigit
2017-06-12 16:02       ` Ferruh Yigit
2017-06-12 16:21     ` Thomas Monjalon
2017-06-13  4:43       ` Jerin Jacob
2017-06-13  7:03         ` Thomas Monjalon
2017-06-13  7:24           ` Jerin Jacob
2017-06-13  8:18             ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170609092707.GA3956@jerin \
    --to=jerin.jacob@caviumnetworks.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).