* [PATCH] powerpc/eeh: drop taken reference to driver on eeh_rmv_device
@ 2014-01-30 13:00 Thadeu Lima de Souza Cascardo
2014-01-31 0:46 ` Gavin Shan
0 siblings, 1 reply; 4+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2014-01-30 13:00 UTC (permalink / raw)
To: linuxppc-dev; +Cc: paulus, shangw, Thadeu Lima de Souza Cascardo
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)
pci_stop_and_remove_bus_device(dev);
pci_unlock_rescan_remove();
+out:
+ if (driver)
+ eeh_pcid_put(dev);
return NULL;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/eeh: drop taken reference to driver on eeh_rmv_device
2014-01-30 13:00 [PATCH] powerpc/eeh: drop taken reference to driver on eeh_rmv_device Thadeu Lima de Souza Cascardo
@ 2014-01-31 0:46 ` Gavin Shan
2014-01-31 17:24 ` Thadeu Lima de Souza Cascardo
0 siblings, 1 reply; 4+ messages in thread
From: Gavin Shan @ 2014-01-31 0:46 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo; +Cc: linuxppc-dev, shangw, paulus
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().
> 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.
> }
>
Thanks,
Gavin
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/eeh: drop taken reference to driver on eeh_rmv_device
2014-01-31 0:46 ` Gavin Shan
@ 2014-01-31 17:24 ` Thadeu Lima de Souza Cascardo
2014-02-04 2:03 ` Gavin Shan
0 siblings, 1 reply; 4+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2014-01-31 17:24 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev, paulus
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 */
---
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/eeh: drop taken reference to driver on eeh_rmv_device
2014-01-31 17:24 ` Thadeu Lima de Souza Cascardo
@ 2014-02-04 2:03 ` Gavin Shan
0 siblings, 0 replies; 4+ messages in thread
From: Gavin Shan @ 2014-02-04 2:03 UTC (permalink / raw)
To: Thadeu Lima de Souza Cascardo; +Cc: linuxppc-dev, Gavin Shan, paulus
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-02-04 2:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-30 13:00 [PATCH] powerpc/eeh: drop taken reference to driver on eeh_rmv_device Thadeu Lima de Souza Cascardo
2014-01-31 0:46 ` Gavin Shan
2014-01-31 17:24 ` Thadeu Lima de Souza Cascardo
2014-02-04 2:03 ` Gavin Shan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.