Re: [PATCH] powerpc/eeh: drop taken reference to driver on eeh_rmv_device

[Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>]

On Fri, Jan 31, 2014 at 08:46:11AM +0800, Gavin Shan wrote:
> On Thu, Jan 30, 2014 at 11:00:48AM -0200, Thadeu Lima de Souza Cascardo wrote:
> >Commit f5c57710dd62dd06f176934a8b4b8accbf00f9f8 ("powerpc/eeh: Use
> >partial hotplug for EEH unaware drivers") introduces eeh_rmv_device,
> >which may grab a reference to a driver, but not release it.
> >
> >That prevents a driver from being removed after it has gone through EEH
> >recovery.
> >
> >This patch drops the reference in either exit path if it was taken.
> >
> >Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
> >---
> > arch/powerpc/kernel/eeh_driver.c |    5 ++++-
> > 1 files changed, 4 insertions(+), 1 deletions(-)
> >
> >diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
> >index 7bb30dc..afe7337 100644
> >--- a/arch/powerpc/kernel/eeh_driver.c
> >+++ b/arch/powerpc/kernel/eeh_driver.c
> >@@ -364,7 +364,7 @@ static void *eeh_rmv_device(void *data, void *userdata)
> > 		return NULL;
> > 	driver = eeh_pcid_get(dev);
> > 	if (driver && driver->err_handler)
> >-		return NULL;
> >+		goto out;
> >
> > 	/* Remove it from PCI subsystem */
> > 	pr_debug("EEH: Removing %s without EEH sensitive driver\n",
> >@@ -377,6 +377,9 @@ static void *eeh_rmv_device(void *data, void *userdata)
> 
> For normal case (driver without EEH support), we probably release the reference
> to the driver before pci_stop_and_remove_bus_device().

You are right, we need to call it before we call
pci_stop_and_remove_bus_device, otherwise dev->driver will be NULL, and
eeh_pcid_put will not do module_put. On the other hand, we could change
the call to eeh_pcid_put to accept struct pci_driver instead.

> 
> > 	pci_stop_and_remove_bus_device(dev);
> > 	pci_unlock_rescan_remove();
> >
> >+out:
> >+	if (driver)
> >+		eeh_pcid_put(dev);
> > 	return NULL;
> 
> We needn't "if (driver)" here as eeh_pcid_put() already had the check.
> 

What if try_module_get returned false on eeh_pcid_get?

How about something like the patch below?

> > }
> >
> 
> Thanks,
> Gavin
---
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index 7bb30dc..3a397fa 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -352,6 +352,7 @@ static void *eeh_rmv_device(void *data, void *userdata)
 	struct eeh_dev *edev = (struct eeh_dev *)data;
 	struct pci_dev *dev = eeh_dev_to_pci_dev(edev);
 	int *removed = (int *)userdata;
+	bool has_err_handler;
 
 	/*
 	 * Actually, we should remove the PCI bridges as well.
@@ -362,8 +363,12 @@ static void *eeh_rmv_device(void *data, void *userdata)
 	 */
 	if (!dev || (dev->hdr_type & PCI_HEADER_TYPE_BRIDGE))
 		return NULL;
+
 	driver = eeh_pcid_get(dev);
-	if (driver && driver->err_handler)
+	has_err_handler = driver && driver->err_handler;
+	if (driver)
+		eeh_pcid_put(dev);
+	if (has_err_handler)
 		return NULL;
 
 	/* Remove it from PCI subsystem */
---