| Message ID | 1484104609-22129-1-git-send-email-gwshan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > In __eeh_clear_pe_frozen_state(), we should pass the flag's value > instead of its address to eeh_unfreeze_pe(). This doesn't introduce > any problems, but the code is just wrong. It means any caller that passes false, will be getting the wrong behaviour. eg. I see at least one call in eeh_reset_device() which passes false to eeh_clear_pe_frozen_state(), which is then passed to __eeh_clear_pe_frozen_state(). But I guess you're saying that caller doesn't actually see a bug because of this? > This fixes the code by passing flag's value to eeh_unfreeze_pe(). > > Cc: stable@vger.kernel.org #3.18+ > Fixes: 5cfb20b96f6 ("powerpc/eeh: Emulate EEH recovery for VFIO devices") > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > > diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c > index d88573b..fa15fa6 100644 > --- a/arch/powerpc/kernel/eeh_driver.c > +++ b/arch/powerpc/kernel/eeh_driver.c > @@ -549,7 +549,7 @@ static void *__eeh_clear_pe_frozen_state(void *data, void *flag) bool *clear_sw_state = flag; > int i, rc = 1; > > for (i = 0; rc && i < 3; i++) > - rc = eeh_unfreeze_pe(pe, clear_sw_state); > + rc = eeh_unfreeze_pe(pe, *clear_sw_state); I think it would be better to just do the dereference once: bool clear_sw_state = *(bool *)flag; int i, rc = 1; for (i = 0; rc && i < 3; i++) rc = eeh_unfreeze_pe(pe, clear_sw_state); cheers
On Wed, Jan 18, 2017 at 04:49:58PM +1100, Michael Ellerman wrote: >Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > >> In __eeh_clear_pe_frozen_state(), we should pass the flag's value >> instead of its address to eeh_unfreeze_pe(). This doesn't introduce >> any problems, but the code is just wrong. > >It means any caller that passes false, will be getting the wrong >behaviour. eg. I see at least one call in eeh_reset_device() which >passes false to eeh_clear_pe_frozen_state(), which is then passed to >__eeh_clear_pe_frozen_state(). > Yes, when __eeh_clear_pe_frozen_state() succeeds on all child PEs, the isolated flag is cleared. No failure from __eeh_clear_pe_frozen_state() was observed previously. >But I guess you're saying that caller doesn't actually see a bug because >of this? > Yes, I'll update the changelog accordingly, to give more details as above. >> This fixes the code by passing flag's value to eeh_unfreeze_pe(). >> >> Cc: stable@vger.kernel.org #3.18+ >> Fixes: 5cfb20b96f6 ("powerpc/eeh: Emulate EEH recovery for VFIO devices") >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> >> diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c >> index d88573b..fa15fa6 100644 >> --- a/arch/powerpc/kernel/eeh_driver.c >> +++ b/arch/powerpc/kernel/eeh_driver.c >> @@ -549,7 +549,7 @@ static void *__eeh_clear_pe_frozen_state(void *data, void *flag) > > bool *clear_sw_state = flag; > >> int i, rc = 1; >> >> for (i = 0; rc && i < 3; i++) >> - rc = eeh_unfreeze_pe(pe, clear_sw_state); >> + rc = eeh_unfreeze_pe(pe, *clear_sw_state); > > >I think it would be better to just do the dereference once: > > bool clear_sw_state = *(bool *)flag; > int i, rc = 1; > > for (i = 0; rc && i < 3; i++) > rc = eeh_unfreeze_pe(pe, clear_sw_state); > Thanks, Michael. I'll update in v2. Thanks, Gavin
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c index d88573b..fa15fa6 100644 --- a/arch/powerpc/kernel/eeh_driver.c +++ b/arch/powerpc/kernel/eeh_driver.c @@ -549,7 +549,7 @@ static void *__eeh_clear_pe_frozen_state(void *data, void *flag) int i, rc = 1; for (i = 0; rc && i < 3; i++) - rc = eeh_unfreeze_pe(pe, clear_sw_state); + rc = eeh_unfreeze_pe(pe, *clear_sw_state); /* Stop immediately on any errors */ if (rc) {
In __eeh_clear_pe_frozen_state(), we should pass the flag's value instead of its address to eeh_unfreeze_pe(). This doesn't introduce any problems, but the code is just wrong. This fixes the code by passing flag's value to eeh_unfreeze_pe(). Cc: stable@vger.kernel.org #3.18+ Fixes: 5cfb20b96f6 ("powerpc/eeh: Emulate EEH recovery for VFIO devices") Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- arch/powerpc/kernel/eeh_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)