diff mbox

[BISECTED] Was Re: acpi/suspend - irq 9: nobody cared

Message ID 1242192968.14626.40.camel@minggr.sh.intel.com (mailing list archive)
State Accepted
Headers show

Commit Message

Lin Ming May 13, 2009, 5:36 a.m. UTC
> From: Bob Copeland <me@bobcopeland.com>
> Date: Wed, May 13, 2009 at 12:05 PM
> Subject: [BISECTED] Was Re: acpi/suspend - irq 9: nobody cared
> To: Bjorn Helgaas <bjorn.helgaas@hp.com>
> Cc: yakui_zhao <yakui.zhao@intel.com>, "linux-kernel@vger.kernel.org"
> <linux-kernel@vger.kernel.org>, "linux-acpi@vger.kernel.org"
> <linux-acpi@vger.kernel.org>, "Rafael J. Wysocki" <rjw@sisk.pl>,
> robert.moore@intel.com
> 
> 
> On Tue, May 12, 2009 at 01:01:06PM -0600, Bjorn Helgaas wrote:
> > Even if you can't bisect this, can you please open a bugzilla at
> > http://bugzilla.kernel.org?
> 
> Done: http://bugzilla.kernel.org/show_bug.cgi?id=13289
> 
> I have bisected it.  I don't know if it's related, but I have a
> "MacBook 1,1" which in sleep.c has a quirk related to SCI_EN, and I
> see that this patch changed ACPI_PM1_CONTROL_PRESERVED_BITS to include
> SCI_EN.  I have verified that reverting the patch on top of latest -rc
> prevents irq 9 from being disabled.

>From ACPI Spec 3.0a 4.7.3.2.1,

"SCI_EN: Selects the power management event to be either an SCI or SMI
interrupt for the following events. When this bit is set, then power
management events will generate an SCI interrupt. When this bit is reset
power management events will generate an SMI interrupt. It is the
responsibility of the hardware to set or reset this bit. OSPM always
preserves this bit position."

So we preserve SCI_EN bit per spec.

Anyway, would you please try below patch to see if the bug is caused by
SCI_EN bit preserved?


---
Lin Ming

> 
> commit 20869dcfde204e1c21b642608d708d82472fee2b
> Author: Bob Moore <robert.moore@intel.com>
> Date:   Fri Mar 13 09:10:46 2009 +0800
> 
>    ACPICA: Preserve all PM control reserved and ignored bits
> 
>    As per the ACPI specification, preserve (read/modify/write) all
>    bits that are defined as either reserved or ignored (PM control
>    control registers only.)
> 
>    Signed-off-by: Bob Moore <robert.moore@intel.com>
>    Signed-off-by: Lin Ming <ming.m.lin@intel.com>
>    Signed-off-by: Len Brown <len.brown@intel.com>
> 
> diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
> index 18a8d96..f01e155 100644
> --- a/drivers/acpi/acpica/aclocal.h
> +++ b/drivers/acpi/acpica/aclocal.h
> @@ -780,7 +780,15 @@ struct acpi_bit_register_info {
>  * must be preserved.
>  */
>  #define ACPI_PM1_STATUS_PRESERVED_BITS          0x0800 /* Bit 11 */
> -#define ACPI_PM1_CONTROL_PRESERVED_BITS         0x0200 /* Bit 9 (whatever) */
> +
> +/* For control registers, both ignored and reserved bits must be preserved */
> +
> +#define ACPI_PM1_CONTROL_IGNORED_BITS           0x0201 /* Bits 9, 0(SCI_EN) */
> +#define ACPI_PM1_CONTROL_RESERVED_BITS          0xC1F8 /* Bits 14-15, 3-8 */
> +#define ACPI_PM1_CONTROL_PRESERVED_BITS \
> +              (ACPI_PM1_CONTROL_IGNORED_BITS | ACPI_PM1_CONTROL_RESERVED_BITS)
> +
> +#define ACPI_PM2_CONTROL_PRESERVED_BITS         0xFFFFFFFE     /* All
> except bit 0 */
> 
>  /*
>  * Register IDs
> diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> index fd7abe2..6117362 100644
> --- a/drivers/acpi/acpica/hwregs.c
> +++ b/drivers/acpi/acpica/hwregs.c
> @@ -328,6 +328,21 @@ acpi_status acpi_hw_register_write(u32
> register_id, u32 value)
> 
>        case ACPI_REGISTER_PM2_CONTROL: /* 8-bit access */
> 
> +               /*
> +                * For control registers, all reserved bits must be preserved,
> +                * as per the ACPI spec.
> +                */
> +               status =
> +                   acpi_read(&read_value, &acpi_gbl_FADT.xpm2_control_block);
> +               if (ACPI_FAILURE(status)) {
> +                       goto exit;
> +               }
> +
> +               /* Insert the bits to be preserved */
> +
> +               ACPI_INSERT_BITS(value, ACPI_PM2_CONTROL_PRESERVED_BITS,
> +                                read_value);
> +
>                status = acpi_write(value, &acpi_gbl_FADT.xpm2_control_block);
>                break;
> 
> 
> 
> --
> Bob Copeland %% www.bobcopeland.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Rafael Wysocki May 13, 2009, 8:15 a.m. UTC | #1
On Wednesday 13 May 2009, Lin Ming wrote:
> 
> > From: Bob Copeland <me@bobcopeland.com>
> > Date: Wed, May 13, 2009 at 12:05 PM
> > Subject: [BISECTED] Was Re: acpi/suspend - irq 9: nobody cared
> > To: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > Cc: yakui_zhao <yakui.zhao@intel.com>, "linux-kernel@vger.kernel.org"
> > <linux-kernel@vger.kernel.org>, "linux-acpi@vger.kernel.org"
> > <linux-acpi@vger.kernel.org>, "Rafael J. Wysocki" <rjw@sisk.pl>,
> > robert.moore@intel.com
> > 
> > 
> > On Tue, May 12, 2009 at 01:01:06PM -0600, Bjorn Helgaas wrote:
> > > Even if you can't bisect this, can you please open a bugzilla at
> > > http://bugzilla.kernel.org?
> > 
> > Done: http://bugzilla.kernel.org/show_bug.cgi?id=13289
> > 
> > I have bisected it.  I don't know if it's related, but I have a
> > "MacBook 1,1" which in sleep.c has a quirk related to SCI_EN, and I
> > see that this patch changed ACPI_PM1_CONTROL_PRESERVED_BITS to include
> > SCI_EN.  I have verified that reverting the patch on top of latest -rc
> > prevents irq 9 from being disabled.
> 
> >From ACPI Spec 3.0a 4.7.3.2.1,
> 
> "SCI_EN: Selects the power management event to be either an SCI or SMI
> interrupt for the following events. When this bit is set, then power
> management events will generate an SCI interrupt. When this bit is reset
> power management events will generate an SMI interrupt. It is the
> responsibility of the hardware to set or reset this bit. OSPM always
> preserves this bit position."
> 
> So we preserve SCI_EN bit per spec.

In fact we do set SCI_EN directly in drivers/acpi/sleep.c in a couple of
places exactly because the hardware is _known_ to wrong things here.  So, we
don't really preserve it, _exactly_ because at least some pieces of hardware
out there don't follow the spec.  Including Macs, IIRC.

Bob, can you please verify if the Lin Ming's patch helps?

Rafael


> Anyway, would you please try below patch to see if the bug is caused by
> SCI_EN bit preserved?
> 
> diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
> index 772ee5c..3d082bb 100644
> --- a/drivers/acpi/acpica/aclocal.h
> +++ b/drivers/acpi/acpica/aclocal.h
> @@ -787,7 +787,7 @@ struct acpi_bit_register_info {
>  
>  /* For control registers, both ignored and reserved bits must be preserved */
>  
> -#define ACPI_PM1_CONTROL_IGNORED_BITS           0x0201	/* Bits 9, 0(SCI_EN) */
> +#define ACPI_PM1_CONTROL_IGNORED_BITS           0x0200	/* Bits 9 */
>  #define ACPI_PM1_CONTROL_RESERVED_BITS          0xC1F8	/* Bits 14-15, 3-8 */
>  #define ACPI_PM1_CONTROL_PRESERVED_BITS \
>  	       (ACPI_PM1_CONTROL_IGNORED_BITS | ACPI_PM1_CONTROL_RESERVED_BITS)
> 
> ---
> Lin Ming
> 
> > 
> > commit 20869dcfde204e1c21b642608d708d82472fee2b
> > Author: Bob Moore <robert.moore@intel.com>
> > Date:   Fri Mar 13 09:10:46 2009 +0800
> > 
> >    ACPICA: Preserve all PM control reserved and ignored bits
> > 
> >    As per the ACPI specification, preserve (read/modify/write) all
> >    bits that are defined as either reserved or ignored (PM control
> >    control registers only.)
> > 
> >    Signed-off-by: Bob Moore <robert.moore@intel.com>
> >    Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> >    Signed-off-by: Len Brown <len.brown@intel.com>
> > 
> > diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
> > index 18a8d96..f01e155 100644
> > --- a/drivers/acpi/acpica/aclocal.h
> > +++ b/drivers/acpi/acpica/aclocal.h
> > @@ -780,7 +780,15 @@ struct acpi_bit_register_info {
> >  * must be preserved.
> >  */
> >  #define ACPI_PM1_STATUS_PRESERVED_BITS          0x0800 /* Bit 11 */
> > -#define ACPI_PM1_CONTROL_PRESERVED_BITS         0x0200 /* Bit 9 (whatever) */
> > +
> > +/* For control registers, both ignored and reserved bits must be preserved */
> > +
> > +#define ACPI_PM1_CONTROL_IGNORED_BITS           0x0201 /* Bits 9, 0(SCI_EN) */
> > +#define ACPI_PM1_CONTROL_RESERVED_BITS          0xC1F8 /* Bits 14-15, 3-8 */
> > +#define ACPI_PM1_CONTROL_PRESERVED_BITS \
> > +              (ACPI_PM1_CONTROL_IGNORED_BITS | ACPI_PM1_CONTROL_RESERVED_BITS)
> > +
> > +#define ACPI_PM2_CONTROL_PRESERVED_BITS         0xFFFFFFFE     /* All
> > except bit 0 */
> > 
> >  /*
> >  * Register IDs
> > diff --git a/drivers/acpi/acpica/hwregs.c b/drivers/acpi/acpica/hwregs.c
> > index fd7abe2..6117362 100644
> > --- a/drivers/acpi/acpica/hwregs.c
> > +++ b/drivers/acpi/acpica/hwregs.c
> > @@ -328,6 +328,21 @@ acpi_status acpi_hw_register_write(u32
> > register_id, u32 value)
> > 
> >        case ACPI_REGISTER_PM2_CONTROL: /* 8-bit access */
> > 
> > +               /*
> > +                * For control registers, all reserved bits must be preserved,
> > +                * as per the ACPI spec.
> > +                */
> > +               status =
> > +                   acpi_read(&read_value, &acpi_gbl_FADT.xpm2_control_block);
> > +               if (ACPI_FAILURE(status)) {
> > +                       goto exit;
> > +               }
> > +
> > +               /* Insert the bits to be preserved */
> > +
> > +               ACPI_INSERT_BITS(value, ACPI_PM2_CONTROL_PRESERVED_BITS,
> > +                                read_value);
> > +
> >                status = acpi_write(value, &acpi_gbl_FADT.xpm2_control_block);
> >                break;
> > 
> > 
> > 
> > --
> > Bob Copeland %% www.bobcopeland.com
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
>
Bob Copeland May 13, 2009, 3:30 p.m. UTC | #2
2009/5/13 Rafael J. Wysocki <rjw@sisk.pl>:
>> So we preserve SCI_EN bit per spec.
>
> In fact we do set SCI_EN directly in drivers/acpi/sleep.c in a couple of
> places exactly because the hardware is _known_ to wrong things here.  So, we
> don't really preserve it, _exactly_ because at least some pieces of hardware
> out there don't follow the spec.  Including Macs, IIRC.
>
> Bob, can you please verify if the Lin Ming's patch helps?

Yes, my laptop is one of those pieces of hardware.  I can confirm that Lin
Ming's patch works on my system.
diff mbox

Patch

diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index 772ee5c..3d082bb 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -787,7 +787,7 @@  struct acpi_bit_register_info {
 
 /* For control registers, both ignored and reserved bits must be preserved */
 
-#define ACPI_PM1_CONTROL_IGNORED_BITS           0x0201	/* Bits 9, 0(SCI_EN) */
+#define ACPI_PM1_CONTROL_IGNORED_BITS           0x0200	/* Bits 9 */
 #define ACPI_PM1_CONTROL_RESERVED_BITS          0xC1F8	/* Bits 14-15, 3-8 */
 #define ACPI_PM1_CONTROL_PRESERVED_BITS \
 	       (ACPI_PM1_CONTROL_IGNORED_BITS | ACPI_PM1_CONTROL_RESERVED_BITS)