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

[Michael Ellerman <michael@ellerman.id.au>]

[-- 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 --]