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.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
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
