RE: [PATCH] [v2] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle

[Chen, Yu C]

Hi, Rafael
Sorry for my late response, just came back from home:)

> -----Original Message-----
> From: linux-pm-owner@xxxxxxxxxxxxxxx [mailto:linux-pm-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Rafael J. Wysocki
> Sent: Thursday, October 01, 2015 9:11 AM
> To: Chen, Yu C
> Cc: lenb@xxxxxxxxxx; Zhang, Rui; jiang.liu@xxxxxxxxxxxxxxx; linux-
> pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at
> http://vger.kernel.org/majordomo-info.html