Michael Bringmann <m...@linux.vnet.ibm.com> writes: > On 10/16/2018 02:57 PM, Tyrel Datwyler wrote: >> On 10/15/2018 05:39 PM, Michael Ellerman wrote: >>> Michael Bringmann <m...@linux.vnet.ibm.com> writes: >>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c >>>> b/arch/powerpc/platforms/pseries/hotplug-memory.c >>>> index 2b796da..9c76345 100644 >>>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c >>>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c >>>> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index) >>>> return rc; >>>> } >>>> >>>> +static int dlpar_memory_readd_multiple(void) >>>> +{ >>>> + struct drmem_lmb *lmb; >>>> + int rc; >>>> + >>>> + pr_info("Attempting to update multiple LMBs\n"); >>>> + >>>> + for_each_drmem_lmb(lmb) { >>>> + if (drmem_lmb_update(lmb)) { >>>> + rc = dlpar_memory_readd_helper(lmb); >>>> + drmem_remove_lmb_update(lmb); >>>> + } >>>> + } >>>> + >>>> + return rc; >>>> +} >>> >>> This leaves rc potentially uninitialised. >>> >>> What should the result be in that case, -EINVAL ? >> >> On another note if there are multiple LMBs to update the value of rc only >> reflects the final dlpar_memory_readd_helper() call. > > Correct. But that is what happens when we compress common code > between two disparate uses i.e. updating memory association after > a migration event with no reporting mechanism other than the console > log, vs re-adding a single LMB by index for the purposes of DLPAR / drmgr. > > I could discard the return value from dlpar_memory_readd_helper entirely > in this function and just return 0, but in my experience, once errors start > to occur in memory dlpar ops, they tend to keep on occurring, so I was > returning the last one. We could also make the code smart enough to > capture and return the first/last non-zero return code. I didn't believe > that the frequency of errors for this operation warranted the overhead.
The actual error value is probably not very relevant. But dropping errors entirely is almost always a bad idea. So I think you should at least return an error if any error occurred, that way at least an error will be returned up to the caller(s). Something like: int rc; rc = 0; for_each_drmem_lmb(lmb) { if (drmem_lmb_update(lmb)) { rc |= dlpar_memory_readd_helper(lmb); drmem_remove_lmb_update(lmb); } } if (rc) return -EIO; cheers