On Wed, Jun 12, 2013 at 10:32:29AM +1000, Benjamin Herrenschmidt wrote:
>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.
>
>
Thanks, Ben. Will update the changelog accordingly.
>> 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()
>
Ok. Will do.
>> 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.
>
Ok. Will update in next revision.
>> +
>> + 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.
>
Ok. Will update in next revision:
- Allow multiple "clients" for same event.
- Make the variable "tmp" to have better name.
>> +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.
>
Ok.
>> 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;
>> }
>
Thanks,
Gavin
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
