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

[Gavin Shan <shangw@linux.vnet.ibm.com>]

On Fri, Jan 31, 2014 at 03:24:58PM -0200, Thadeu Lima de Souza Cascardo wrote:
>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 */

It looks good to me. Could you please send v2 with:

Acked-by: Gavin Shan <shangw@linux.vnet.ibm.com>

Or we can simply do like this. It depends your preference :-)

	if (driver) {
		eeh_pcid_put(dev);
		if (driver->err_handler)
			return NULL;
	}

Thanks,
Gavin