[PATCH] pcie_portdriver: FIX: pcie_port_device_remove

[PATCH] pcie_portdriver: FIX: pcie_port_device_remove

[Eric W. Biederman]

pcie_port_device_remove currently calls the remove method
of port drivers twice.  Ouch!

We don't get the correct interrupt mode unless there is a port device
present.

We are calling device_for_each_child multiple times for no apparent
reason.

So make it simple.  Use pcie_port_driver_ext so we always properly
know the interrupt mode the we placed the pci device in.  Place
put_device and device_unregister into remove_iter, and throw out the
rest.  Only call device_for_each_child once.

The code is simpler and actually works!

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

---
 drivers/pci/pcie/portdrv_core.c |   31 +++++++------------------------
 1 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 8b3f8c1..c642828 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -326,16 +326,9 @@ int pcie_port_device_resume(struct pci_dev *dev)
 
 static int remove_iter(struct device *dev, void *data)
 {
-	struct pcie_port_service_driver *service_driver;
-
 	if (dev->bus == &pcie_port_bus_type) {
-		if (dev->driver) {
-			service_driver = to_service_driver(dev->driver);
-			if (service_driver->remove)
-				service_driver->remove(to_pcie_device(dev));
-		}
-		*(unsigned long*)data = (unsigned long)dev;
-		return 1;
+		put_device(dev);
+		device_unregister(dev);
 	}
 	return 0;
 }
@@ -349,24 +342,14 @@ static int remove_iter(struct device *dev, void *data)
  */
 void pcie_port_device_remove(struct pci_dev *dev)
 {
-	struct device *device;
-	unsigned long device_addr;
-	int interrupt_mode = PCIE_PORT_INTx_MODE;
-	int status;
+	struct pcie_port_device_ext *p_ext = pci_get_drvdata(dev);
+
+	device_for_each_child(&dev->dev, NULL, remove_iter);
 
-	do {
-		status = device_for_each_child(&dev->dev, &device_addr, remove_iter);
-		if (status) {
-			device = (struct device*)device_addr;
-			interrupt_mode = (to_pcie_device(device))->interrupt_mode;
-			put_device(device);
-			device_unregister(device);
-		}
-	} while (status);
 	/* Switch to INTx by default if MSI enabled */
-	if (interrupt_mode == PCIE_PORT_MSIX_MODE)
+	if (p_ext->interrupt_mode == PCIE_PORT_MSIX_MODE)
 		pci_disable_msix(dev);
-	else if (interrupt_mode == PCIE_PORT_MSI_MODE)
+	else if (p_ext->interrupt_mode == PCIE_PORT_MSI_MODE)
 		pci_disable_msi(dev);
 }
 
-- 
1.6.0.6

Re: [PATCH] pcie_portdriver: FIX: pcie_port_device_remove

[Andrew Morton]

On Fri, 13 Feb 2009 20:23:03 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> 
> pcie_port_device_remove currently calls the remove method
> of port drivers twice.  Ouch!
> 
> We don't get the correct interrupt mode unless there is a port device
> present.
> 
> We are calling device_for_each_child multiple times for no apparent
> reason.
> 
> So make it simple.  Use pcie_port_driver_ext so we always properly
> know the interrupt mode the we placed the pci device in.  Place
> put_device and device_unregister into remove_iter, and throw out the
> rest.  Only call device_for_each_child once.
> 
> The code is simpler and actually works!
> 

What's happening with this?

> 
> ---
>  drivers/pci/pcie/portdrv_core.c |   31 +++++++------------------------
>  1 files changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 8b3f8c1..c642828 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -326,16 +326,9 @@ int pcie_port_device_resume(struct pci_dev *dev)
>  
>  static int remove_iter(struct device *dev, void *data)
>  {
> -	struct pcie_port_service_driver *service_driver;
> -
>  	if (dev->bus == &pcie_port_bus_type) {
> -		if (dev->driver) {
> -			service_driver = to_service_driver(dev->driver);
> -			if (service_driver->remove)
> -				service_driver->remove(to_pcie_device(dev));
> -		}
> -		*(unsigned long*)data = (unsigned long)dev;
> -		return 1;
> +		put_device(dev);
> +		device_unregister(dev);
>  	}
>  	return 0;
>  }
> @@ -349,24 +342,14 @@ static int remove_iter(struct device *dev, void *data)
>   */
>  void pcie_port_device_remove(struct pci_dev *dev)
>  {
> -	struct device *device;
> -	unsigned long device_addr;
> -	int interrupt_mode = PCIE_PORT_INTx_MODE;
> -	int status;
> +	struct pcie_port_device_ext *p_ext = pci_get_drvdata(dev);
> +
> +	device_for_each_child(&dev->dev, NULL, remove_iter);
>  
> -	do {
> -		status = device_for_each_child(&dev->dev, &device_addr, remove_iter);
> -		if (status) {
> -			device = (struct device*)device_addr;
> -			interrupt_mode = (to_pcie_device(device))->interrupt_mode;
> -			put_device(device);
> -			device_unregister(device);
> -		}
> -	} while (status);
>  	/* Switch to INTx by default if MSI enabled */
> -	if (interrupt_mode == PCIE_PORT_MSIX_MODE)
> +	if (p_ext->interrupt_mode == PCIE_PORT_MSIX_MODE)
>  		pci_disable_msix(dev);
> -	else if (interrupt_mode == PCIE_PORT_MSI_MODE)
> +	else if (p_ext->interrupt_mode == PCIE_PORT_MSI_MODE)
>  		pci_disable_msi(dev);
>  }
>  

There are large-scale and conflicting changes to this file in linux-next.

If we want to jam this fix into 2.6.29 (and it looks like something we
want) then this will trash the linux-next changes.  It will cause me
grief, and will cause Stephen grief unless the pci tree is suitably
changed, which will cause Jesse grief. Either way: grief.

Re: [PATCH] pcie_portdriver: FIX: pcie_port_device_remove

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

>
> There are large-scale and conflicting changes to this file in linux-next.
>
> If we want to jam this fix into 2.6.29 (and it looks like something we
> want) then this will trash the linux-next changes.  It will cause me
> grief, and will cause Stephen grief unless the pci tree is suitably
> changed, which will cause Jesse grief. Either way: grief.

Ugh.

I had better have a good hard look at linux-next.  I tried to ask earlier about
ongoing working but I didn't hear anything.

Eric

Re: [PATCH] pcie_portdriver: FIX: pcie_port_device_remove

[Rafael J. Wysocki]

On Thursday 19 February 2009, Eric W. Biederman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> >
> > There are large-scale and conflicting changes to this file in linux-next.
> >
> > If we want to jam this fix into 2.6.29 (and it looks like something we
> > want) then this will trash the linux-next changes.  It will cause me
> > grief, and will cause Stephen grief unless the pci tree is suitably
> > changed, which will cause Jesse grief. Either way: grief.
> 
> Ugh.
> 
> I had better have a good hard look at linux-next.  I tried to ask earlier about
> ongoing working but I didn't hear anything.

Yes, you did: http://marc.info/?l=linux-kernel&m=123342565225134&w=4

Thanks,
Rafael

Re: [PATCH] pcie_portdriver: FIX: pcie_port_device_remove

[Eric W. Biederman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Thursday 19 February 2009, Eric W. Biederman wrote:
>> Andrew Morton <akpm@linux-foundation.org> writes:
>> 
>> >
>> > There are large-scale and conflicting changes to this file in linux-next.
>> >
>> > If we want to jam this fix into 2.6.29 (and it looks like something we
>> > want) then this will trash the linux-next changes.  It will cause me
>> > grief, and will cause Stephen grief unless the pci tree is suitably
>> > changed, which will cause Jesse grief. Either way: grief.
>> 
>> Ugh.
>> 
>> I had better have a good hard look at linux-next.  I tried to ask earlier
> about
>> ongoing working but I didn't hear anything.
>
> Yes, you did: http://marc.info/?l=linux-kernel&m=123342565225134&w=4

My apologies.  I should have said I did not recognize anything that
was likely to conflict from what I hear from what I heard.  I did
hear that the pcie port driver was touched to properly handle msi-x and I
foolishly thought that was already merged upstream.  Especially after
I glanced at Jesse's tree and I did not see any recent pci patches.
Which led me to foolishly assume everything had already been merged
into Linus's tree.

Andrew with respect to 2.6.29 my patch while correct and useful has
no immediate utility, as the pciehp driver will crash and burn horribly
if you attempt to hotplug it.  So I should find the other pci port driver
changes and rebase on top of them.  Assuming that patch is scheduled to
merge for 2.6.30.

Eric

Re: [PATCH] pcie_portdriver: FIX: pcie_port_device_remove

[Jesse Barnes]

On Thursday 19 February 2009 15:30:39 Eric W. Biederman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> > On Thursday 19 February 2009, Eric W. Biederman wrote:
> >> Andrew Morton <akpm@linux-foundation.org> writes:
> >> > There are large-scale and conflicting changes to this file in
> >> > linux-next.
> >> >
> >> > If we want to jam this fix into 2.6.29 (and it looks like something we
> >> > want) then this will trash the linux-next changes.  It will cause me
> >> > grief, and will cause Stephen grief unless the pci tree is suitably
> >> > changed, which will cause Jesse grief. Either way: grief.
> >>
> >> Ugh.
> >>
> >> I had better have a good hard look at linux-next.  I tried to ask
> >> earlier
> >
> > about
> >
> >> ongoing working but I didn't hear anything.
> >
> > Yes, you did: http://marc.info/?l=linux-kernel&m=123342565225134&w=4
>
> My apologies.  I should have said I did not recognize anything that
> was likely to conflict from what I hear from what I heard.  I did
> hear that the pcie port driver was touched to properly handle msi-x and I
> foolishly thought that was already merged upstream.  Especially after
> I glanced at Jesse's tree and I did not see any recent pci patches.
> Which led me to foolishly assume everything had already been merged
> into Linus's tree.
>
> Andrew with respect to 2.6.29 my patch while correct and useful has
> no immediate utility, as the pciehp driver will crash and burn horribly
> if you attempt to hotplug it.  So I should find the other pci port driver
> changes and rebase on top of them.  Assuming that patch is scheduled to
> merge for 2.6.30.

Yeah, if it's in my -next tree, it's scheduled for .30.  I have one other 
hotplug related set from Kenji-san for .29 as well (to fix a regression James 
reported).  Search for "fix wrong assumption" to check that one out.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pcie_portdriver: FIX: pcie_port_device_remove

[Rafael J. Wysocki]

On Friday 20 February 2009, Eric W. Biederman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Thursday 19 February 2009, Eric W. Biederman wrote:
> >> Andrew Morton <akpm@linux-foundation.org> writes:
> >> 
> >> >
> >> > There are large-scale and conflicting changes to this file in linux-next.
> >> >
> >> > If we want to jam this fix into 2.6.29 (and it looks like something we
> >> > want) then this will trash the linux-next changes.  It will cause me
> >> > grief, and will cause Stephen grief unless the pci tree is suitably
> >> > changed, which will cause Jesse grief. Either way: grief.
> >> 
> >> Ugh.
> >> 
> >> I had better have a good hard look at linux-next.  I tried to ask earlier
> > about
> >> ongoing working but I didn't hear anything.
> >
> > Yes, you did: http://marc.info/?l=linux-kernel&m=123342565225134&w=4
> 
> My apologies.

No problem. :-)

Rafael

[PATCH] pcie_portdriver: FIX: pcie_port_device_remove (take 2)

[Eric W. Biederman]

pcie_port_device_remove currently calls the remove method of port
drivers twice.  Ouch!

We are calling device_for_each_child multiple times for no apparent
reason.

So make it simple. Place put_device and device_unregister into
remove_iter, and throw out the rest.  Only call device_for_each_child
once.

The code is simpler and actually works!

Changelog:
v2 rebase against the linux-next tree so I don't conflict with Rafael's
   irq work, and remove the irq handling cleanups as Rafael's patch already
   made them.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---

 drivers/pci/pcie/portdrv_core.c |   23 +++--------------------
 1 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 3aea92a..569af00 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -456,16 +456,9 @@ int pcie_port_device_resume(struct pci_dev *dev)
 
 static int remove_iter(struct device *dev, void *data)
 {
-	struct pcie_port_service_driver *service_driver;
-
 	if (dev->bus == &pcie_port_bus_type) {
-		if (dev->driver) {
-			service_driver = to_service_driver(dev->driver);
-			if (service_driver->remove)
-				service_driver->remove(to_pcie_device(dev));
-		}
-		*(unsigned long*)data = (unsigned long)dev;
-		return 1;
+		put_device(dev);
+		device_unregister(dev);
 	}
 	return 0;
 }
@@ -480,18 +473,8 @@ static int remove_iter(struct device *dev, void *data)
 void pcie_port_device_remove(struct pci_dev *dev)
 {
 	struct pcie_port_data *port_data = pci_get_drvdata(dev);
-	int status;
-
-	do {
-		unsigned long device_addr;
 
-		status = device_for_each_child(&dev->dev, &device_addr, remove_iter);
-		if (status) {
-			struct device *device = (struct device*)device_addr;
-			put_device(device);
-			device_unregister(device);
-		}
-	} while (status);
+	device_for_each_child(&dev->dev, NULL, remove_iter);
 
 	switch (port_data->port_irq_mode) {
 	case PCIE_PORT_MSIX_MODE:
-- 
1.6.0.6

Re: [PATCH] pcie_portdriver: FIX: pcie_port_device_remove (take 2)

[Jesse Barnes]

On Friday, February 20, 2009 8:16:07 pm Eric W. Biederman wrote:
> pcie_port_device_remove currently calls the remove method of port
> drivers twice.  Ouch!
>
> We are calling device_for_each_child multiple times for no apparent
> reason.
>
> So make it simple. Place put_device and device_unregister into
> remove_iter, and throw out the rest.  Only call device_for_each_child
> once.
>
> The code is simpler and actually works!
>
> Changelog:
> v2 rebase against the linux-next tree so I don't conflict with Rafael's
>    irq work, and remove the irq handling cleanups as Rafael's patch already
>    made them.

Thanks for rebasing; it's queued up in my linux-next branch now.  If we really 
need this in the stable kernel it'll be a bit painful, since some of these 
structures have changed around a bit...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pcie_portdriver: FIX: pcie_port_device_remove (take 2)

[Eric W. Biederman]

Jesse Barnes <jbarnes@virtuousgeek.org> writes:

> On Friday, February 20, 2009 8:16:07 pm Eric W. Biederman wrote:
>> pcie_port_device_remove currently calls the remove method of port
>> drivers twice.  Ouch!
>>
>> We are calling device_for_each_child multiple times for no apparent
>> reason.
>>
>> So make it simple. Place put_device and device_unregister into
>> remove_iter, and throw out the rest.  Only call device_for_each_child
>> once.
>>
>> The code is simpler and actually works!
>>
>> Changelog:
>> v2 rebase against the linux-next tree so I don't conflict with Rafael's
>>    irq work, and remove the irq handling cleanups as Rafael's patch already
>>    made them.
>
> Thanks for rebasing; it's queued up in my linux-next branch now.  If we really 
> need this in the stable kernel it'll be a bit painful, since some of these 
> structures have changed around a bit...

I don't think it is necessary for stable.  The pciehp still needs more
work, and if anyone of the other port drivers was being hotplugged I
expect someone would have noticed the bug and fixed it before now.

Eric