Deb McLemore <de...@linux.vnet.ibm.com> writes:
> Problem being solved is when issuing a BMC soft poweroff during IPL,
> the poweroff was being lost so the machine would not poweroff.
>
> Opal messages were being received before the opal-power code
> registered its notifiers.
>
> Alternatives discussed (option #3 was chosen):
>
> 1 - Have opal_message_init() explicitly call opal_power_control_init()
> before it dequeues any OPAL messages (i.e. before we register the
> opal-msg IRQ handler).
>
> 2 - Introduce concept of critical message types and when we register
> handlers we track which message types have a registered handler,
> then defer the opal-msg IRQ registration until we have a handler
> registered for all the critical types.
>
> 3 - Buffering messages, if we receive a message and do not yet
> have a handler for that type, store the message and replay when
> a handler for that type is registered.
>
> Signed-off-by: Deb McLemore <de...@linux.vnet.ibm.com>
> ---
> arch/powerpc/platforms/powernv/opal.c | 83
> ++++++++++++++++++++++++++++++++---
> 1 file changed, 78 insertions(+), 5 deletions(-)
Hi Deb,
Sorry for the delay (!) on this one.
A couple more comments ...
> diff --git a/arch/powerpc/platforms/powernv/opal.c
> b/arch/powerpc/platforms/powernv/opal.c
> index 65c79ec..46d7604 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -238,14 +288,25 @@ machine_early_initcall(powernv,
> opal_register_exception_handlers);
> int opal_message_notifier_register(enum opal_msg_type msg_type,
> struct notifier_block *nb)
> {
> + int ret;
> +
> if (!nb || msg_type >= OPAL_MSG_TYPE_MAX) {
> pr_warning("%s: Invalid arguments, msg_type:%d\n",
> __func__, msg_type);
> return -EINVAL;
> }
We still want to take the msg_list_lock around the registration, ie.
from here to the end of the function.
> - return atomic_notifier_chain_register(
> - &opal_msg_notifier_head[msg_type], nb);
> + ret = atomic_notifier_chain_register(
> + &opal_msg_notifier_head[msg_type], nb);
> +
> + if (ret)
> + return ret;
Otherwise a message that arrives here will be delivered before any
queued messages, which might be a problem.
> +
> + /* Replay any queued messages that came in */
> + /* prior to the notifier chain registration */
> + dequeue_replay_msg(msg_type);
> +
> + return 0;
> }
> EXPORT_SYMBOL_GPL(opal_message_notifier_register);
>
> @@ -259,9 +320,21 @@ EXPORT_SYMBOL_GPL(opal_message_notifier_unregister);
>
> static void opal_message_do_notify(uint32_t msg_type, void *msg)
> {
> - /* notify subscribers */
> - atomic_notifier_call_chain(&opal_msg_notifier_head[msg_type],
> - msg_type, msg);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&msg_list_lock, flags);
> +
> + if (opal_msg_notifier_head[msg_type].head != NULL) {
> + /* notify subscribers */
> + atomic_notifier_call_chain(&opal_msg_notifier_head[msg_type],
> + msg_type, msg);
> + } else {
> + /* Queue up the msg since no notifiers have */
> + /* registered yet for this msg_type */
> + queue_replay_msg(msg);
> + }
> +
> + spin_unlock_irqrestore(&msg_list_lock, flags);
I don't think it's actually likely to be a problem, but it would be nice
if we didn't have to hold the lock while calling the notifier in the
general case.
I think we can do that with eg:
bool queued = false;
spin_lock_irqsave(&msg_list_lock, flags);
if (opal_msg_notifier_head[msg_type].head == NULL) {
/*
* Queue up the msg since no notifiers have registered
* yet for this msg_type.
*/
queue_replay_msg(msg);
queued = true;
}
spin_unlock_irqrestore(&msg_list_lock, flags);
if (queued)
return;
/* notify subscribers */
atomic_notifier_call_chain(&opal_msg_notifier_head[msg_type], msg_type,
msg);
cheers
