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

[Mike Mason <mmlnx@us.ibm.com>]

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