On Wed, 2013-06-05 at 15:34 +0800, Gavin Shan wrote:
> The patch intends to implement the notifier for variable OPAL events.
> It's notable that the notifier can be disabled dynamically. Also, the
> notifier could be fired upon incoming OPAL interrupts, or enabling
> the OPAL notifier.
"This patch implements a notifier to receive a notification on OPAL
event mask changes." is probably better. No need to blurb about
enable/disable, however add something along the lines of
"The notifier is only called as a result of an OPAL interrupt, which
will happen upon reception of FSP messages or PCI errors. Any event
mask change detected as a result of opal_poll_events() will not result
in a notifier call.
With OPALv3, opal_poll_event() will not clear interrupt conditions from
the FSP however, even if it consumes the messages (and thus updates the
event mask). Thus the interrupt notifier is a reliable way to get
the completion for FSP based OPAL operations. The specific list will
be added to the header file.
> Signed-off-by: Gavin Shan <sha...@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/opal.h | 3 +
> arch/powerpc/platforms/powernv/opal.c | 79
> ++++++++++++++++++++++++++++++++-
> 2 files changed, 81 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 2880797..64e7c84 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -644,6 +644,9 @@ extern void hvc_opal_init_early(void);
> extern int early_init_dt_scan_opal(unsigned long node, const char *uname,
> int depth, void *data);
>
> +extern int opal_notifier_register(uint64_t mask, void (*cb)(uint64_t));
> +extern void opal_notifier_enable(bool enable);
Make it two functions
opal_enable_notifier() vs. opal_disable_notifier()
> extern int opal_get_chars(uint32_t vtermno, char *buf, int count);
> extern int opal_put_chars(uint32_t vtermno, const char *buf, int total_len);
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c
> b/arch/powerpc/platforms/powernv/opal.c
> index 628c564..9bbbf93 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -26,11 +26,20 @@ struct opal {
> u64 entry;
> } opal;
>
> +struct opal_cb {
> + struct list_head list;
> + uint64_t mask;
> + void (*cb)(uint64_t);
> +};
> +
> static struct device_node *opal_node;
> static DEFINE_SPINLOCK(opal_write_lock);
> extern u64 opal_mc_secondary_handler[];
> static unsigned int *opal_irqs;
> static unsigned int opal_irq_count;
> +static LIST_HEAD(opal_notifier);
> +static DEFINE_SPINLOCK(opal_notifier_lock);
> +static atomic_t opal_notifier_hold = ATOMIC_INIT(0);
>
> int __init early_init_dt_scan_opal(unsigned long node,
> const char *uname, int depth, void *data)
> @@ -95,6 +104,74 @@ static int __init opal_register_exception_handlers(void)
>
> early_initcall(opal_register_exception_handlers);
>
> +int opal_notifier_register(uint64_t mask, void (*cb)(uint64_t))
> +{
> + unsigned long flags;
> + struct opal_cb *p, *tmp;
> +
> + if (!mask || !cb) {
> + pr_warning("%s: Invalid argument (%llx, %p)!\n",
> + __func__, mask, cb);
> + return -EINVAL;
> + }
> +
> + p = kzalloc(sizeof(*p), GFP_KERNEL);
> + if (!p) {
> + pr_warning("%s: Out of memory (%llx, %p)!\n",
> + __func__, mask, cb);
> + return -ENOMEM;
> + }
> + p->mask = mask;
> + p->cb = cb;
> +
> + spin_lock_irqsave(&opal_notifier_lock, flags);
> + list_for_each_entry(tmp, &opal_notifier, list) {
> + if (tmp->cb == cb || tmp->mask & mask) {
> + pr_warning("%s: Duplicate evnet handler (%llx, %p)\n",
> + __func__, tmp->mask, tmp->cb);
> + spin_unlock_irqrestore(&opal_notifier_lock, flags);
> + kfree(p);
> + return -EEXIST;
> + }
> + }
Don't bother with checking the list already. This is not useful. Also
it's fine for two things to listen on the same event.
> +
> + list_add_tail(&p->list, &opal_notifier);
> + spin_unlock_irqrestore(&opal_notifier_lock, flags);
> +
> + return 0;
> +}
> +
> +static void opal_do_notifier(uint64_t events)
> +{
> + struct opal_cb *tmp;
> +
> + if (atomic_read(&opal_notifier_hold))
> + return;
> + if (!events)
> + return;
> +
> + list_for_each_entry(tmp, &opal_notifier, list) {
> + if (events & tmp->mask)
> + tmp->cb(events & tmp->mask);
> + }
> +}
My idea was to call this if the event bit has changed since the last
time we called opal_do_notifier. IE. Use a static last_notified_mask
and do something like
changed_mask = last_notified_mask ^ events;
list_for_each_entry(tmp, &opal_notifier, list) {
if (changed_mask & tmp->mask)
tmp->cb(events);
Also, always pass the whole events to the callback, no point in
filtering.
BTW, "tmp" isn't a nice name here.
> +void opal_notifier_enable(bool enable)
> +{
> + int64_t rc;
> + uint64_t evt = 0;
> +
> + if (enable) {
> + atomic_set(&opal_notifier_hold, 0);
> +
> + /* Process pending events */
> + rc = opal_poll_events(&evt);
> + if (rc == OPAL_SUCCESS && evt)
> + opal_do_notifier(evt);
> + } else
> + atomic_set(&opal_notifier_hold, 1);
> +}
As I said, two functions.
> int opal_get_chars(uint32_t vtermno, char *buf, int count)
> {
> s64 len, rc;
> @@ -297,7 +374,7 @@ static irqreturn_t opal_interrupt(int irq, void *data)
>
> opal_handle_interrupt(virq_to_hw(irq), &events);
>
> - /* XXX TODO: Do something with the events */
> + opal_do_notifier(events);
>
> return IRQ_HANDLED;
> }
Cheers,
Ben.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
