Re: [PATCH 32/38] pcie: add missing put_device call

[Bjorn Helgaas <bhelgaas@google.com>]

[+cc Greg, Yinghai]

On Thu, Dec 19, 2013 at 04:06:46PM +0100, Levente Kurusa wrote:
> This is required so that we give up the last reference to the device.
> Removed the kfree() as put_device will result in release_pcie_device being
> called and hence the container of the device will be kfree'd.
> 
> Signed-off-by: Levente Kurusa <levex@linux.com>

Thanks, I applied a slightly modified version of this to my pci/deletion
branch for v3.14.

I think the get_device() after device_register() succeeds and the
put_device() before device_unregister() are superfluous, so I propose the
series included below.  Any comments?

> ---
>  drivers/pci/pcie/portdrv_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 08d131f..80fb1f2 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -345,7 +345,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
>  
>  	retval = device_register(device);
>  	if (retval)
> -		kfree(pcie);
> +		put_device(device);
>  	else
>  		get_device(device);
>  	return retval;


commit 8f3acca9acec1503f6b374faef2d1013cbf502af
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Thu Dec 19 14:20:09 2013 -0700

    PCI/portdrv: Cleanup error paths
    
    Make the straightline path the normal no-error path.  Check for errors and
    return them directly, instead of checking for success and putting the
    normal path in an "if" body.
    
    No functional change.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 0b6e76604068..fc86d323fecc 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -344,11 +344,13 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
 	device_enable_async_suspend(device);
 
 	retval = device_register(device);
-	if (retval)
+	if (retval) {
 		kfree(pcie);
-	else
-		get_device(device);
-	return retval;
+		return retval;
+	}
+
+	get_device(device);
+	return 0;
 }
 
 /**
@@ -498,12 +500,12 @@ static int pcie_port_probe_service(struct device *dev)
 
 	pciedev = to_pcie_device(dev);
 	status = driver->probe(pciedev);
-	if (!status) {
-		dev_printk(KERN_DEBUG, dev, "service driver %s loaded\n",
-			driver->name);
-		get_device(dev);
-	}
-	return status;
+	if (status)
+		return status;
+
+	dev_printk(KERN_DEBUG, dev, "service driver %s loaded\n", driver->name);
+	get_device(dev);
+	return 0;
 }
 
 /**

commit f39862058e1f278e0495cd9ea57de571e74aa1fe
Author: Levente Kurusa <levex@linux.com>
Date:   Thu Dec 19 14:22:35 2013 -0700

    PCI/portdrv: Add put_device() after device_register() failure
    
    This is required so that we give up the last reference to the device.
    Removed the kfree() as put_device will result in release_pcie_device()
    being called and hence the container of the device will be kfree'd.
    
    [bhelgaas: fix conflict after my previous cleanup]
    Signed-off-by: Levente Kurusa <levex@linux.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index fc86d323fecc..9811eea53511 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -345,7 +345,7 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
 
 	retval = device_register(device);
 	if (retval) {
-		kfree(pcie);
+		put_device(device);
 		return retval;
 	}
 

commit e75f34ce6633549486a044d64b2a79240d4113a8
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Thu Dec 19 14:24:13 2013 -0700

    PCI/portdrv: Remove extra get_device()/put_device() for pcie_device
    
    Previously pcie_device_init() called get_device() if device_register() for
    the new pcie_device succeeded, and remove_iter() called put_device() when
    removing before unregistering the device.
    
    But device_register() already increments the reference count in
    device_add(), so we don't need to do it again here.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 9811eea53511..6a6e54909335 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -349,7 +349,6 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
 		return retval;
 	}
 
-	get_device(device);
 	return 0;
 }
 
@@ -456,10 +455,8 @@ int pcie_port_device_resume(struct device *dev)
 
 static int remove_iter(struct device *dev, void *data)
 {
-	if (dev->bus == &pcie_port_bus_type) {
-		put_device(dev);
+	if (dev->bus == &pcie_port_bus_type)
 		device_unregister(dev);
-	}
 	return 0;
 }