Re: [PATCH 12/29] powerpc/pseries/mobility: extract VASI session polling logic

[Nathan Lynch <nathanl@linux.ibm.com>]

Michael Ellerman <mpe@ellerman.id.au> writes:
> Nathan Lynch <nathanl@linux.ibm.com> writes:
>> The behavior of rtas_ibm_suspend_me_unsafe() is to return -EAGAIN to
>> the caller until the specified VASI suspend session state makes the
>> transition from H_VASI_ENABLED to H_VASI_SUSPENDING. In the interest
>> of separating concerns to prepare for a new implementation of the
>> join/suspend sequence, extract VASI session polling logic into a
>> couple of local functions. Waiting for the session state to reach
>> H_VASI_SUSPENDING before calling rtas_ibm_suspend_me_unsafe() ensures
>> that we will never get an EAGAIN result necessitating a retry. No
>> user-visible change in behavior is intended.
>>
>> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/mobility.c | 76 +++++++++++++++++++++--
>>  1 file changed, 71 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
>> index dc6abf164db7..1b8ae221b98a 100644
>> --- a/arch/powerpc/platforms/pseries/mobility.c
>> +++ b/arch/powerpc/platforms/pseries/mobility.c
>> @@ -345,6 +345,73 @@ void post_mobility_fixup(void)
> ...
>
>> +
>> +static int wait_for_vasi_session_suspending(u64 handle)
>> +{
>> +	unsigned long state;
>> +	bool keep_polling;
>> +	int ret;
>> +
>> +	/*
>> +	 * Wait for transition from H_VASI_ENABLED to
>> +	 * H_VASI_SUSPENDING. Treat anything else as an error.
>> +	 */
>> +	do {
>> +		keep_polling = false;
>> +		ret = poll_vasi_state(handle, &state);
>> +		if (ret != 0)
>> +			break;
>> +
>> +		switch (state) {
>> +		case H_VASI_SUSPENDING:
>> +			break;
>> +		case H_VASI_ENABLED:
>> +			keep_polling = true;
>> +			ssleep(1);
>> +			break;
>> +		default:
>> +			pr_err("unexpected H_VASI_STATE result %lu\n", state);
>> +			ret = -EIO;
>> +			break;
>> +		}
>> +	} while (keep_polling);
>
> This seems like it could be simpler?
>
> eg:
>
> 	while (true) {
> 		ret = poll_vasi_state(handle, &state);
>
> 		if (ret != 0 || state == H_VASI_SUSPENDING)
> 			break;
> 		else if (state == H_VASI_ENABLED)
> 			ssleep(1);
> 		else {
> 			pr_err("unexpected H_VASI_STATE result %lu\n", state);
> 			ret = -EIO;
> 			break;
> 		}
> 	}
>
>
> Or did I miss something?

No I think you're right, thanks.