Re: [PATCH] reorg RTAS delay code

[Nathan Lynch <ntl@pobox.com>]

Hi folks-

John Rose wrote:
> This patch attempts to handle RTAS "busy" return codes in a more simple
> and consistent manner.  Typical callers of RTAS shouldn't have to
> manage wait times and delay calls.
> 
> This patch also changes the kernel to use msleep() rather than udelay()
> when a runtime delay is necessary.  This will avoid CPU soft lockups
> for extended delay conditions.

...

> +/* For an RTAS busy status code, perform the hinted delay. */
> +unsigned int rtas_busy_delay(int status)
> +{
> +	unsigned int ms;
>  
> -	/* Use microseconds for reasonable accuracy */
> -	for (ms = 1; order > 0; order--)
> -		ms *= 10;
> +	might_sleep();
> +	ms = rtas_busy_delay_time(status);
> +	if (ms)
> +		msleep(ms);
>  
> -	return ms; 
> +	return ms;
>  }

So I managed to test this with cpu hotplug recently and the
might_sleep warning triggers in the cpu offline path (I had to
reconstruct this from xmon due to the kernel crashing later on for a
different reason, so it might be a little off):

BUG: sleeping function called from invalid context at arch/powerpc/kernel/rtas.c:463.
in_atomic():1, irqs_disabled():1.
Call Trace:
[C0000000AAD379A0] [C000000000010ADC] .show_stack+0x68/0x1b4 (unreliable)
[C0000000AAD37A50] [C000000000050C98] .__might_sleep+0xd0/0xec
[C0000000AAD37AE0] [C00000000001DF5C] .rtas_busy_delay+0x20/0x54
[C0000000AAD37B70] [C00000000001E2D0] .rtas_set_indicator+0x70/0xd4
[C0000000AAD37C10] [C0000000000491C8] .xics_migrate_irqs_away+0x78/0x228
[C0000000AAD37CD0] [C000000000047C14] .pSeries_cpu_disable+0x98/0xb4
[C0000000AAD37D50] [C000000000029A5C] .__cpu_disable+0x4c/0x60
[C0000000AAD37DC0] [C000000000080834] .take_cpu_down+0x10/0x38
[C0000000AAD37E40] [C00000000008D1C0] .do_stop+0x198/0x248
[C0000000AAD37EE0] [C000000000078124] .kthread+0x124/0x174
[C0000000AAD37F90] [C000000000026568] .kernel_thread+0x4c/0x68


As it turns out, set-indicator is not allowed to return a busy delay
in this context, so we're actually safe here.  Maybe we need an
rtas_set_indicator_fast which could take that into account, e.g.

int rtas_set_indicator_fast(int indicator, int index, int new_value)
{
	int token = rtas_token("set-indicator");
	int rc;

	rc = rtas_call(token, 3, 1, NULL, indicator, index, new_value);

	WARN_ON(rc == -2 || rc >= 9000 || rc <= 9005);

	if (rc < 0)
		return rtas_error_rc(rc);
	return rc;
}