| Message ID | 1511421587-26694-1-git-send-email-maddy@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hw/imc: Check ucode state in all the chip | expand |
Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes: > disable_unavailable_units() checks whether the ucode > is in the running state before enabling the nest units > in the device tree. From a recent debug, it is found > that on some system boot, ucode is not loaded and > running in all the chips in the system. And this > caused a fail in OPAL_IMC_COUNTERS_STOP call where > we check for ucode state on each chip. Bug here is > that disable_unavailable_units() checks the state > of the ucode only in boot cpu chip. Patch adds a > condition in disable_unavailable_units() to check > for the ucode state in all the chip before enabling > the nest units in the device tree node. > > Fixes: f98d59958db19 ('skiboot: Find the IMC DTB') > Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> > --- > hw/imc.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/hw/imc.c b/hw/imc.c > index 88d0302d9da6..1db94ec6ac1b 100644 > --- a/hw/imc.c > +++ b/hw/imc.c > @@ -320,10 +320,23 @@ static void disable_unavailable_units(struct dt_node *dev) > struct imc_chip_cb *cb; > struct dt_node *target; > int i; > + bool flag = true; > + struct proc_chip *chip; > + > + /* > + * Check the state of ucode in all the chip. > + * Disable the nest unit if ucode is not initialized > + * in any of the chip. > + */ > + for_each_chip(chip) { > + cb = get_imc_cb(chip->id); > + if (!cb) > + flag = false; > + } > > /* Fetch the IMC control block structure */ > cb = get_imc_cb(this_cpu()->chip_id); > - if (cb) > + if (cb && flag) > avl_vec = be64_to_cpu(cb->imc_chip_avl_vector); > else { > avl_vec = 0; /* Remove only nest imc device nodes */ (just confirming what we chatted about on slack), there's a bug here where it's not properly handling things unless it's the last chip that the ucode isn't running on. Awaiting v2 :)
On Friday 01 December 2017 11:06 AM, Stewart Smith wrote: > Madhavan Srinivasan <maddy@linux.vnet.ibm.com> writes: >> disable_unavailable_units() checks whether the ucode >> is in the running state before enabling the nest units >> in the device tree. From a recent debug, it is found >> that on some system boot, ucode is not loaded and >> running in all the chips in the system. And this >> caused a fail in OPAL_IMC_COUNTERS_STOP call where >> we check for ucode state on each chip. Bug here is >> that disable_unavailable_units() checks the state >> of the ucode only in boot cpu chip. Patch adds a >> condition in disable_unavailable_units() to check >> for the ucode state in all the chip before enabling >> the nest units in the device tree node. >> >> Fixes: f98d59958db19 ('skiboot: Find the IMC DTB') >> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >> --- >> hw/imc.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/hw/imc.c b/hw/imc.c >> index 88d0302d9da6..1db94ec6ac1b 100644 >> --- a/hw/imc.c >> +++ b/hw/imc.c >> @@ -320,10 +320,23 @@ static void disable_unavailable_units(struct dt_node *dev) >> struct imc_chip_cb *cb; >> struct dt_node *target; >> int i; >> + bool flag = true; >> + struct proc_chip *chip; >> + >> + /* >> + * Check the state of ucode in all the chip. >> + * Disable the nest unit if ucode is not initialized >> + * in any of the chip. >> + */ >> + for_each_chip(chip) { >> + cb = get_imc_cb(chip->id); >> + if (!cb) >> + flag = false; >> + } >> >> /* Fetch the IMC control block structure */ >> cb = get_imc_cb(this_cpu()->chip_id); >> - if (cb) >> + if (cb && flag) >> avl_vec = be64_to_cpu(cb->imc_chip_avl_vector); >> else { >> avl_vec = 0; /* Remove only nest imc device nodes */ > (just confirming what we chatted about on slack), there's a bug here > where it's not properly handling things unless it's the last chip that > the ucode isn't running on. > > Awaiting v2 :) Definitely respining the patch. My bad. But nice catch. Thanks for review comments Maddy >
diff --git a/hw/imc.c b/hw/imc.c index 88d0302d9da6..1db94ec6ac1b 100644 --- a/hw/imc.c +++ b/hw/imc.c @@ -320,10 +320,23 @@ static void disable_unavailable_units(struct dt_node *dev) struct imc_chip_cb *cb; struct dt_node *target; int i; + bool flag = true; + struct proc_chip *chip; + + /* + * Check the state of ucode in all the chip. + * Disable the nest unit if ucode is not initialized + * in any of the chip. + */ + for_each_chip(chip) { + cb = get_imc_cb(chip->id); + if (!cb) + flag = false; + } /* Fetch the IMC control block structure */ cb = get_imc_cb(this_cpu()->chip_id); - if (cb) + if (cb && flag) avl_vec = be64_to_cpu(cb->imc_chip_avl_vector); else { avl_vec = 0; /* Remove only nest imc device nodes */
disable_unavailable_units() checks whether the ucode is in the running state before enabling the nest units in the device tree. From a recent debug, it is found that on some system boot, ucode is not loaded and running in all the chips in the system. And this caused a fail in OPAL_IMC_COUNTERS_STOP call where we check for ucode state on each chip. Bug here is that disable_unavailable_units() checks the state of the ucode only in boot cpu chip. Patch adds a condition in disable_unavailable_units() to check for the ucode state in all the chip before enabling the nest units in the device tree node. Fixes: f98d59958db19 ('skiboot: Find the IMC DTB') Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> --- hw/imc.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)