Hi, Andi,
Thank you very much for your review!
On Fri, 2010-10-22 at 19:57 +0800, Andi Kleen wrote:
> On Wed, Oct 20, 2010 at 09:37:00AM +0800, Huang Ying wrote:
>
> Hi Ying,
>
> Only some minor nits found in review.
>
> I don't think they're merge blockers, but could be all fixed
> in followups later.
>
> Overall it looks all good.
>
> Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>
> > + * These 2 spinlock is used to prevent atomic ioremap virtual memory
> > + * area from being mapped simultaneously.
> > + */
> > +static DEFINE_RAW_SPINLOCK(ghes_ioremap_lock_nmi);
> > +static DEFINE_SPINLOCK(ghes_ioremap_lock_irq);
> > +
> > +static int ghes_ioremap_init(void)
> > +{
> > + ghes_ioremap_area = __get_vm_area(PAGE_SIZE * 2, VM_IOREMAP,
>
> Should make the magic PAGE_SIZE * 2 into a define with a comment?
Yes. Will do it.
> > + vaddr = ghes_ioremap_pfn_nmi(paddr >> PAGE_SHIFT);
> > + } else {
> > + spin_lock_irqsave(&ghes_ioremap_lock_irq, flags);
> > + vaddr = ghes_ioremap_pfn_irq(paddr >> PAGE_SHIFT);
> > + }
> > + trunk = PAGE_SIZE - offset;
> > + trunk = min(trunk, len);
> > + if (from_phys)
> > + memcpy(buffer, vaddr + offset, trunk);
> > + else
> > + memcpy(vaddr + offset, buffer, trunk);
>
> In generic Linux this would be memcpy_fromio. On x86 of course it's the same.
> Still perhaps better use that to prevent sparse from freaking out with its
> address space checks.
Will do it.
> > @@ -303,6 +390,43 @@ out:
> > return 0;
> > }
> >
> > +static void ghes_add_timer(struct ghes *ghes)
> > +{
> > + struct acpi_hest_generic *g = ghes->generic;
> > + unsigned long expire;
> > +
> > + if (!g->notify.poll_interval) {
> > + pr_warning(FW_WARN GHES_PFX "Poll interval is 0 for "
> > + "generaic hardware error source: %d, disabled.",
>
> generaic -> generic
>
> This is user visible so should be fixed.
Yes. Will do it.
> > + g->header.source_id);
> > + return;
> > + }
> > + expire = jiffies + msecs_to_jiffies(g->notify.poll_interval);
> > + ghes->timer.expires = round_jiffies_relative(expire);
> > + add_timer(&ghes->timer);
>
> Could actually make this a deferrable timer I guess to be even more
> friendly to power.
Yes. Will do it.
> > + }
> > + ret = NOTIFY_STOP;
> > + }
> > +
> > + if (ret == NOTIFY_DONE)
> > + goto out;
> > +
> > + if (sev_global >= GHES_SEV_PANIC) {
> > + herr_persist_all_records();
> > + oops_begin();
> > + /* reboot to log the error! */
> > + if (panic_timeout == 0)
> > + panic_timeout = ghes_panic_timeout;
> > + panic(GHES_PFX "generic hardware fatal error!\n");
>
> I suspect need some more explanation on this one.
Yes. Will do it.
> > + case ACPI_HEST_NOTIFY_NMI:
> > + mutex_lock(&ghes_list_mutex);
> > + list_del_rcu(&ghes->list);
> > + if (list_empty(&ghes_nmi))
> > + unregister_die_notifier(&ghes_notifier_nmi);
> > + mutex_unlock(&ghes_list_mutex);
> > + synchronize_rcu();
Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
