Re: Bug#680707: [3.4-rc5 -> 3.4-rc6 regression] Asus P5NSLI: lockup on resume from suspend

["Octavio Alvarez" <alvarezp@alvarezp.com>]

On Sun, 08 Jul 2012 02:04:32 -0700, Jonathan Nieder <jrnieder@gmail.com>  
wrote:

> So presumably it's the new writes to the ACPI_BITREG_ARB_DISABLE
> register that cause trouble.  The patch below tests that guess.

Given the complexity of the ACPI_DEBUG logging (particularly the
console part), I decided to give your patch a shot directly.

Result: the system no longer locks up. I paste the following info
so you can make sure I'm working with the correct commit:

[Sun Jul 08 02:41:37 -0700 -- alvarezp@octavio:~/src/linux]
$ git status
# Not currently on any branch.
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working  
directory)
#
#	modified:   drivers/acpi/acpica/hwsleep.c
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#	arch/x86/tools/relocs
no changes added to commit (use "git add" and/or "git commit -a")

[Sun Jul 08 02:41:41 -0700 -- alvarezp@octavio:~/src/linux]
$ git log HEAD^..HEAD
commit 2feec47d4c5f80b05f1650f5a24865718978eea4
Author: Bob Moore <robert.moore@intel.com>
Date:   Tue Feb 14 15:00:53 2012 +0800

     ACPICA: ACPI 5: Support for new FADT SleepStatus, SleepControl  
registers

     Adds sleep and wake support for systems with these registers.
     One new file, hwxfsleep.c

     Signed-off-by: Bob Moore <robert.moore@intel.com>
     Signed-off-by: Len Brown <len.brown@intel.com>

[Sun Jul 08 02:41:49 -0700 -- alvarezp@octavio:~/src/linux]
$ git diff
diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index 59a2a6b..66dd2b8 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -241,18 +241,6 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
                 return_ACPI_STATUS(status);
         }

-       if (sleep_state != ACPI_STATE_S5) {
-               /*
-                * Disable BM arbitration. This feature is contained  
within an
-                * optional register (PM2 Control), so ignore a BAD_ADDRESS
-                * exception.
-                */
-               status = acpi_write_bit_register(ACPI_BITREG_ARB_DISABLE,  
1);
-               if (ACPI_FAILURE(status) && (status != AE_BAD_ADDRESS)) {
-                       return_ACPI_STATUS(status);
-               }
-       }
-
         /*
          * 1) Disable/Clear all GPEs
          * 2) Enable all wakeup GPEs
@@ -498,16 +486,6 @@ acpi_status acpi_hw_legacy_wake(u8 sleep_state)
                                     [ACPI_EVENT_POWER_BUTTON].
                                     status_register_id, ACPI_CLEAR_STATUS);

-       /*
-        * Enable BM arbitration. This feature is contained within an
-        * optional register (PM2 Control), so ignore a BAD_ADDRESS
-        * exception.
-        */
-       status = acpi_write_bit_register(ACPI_BITREG_ARB_DISABLE, 0);
-       if (ACPI_FAILURE(status) && (status != AE_BAD_ADDRESS)) {
-               return_ACPI_STATUS(status);
-       }
-
         acpi_hw_execute_SST(ACPI_SST_WORKING);
         return_ACPI_STATUS(status);
  }


> Now for a complaint.  This would have been a lot easier if cleanups
> that do not change behavior were split into separate commits --- one
> commit per change.  That makes it easy to verify that each patch
> correctly does what it promises with no unintended side effects.

I have to back you up on this one. This patch could have been 3 easily.
Git bisect would have done a better job.

The question is: if he had dont it that way, would the kernel have
compiled in intermediate commits?

> diff --git i/drivers/acpi/acpica/hwsleep.c  
> w/drivers/acpi/acpica/hwsleep.c
> index 0ed85cac3231..615996a36bed 100644
> --- i/drivers/acpi/acpica/hwsleep.c
> +++ w/drivers/acpi/acpica/hwsleep.c
> @@ -95,18 +95,6 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state, u8  
> flags)
>  		return_ACPI_STATUS(status);
>  	}
> -	if (sleep_state != ACPI_STATE_S5) {
> -		/*
> -		 * Disable BM arbitration. This feature is contained within an
> -		 * optional register (PM2 Control), so ignore a BAD_ADDRESS
> -		 * exception.
> -		 */
> -		status = acpi_write_bit_register(ACPI_BITREG_ARB_DISABLE, 1);
> -		if (ACPI_FAILURE(status) && (status != AE_BAD_ADDRESS)) {
> -			return_ACPI_STATUS(status);
> -		}
> -	}
> -
>  	/*
>  	 * 1) Disable/Clear all GPEs
>  	 * 2) Enable all wakeup GPEs
> @@ -364,16 +352,6 @@ acpi_status acpi_hw_legacy_wake(u8 sleep_state, u8  
> flags)
>  				    [ACPI_EVENT_POWER_BUTTON].
>  				    status_register_id, ACPI_CLEAR_STATUS);
> -	/*
> -	 * Enable BM arbitration. This feature is contained within an
> -	 * optional register (PM2 Control), so ignore a BAD_ADDRESS
> -	 * exception.
> -	 */
> -	status = acpi_write_bit_register(ACPI_BITREG_ARB_DISABLE, 0);
> -	if (ACPI_FAILURE(status) && (status != AE_BAD_ADDRESS)) {
> -		return_ACPI_STATUS(status);
> -	}
> -
>  	acpi_hw_execute_sleep_method(METHOD_PATHNAME__SST, ACPI_SST_WORKING);
>  	return_ACPI_STATUS(status);
>  }

Thanks for the patch.

-- 
Octavio.