Re: [PATCH] Hold reference to device_node during EEH event handling
2009/7/16 Michael Ellerman mich...@ellerman.id.au:
On Thu, 2009-07-16 at 09:33 -0700, Mike Mason wrote:
Michael Ellerman wrote:
On Wed, 2009-07-15 at 14:43 -0700, Mike Mason wrote:
This patch increments the device_node reference counter when an EEH
error occurs and decrements the counter when the event has been
handled. This is to prevent the device_node from being released until
eeh_event_handler() has had a chance to deal with the event. We've
seen cases where the device_node is released too soon when an EEH
event occurs during a dlpar remove, causing the event handler to
attempt to access bad memory locations.
Please review and let me know of any concerns.
Taking a reference sounds sane, but ...
Signed-off-by: Mike Mason mm...@us.ibm.com
--- a/arch/powerpc/platforms/pseries/eeh_event.c 2008-10-09
15:13:53.0 -0700
+++ b/arch/powerpc/platforms/pseries/eeh_event.c 2009-07-14
14:14:00.0 -0700
@@ -75,6 +75,14 @@ static int eeh_event_handler(void * dumm
if (event == NULL)
return 0;
+ /* EEH holds a reference to the device_node, so if it
+ * equals 1 it's no longer valid and the event should
+ * be ignored */
+ if (atomic_read(event-dn-kref.refcount) == 1) {
+ of_node_put(event-dn);
+ return 0;
+ }
That's really gross :)
Agreed. I'll look for another way to determine if device is gone and
the event should be ignored. Suggestions are welcome :-)
Benh and I had a quick chat about it, and were wondering whether what
you really should be doing is taking a reference to the pci device
(perhaps as well as the device node).
@@ -140,7 +149,7 @@ int eeh_send_failure_event (struct devic
if (dev)
pci_dev_get(dev);
- event-dn = dn;
+ event-dn = of_node_get(dn);
event-dev = dev;
pci devs are refcounted too, see pci_dev_get(), so taking a reference
there would be the right thing to do - otherwise there's no guarantee
it still exists later, unless there's some other trick in the EEH code.
I thought that the eeh code did pci gets and puts in the right locations,
perhaps I (incorrectly) assumed that this meant that the of_dn use count
never dropped to zero ...
I think my logic was:
-- pci device init does of_node_get
-- pci device shutdown does of_node_put
-- pci device shutdown can never run as long as pci use count is 0
Thus, explicit of_node_get was usually not needed.
So, for example, see above: I was figuring that the pci_dev_get(dev);
was enough to protect the dn too .. although maybe if dev is null,
then things go wrong ...
--linas
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Hold reference to device_node during EEH event handling
Michael Ellerman wrote:
On Thu, 2009-07-16 at 09:33 -0700, Mike Mason wrote:
Michael Ellerman wrote:
On Wed, 2009-07-15 at 14:43 -0700, Mike Mason wrote:
This patch increments the device_node reference counter when an EEH
error occurs and decrements the counter when the event has been
handled. This is to prevent the device_node from being released until
eeh_event_handler() has had a chance to deal with the event. We've
seen cases where the device_node is released too soon when an EEH
event occurs during a dlpar remove, causing the event handler to
attempt to access bad memory locations.
Please review and let me know of any concerns.
Taking a reference sounds sane, but ...
Signed-off-by: Mike Mason mm...@us.ibm.com
--- a/arch/powerpc/platforms/pseries/eeh_event.c2008-10-09
15:13:53.0 -0700
+++ b/arch/powerpc/platforms/pseries/eeh_event.c2009-07-14
14:14:00.0 -0700
@@ -75,6 +75,14 @@ static int eeh_event_handler(void * dumm
if (event == NULL)
return 0;
+ /* EEH holds a reference to the device_node, so if it
+* equals 1 it's no longer valid and the event should
+* be ignored */
+ if (atomic_read(event-dn-kref.refcount) == 1) {
+ of_node_put(event-dn);
+ return 0;
+ }
That's really gross :)
Agreed. I'll look for another way to determine if device is gone and
the event should be ignored. Suggestions are welcome :-)
Actually, it turns out the atomic_read() isn't necessary. I just need to take
the reference to the device_node when the EEH error is detected and let EEH try
to handle the error. EEH detects the fact that the device is no longer valid,
aborts the recovery attempt, then gives the device_node reference back. Works
as expected.
I'll resubmit the patch without the atomic_read().
Benh and I had a quick chat about it, and were wondering whether what
you really should be doing is taking a reference to the pci device
(perhaps as well as the device node).
EEH already does that 3 lines before the of_node_get (see below).
@@ -140,7 +149,7 @@ int eeh_send_failure_event (struct devic
if (dev)
pci_dev_get(dev);
- event-dn = dn;
+ event-dn = of_node_get(dn);
event-dev = dev;
Thanks,
Mike
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Hold reference to device_node during EEH event handling
On Wed, 2009-07-22 at 16:41 -0700, Mike Mason wrote:
Michael Ellerman wrote:
On Thu, 2009-07-16 at 09:33 -0700, Mike Mason wrote:
Michael Ellerman wrote:
On Wed, 2009-07-15 at 14:43 -0700, Mike Mason wrote:
This patch increments the device_node reference counter when an EEH
error occurs and decrements the counter when the event has been
handled. This is to prevent the device_node from being released until
eeh_event_handler() has had a chance to deal with the event. We've
seen cases where the device_node is released too soon when an EEH
event occurs during a dlpar remove, causing the event handler to
attempt to access bad memory locations.
Please review and let me know of any concerns.
Taking a reference sounds sane, but ...
Signed-off-by: Mike Mason mm...@us.ibm.com
--- a/arch/powerpc/platforms/pseries/eeh_event.c 2008-10-09
15:13:53.0 -0700
+++ b/arch/powerpc/platforms/pseries/eeh_event.c 2009-07-14
14:14:00.0 -0700
@@ -75,6 +75,14 @@ static int eeh_event_handler(void * dumm
if (event == NULL)
return 0;
+/* EEH holds a reference to the device_node, so if it
+ * equals 1 it's no longer valid and the event should
+ * be ignored */
+if (atomic_read(event-dn-kref.refcount) == 1) {
+of_node_put(event-dn);
+return 0;
+}
That's really gross :)
Agreed. I'll look for another way to determine if device is gone and
the event should be ignored. Suggestions are welcome :-)
Actually, it turns out the atomic_read() isn't necessary. I just need
to take the reference to the device_node when the EEH error is
detected and let EEH try to handle the error. EEH detects the fact
that the device is no longer valid, aborts the recovery attempt, then
gives the device_node reference back. Works as expected.
How does it detect that the device is no longer valid?
I'll resubmit the patch without the atomic_read().
Benh and I had a quick chat about it, and were wondering whether what
you really should be doing is taking a reference to the pci device
(perhaps as well as the device node).
EEH already does that 3 lines before the of_node_get (see below).
Ah right, while you're touching the code, mind changing it to the
simpler and more obvious:
event-dev = pci_dev_get(dev);
cheers
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Hold reference to device_node during EEH event handling
Michael Ellerman wrote:
On Wed, 2009-07-15 at 14:43 -0700, Mike Mason wrote:
This patch increments the device_node reference counter when an EEH
error occurs and decrements the counter when the event has been
handled. This is to prevent the device_node from being released until
eeh_event_handler() has had a chance to deal with the event. We've
seen cases where the device_node is released too soon when an EEH
event occurs during a dlpar remove, causing the event handler to
attempt to access bad memory locations.
Please review and let me know of any concerns.
Taking a reference sounds sane, but ...
Signed-off-by: Mike Mason mm...@us.ibm.com
--- a/arch/powerpc/platforms/pseries/eeh_event.c2008-10-09
15:13:53.0 -0700
+++ b/arch/powerpc/platforms/pseries/eeh_event.c2009-07-14
14:14:00.0 -0700
@@ -75,6 +75,14 @@ static int eeh_event_handler(void * dumm
if (event == NULL)
return 0;
+ /* EEH holds a reference to the device_node, so if it
+* equals 1 it's no longer valid and the event should
+* be ignored */
+ if (atomic_read(event-dn-kref.refcount) == 1) {
+ of_node_put(event-dn);
+ return 0;
+ }
That's really gross :)
Agreed. I'll look for another way to determine if device is gone and the event
should be ignored. Suggestions are welcome :-)
And what happens if the refcount goes to 1 just after the check? ie.
here.
/* Serialize processing of EEH events */
mutex_lock(eeh_event_mutex);
eeh_mark_slot(event-dn, EEH_MODE_RECOVERING);
cheers
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Hold reference to device_node during EEH event handling
On Thu, 2009-07-16 at 09:33 -0700, Mike Mason wrote:
Michael Ellerman wrote:
On Wed, 2009-07-15 at 14:43 -0700, Mike Mason wrote:
This patch increments the device_node reference counter when an EEH
error occurs and decrements the counter when the event has been
handled. This is to prevent the device_node from being released until
eeh_event_handler() has had a chance to deal with the event. We've
seen cases where the device_node is released too soon when an EEH
event occurs during a dlpar remove, causing the event handler to
attempt to access bad memory locations.
Please review and let me know of any concerns.
Taking a reference sounds sane, but ...
Signed-off-by: Mike Mason mm...@us.ibm.com
--- a/arch/powerpc/platforms/pseries/eeh_event.c 2008-10-09
15:13:53.0 -0700
+++ b/arch/powerpc/platforms/pseries/eeh_event.c 2009-07-14
14:14:00.0 -0700
@@ -75,6 +75,14 @@ static int eeh_event_handler(void * dumm
if (event == NULL)
return 0;
+ /* EEH holds a reference to the device_node, so if it
+ * equals 1 it's no longer valid and the event should
+ * be ignored */
+ if (atomic_read(event-dn-kref.refcount) == 1) {
+ of_node_put(event-dn);
+ return 0;
+ }
That's really gross :)
Agreed. I'll look for another way to determine if device is gone and
the event should be ignored. Suggestions are welcome :-)
Benh and I had a quick chat about it, and were wondering whether what
you really should be doing is taking a reference to the pci device
(perhaps as well as the device node).
@@ -140,7 +149,7 @@ int eeh_send_failure_event (struct devic
if (dev)
pci_dev_get(dev);
- event-dn = dn;
+ event-dn = of_node_get(dn);
event-dev = dev;
pci devs are refcounted too, see pci_dev_get(), so taking a reference
there would be the right thing to do - otherwise there's no guarantee
it still exists later, unless there's some other trick in the EEH code.
Taking a reference would presumably block a concurrent hotunplug until
you'd processed the EEH event and dropped your reference. That might be
OK, or you could add a hotplug notifier to the EEH code and drop the
reference there and mark the event as handled or something.
All of that with the caveat that I don't really know the EEH or hotplug
code :D
cheers
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] Hold reference to device_node during EEH event handling
This patch increments the device_node reference counter when an EEH error
occurs and decrements the counter when the event has been handled. This is to
prevent the device_node from being released until eeh_event_handler() has had a
chance to deal with the event. We've seen cases where the device_node is
released too soon when an EEH event occurs during a dlpar remove, causing the
event handler to attempt to access bad memory locations.
Please review and let me know of any concerns.
Signed-off-by: Mike Mason mm...@us.ibm.com
--- a/arch/powerpc/platforms/pseries/eeh_event.c2008-10-09
15:13:53.0 -0700
+++ b/arch/powerpc/platforms/pseries/eeh_event.c2009-07-14
14:14:00.0 -0700
@@ -75,6 +75,14 @@ static int eeh_event_handler(void * dumm
if (event == NULL)
return 0;
+ /* EEH holds a reference to the device_node, so if it
+* equals 1 it's no longer valid and the event should
+* be ignored */
+ if (atomic_read(event-dn-kref.refcount) == 1) {
+ of_node_put(event-dn);
+ return 0;
+ }
+
/* Serialize processing of EEH events */
mutex_lock(eeh_event_mutex);
eeh_mark_slot(event-dn, EEH_MODE_RECOVERING);
@@ -86,6 +94,7 @@ static int eeh_event_handler(void * dumm
eeh_clear_slot(event-dn, EEH_MODE_RECOVERING);
pci_dev_put(event-dev);
+ of_node_put(event-dn);
kfree(event);
mutex_unlock(eeh_event_mutex);
@@ -140,7 +149,7 @@ int eeh_send_failure_event (struct devic
if (dev)
pci_dev_get(dev);
- event-dn = dn;
+ event-dn = of_node_get(dn);
event-dev = dev;
/* We may or may not be called in an interrupt context */
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] Hold reference to device_node during EEH event handling
On Wed, 2009-07-15 at 14:43 -0700, Mike Mason wrote:
This patch increments the device_node reference counter when an EEH
error occurs and decrements the counter when the event has been
handled. This is to prevent the device_node from being released until
eeh_event_handler() has had a chance to deal with the event. We've
seen cases where the device_node is released too soon when an EEH
event occurs during a dlpar remove, causing the event handler to
attempt to access bad memory locations.
Please review and let me know of any concerns.
Taking a reference sounds sane, but ...
Signed-off-by: Mike Mason mm...@us.ibm.com
--- a/arch/powerpc/platforms/pseries/eeh_event.c 2008-10-09
15:13:53.0 -0700
+++ b/arch/powerpc/platforms/pseries/eeh_event.c 2009-07-14
14:14:00.0 -0700
@@ -75,6 +75,14 @@ static int eeh_event_handler(void * dumm
if (event == NULL)
return 0;
+ /* EEH holds a reference to the device_node, so if it
+ * equals 1 it's no longer valid and the event should
+ * be ignored */
+ if (atomic_read(event-dn-kref.refcount) == 1) {
+ of_node_put(event-dn);
+ return 0;
+ }
That's really gross :)
And what happens if the refcount goes to 1 just after the check? ie.
here.
/* Serialize processing of EEH events */
mutex_lock(eeh_event_mutex);
eeh_mark_slot(event-dn, EEH_MODE_RECOVERING);
cheers
signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
