[PATCH] Hold reference to device_node during EEH event handling

[PATCH] Hold reference to device_node during EEH event handling

[Mike Mason]

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 <mmlnx@us.ibm.com> 

--- a/arch/powerpc/platforms/pseries/eeh_event.c	2008-10-09 15:13:53.000000000 -0700
+++ b/arch/powerpc/platforms/pseries/eeh_event.c	2009-07-14 14:14:00.000000000 -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 */

Re: [PATCH] Hold reference to device_node during EEH event handling

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

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 <mmlnx@us.ibm.com> 
> 
> --- a/arch/powerpc/platforms/pseries/eeh_event.c	2008-10-09 15:13:53.000000000 -0700
> +++ b/arch/powerpc/platforms/pseries/eeh_event.c	2009-07-14 14:14:00.000000000 -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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Hold reference to device_node during EEH event handling

[Mike Mason]

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 <mmlnx@us.ibm.com> 
>>
>> --- a/arch/powerpc/platforms/pseries/eeh_event.c	2008-10-09 15:13:53.000000000 -0700
>> +++ b/arch/powerpc/platforms/pseries/eeh_event.c	2009-07-14 14:14:00.000000000 -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
> 

Re: [PATCH] Hold reference to device_node during EEH event handling

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]

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 <mmlnx@us.ibm.com> 
> >>
> >> --- a/arch/powerpc/platforms/pseries/eeh_event.c	2008-10-09 15:13:53.000000000 -0700
> >> +++ b/arch/powerpc/platforms/pseries/eeh_event.c	2009-07-14 14:14:00.000000000 -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




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Hold reference to device_node during EEH event handling

[Mike Mason]

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 <mmlnx@us.ibm.com> 
>>>>
>>>> --- a/arch/powerpc/platforms/pseries/eeh_event.c	2008-10-09 15:13:53.000000000 -0700
>>>> +++ b/arch/powerpc/platforms/pseries/eeh_event.c	2009-07-14 14:14:00.000000000 -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

Re: [PATCH] Hold reference to device_node during EEH event handling

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]

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 <mmlnx@us.ibm.com> 
> >>>>
> >>>> --- a/arch/powerpc/platforms/pseries/eeh_event.c	2008-10-09 15:13:53.000000000 -0700
> >>>> +++ b/arch/powerpc/platforms/pseries/eeh_event.c	2009-07-14 14:14:00.000000000 -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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Hold reference to device_node during EEH event handling

[Linas Vepstas]

2009/7/16 Michael Ellerman <michael@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. =C2=A0This is to prevent the device_node from being released=
 until
>> >> eeh_event_handler() has had a chance to deal with the event. =C2=A0We=
'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 <mmlnx@us.ibm.com>
>> >>
>> >> --- a/arch/powerpc/platforms/pseries/eeh_event.c =C2=A0 2008-10-09 15=
:13:53.000000000 -0700
>> >> +++ b/arch/powerpc/platforms/pseries/eeh_event.c =C2=A0 2009-07-14 14=
:14:00.000000000 -0700
>> >> @@ -75,6 +75,14 @@ static int eeh_event_handler(void * dumm
>> >> =C2=A0 =C2=A0if (event =3D=3D NULL)
>> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
>> >>
>> >> + =C2=A0/* EEH holds a reference to the device_node, so if it
>> >> + =C2=A0 * equals 1 it's no longer valid and the event should
>> >> + =C2=A0 * be ignored */
>> >> + =C2=A0if (atomic_read(&event->dn->kref.refcount) =3D=3D 1) {
>> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0of_node_put(event->dn);
>> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
>> >> + =C2=A0}
>> >
>> > That's really gross :)
>>
>> Agreed. =C2=A0I'll look for another way to determine if device is gone a=
nd
>> the event should be ignored. =C2=A0Suggestions 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
> =C2=A0 =C2=A0 =C2=A0 =C2=A0if (dev)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pci_dev_get(dev);
>
> - =C2=A0 =C2=A0 =C2=A0 event->dn =3D dn;
> + =C2=A0 =C2=A0 =C2=A0 event->dn =3D of_node_get(dn);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0event->dev =3D 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