[PATCH 2/3] powerpc/pseries/memhp: Remove unbalanced dlpar_release_drc() call

[Nathan Lynch]

Nick Child <nnac123 at linux.ibm.com> writes:
> Hi Nathan,
> Patches 1 and 3 LGTM

thanks.

> Regarding this patch, dlpar_memory_remove_by_count() calls 
> dlpar_add_lmb() and does not free drc on add error.
> dlpar_add_lmb() is called here in error recovery so probably
> not a big deal.
>
> This is all new code to me but it looks like if the requested
> number of lmbs cannot be removed then it attempts to add back
> the ones that were successfully removed. So if you cannot add
> an lmb that WAS successfully removed, it seems sane to also
> release the drc.

Maybe I'll drop this one for now and turn my attention to removing all
the high-level rollback logic in this code. There's no reliable way to
accomplish what it's trying to do (i.e. restore the original quantity of
LMBs) and it just complicates things.


> On 11/14/23 11:01, Nathan Lynch via B4 Relay wrote:
>> From: Nathan Lynch <nathanl at linux.ibm.com>
>> 
>> Callers of dlpar_add_lmb() are responsible for first acquiring the DRC
>> and releasing it if dlpar_add_lmb() fails.
>> 
>> However, dlpar_add_lmb() performs a dlpar_release_drc() in one error
>> branch.  There is no corresponding dlpar_acquire_drc() in the
>> function, nor is there any stated justification. None of the other
>> error paths in dlpar_add_lmb() release the DRC.
>> 
>> This is a potential source of redundant attempts to release DRCs,
>> which is likely benign, but is confusing and inconsistent. Remove it.
>> 
>> Signed-off-by: Nathan Lynch <nathanl at linux.ibm.com>
>> ---
>>   arch/powerpc/platforms/pseries/hotplug-memory.c | 1 -
>>   1 file changed, 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index 6f2eebae7bee..ba883c1b9f6d 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -575,7 +575,6 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
>>   
>>   	rc = update_lmb_associativity_index(lmb);
>>   	if (rc) {
>> -		dlpar_release_drc(lmb->drc_index);
>>   		return rc;
>>   	}
>>   
>>