[PATCH 0/5] Simplify code by using hotplug safe pci_get_domain_bus_and_slot()

[PATCH 0/5] Simplify code by using hotplug safe pci_get_domain_bus_and_slot()

[Jiang Liu]

Following code has a race window between pci_find_bus() and pci_get_slot()
if PCI hotplug operation happens between them which removes the pci_bus.
So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
which also reduces code complexity.

struct pci_bus *pci_bus = pci_find_bus(domain, busno);
struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);

Jiang Liu (5):
  PCI/IA64: simplify code by hotplug safe pci_get_domain_bus_and_slot()
  PCI/vga: simplify code by hotplug safe pci_get_domain_bus_and_slot()
  PCI/cpcihp: simplify code by hotplug safe
    pci_get_domain_bus_and_slot()
  PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()
  PCI/xen-pcifront: simplify code by hotplug safe
    pci_get_domain_bus_and_slot()

 arch/ia64/sn/kernel/io_common.c      |    4 +---
 drivers/gpu/vga/vgaarb.c             |   15 +++------------
 drivers/pci/hotplug/cpcihp_generic.c |    8 ++------
 drivers/pci/iov.c                    |    8 ++------
 drivers/pci/xen-pcifront.c           |   10 ++--------
 5 files changed, 10 insertions(+), 35 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] PCI/IA64: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Jiang Liu]

Following code has a race window between pci_find_bus() and pci_get_slot()
if PCI hotplug operation happens between them which removes the pci_bus.
So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
which also reduces code complexity.

struct pci_bus *pci_bus = pci_find_bus(domain, busno);
struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/ia64/sn/kernel/io_common.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/ia64/sn/kernel/io_common.c b/arch/ia64/sn/kernel/io_common.c
index fbb5f2f..8630875 100644
--- a/arch/ia64/sn/kernel/io_common.c
+++ b/arch/ia64/sn/kernel/io_common.c
@@ -229,7 +229,6 @@ void sn_pci_fixup_slot(struct pci_dev *dev, struct pcidev_info *pcidev_info,
 {
 	int segment = pci_domain_nr(dev->bus);
 	struct pcibus_bussoft *bs;
-	struct pci_bus *host_pci_bus;
 	struct pci_dev *host_pci_dev;
 	unsigned int bus_no, devfn;
 
@@ -245,8 +244,7 @@ void sn_pci_fixup_slot(struct pci_dev *dev, struct pcidev_info *pcidev_info,
 
 	bus_no = (pcidev_info->pdi_slot_host_handle >> 32) & 0xff;
 	devfn = pcidev_info->pdi_slot_host_handle & 0xffffffff;
- 	host_pci_bus = pci_find_bus(segment, bus_no);
- 	host_pci_dev = pci_get_slot(host_pci_bus, devfn);
+	host_pci_dev = pci_get_domain_bus_and_slot(segment, bus_no, devfn);
 
 	pcidev_info->host_pci_dev = host_pci_dev;
 	pcidev_info->pdi_linux_pcidev = dev;
-- 
1.7.9.5

[PATCH 2/5] PCI/vga: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Jiang Liu]

Following code has a race window between pci_find_bus() and pci_get_slot()
if PCI hotplug operation happens between them which removes the pci_bus.
So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
which also reduces code complexity.

struct pci_bus *pci_bus = pci_find_bus(domain, busno);
struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/gpu/vga/vgaarb.c |   15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 3df8fc0..b6852b7 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1066,7 +1066,6 @@ static ssize_t vga_arb_write(struct file *file, const char __user * buf,
 		}
 
 	} else if (strncmp(curr_pos, "target ", 7) == 0) {
-		struct pci_bus *pbus;
 		unsigned int domain, bus, devfn;
 		struct vga_device *vgadev;
 
@@ -1085,19 +1084,11 @@ static ssize_t vga_arb_write(struct file *file, const char __user * buf,
 			pr_debug("vgaarb: %s ==> %x:%x:%x.%x\n", curr_pos,
 				domain, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 
-			pbus = pci_find_bus(domain, bus);
-			pr_debug("vgaarb: pbus %p\n", pbus);
-			if (pbus == NULL) {
-				pr_err("vgaarb: invalid PCI domain and/or bus address %x:%x\n",
-					domain, bus);
-				ret_val = -ENODEV;
-				goto done;
-			}
-			pdev = pci_get_slot(pbus, devfn);
+			pdev = pci_get_domain_bus_and_slot(domain, bus, devfn);
 			pr_debug("vgaarb: pdev %p\n", pdev);
 			if (!pdev) {
-				pr_err("vgaarb: invalid PCI address %x:%x\n",
-					bus, devfn);
+				pr_err("vgaarb: invalid PCI address %x:%x:%x\n",
+					domain, bus, devfn);
 				ret_val = -ENODEV;
 				goto done;
 			}
-- 
1.7.9.5

[PATCH 3/5] PCI/cpcihp: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Jiang Liu]

Following code has a race window between pci_find_bus() and pci_get_slot()
if PCI hotplug operation happens between them which removes the pci_bus.
So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
which also reduces code complexity.

struct pci_bus *pci_bus = pci_find_bus(domain, busno);
struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/pci/hotplug/cpcihp_generic.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/hotplug/cpcihp_generic.c b/drivers/pci/hotplug/cpcihp_generic.c
index 81af764..a6a71c4 100644
--- a/drivers/pci/hotplug/cpcihp_generic.c
+++ b/drivers/pci/hotplug/cpcihp_generic.c
@@ -154,12 +154,8 @@ static int __init cpcihp_generic_init(void)
 	if(!r)
 		return -EBUSY;
 
-	bus = pci_find_bus(0, bridge_busnr);
-	if (!bus) {
-		err("Invalid bus number %d", bridge_busnr);
-		return -EINVAL;
-	}
-	dev = pci_get_slot(bus, PCI_DEVFN(bridge_slot, 0));
+	dev = pci_get_domain_bus_and_slot(0, bridge_busnr,
+					  PCI_DEVFN(bridge_slot, 0));
 	if(!dev || dev->hdr_type != PCI_HEADER_TYPE_BRIDGE) {
 		err("Invalid bridge device %s", bridge);
 		pci_dev_put(dev);
-- 
1.7.9.5

[PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Jiang Liu]

Following code has a race window between pci_find_bus() and pci_get_slot()
if PCI hotplug operation happens between them which removes the pci_bus.
So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
which also reduces code complexity.

struct pci_bus *pci_bus = pci_find_bus(domain, busno);
struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/pci/iov.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index aeccc91..b0fe771 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -152,15 +152,11 @@ failed1:
 static void virtfn_remove(struct pci_dev *dev, int id, int reset)
 {
 	char buf[VIRTFN_ID_LEN];
-	struct pci_bus *bus;
 	struct pci_dev *virtfn;
 	struct pci_sriov *iov = dev->sriov;
 
-	bus = pci_find_bus(pci_domain_nr(dev->bus), virtfn_bus(dev, id));
-	if (!bus)
-		return;
-
-	virtfn = pci_get_slot(bus, virtfn_devfn(dev, id));
+	virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
+			virtfn_bus(dev, id), virtfn_devfn(dev, id));
 	if (!virtfn)
 		return;
 
-- 
1.7.9.5

[PATCH 5/5] PCI/xen-pcifront: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Jiang Liu]

Following code has a race window between pci_find_bus() and pci_get_slot()
if PCI hotplug operation happens between them which removes the pci_bus.
So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
which also reduces code complexity.

struct pci_bus *pci_bus = pci_find_bus(domain, busno);
struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/pci/xen-pcifront.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index d6cc62c..def8d0b 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
 	int err = 0;
 	int i, num_devs;
 	unsigned int domain, bus, slot, func;
-	struct pci_bus *pci_bus;
 	struct pci_dev *pci_dev;
 	char str[64];
 
@@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
 			goto out;
 		}
 
-		pci_bus = pci_find_bus(domain, bus);
-		if (!pci_bus) {
-			dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n",
-				domain, bus);
-			continue;
-		}
-		pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func));
+		pci_dev = pci_get_domain_bus_and_slot(domain, bus,
+				PCI_DEVFN(slot, func));
 		if (!pci_dev) {
 			dev_dbg(&pdev->xdev->dev,
 				"Cannot get PCI device %04x:%02x:%02x.%d\n",
-- 
1.7.9.5

Re: [PATCH 5/5] PCI/xen-pcifront: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Konrad Rzeszutek Wilk]

On Tue, Aug 28, 2012 at 11:43:58PM +0800, Jiang Liu wrote:
> Following code has a race window between pci_find_bus() and pci_get_slot()
> if PCI hotplug operation happens between them which removes the pci_bus.
> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
> which also reduces code complexity.

Has this happend in practice? Is this something one can reproduce
by manipulating SysFS and at the same time unplugging the PCI
devices?
> 
> struct pci_bus *pci_bus = pci_find_bus(domain, busno);
> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  drivers/pci/xen-pcifront.c |   10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index d6cc62c..def8d0b 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>  	int err = 0;
>  	int i, num_devs;
>  	unsigned int domain, bus, slot, func;
> -	struct pci_bus *pci_bus;
>  	struct pci_dev *pci_dev;
>  	char str[64];
>  
> @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>  			goto out;
>  		}
>  
> -		pci_bus = pci_find_bus(domain, bus);
> -		if (!pci_bus) {
> -			dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n",
> -				domain, bus);
> -			continue;
> -		}
> -		pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func));
> +		pci_dev = pci_get_domain_bus_and_slot(domain, bus,
> +				PCI_DEVFN(slot, func));
>  		if (!pci_dev) {
>  			dev_dbg(&pdev->xdev->dev,
>  				"Cannot get PCI device %04x:%02x:%02x.%d\n",
> -- 
> 1.7.9.5
> 

Re: [PATCH 5/5] PCI/xen-pcifront: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Jiang Liu]

On 08/29/2012 12:59 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 28, 2012 at 11:43:58PM +0800, Jiang Liu wrote:
>> Following code has a race window between pci_find_bus() and pci_get_slot()
>> if PCI hotplug operation happens between them which removes the pci_bus.
>> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
>> which also reduces code complexity.
> 
> Has this happend in practice? Is this something one can reproduce
> by manipulating SysFS and at the same time unplugging the PCI
> devices?
Hi Konrad,
	We just noticed this issue by code inspection when improving PCI
hotplug implementation. I guess we could trigger such issue by adding
some delay between pci_find_bus() and pci_get_slot().
	Regards!
	Gerry

>>
>> struct pci_bus *pci_bus = pci_find_bus(domain, busno);
>> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  drivers/pci/xen-pcifront.c |   10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
>> index d6cc62c..def8d0b 100644
>> --- a/drivers/pci/xen-pcifront.c
>> +++ b/drivers/pci/xen-pcifront.c
>> @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>>  	int err = 0;
>>  	int i, num_devs;
>>  	unsigned int domain, bus, slot, func;
>> -	struct pci_bus *pci_bus;
>>  	struct pci_dev *pci_dev;
>>  	char str[64];
>>  
>> @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>>  			goto out;
>>  		}
>>  
>> -		pci_bus = pci_find_bus(domain, bus);
>> -		if (!pci_bus) {
>> -			dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n",
>> -				domain, bus);
>> -			continue;
>> -		}
>> -		pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func));
>> +		pci_dev = pci_get_domain_bus_and_slot(domain, bus,
>> +				PCI_DEVFN(slot, func));
>>  		if (!pci_dev) {
>>  			dev_dbg(&pdev->xdev->dev,
>>  				"Cannot get PCI device %04x:%02x:%02x.%d\n",
>> -- 
>> 1.7.9.5
>>

Re: [PATCH 5/5] PCI/xen-pcifront: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Konrad Rzeszutek Wilk]

On Wed, Aug 29, 2012 at 07:56:43AM +0800, Jiang Liu wrote:
> On 08/29/2012 12:59 AM, Konrad Rzeszutek Wilk wrote:
> > On Tue, Aug 28, 2012 at 11:43:58PM +0800, Jiang Liu wrote:
> >> Following code has a race window between pci_find_bus() and pci_get_slot()
> >> if PCI hotplug operation happens between them which removes the pci_bus.
> >> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
> >> which also reduces code complexity.
> > 
> > Has this happend in practice? Is this something one can reproduce
> > by manipulating SysFS and at the same time unplugging the PCI
> > devices?
> Hi Konrad,
> 	We just noticed this issue by code inspection when improving PCI
> hotplug implementation. I guess we could trigger such issue by adding
> some delay between pci_find_bus() and pci_get_slot().
> 	Regards!

OK. Let me test it out.
> 	Gerry
> 
> >>
> >> struct pci_bus *pci_bus = pci_find_bus(domain, busno);
> >> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);
> >>
> >> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> >> ---
> >>  drivers/pci/xen-pcifront.c |   10 ++--------
> >>  1 file changed, 2 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> >> index d6cc62c..def8d0b 100644
> >> --- a/drivers/pci/xen-pcifront.c
> >> +++ b/drivers/pci/xen-pcifront.c
> >> @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> >>  	int err = 0;
> >>  	int i, num_devs;
> >>  	unsigned int domain, bus, slot, func;
> >> -	struct pci_bus *pci_bus;
> >>  	struct pci_dev *pci_dev;
> >>  	char str[64];
> >>  
> >> @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
> >>  			goto out;
> >>  		}
> >>  
> >> -		pci_bus = pci_find_bus(domain, bus);
> >> -		if (!pci_bus) {
> >> -			dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n",
> >> -				domain, bus);
> >> -			continue;
> >> -		}
> >> -		pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func));
> >> +		pci_dev = pci_get_domain_bus_and_slot(domain, bus,
> >> +				PCI_DEVFN(slot, func));
> >>  		if (!pci_dev) {
> >>  			dev_dbg(&pdev->xdev->dev,
> >>  				"Cannot get PCI device %04x:%02x:%02x.%d\n",
> >> -- 
> >> 1.7.9.5
> >>

Re: [PATCH 5/5] PCI/xen-pcifront: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Konrad Rzeszutek Wilk]

On Tue, Aug 28, 2012 at 11:43:58PM +0800, Jiang Liu wrote:
> Following code has a race window between pci_find_bus() and pci_get_slot()
> if PCI hotplug operation happens between them which removes the pci_bus.
> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
> which also reduces code complexity.
> 
> struct pci_bus *pci_bus = pci_find_bus(domain, busno);
> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>

So:

Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> ---
>  drivers/pci/xen-pcifront.c |   10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index d6cc62c..def8d0b 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -982,7 +982,6 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>  	int err = 0;
>  	int i, num_devs;
>  	unsigned int domain, bus, slot, func;
> -	struct pci_bus *pci_bus;
>  	struct pci_dev *pci_dev;
>  	char str[64];
>  
> @@ -1032,13 +1031,8 @@ static int pcifront_detach_devices(struct pcifront_device *pdev)
>  			goto out;
>  		}
>  
> -		pci_bus = pci_find_bus(domain, bus);
> -		if (!pci_bus) {
> -			dev_dbg(&pdev->xdev->dev, "Cannot get bus %04x:%02x\n",
> -				domain, bus);
> -			continue;
> -		}
> -		pci_dev = pci_get_slot(pci_bus, PCI_DEVFN(slot, func));
> +		pci_dev = pci_get_domain_bus_and_slot(domain, bus,
> +				PCI_DEVFN(slot, func));
>  		if (!pci_dev) {
>  			dev_dbg(&pdev->xdev->dev,
>  				"Cannot get PCI device %04x:%02x:%02x.%d\n",
> -- 
> 1.7.9.5

Re: [PATCH 0/5] Simplify code by using hotplug safe pci_get_domain_bus_and_slot()

[Bjorn Helgaas]

On Tue, Aug 28, 2012 at 9:43 AM, Jiang Liu <liuj97@gmail.com> wrote:
> Following code has a race window between pci_find_bus() and pci_get_slot()
> if PCI hotplug operation happens between them which removes the pci_bus.
> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
> which also reduces code complexity.
>
> struct pci_bus *pci_bus = pci_find_bus(domain, busno);
> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);
>
> Jiang Liu (5):
>   PCI/IA64: simplify code by hotplug safe pci_get_domain_bus_and_slot()
>   PCI/vga: simplify code by hotplug safe pci_get_domain_bus_and_slot()
>   PCI/cpcihp: simplify code by hotplug safe
>     pci_get_domain_bus_and_slot()
>   PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()
>   PCI/xen-pcifront: simplify code by hotplug safe
>     pci_get_domain_bus_and_slot()

I applied these to my "next" branch, including the vgaarb and xen bits.  Thanks!

>  arch/ia64/sn/kernel/io_common.c      |    4 +---
>  drivers/gpu/vga/vgaarb.c             |   15 +++------------
>  drivers/pci/hotplug/cpcihp_generic.c |    8 ++------
>  drivers/pci/iov.c                    |    8 ++------
>  drivers/pci/xen-pcifront.c           |   10 ++--------
>  5 files changed, 10 insertions(+), 35 deletions(-)
>
> --
> 1.7.9.5
>

Re: [PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Yinghai Lu]

On Tue, Aug 28, 2012 at 8:43 AM, Jiang Liu <liuj97@gmail.com> wrote:
> Following code has a race window between pci_find_bus() and pci_get_slot()
> if PCI hotplug operation happens between them which removes the pci_bus.
> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
> which also reduces code complexity.
>
> struct pci_bus *pci_bus = pci_find_bus(domain, busno);
> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  drivers/pci/iov.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index aeccc91..b0fe771 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -152,15 +152,11 @@ failed1:
>  static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>  {
>         char buf[VIRTFN_ID_LEN];
> -       struct pci_bus *bus;
>         struct pci_dev *virtfn;
>         struct pci_sriov *iov = dev->sriov;
>
> -       bus = pci_find_bus(pci_domain_nr(dev->bus), virtfn_bus(dev, id));
> -       if (!bus)
> -               return;
> -
> -       virtfn = pci_get_slot(bus, virtfn_devfn(dev, id));
> +       virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
> +                       virtfn_bus(dev, id), virtfn_devfn(dev, id));
>         if (!virtfn)
>                 return;
>

Hi,

This one cause IOV regression, when remove bridge with pci devices under that.

in that case, VFs  are stopped before PF, so they are not in device
tree anymore.
so pci_get_domain_bus_and_slot will not find those VFs.

So the reference to PF is not released. Also vit_bus may not be released too.

So you have to rework
    pci_get_domain_bus_and_slot to make it work on pci devices get stopped only.

or just drop this from the tree.

BTW, Bjorn,
for the similar reason,  you need to apply
http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=e5a50aa3dfca1331c7c783412b1524bea06d2752

 PCI: Split out stop_bus_device and remove_bus_dev again.

 So later could use them for pci root bus hotplug support.

Also restore old behavoir: stop all at first then remove all.

-v2: only split the functions.

the reason is:

We stop all VFs at first , stop PF

before we stop PF, we can not remove VFs,

otherwise virtfn_remove does not work properly to remove reference and
virtfn bus while can not find vf.

I would like to have
 PCI: Split out stop_bus_device and remove_bus_dev again.
fold into

http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=commitdiff;h=282e1d655fe7c7c2e6b0dd8166c4c6b7c2a1219b

-Yinghai

Re: [PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Bjorn Helgaas]

On Thu, Sep 20, 2012 at 2:38 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Aug 28, 2012 at 8:43 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> Following code has a race window between pci_find_bus() and pci_get_slot()
>> if PCI hotplug operation happens between them which removes the pci_bus.
>> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
>> which also reduces code complexity.
>>
>> struct pci_bus *pci_bus = pci_find_bus(domain, busno);
>> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  drivers/pci/iov.c |    8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index aeccc91..b0fe771 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -152,15 +152,11 @@ failed1:
>>  static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>>  {
>>         char buf[VIRTFN_ID_LEN];
>> -       struct pci_bus *bus;
>>         struct pci_dev *virtfn;
>>         struct pci_sriov *iov = dev->sriov;
>>
>> -       bus = pci_find_bus(pci_domain_nr(dev->bus), virtfn_bus(dev, id));
>> -       if (!bus)
>> -               return;
>> -
>> -       virtfn = pci_get_slot(bus, virtfn_devfn(dev, id));
>> +       virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
>> +                       virtfn_bus(dev, id), virtfn_devfn(dev, id));
>>         if (!virtfn)
>>                 return;
>>
>
> Hi,
>
> This one cause IOV regression, when remove bridge with pci devices under that.
>
> in that case, VFs  are stopped before PF, so they are not in device
> tree anymore.
> so pci_get_domain_bus_and_slot will not find those VFs.
>
> So the reference to PF is not released. Also vit_bus may not be released too.
>
> So you have to rework
>     pci_get_domain_bus_and_slot to make it work on pci devices get stopped only.
>
> or just drop this from the tree.

pci_find_bus() is a broken interface (because there's no reference
counting or safety with respect to hot-plug), and if the design
depends on it, that means the design is broken, too.  I don't think
reworking pci_get_domain_bus_and_slot() is the right answer.

It's not clear to me why we need the split between stopping and
removing devices.  That split leads to these zombie devices that have
been stopped and are no longer findable by bus_find_device() (which is
used by pci_get_domain_bus_and_slot()), but still "exist" in some
fashion until they're removed.  It's unreasonable for random PCI and
driver code to have to worry about that zombie state.

I'll revert this patch for now to fix the regression.  Of course, that
means we will still have the old problem of using the unsafe
pci_find_bus().

> BTW, Bjorn,
> for the similar reason,  you need to apply
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=e5a50aa3dfca1331c7c783412b1524bea06d2752
> ...
> the reason is:
>
> We stop all VFs at first , stop PF
> before we stop PF, we can not remove VFs,
>
> otherwise virtfn_remove does not work properly to remove reference and
> virtfn bus while can not find vf.

I'll apply this one, too, for now.  I put them both on the
pci/yinghai-revert-pci_find_bus-and-remove-cleanup branch.  Let me
know if that's not what you expected.

I'm not happy about either reverting Jiang's patch or splitting
stop/remove again.  It complicates the design and the code.  I'll
apply them because they're regressions, and we don't have time for
redesign before 3.7.  But I encourage you to think about how to do
this more cleanly.

Bjorn

Re: [PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Jiang Liu]

On 09/21/2012 07:59 AM, Bjorn Helgaas wrote:
> On Thu, Sep 20, 2012 at 2:38 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, Aug 28, 2012 at 8:43 AM, Jiang Liu <liuj97@gmail.com> wrote:
>>> Following code has a race window between pci_find_bus() and pci_get_slot()
>>> if PCI hotplug operation happens between them which removes the pci_bus.
>>> So use PCI hotplug safe interface pci_get_domain_bus_and_slot() instead,
>>> which also reduces code complexity.
>>>
>>> struct pci_bus *pci_bus = pci_find_bus(domain, busno);
>>> struct pci_dev *pci_dev = pci_get_slot(pci_bus, devfn);
>>>
>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>> ---
>>>  drivers/pci/iov.c |    8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>> index aeccc91..b0fe771 100644
>>> --- a/drivers/pci/iov.c
>>> +++ b/drivers/pci/iov.c
>>> @@ -152,15 +152,11 @@ failed1:
>>>  static void virtfn_remove(struct pci_dev *dev, int id, int reset)
>>>  {
>>>         char buf[VIRTFN_ID_LEN];
>>> -       struct pci_bus *bus;
>>>         struct pci_dev *virtfn;
>>>         struct pci_sriov *iov = dev->sriov;
>>>
>>> -       bus = pci_find_bus(pci_domain_nr(dev->bus), virtfn_bus(dev, id));
>>> -       if (!bus)
>>> -               return;
>>> -
>>> -       virtfn = pci_get_slot(bus, virtfn_devfn(dev, id));
>>> +       virtfn = pci_get_domain_bus_and_slot(pci_domain_nr(dev->bus),
>>> +                       virtfn_bus(dev, id), virtfn_devfn(dev, id));
>>>         if (!virtfn)
>>>                 return;
>>>
>>
>> Hi,
>>
>> This one cause IOV regression, when remove bridge with pci devices under that.
>>
>> in that case, VFs  are stopped before PF, so they are not in device
>> tree anymore.
>> so pci_get_domain_bus_and_slot will not find those VFs.
>>
>> So the reference to PF is not released. Also vit_bus may not be released too.
>>
>> So you have to rework
>>     pci_get_domain_bus_and_slot to make it work on pci devices get stopped only.
>>
>> or just drop this from the tree.
> 
> pci_find_bus() is a broken interface (because there's no reference
> counting or safety with respect to hot-plug), and if the design
> depends on it, that means the design is broken, too.  I don't think
> reworking pci_get_domain_bus_and_slot() is the right answer.
> 
> It's not clear to me why we need the split between stopping and
> removing devices.  That split leads to these zombie devices that have
> been stopped and are no longer findable by bus_find_device() (which is
> used by pci_get_domain_bus_and_slot()), but still "exist" in some
> fashion until they're removed.  It's unreasonable for random PCI and
> driver code to have to worry about that zombie state.
> 
> I'll revert this patch for now to fix the regression.  Of course, that
> means we will still have the old problem of using the unsafe
> pci_find_bus().
Hi Bjorn,
	I'm working on to enhance unsafe calling of pci_find_bus().
	--Gerry

Re: [PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Yinghai Lu]

On Thu, Sep 20, 2012 at 4:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Thu, Sep 20, 2012 at 2:38 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> in that case, VFs  are stopped before PF, so they are not in device
>> tree anymore.
>> so pci_get_domain_bus_and_slot will not find those VFs.
>>
>> So the reference to PF is not released. Also vit_bus may not be released too.
>>
>> So you have to rework
>>     pci_get_domain_bus_and_slot to make it work on pci devices get stopped only.
>>
>> or just drop this from the tree.
>
> pci_find_bus() is a broken interface (because there's no reference
> counting or safety with respect to hot-plug), and if the design
> depends on it, that means the design is broken, too.  I don't think
> reworking pci_get_domain_bus_and_slot() is the right answer.
>
> It's not clear to me why we need the split between stopping and
> removing devices.  That split leads to these zombie devices that have
> been stopped and are no longer findable by bus_find_device() (which is
> used by pci_get_domain_bus_and_slot()), but still "exist" in some
> fashion until they're removed.  It's unreasonable for random PCI and
> driver code to have to worry about that zombie state.

That is not zombie state. that is driver unloaded, and not in /sys, /proc.
that pci device only can be found under bus->devices.

just like we have pci_device_add and pci_bus_add_device
or acpi add and acpi start.

Splitting stop and remove should be more clean than mixing stop and remove.

>
> I'll revert this patch for now to fix the regression.  Of course, that
> means we will still have the old problem of using the unsafe
> pci_find_bus().
>
>> BTW, Bjorn,
>> for the similar reason,  you need to apply
>> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=e5a50aa3dfca1331c7c783412b1524bea06d2752
>> ...
>> the reason is:
>>
>> We stop all VFs at first , stop PF
>> before we stop PF, we can not remove VFs,
>>
>> otherwise virtfn_remove does not work properly to remove reference and
>> virtfn bus while can not find vf.
>
> I'll apply this one, too, for now.  I put them both on the
> pci/yinghai-revert-pci_find_bus-and-remove-cleanup branch.  Let me
> know if that's not what you expected.

Yes, they are good.

>
> I'm not happy about either reverting Jiang's patch or splitting
> stop/remove again.  It complicates the design and the code.  I'll
> apply them because they're regressions, and we don't have time for
> redesign before 3.7.  But I encourage you to think about how to do
> this more cleanly.

That will need to redesign sriov implementation.

Also that pci root bus add/start, stop/remove will need special
sequence to make ioapic
and dmar to be started early before normal pci device drivers and
stopped after normal pci device drivers.

-Yinghai

Re: [PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Bjorn Helgaas]

On Thu, Sep 20, 2012 at 7:51 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Sep 20, 2012 at 4:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Thu, Sep 20, 2012 at 2:38 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>>> in that case, VFs  are stopped before PF, so they are not in device
>>> tree anymore.
>>> so pci_get_domain_bus_and_slot will not find those VFs.
>>>
>>> So the reference to PF is not released. Also vit_bus may not be released too.
>>>
>>> So you have to rework
>>>     pci_get_domain_bus_and_slot to make it work on pci devices get stopped only.
>>>
>>> or just drop this from the tree.
>>
>> pci_find_bus() is a broken interface (because there's no reference
>> counting or safety with respect to hot-plug), and if the design
>> depends on it, that means the design is broken, too.  I don't think
>> reworking pci_get_domain_bus_and_slot() is the right answer.
>>
>> It's not clear to me why we need the split between stopping and
>> removing devices.  That split leads to these zombie devices that have
>> been stopped and are no longer findable by bus_find_device() (which is
>> used by pci_get_domain_bus_and_slot()), but still "exist" in some
>> fashion until they're removed.  It's unreasonable for random PCI and
>> driver code to have to worry about that zombie state.
>
> That is not zombie state. that is driver unloaded, and not in /sys, /proc.
> that pci device only can be found under bus->devices.

It doesn't matter whether we call this a "zombie state" or just refer
to it as "devices not in /sys & /proc but still in bus->devices."  The
point is that this state is not very useful, and code outside the PCI
core should not have to know that it exists.

> just like we have pci_device_add and pci_bus_add_device
> or acpi add and acpi start.

The fact that ACPI drivers have both .add() and .start() methods is
another artifact of poor design, in my opinion.  No other subsystem
has that split, as far as I know.  The ACPI split exists because of a
messed-up ACPI hotplug implementation.  That doesn't mean we should
copy it.

>> I'm not happy about either reverting Jiang's patch or splitting
>> stop/remove again.  It complicates the design and the code.  I'll
>> apply them because they're regressions, and we don't have time for
>> redesign before 3.7.  But I encourage you to think about how to do
>> this more cleanly.
>
> That will need to redesign sriov implementation.

That's right.  If we can improve the situation by redesigning, that's
what we should do.  The present situation, where we keep adding
special cases because "that's the way the rest of the system works" is
not sustainable in the long term.

> Also that pci root bus add/start, stop/remove will need special
> sequence to make ioapic
> and dmar to be started early before normal pci device drivers and
> stopped after normal pci device drivers.

This is another thing I'm curious about.  How do you handle this
situation today (before host bridge hot-add)?

The DMAR I'm not so worried about because as far as I know, there's no
such thing as a DMAR that's discovered by PCI enumeration.  We should
discover it via ACPI, and that can happen before we enumerate anything
behind a host bridge, so I don't really see any ordering problem
between the DMAR and the PCI devices that would use it.

However, I know there *are* IOAPICs that are enumerated as PCI
devices, and I don't know whether we can deduce a relationship between
the IOAPIC and the devices that use it.  Don't we have this problem
already?  I assume that even without hot-adding a host bridge, we
might discover a PCI IOAPIC that was present at boot, and we'd have to
make sure to bind a driver to it before we use any of the PCI devices
connected to it.  How does that work?

Bjorn

Re: [PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Yinghai Lu]

On Thu, Sep 20, 2012 at 7:56 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> This is another thing I'm curious about.  How do you handle this
> situation today (before host bridge hot-add)?
>
> The DMAR I'm not so worried about because as far as I know, there's no
> such thing as a DMAR that's discovered by PCI enumeration.  We should
> discover it via ACPI, and that can happen before we enumerate anything
> behind a host bridge, so I don't really see any ordering problem
> between the DMAR and the PCI devices that would use it.

only need to have pci devices on that root bus scanned, and current intel iommu
maintain one device scope to drhd with pointer to pci device... that
need to be fixed
too.

>
> However, I know there *are* IOAPICs that are enumerated as PCI
> devices, and I don't know whether we can deduce a relationship between
> the IOAPIC and the devices that use it.  Don't we have this problem
> already?  I assume that even without hot-adding a host bridge, we
> might discover a PCI IOAPIC that was present at boot, and we'd have to
> make sure to bind a driver to it before we use any of the PCI devices
> connected to it.  How does that work?

I converted it to acpi way to discover it, and it could handle that case.

will search _GSB and try to get pci device, if there is pci device
will try to get BAR as ioapic base.

http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=blob;f=drivers/pci/ioapic.c;h=504ca93ac692646a7754fff83a04e3d07d98f648;hb=refs/heads/for-x86-irq

something like:

static void handle_ioapic_add(acpi_handle handle, struct pci_dev **pdev,
				 u32 *pgsi_base)
{
	acpi_status status;
	unsigned long long gsb;
	struct pci_dev *dev;
	u32 gsi_base;
	int ret;
	char *type;
	struct resource r;
	struct resource *res = &r;
	char objname[64];
	struct acpi_buffer buffer = {sizeof(objname), objname};

	*pdev = NULL;
	*pgsi_base = 0;

	status = acpi_evaluate_integer(handle, "_GSB", NULL, &gsb);
	if (ACPI_FAILURE(status) || !gsb)
		return;

	dev = acpi_get_pci_dev(handle);
	if (!dev) {
		struct acpi_device_info *info;
		char *hid = NULL;

		status = acpi_get_object_info(handle, &info);
		if (ACPI_FAILURE(status))
			return;
		if (info->valid & ACPI_VALID_HID)
			hid = info->hardware_id.string;
		if (!hid || strcmp(hid, "ACPI0009")) {
			kfree(info);
			return;
		}
		kfree(info);
		memset(res, 0, sizeof(*res));
		acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
		if (!res->flags)
			return;
	}

	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);

	gsi_base = gsb;
	type = "IOxAPIC";
	if (dev) {
		ret = pci_enable_device(dev);
		if (ret < 0)
			goto exit_put;

		pci_set_master(dev);

		if (dev->class == PCI_CLASS_SYSTEM_PIC_IOAPIC)
			type = "IOAPIC";

		if (pci_request_region(dev, 0, type))
			goto exit_disable;

		res = &dev->resource[0];
	}

	if (acpi_register_ioapic(handle, res->start, gsi_base)) {
		if (dev)
			goto exit_release;
		return;
	}

	printk(KERN_INFO "%s %s %s at %pR, GSI %u\n",
		dev ? dev_name(&dev->dev) : "", objname, type,
		res, gsi_base);

	*pdev = dev;
	*pgsi_base = gsi_base;
	return;

exit_release:
	pci_release_region(dev, 0);
exit_disable:
	pci_disable_device(dev);
exit_put:
	pci_dev_put(dev);
}

Re: [PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Don Dutile]

On 09/21/2012 02:22 AM, Yinghai Lu wrote:
> On Thu, Sep 20, 2012 at 7:56 PM, Bjorn Helgaas<bhelgaas@google.com>  wrote:
>> This is another thing I'm curious about.  How do you handle this
>> situation today (before host bridge hot-add)?
>>
>> The DMAR I'm not so worried about because as far as I know, there's no
>> such thing as a DMAR that's discovered by PCI enumeration.  We should
>> discover it via ACPI, and that can happen before we enumerate anything
>> behind a host bridge, so I don't really see any ordering problem
>> between the DMAR and the PCI devices that would use it.
>
> only need to have pci devices on that root bus scanned, and current intel iommu
> maintain one device scope to drhd with pointer to pci device... that
> need to be fixed
> too.
>
translation: you have an ACPI-DMAR setup bug?  a drhd can have multiple device
scopes, one of which can be "all devices under bus X uses this IOMMU".
If (dynamic) DMARs are scanned at root hot-plug time in ACPI hot-plug,
the proper dmar-init should be completed before any PCI devs are scanned
(and put into the proper iommu domain).

>>
>> However, I know there *are* IOAPICs that are enumerated as PCI
>> devices, and I don't know whether we can deduce a relationship between
>> the IOAPIC and the devices that use it.  Don't we have this problem
>> already?  I assume that even without hot-adding a host bridge, we
>> might discover a PCI IOAPIC that was present at boot, and we'd have to
>> make sure to bind a driver to it before we use any of the PCI devices
>> connected to it.  How does that work?
>
> I converted it to acpi way to discover it, and it could handle that case.
>
> will search _GSB and try to get pci device, if there is pci device
> will try to get BAR as ioapic base.
>
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=blob;f=drivers/pci/ioapic.c;h=504ca93ac692646a7754fff83a04e3d07d98f648;hb=refs/heads/for-x86-irq
>
> something like:
>
> static void handle_ioapic_add(acpi_handle handle, struct pci_dev **pdev,
> 				 u32 *pgsi_base)
> {
> 	acpi_status status;
> 	unsigned long long gsb;
> 	struct pci_dev *dev;
> 	u32 gsi_base;
> 	int ret;
> 	char *type;
> 	struct resource r;
> 	struct resource *res =&r;
> 	char objname[64];
> 	struct acpi_buffer buffer = {sizeof(objname), objname};
>
> 	*pdev = NULL;
> 	*pgsi_base = 0;
>
> 	status = acpi_evaluate_integer(handle, "_GSB", NULL,&gsb);
> 	if (ACPI_FAILURE(status) || !gsb)
> 		return;
>
> 	dev = acpi_get_pci_dev(handle);
> 	if (!dev) {
> 		struct acpi_device_info *info;
> 		char *hid = NULL;
>
> 		status = acpi_get_object_info(handle,&info);
> 		if (ACPI_FAILURE(status))
> 			return;
> 		if (info->valid&  ACPI_VALID_HID)
> 			hid = info->hardware_id.string;
> 		if (!hid || strcmp(hid, "ACPI0009")) {
> 			kfree(info);
> 			return;
> 		}
> 		kfree(info);
> 		memset(res, 0, sizeof(*res));
> 		acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
> 		if (!res->flags)
> 			return;
> 	}
>
> 	acpi_get_name(handle, ACPI_FULL_PATHNAME,&buffer);
>
> 	gsi_base = gsb;
> 	type = "IOxAPIC";
> 	if (dev) {
> 		ret = pci_enable_device(dev);
> 		if (ret<  0)
> 			goto exit_put;
>
> 		pci_set_master(dev);
>
> 		if (dev->class == PCI_CLASS_SYSTEM_PIC_IOAPIC)
> 			type = "IOAPIC";
>
> 		if (pci_request_region(dev, 0, type))
> 			goto exit_disable;
>
> 		res =&dev->resource[0];
> 	}
>
> 	if (acpi_register_ioapic(handle, res->start, gsi_base)) {
> 		if (dev)
> 			goto exit_release;
> 		return;
> 	}
>
> 	printk(KERN_INFO "%s %s %s at %pR, GSI %u\n",
> 		dev ? dev_name(&dev->dev) : "", objname, type,
> 		res, gsi_base);
>
> 	*pdev = dev;
> 	*pgsi_base = gsi_base;
> 	return;
>
> exit_release:
> 	pci_release_region(dev, 0);
> exit_disable:
> 	pci_disable_device(dev);
> exit_put:
> 	pci_dev_put(dev);
> }

Re: [PATCH 4/5] PCI/IOV: simplify code by hotplug safe pci_get_domain_bus_and_slot()

[Yinghai Lu]

On Fri, Sep 21, 2012 at 7:15 AM, Don Dutile <ddutile@redhat.com> wrote:
> On 09/21/2012 02:22 AM, Yinghai Lu wrote:
>>
>> On Thu, Sep 20, 2012 at 7:56 PM, Bjorn Helgaas<bhelgaas@google.com>
>> wrote:
>>>
>>> This is another thing I'm curious about.  How do you handle this
>>> situation today (before host bridge hot-add)?
>>>
>>> The DMAR I'm not so worried about because as far as I know, there's no
>>> such thing as a DMAR that's discovered by PCI enumeration.  We should
>>> discover it via ACPI, and that can happen before we enumerate anything
>>> behind a host bridge, so I don't really see any ordering problem
>>> between the DMAR and the PCI devices that would use it.
>>
>>
>> only need to have pci devices on that root bus scanned, and current intel
>> iommu
>> maintain one device scope to drhd with pointer to pci device... that
>> need to be fixed
>> too.
>>
> translation: you have an ACPI-DMAR setup bug?  a drhd can have multiple
> device
> scopes, one of which can be "all devices under bus X uses this IOMMU".
> If (dynamic) DMARs are scanned at root hot-plug time in ACPI hot-plug,
> the proper dmar-init should be completed before any PCI devs are scanned
> (and put into the proper iommu domain).

if you can check for-iommu branch,

http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-iommu
current implementation:

after root bus plugged in,
drivers/acpi/pci_root_hp.c::handle_root_bridge_insert() ==>
acpi_bus_add ==> drivers/acpi/pci_root.c::acpi_pci_root_add ==>
arch/x86/pci/acpi.c::pci_acpi_scan_root
so all pci devices get scanned.

later
handl_root_bridge_insert ==>acpi_bus_add ==>
drivers/acpi/pci_root.c::acpi_pci_root_start==>
pcibios_reserouce_survey_bus/pci_assign_unassigned_bus_resource
and call iommu acpi_pci_driver .add aka
drivers/iommu/dmar.c::acpi_pci_iommu_add ==> register_iommu ==>
handle_iommu_add ==> dmar_parse_dev ==> dmar_parse_dev_scope ==>
dmar_parse_one_dev_scope

that dmar_parse_one_dev_scope will find pci device pointer and put it
back into dmaru device pointer array.

in the boot path, it the same, dev_scope is parsed later after pci
devices get scanned.

We really should remove that cache array for device pointer...

-Yinghai