| Message ID | 4A5E4D68.6070909@us.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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 >
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
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
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
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. 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. 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
--- 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 */
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>