Hi, Rafael
Sorry for my late response, just came back from home:)
> -----Original Message-----
> From: [email protected] [mailto:linux-pm-
> [email protected]] On Behalf Of Rafael J. Wysocki
> Sent: Thursday, October 01, 2015 9:11 AM
> To: Chen, Yu C
> Cc: [email protected]; Zhang, Rui; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH] [v2] ACPI / PM: Fix incorrect wakeup irq setting before
> suspend-to-idle
>
> On Sunday, September 27, 2015 09:23:10 AM Chen Yu wrote:
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > 739a4a6..97507fc 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -81,6 +81,7 @@ static struct workqueue_struct *kacpid_wq; static
> > struct workqueue_struct *kacpi_notify_wq; static struct
> > workqueue_struct *kacpi_hotplug_wq; static bool acpi_os_initialized;
> > +unsigned int acpi_inuse_irq = INVALID_ACPI_IRQ;
>
> What about calling it acpi_sci_irq instead?
>
OK.
> > @@ -865,6 +867,9 @@ acpi_status
> acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> > if (irq != acpi_gbl_FADT.sci_interrupt)
> > return AE_BAD_PARAMETER;
> >
> > + if (!IS_INVALID_ACPI_IRQ(acpi_inuse_irq))
> > + irq = acpi_inuse_irq;
>
> I'd think that we should return from here if acpi_inuse_irq is invalid?
>
> It surely can't be invalid if acpi_os_install_interrupt_handler() has
> succeeded,
> right?
>
Yes, there is no need to remove the handler if it is not registered, will
rewite it.
> > +
> > free_irq(irq, acpi_irq);
> > acpi_irq_handler = NULL;
> >
> > @@ -1176,12 +1181,14 @@ EXPORT_SYMBOL(acpi_os_execute);
> >
> > void acpi_os_wait_events_complete(void)
> > {
> > + unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
> > + acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;
>
> That, again. If acpi_inuse_irq is invalid,
> acpi_os_install_interrupt_handler()
> has failed, so we have nothing to synchronize here, or am I missing anything?
>
Right, will optimize this.
> > static int acpi_freeze_prepare(void)
> > {
> > + unsigned int irq = IS_INVALID_ACPI_IRQ(acpi_inuse_irq) ?
> > + acpi_gbl_FADT.sci_interrupt : acpi_inuse_irq;
>
> Same comment as above.
>
> Plus, you seem to be duplicating code from acpi_os_wait_events_complete()
> here, so what about putting it into a separate routine (maybe static inline)?
>
I've sent out another version 3 patch, which simplified this logic that,
the code flow will firstly check if the acpi irq is registered,
If not, we simply bypass the code. So the duplicating code would become
one line:
" if (!IS_INVALID_ACPI_IRQ(acpi_sci_irq))"
v3:
- 1.Rename acpi_inuse_irq to acpi_sci_irq for better understanding.
2.If the irq handler is not registered, skip the synchronize_hardirq
in acpi_os_wait_events_complete and return immediately in
acpi_os_remove_interrupt_handler.
3.For acpi_freeze_prepare and acpi_freeze_restore, if the acpi irq
handler is not properly registered, we do not leverage acpi irq
to wake up the system, but expect other peripherals(such as PCI devices
and USB devices)to invoke enable_irq_wake for us.
> Thanks,
> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in the
> body of a message to [email protected] More majordomo info at
> http://vger.kernel.org/majordomo-info.html
