from:"Zheng, Lv"

  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Reviewed-by: Len Brown len.br...@intel.com
  Reviewed-by: Rui Zhang rui.zh...@intel.com
  Reviewed-by: Ying Huang ying.hu...@intel.com
  Reviewed-by: Konrad Rzeszutek Wilk kon...@kernel.org
 Please don't include that unless I (or other folks looking at your code) say
 explicitly 'Acked' or 'Reviewed-by'

ACK.
I'll remove these names and resend.  Thanks.

  +#define DEBUG
 That should not be the default case.

RFC.
If we do not add ignore_loglevel to the command line, the acpi earlycon will be 
mute just as what you want.
Do you really want this to be:
#undef DEBUG
If we do this, all pr_debug invocations in this file will be empty in 
compilation stage and are there any other means for us to view the output of 
ACPI earlycon without recompiling?

  +DECLARE_BITMAP(acpi_early_flags, MAX_ACPI_DBG_PORTS);
 static?

ACK.
I'll do the modification.

  +int acpi_early_enabled;
 __read_mostly and you could also make it a bool.

NAK.
I think this variable will be read only once and written up to 16 times so 
__read_mostly is not required.
I'll check and add __init and __initdata for this patch.

  +   set_bit(port, acpi_early_flags);
  +   if (keep)
  +   set_bit(port+MAX_ACPI_DBG_PORTS, acpi_early_flags);
 Huh? The bitmap is up to MAX_ACPI_DB_PORTS, but here you offset it past
 that? Why?

ACK.
It's my mistake.  The size of the bitmap should be MAX_ACPI_DBG_PORTS1 or 
MAX_ACPI_DBG_PORTS*2.
The reason is:
I think MAX_ACPI_DBG_PORTS=4 is enough in this case.
Since the systems running Linux are 32/64 bit architectures, using 2 bitmaps 
will be waste.
As the systems are at least 32 bit, the MAX_ACPI_DB_PORTS is defined as 16 to 
make full use of the bitmap.

  +   if (!acpi_early_console_enabled(info-port_index))
  +   return 0;
 Not -ENODEV?

NAK.
This is to support MULTIPLE DBG2 debug ports.

** MULTIPLE DBGP table versions and MULTIPLE DBG2 debug ports support **
The whole patch takes ENODEV as the semantics of table not exist or table 
version not supported in PROBE/START stage so that we can obtain the ability 
of probing Microsoft's future tables in the order from DBGn, ..., DBG2, DBGP.
We should not return here as users may pass the following command line:
earlyprintk=acpi2,keep
to let the first 2 debug ports mute, but take the 3rd as a Linux earlycon.

  +   while (((unsigned long)entry) + sizeof(struct acpi_dbg2_device) 
  +  tbl_end) {
 Just make it one line. Ignore the 80 characters limit here.

ACK.
I'll try to implement this within 80 characters.

  +   if (!max_entries || count++  max_entries) {
 How about you just make this 'count'
  +   pr_debug(early: DBG2 PROBE - console %d(%04x:%04x).\n,
  +count-1,
  +entry-port_type, entry-port_subtype);
  +   devinfo.port_index = (u8)count-1;
 Then you don't this 'count -1'
 and then do
   count++ here?

ACK.
It's my mistake, the previous version uses port_index=1 as the first debug 
port, but this version takes 0 as the first port.

  +   acpi_early_console_start(devinfo);
 no check of the return value to see whether you should return immediately?

NAK.
See  MULTIPLE DBGP tables and MULTIPLE DBG2 debug ports support above.

  +   acpi_early_console_start(devinfo);
 how about 'return acpi_early_console_start(..)'

NAK.
See  MULTIPLE DBGP tables and MULTIPLE DBG2 debug ports support above.

  +   if (acpi_table_parse(ACPI_SIG_DBG2, acpi_parse_dbg2) != 0)
  +   acpi_table_parse(ACPI_SIG_DBGP, acpi_parse_dbgp);

RFC.
This is to support MULTIPLE DBGP table versions.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v5 2/2] ACPI: Add Intel MID SPI early console support.

earlyprintk=acpi
 .. or earlyprintk=mrst
 ?

ACK.
The two launchers are all workable for MID_SPI.  I'll add more comments and 
resend this patch.  Thanks

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v5 1/2] ACPI: Add early console framework for DBGP/DBG2.

  +int __init acpi_early_console_keep(struct acpi_debug_port *info)
 
 Why not make it 'bool' like the other (acpi_early_console_enabled)?

NAK.
keep is int in setup_early_printk.

Best regards/Lv Zheng
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v5 2/2] ACPI: Add Intel MID SPI early console support.

  +#ifdef CONFIG_EARLY_PRINTK_INTEL_MID_SPI
  +   if (info-port_type == ACPI_DBG2_SERIAL_PORT
  +info-port_subtype == ACPI_DBG2_INTEL_MID_SPI
  +info-register_count  0) {
 Is it ever going to be zero?

NAK.
No register base definition (buggy BIOS?) is meaningless for ACPI launched 
MID_SPI earlycon.

Thanks and best regards/Lv Zheng
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v6 1/2] ACPI: Add early console framework for DBGP/DBG2.

2012-10-10 Thread Zheng, Lv

  +static __initdata DECLARE_BITMAP(acpi_early_flags,
  +MAX_ACPI_DBG_PORTS*2);

It's OK since the keep bit will be derived by the real earlycon drivers in the 
__acpi_early_console_start() which is an arch specific interface.
You can find this usage in the [PATCH v6 2/2].

  +   set_bit(port, acpi_early_flags);
  +   if (keep)
  +   set_bit(port+MAX_ACPI_DBG_PORTS, acpi_early_flags);
 Put a comment explaining why you use half of the bitmap to mark them as
 'keep'. Thought wouldn't be just easier if you had another bitmap:
 acpi_keep_ports?
 To set those instead of using this bitmap?

I prefer to put comment here.
I've been a deep embedded engineer for the last 5 years, implementing software 
containing 4 bus protocol stacks within 128bytes ram and 16kbytes rom, where we 
used high modularity design patterns.
Thus made my habit being critical to ram/rom consumption...

I'm OOO now, the updated version will be sent next week.

Thanks for your comments and best regards/Lv Zheng
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] ACPI: remove config ACPI_DEBUG_FUNC_TRACE

2013-04-06 Thread Zheng, Lv

  Kconfig symbol ACPI_DEBUG_FUNC_TRACE was only used (through its
  corresponding macro) in drivers/acpi/acpica/acmacros.h. That macro was
  removed from that header in v3.8, with commit
  86ff0e508f88eda6e479a897476026055831d2d8 (ACPICA: Fix unmerged
  acmacros.h divergences.). That commit did not explain why that macro
  was removed. But it seems that ACPI_DEBUG_FUNC_TRACE can now be
  removed too, as it is unused.
 
 Lv, can you please comment here?

Sorry for the delayed reply.

The commit should have done something that already have been done in the ACPICA.
I'll take a look at this.

Thanks and best regards
-Lv

RE: [PATCH] ACPI: remove config ACPI_DEBUG_FUNC_TRACE

2013-04-07 Thread Zheng, Lv

   Kconfig symbol ACPI_DEBUG_FUNC_TRACE was only used (through its
   corresponding macro) in drivers/acpi/acpica/acmacros.h. That macro
   was removed from that header in v3.8, with commit
   86ff0e508f88eda6e479a897476026055831d2d8 (ACPICA: Fix unmerged
   acmacros.h divergences.). That commit did not explain why that
   macro was removed. But it seems that ACPI_DEBUG_FUNC_TRACE can
 now
   be removed too, as it is unused.
 
  Lv, can you please comment here?
 
 Sorry for the delayed reply.
 
 The commit should have done something that already have been done in the
 ACPICA.
 I'll take a look at this.

The original commit deleted CONFIG_ACPI_DEBUG_FUNC_TRACE from acmacros.h as it 
is in the __wrong__ place.
CONFIG_xxx is linux specific codes thus should appear in OSL layer - aclinux.h.
ACPICA specific codes should only use selective definitions in #ifdef ACPI_xxx 
style and such codes should be merged into ACPICA rather than being merged into 
Linux ACPI.

Now we have two choices:
Re-implement it in the aclinux.h using ACPI_DEBUG_OUTPUT defined/undefined or 
delete this kernel configuration item temporarily.

Thanks and best regards
-Lv

RE: [PATCH] ACPI: remove config ACPI_DEBUG_FUNC_TRACE

2013-04-07 Thread Zheng, Lv

Kconfig symbol ACPI_DEBUG_FUNC_TRACE was only used (through its
corresponding macro) in drivers/acpi/acpica/acmacros.h. That macro
was removed from that header in v3.8, with commit
86ff0e508f88eda6e479a897476026055831d2d8 (ACPICA: Fix unmerged
acmacros.h divergences.). That commit did not explain why that
macro was removed. But it seems that ACPI_DEBUG_FUNC_TRACE can
  now
be removed too, as it is unused.
  
   Lv, can you please comment here?
 
  Sorry for the delayed reply.
 
  The commit should have done something that already have been done in
  the ACPICA.
  I'll take a look at this.
 
 The original commit deleted CONFIG_ACPI_DEBUG_FUNC_TRACE from
 acmacros.h as it is in the __wrong__ place.
 CONFIG_xxx is linux specific codes thus should appear in OSL layer - 
 aclinux.h.
 ACPICA specific codes should only use selective definitions in #ifdef ACPI_xxx
 style and such codes should be merged into ACPICA rather than being merged
 into Linux ACPI.
 
 Now we have two choices:
 Re-implement it in the aclinux.h using ACPI_DEBUG_OUTPUT
 defined/undefined or delete this kernel configuration item temporarily.

Sorry, I made a mistake here.
ACPI_DEBUG_OUTPUT is already covered by the CONFIG_ACPI_DEBUG.
The corrected statement should be:
If we want to fine tune the output, then the codes might be implemented in 
ACPICA first and then get utilized in Linux using the new ACPICA feature.

Best regards
-Lv

RE: [RESEND PATCH 3/6] acpi: Remove the leading spaces of finish_override label in acpi_tb_table_override().

2013-03-08 Thread Zheng, Lv

Please do not try to fix ACPICA indentation problems in Linux side.
This will introduce source code divergences between ACPICA and Linux, which 
will add difficulties to ACPICA release work.
Please find real issues in ACPICA release scripts at:

https://github.com/acpica/acpica/tree/master/generate/linux

Thanks
-Lv

 -Original Message-
 From: Tang Chen [mailto:tangc...@cn.fujitsu.com]
 Sent: Thursday, March 07, 2013 6:38 PM
 To: l...@kernel.org; r...@sisk.pl; Moore, Robert; Zheng, Lv;
 ming.m@intel.com; m...@selenic.com; herb...@gondor.apana.org.au;
 r...@landley.net
 Cc: linux-a...@vger.kernel.org; linux-kernel@vger.kernel.org;
 linux-...@vger.kernel.org; triv...@kernel.org
 Subject: [RESEND PATCH 3/6] acpi: Remove the leading spaces of
 finish_override label in acpi_tb_table_override().
 
 There is 7 leading spaces in front of finish_override label in
 acpi_tb_table_override(). Remove them.
 
 Signed-off-by: Tang Chen tangc...@cn.fujitsu.com
 ---
  drivers/acpi/acpica/tbinstal.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/drivers/acpi/acpica/tbinstal.c b/drivers/acpi/acpica/tbinstal.c 
 index
 e57cd38..b1e098d 100644
 --- a/drivers/acpi/acpica/tbinstal.c
 +++ b/drivers/acpi/acpica/tbinstal.c
 @@ -313,7 +313,7 @@ struct acpi_table_header
 *acpi_tb_table_override(struct acpi_table_header
 
   return (NULL);  /* There was no override */
 
 -  finish_override:
 +finish_override:
 
   ACPI_INFO((AE_INFO,
  %4.4s %p %s table override, new table: %p,
 --
 1.7.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [RESEND PATCH 3/6] acpi: Remove the leading spaces of finish_override label in acpi_tb_table_override().

2013-03-08 Thread Zheng, Lv

   https://github.com/acpica/acpica/tree/master/generate/linux
  Hi Lv,
 
  Thanks for telling me this. :)
 
  One more thing, if I want to fix something in acpica, such as this
  patch set, who and which mail list should I send patches to ?
 
 Please post them to linux-a...@vger.kernel.org with CCs to me and Bob
 Moore.

Yes. More information can be found at https://acpica.org/.
You can also just blame me if you find issues caused by the ACPICA release 
process. :-)
I have the duty to make the ACPICA release process better.
Thanks for your contribution.

Best regards
-Lv

RE: linux-next: manual merge of the acpi tree with the pm tree

2013-02-16 Thread Zheng, Lv

 Hi Len,
 
 On Mon, 11 Feb 2013 18:34:06 -0500 Len Brown l...@kernel.org wrote:
 
  BTW. Rafael's pm tree now carries the ACPI patch stream, so it is
  probably a mis-representation to call my tree the acpi tree.
  My tree is primarily focused on the idle part of pm these days.
 
 OK, I have renamed your tree to idle.

Thanks for fixing the conflicts.

The new ACPICA release contains a patch renaming the PM_TIMER_FREQUENCY macro.
A further CA release patch cleans up the repo (pm/linux-next) where the release 
patch set based on.
The conflicts would happen if other repos contained new codes using the old 
macro definition.

Thanks
-Lv
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/2] platform / ACPI: Attach/detach ACPI PM during probe/remove/shutdown

2012-11-26 Thread Zheng, Lv

It looks cleanest unless more abstractions are introduced for other power 
management providers.

Best regards/Lv Zheng

 -Original Message-
 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Sunday, November 25, 2012 10:58 PM
 To: LKML
 Cc: Greg Kroah-Hartman; Linux PM list; ACPI Devel Maling List; Zhang, Rui;
 Svahn, Kai; Mika Westerberg; Huang, Ying; Lan, Tianyu; Zheng, Lv; Lu, Aaron;
 Grant Likely
 Subject: [PATCH 2/2] platform / ACPI: Attach/detach ACPI PM during
 probe/remove/shutdown

 From: Rafael J. Wysocki rafael.j.wyso...@intel.com

 Drivers usually expect that the devices they are supposed to handle will be
 operational when their .probe() routines are called, but that need not be the
 case on some ACPI-based systems with ACPI-based device enumeration where
 the BIOSes don't put devices into D0 by default.  To work around this problem
 it is sufficient to change bus type .probe() routines to ensure that devices 
 will
 be powered on before the drivers' .probe() routines run (and their .remove()
 and .shutdown() routines accordingly).

 Modify platform_drv_probe() to run acpi_dev_pm_attach() for devices whose
 ACPI handles are present, so that ACPI power management is used to change
 their power states and change their power states to D0 before driver probing.
 Analogously, modify platform_drv_remove() and
 platform_drv_shutdown() to call acpi_dev_pm_detach() for those devices, so
 that they are not subject to ACPI PM any more.

 Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
 ---
  drivers/base/platform.c |   19 +--
  1 file changed, 17 insertions(+), 2 deletions(-)

 Index: linux/drivers/base/platform.c

 ===
 --- linux.orig/drivers/base/platform.c
 +++ linux/drivers/base/platform.c
 @@ -484,8 +484,16 @@ static int platform_drv_probe(struct dev  {
   struct platform_driver *drv = to_platform_driver(_dev-driver);
   struct platform_device *dev = to_platform_device(_dev);
 + int ret;

 - return drv-probe(dev);
 + if (ACPI_HANDLE(_dev))
 + acpi_dev_pm_attach(_dev, true);
 +
 + ret = drv-probe(dev);
 + if (ret  ACPI_HANDLE(_dev))
 + acpi_dev_pm_detach(_dev, true);
 +
 + return ret;
  }

  static int platform_drv_probe_fail(struct device *_dev) @@ -497,8 +505,13
 @@ static int platform_drv_remove(struct de  {
   struct platform_driver *drv = to_platform_driver(_dev-driver);
   struct platform_device *dev = to_platform_device(_dev);
 + int ret;
 +
 + ret = drv-remove(dev);
 + if (ACPI_HANDLE(_dev))
 + acpi_dev_pm_detach(_dev, true);

 - return drv-remove(dev);
 + return ret;
  }

  static void platform_drv_shutdown(struct device *_dev) @@ -507,6 +520,8
 @@ static void platform_drv_shutdown(struct
   struct platform_device *dev = to_platform_device(_dev);

   drv-shutdown(dev);
 + if (ACPI_HANDLE(_dev))
 + acpi_dev_pm_detach(_dev, true);
  }

  /**

RE: [PATCH 1/6] ACPI / PM: Change the way power transitions to D3cold are carried out

2013-01-04 Thread Zheng, Lv

Thanks for your patch, it might be useful as_PR3 off method of I2C hosts and 
targets might be different from their _PR0 off method, ACPI BIOS may implement 
protection in the _PR3 off method in order not to break the transactions during 
the powering off process:
As I2C is wired-AND logic bus, if a device will be LOW on SDA or SCL during the 
powering off process, it will pull the bus from HIGH to LOW which may break the 
current transaction on the bus that targeted to another slave device.

I just wonder one more thing which is not related to the ACPI BIOS.
If busses like I2C have such non-hotpluggable nature, we need to cut power of 
single target device only when there are not any transactions visible in the 
same segment.

How could an equivalent solution be implemented in the Linux kernel for I2C 
busses?
It could be useful for those platforms without such firmware deployed to 
protect the OS.

It seems we may need to redesign acpi_device_set_power/acpi_device_power_state 
to meet the following requirements:
1. suspend/resume - One function can be used to switch device power state from 
0 to 3 or 3 to 0.
2. poweroff - One function can be used to cut device power currently applied.
And we may need new interface in the power core as poweroff (maybe also 
poweron) or likewise for platform_suspend/hibernate_ops.

Then I2C in the kernel can also implement a solution putting all of the devices 
(the masters and all of their slaves) in one segment (where there is wired-AND 
logic) into D3 (suspend all non-hotpluggable devices in one segment), and 
poweroff one of them when there are not any transactions visible on the bus.

Thanks and best regards
-Lv

 -Original Message-
 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Saturday, January 05, 2013 6:00 AM
 To: ACPI Devel Maling List
 Cc: LKML; Len Brown; Zheng, Lv; Huang, Ying
 Subject: [PATCH 1/6] ACPI / PM: Change the way power transitions to D3cold
 are carried out
 
 From: Rafael J. Wysocki rafael.j.wyso...@intel.com
 
 During power transitions into D3cold from any shallower power states we are
 supposed to transition the device into D3hot and remove power from it
 afterward, but the current code in acpi_device_set_power() doesn't work this
 way.
 
 At the same time, though, we need to be careful enough to preserve
 backwards compatibility for systems that don't distinguish between D3hot and
 D3cold (e.g. designed before ACPI 4).
 
 Modify acpi_device_set_power() so that it works in accordance with the
 expectations in both cases.
 
 Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
 ---
  drivers/acpi/bus.c |   12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)
 
 Index: linux/drivers/acpi/bus.c
 
 ===
 --- linux.orig/drivers/acpi/bus.c
 +++ linux/drivers/acpi/bus.c
 @@ -270,6 +270,7 @@ int acpi_device_set_power(struct acpi_de
   int result = 0;
   acpi_status status = AE_OK;
   char object_name[5] = { '_', 'P', 'S', '0' + state, '\0' };
 + bool cut_power = false;
 
   if (!device || (state  ACPI_STATE_D0) || (state  ACPI_STATE_D3_COLD))
   return -EINVAL;
 @@ -294,9 +295,13 @@ int acpi_device_set_power(struct acpi_de
   return -ENODEV;
   }
 
 - /* For D3cold we should execute _PS3, not _PS4. */
 - if (state == ACPI_STATE_D3_COLD)
 + /* For D3cold we should first transition into D3hot. */
 + if (state == ACPI_STATE_D3_COLD
 + 
 device-power.states[ACPI_STATE_D3_COLD].flags.os_accessible) {
 + state = ACPI_STATE_D3_HOT;
   object_name[3] = '3';
 + cut_power = true;
 + }
 
   /*
* Transition Power
 @@ -341,6 +346,9 @@ int acpi_device_set_power(struct acpi_de
   }
   }
 
 + if (cut_power)
 + result = acpi_power_transition(device, ACPI_STATE_D3_COLD);
 +
end:
   if (result)
   printk(KERN_WARNING PREFIX

N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path

 From: Moore, Robert
 Sent: Wednesday, July 24, 2013 10:39 PM
 
 I haven't found a high-level description of acpi_os_prepare_sleep, perhaps I
 missed it.

If we take a look at the declaration of this new OSL API, its name is 
acpi_os_prepare_sleep, but originally it only hacks two registers' values.
It is more like a Xen only hacking logic rather than an OSL API from ACPICA's 
perspective.
So the API declaration is just looking ugly to ACPICA, this has prevented this 
OSL API from being back ported to ACPICA for long time.
If ACPICA merged this codes, then it could be very hard for ACPICA to do any 
future enhancement to modify the logic in the 
acpi_hw_legacy_sleep/acpi_hw_extended_sleep.
Thus this is not clean for ACPICA, it will introduce unwanted software entropy 
to ACPICA.

This patchset enhances the OSL API, but doesn't make it cleaner, and just add a 
new parameter, it is a hack on top of the original hack.

IMO, from ACPICA's perspective, the OSL API should be designed to be re-used by 
any other OSPMs and might have more meaningful function declaration to ACPICA.
OSPM codes like Xen can implement this OSL API.  The Xen only hacking logic 
should be put inside the OSL API implementation.

Hers is just a suggestion and I don't know if this can work for Xen:

status = acpi_os_prepare_sleep(u8 sleep_state);
if (ACPI_SKIP(status))
...

And export:
1. acpi_gbl_sleep_type_a/acpi_gbl_sleep_type_b
2. acpi_gbl_reduced_hardware
3. acpi_hw_get_bit_register_info
to be used by acpi_os_prepare_sleep.
At least you can just copy the logic in the 
acpi_hw_legacy_sleep/acpi_hw_extended_sleep to the acpi_os_prepare_sleep.
That kind of complexity is Xen's complexity, you shouldn't move this complexity 
to ACPICA just because you want Xen side codes to be simpler.
If Xen and tboot already have knowledge about above what will actually happen 
to the sleep_state, we don't need to export them.

This way might be better for both parties.

Thanks and best regards
-Lv


 
 Can someone point me to the overall description of this change and why it is
 being considered?
 
 Thanks,
 Bob
 
 
  -Original Message-
  From: Ben Guthro [mailto:benjamin.gut...@citrix.com]
  Sent: Wednesday, July 24, 2013 6:23 AM
  To: Moore, Robert
  Cc: Zheng, Lv; Konrad Rzeszutek Wilk; Jan Beulich; Rafael J . Wysocki;
  linux-kernel@vger.kernel.org; linux-a...@vger.kernel.org; xen-
  de...@lists.xen.org
  Subject: Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in
  reduced hardware sleep path
 
  On 07/24/2013 09:18 AM, Moore, Robert wrote:
   I have not looked closely at this, but we typically do things like
   this
  in ACPICA so that they only need to be implemented once to support all
  of the various acpica-hosted operating systems - linux, solaris,
  hp-ux, apple, freebsd, etc. -- even if they could be implemented
  cleaner in some way on any given host.
 
  Even when the resulting simplification results in reduced functionality?
 
  Maybe I am misunderstanding the suggestion...but it sounded like it
  was basically to mimic the traditional behavior, and mask out the
  reduced hardware capabilities on these system types.
 
  It seems to me that if the system supports the reduced hardware ACPI
  sleep, you would want to make use of it...
 
 
 
  
  
  
   -Original Message-
   From: Ben Guthro [mailto:benjamin.gut...@citrix.com]
   Sent: Wednesday, July 24, 2013 5:01 AM
   To: Zheng, Lv
   Cc: Konrad Rzeszutek Wilk; Jan Beulich; Rafael J . Wysocki; linux-
   ker...@vger.kernel.org; linux-a...@vger.kernel.org; xen-
   de...@lists.xen.org; Moore, Robert
   Subject: Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook
   in reduced hardware sleep path
  
  
  
   On 07/24/2013 02:24 AM, Zheng, Lv wrote:
   Hi,
  
   Sorry for the delayed response.
  
   From: Ben Guthro [mailto:benjamin.gut...@citrix.com]
   Sent: Tuesday, July 02, 2013 7:43 PM
  
  
   On 07/02/2013 02:19 AM, Zheng, Lv wrote:
   Thanks for your efforts!
  
   I wonder if it is possible to remove the argument - u8 extended
   and convert
   pm1a_control, pm1b_control into some u8 values that are
   equivalent to acpi_gbl_sleep_type_a, acpi_gbl_sleep_type_b in
   the legacy sleep
   path.
   It can also simplify Xen codes.
  
   Thanks for your time to review this.
  
   I'm not sure that this simplifies things. I think that, in fact,
   it would make them quite a bit more complicated, but perhaps I
   misunderstand.
  
   Is it not preferred to use the reduced hardware sleep, over the
   old
   method?
   While these register definitions may be equivalent below, doing
   the translation in linux, only to translate them back again at a
   lower
   layer seems unnecessary.
  
  
   Yes, it would require tboot layer to be able to be aware of how
   such
   fields locate in the PM registers.
   So I think you can pass the register address of the field and the
   field
   name/value pair to the tboot, this could simplify things, no lower
   layer effort

RE: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path

Let me just give an example to let you know the difficulties for ACPICA 
developers to merge Xen's acpi_os_prepare_sleep.

The original logic in the acpi_hw_legacy_sleep is:
111 /* Get current value of PM1A control */
112 
113 status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
114pm1a_control);
115 if (ACPI_FAILURE(status)) {
116 return_ACPI_STATUS(status);
117 }
118 ACPI_DEBUG_PRINT((ACPI_DB_INIT,
119   Entering sleep state [S%u]\n, sleep_state));
120 
121 /* Clear the SLP_EN and SLP_TYP fields */
122 
123 pm1a_control = ~(sleep_type_reg_info-access_bit_mask |
124   sleep_enable_reg_info-access_bit_mask);
125 pm1b_control = pm1a_control;
126 
127 /* Insert the SLP_TYP bits */
128 
129 pm1a_control |=
130 (acpi_gbl_sleep_type_a  sleep_type_reg_info-bit_position);
131 pm1b_control |=
132 (acpi_gbl_sleep_type_b  sleep_type_reg_info-bit_position);
133 
134 /*
135  * We split the writes of SLP_TYP and SLP_EN to workaround
136  * poorly implemented hardware.
137  */
138 
139 /* Write #1: write the SLP_TYP data to the PM1 Control registers */
140 
141 status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
142 if (ACPI_FAILURE(status)) {
143 return_ACPI_STATUS(status);
144 }
145 
146 /* Insert the sleep enable (SLP_EN) bit */
147 
148 pm1a_control |= sleep_enable_reg_info-access_bit_mask;
149 pm1b_control |= sleep_enable_reg_info-access_bit_mask;
150 
151 /* Flush caches, as per ACPI specification */
152 
153 ACPI_FLUSH_CPU_CACHE();
154 
===
[Now Xen's hook appears here)
===
161 /* Write #2: Write both SLP_TYP + SLP_EN */
162 
163 status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
164 if (ACPI_FAILURE(status)) {
165 return_ACPI_STATUS(status);
166 }

If the whole block is re-implemented by ACPICA in the future:

Acpi_hw_write_field_register(ACPI_SLEEP_REGISTER, ACPI_SLP_TYPE | ACPI_SLP_EN, 
slp_type | slp_en);

Then where should ACPICA put this hook under the new design?
Can it go inside acpi_hw_write_field_register?
If the hook is in the current position, then future ACPICA development work on 
the suspend/resume codes are likely broken.

IMO,
1. acpi_os_preapre_sleep() should be put before Line 111
2. acpi_os_preapre_sleep()'s parameters should be re-designed
3. Xen only register hacking logic should be put inside acpi_os_prepare_sleep().

Hope the above example can make my concern clearer now. :-)

Thanks
-Lv

 -Original Message-
 From: linux-acpi-ow...@vger.kernel.org
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Konrad Rzeszutek Wilk
 Sent: Thursday, July 25, 2013 12:32 AM
 To: Ben Guthro
 Cc: Moore, Robert; Zheng, Lv; Jan Beulich; Rafael J . Wysocki;
 linux-kernel@vger.kernel.org; linux-a...@vger.kernel.org;
 xen-de...@lists.xen.org
 Subject: Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced
 hardware sleep path
 
 On Wed, Jul 24, 2013 at 11:14:06AM -0400, Ben Guthro wrote:
 
 
  On 07/24/2013 10:38 AM, Moore, Robert wrote:
   I haven't found a high-level description of acpi_os_prepare_sleep,
 perhaps I missed it.
  
   Can someone point me to the overall description of this change and why it 
   is
 being considered?
 
  Hi Bob,
 
  For this series, the v6 of this series does a decent job of what it is
  trying to accomplish:
  https://lkml.org/lkml/2013/7/1/205
 
  However, I recognize that this does not really describe *why*
  acpi_os_prepare_sleep is necessary to begin with. For that, we need to
  go back a little more.
 
  The summary for the series that introduced it is a good description,
  of the reasons it is necessary:
  http://lkml.indiana.edu/hypermail/linux/kernel/1112.2/00450.html
 
  In summary though - in the case of Xen (and I believe this is also
  true in tboot) a value inappropriate for a VM (which dom0 is a special
  case
  of) was being written to cr3, and the physical machine would never
  come out of S3.
 
  This mechanism gives an os specific hook to do something else down at
  the lower levels, while still being able to take advantage of the
  large amount of OS independent code in ACPICA.
 
 It might be also prudent to look at original 'hook' that was added by Intel 
 in the
 Linux code to support TXT:
 
 
 commit 86886e55b273f565935491816c7c96b82469d4f8
 Author: Joseph Cihula joseph.cih...@intel.com
 Date:   Tue Jun 30 19:31:07 2009 -0700
 
 x86, intel_txt: Intel TXT Sx shutdown support
 
 Support for graceful handling of sleep states (S3/S4/S5) after an Intel(R)
 TXT launch.
 
 Without this patch, attempting to place the system in one of the ACPI
 sleep
 states (S3/S4/S5) will cause the TXT hardware to treat

RE: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path

Yes, I agree.
As what I've said, it's up to the others to determine if the patch is OK.
I just need to make my concerns visible in the community. :-)

Thanks and best regards
-Lv

From: ben.gut...@gmail.com [mailto:ben.gut...@gmail.com] On Behalf Of Ben Guthro
Sent: Thursday, July 25, 2013 9:38 AM

I'm afraid this is well outside of the scope of the bug I was trying to fix.
Given the interactions with the acpi code I have had so far - I am somewhat 
disinclined to make such sweeping changes.

I guess any distro supporting Xen, or tboot will have to carry a patch to avoid 
such a bug.


On Wed, Jul 24, 2013 at 9:28 PM, Zheng, Lv lv.zh...@intel.com wrote:
Let me just give an example to let you know the difficulties for ACPICA 
developers to merge Xen's acpi_os_prepare_sleep.

The original logic in the acpi_hw_legacy_sleep is:
111         /* Get current value of PM1A control */
112
113         status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
114                                        pm1a_control);
115         if (ACPI_FAILURE(status)) {
116                 return_ACPI_STATUS(status);
117         }
118         ACPI_DEBUG_PRINT((ACPI_DB_INIT,
119                           Entering sleep state [S%u]\n, sleep_state));
120
121         /* Clear the SLP_EN and SLP_TYP fields */
122
123         pm1a_control = ~(sleep_type_reg_info-access_bit_mask |
124                           sleep_enable_reg_info-access_bit_mask);
125         pm1b_control = pm1a_control;
126
127         /* Insert the SLP_TYP bits */
128
129         pm1a_control |=
130             (acpi_gbl_sleep_type_a  sleep_type_reg_info-bit_position);
131         pm1b_control |=
132             (acpi_gbl_sleep_type_b  sleep_type_reg_info-bit_position);
133
134         /*
135          * We split the writes of SLP_TYP and SLP_EN to workaround
136          * poorly implemented hardware.
137          */
138
139         /* Write #1: write the SLP_TYP data to the PM1 Control registers */
140
141         status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
142         if (ACPI_FAILURE(status)) {
143                 return_ACPI_STATUS(status);
144         }
145
146         /* Insert the sleep enable (SLP_EN) bit */
147
148         pm1a_control |= sleep_enable_reg_info-access_bit_mask;
149         pm1b_control |= sleep_enable_reg_info-access_bit_mask;
150
151         /* Flush caches, as per ACPI specification */
152
153         ACPI_FLUSH_CPU_CACHE();
154
===
[Now Xen's hook appears here)
===
161         /* Write #2: Write both SLP_TYP + SLP_EN */
162
163         status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
164         if (ACPI_FAILURE(status)) {
165                 return_ACPI_STATUS(status);
166         }

If the whole block is re-implemented by ACPICA in the future:

Acpi_hw_write_field_register(ACPI_SLEEP_REGISTER, ACPI_SLP_TYPE | ACPI_SLP_EN, 
slp_type | slp_en);

Then where should ACPICA put this hook under the new design?
Can it go inside acpi_hw_write_field_register?
If the hook is in the current position, then future ACPICA development work on 
the suspend/resume codes are likely broken.

IMO,
1. acpi_os_preapre_sleep() should be put before Line 111
2. acpi_os_preapre_sleep()'s parameters should be re-designed
3. Xen only register hacking logic should be put inside acpi_os_prepare_sleep().

Hope the above example can make my concern clearer now. :-)

Thanks
-Lv

 -Original Message-
 From: linux-acpi-ow...@vger.kernel.org
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Konrad Rzeszutek Wilk
 Sent: Thursday, July 25, 2013 12:32 AM
 To: Ben Guthro
 Cc: Moore, Robert; Zheng, Lv; Jan Beulich; Rafael J . Wysocki;
 linux-kernel@vger.kernel.org; linux-a...@vger.kernel.org;
 xen-de...@lists.xen.org
 Subject: Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced
 hardware sleep path

 On Wed, Jul 24, 2013 at 11:14:06AM -0400, Ben Guthro wrote:
 
 
  On 07/24/2013 10:38 AM, Moore, Robert wrote:
   I haven't found a high-level description of acpi_os_prepare_sleep,
 perhaps I missed it.
  
   Can someone point me to the overall description of this change and why it 
   is
 being considered?
 
  Hi Bob,
 
  For this series, the v6 of this series does a decent job of what it is
  trying to accomplish:
  https://lkml.org/lkml/2013/7/1/205
 
  However, I recognize that this does not really describe *why*
  acpi_os_prepare_sleep is necessary to begin with. For that, we need to
  go back a little more.
 
  The summary for the series that introduced it is a good description,
  of the reasons it is necessary:
  http://lkml.indiana.edu/hypermail/linux/kernel/1112.2/00450.html
 
  In summary though - in the case of Xen (and I believe this is also
  true in tboot) a value inappropriate for a VM (which dom0 is a special
  case
  of) was being written to cr3, and the physical machine would never
  come out of S3.
 
  This mechanism gives an os specific hook to do something else down at
  the lower levels

RE: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers

-stable according to the previous conversation.

 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Thursday, July 25, 2013 7:38 AM
 
 On Tuesday, July 23, 2013 04:09:15 PM Lv Zheng wrote:
  This patch fixes races caused by unprotected ACPI IPMI transfers.
 
  We can see the following crashes may occur:
  1. There is no tx_msg_lock held for iterating tx_msg_list in
 ipmi_flush_tx_msg() while it is parellel unlinked on failure in
 acpi_ipmi_space_handler() under protection of tx_msg_lock.
  2. There is no lock held for freeing tx_msg in acpi_ipmi_space_handler()
 while it is parellel accessed in ipmi_flush_tx_msg() and
 ipmi_msg_handler().
 
  This patch enhances tx_msg_lock to protect all tx_msg accesses to
  solve this issue.  Then tx_msg_lock is always held around complete()
  and tx_msg accesses.
  Calling smp_wmb() before setting msg_done flag so that messages
  completed due to flushing will not be handled as 'done' messages while
  their contents are not vaild.
 
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Cc: Zhao Yakui yakui.z...@intel.com
  Reviewed-by: Huang Ying ying.hu...@intel.com
  ---
   drivers/acpi/acpi_ipmi.c |   10 --
   1 file changed, 8 insertions(+), 2 deletions(-)
 
  diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
  b37c189..527ee43 100644
  --- a/drivers/acpi/acpi_ipmi.c
  +++ b/drivers/acpi/acpi_ipmi.c
  @@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct
 acpi_ipmi_device *ipmi)
  struct acpi_ipmi_msg *tx_msg, *temp;
  int count = HZ / 10;
  struct pnp_dev *pnp_dev = ipmi-pnp_dev;
  +   unsigned long flags;
 
  +   spin_lock_irqsave(ipmi-tx_msg_lock, flags);
  list_for_each_entry_safe(tx_msg, temp, ipmi-tx_msg_list, head) {
  /* wake up the sleep thread on the Tx msg */
  complete(tx_msg-tx_complete);
  }
  +   spin_unlock_irqrestore(ipmi-tx_msg_lock, flags);
 
  /* wait for about 100ms to flush the tx message list */
  while (count--) {
  @@ -268,13 +271,12 @@ static void ipmi_msg_handler(struct
 ipmi_recv_msg *msg, void *user_msg_data)
  break;
  }
  }
  -   spin_unlock_irqrestore(ipmi_device-tx_msg_lock, flags);
 
  if (!msg_found) {
  dev_warn(pnp_dev-dev,
   Unexpected response (msg id %ld) is returned.\n,
   msg-msgid);
  -   goto out_msg;
  +   goto out_lock;
  }
 
  /* copy the response data to Rx_data buffer */ @@ -286,10 +288,14 @@
  static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void
 *user_msg_data)
  }
  tx_msg-rx_len = msg-msg.data_len;
  memcpy(tx_msg-data, msg-msg.data, tx_msg-rx_len);
  +   /* tx_msg content must be valid before setting msg_done flag */
  +   smp_wmb();
 
 That's suspicious.
 
 If you need the write barrier here, you'll most likely need a read barrier
 somewhere else.  Where's that?

It might depend on whether the content written before the smp_wmb() is used or 
not by the other side codes under the condition set after the smp_wmb().

So comment could be treated as 2 parts:
1. do we need a paired smp_rmb().
2. do we need a smp_wmb().

For 1.
If we want a paired smp_rmb(), then it will appear in this function:

186 static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
187 acpi_integer *value, int rem_time)
188 {
189 struct acpi_ipmi_buffer *buffer;
190 
191 /*
192  * value is also used as output parameter. It represents the 
response
193  * IPMI message returned by IPMI command.
194  */
195 buffer = (struct acpi_ipmi_buffer *)value;
196 if (!rem_time  !msg-msg_done) {
197 buffer-status = ACPI_IPMI_TIMEOUT;
198 return;
199 }
200 /*
201  * If the flag of msg_done is not set or the recv length is zero, it
202  * means that the IPMI command is not executed correctly.
203  * The status code will be ACPI_IPMI_UNKNOWN.
204  */
205 if (!msg-msg_done || !msg-rx_len) {
206 buffer-status = ACPI_IPMI_UNKNOWN;
207 return;
208 }
+ smp_rmb();
209 /*
210  * If the IPMI response message is obtained correctly, the status 
code
211  * will be ACPI_IPMI_OK
212  */
213 buffer-status = ACPI_IPMI_OK;
214 buffer-length = msg-rx_len;
215 memcpy(buffer-data, msg-rx_data, msg-rx_len);
216 }

If we don't then there will only be msg content not correctly read from 
msg-rx_data.
Note that the rx_len is 0 during initialization and will never exceed the 
sizeof(buffer-data), so the read is safe.

Being without smp_rmb() is also OK in this case, since:
1. buffer-data will never be used when buffer-status is not ACPI_IPMI_OK and
2. the smp_rmb()/smp_wmb() added in this patch will be deleted in [PATCH 07].

So IMO, we needn't add the smp_rmb(), what do you think of

RE: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers

 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Thursday, July 25, 2013 8:07 PM

 On Thursday, July 25, 2013 03:09:35 AM Zheng, Lv wrote:
  -stable according to the previous conversation.

   From: Rafael J. Wysocki [mailto:r...@sisk.pl]
   Sent: Thursday, July 25, 2013 7:38 AM

   On Tuesday, July 23, 2013 04:09:15 PM Lv Zheng wrote:
This patch fixes races caused by unprotected ACPI IPMI transfers.

We can see the following crashes may occur:
1. There is no tx_msg_lock held for iterating tx_msg_list in
   ipmi_flush_tx_msg() while it is parellel unlinked on failure in
   acpi_ipmi_space_handler() under protection of tx_msg_lock.
2. There is no lock held for freeing tx_msg in acpi_ipmi_space_handler()
   while it is parellel accessed in ipmi_flush_tx_msg() and
   ipmi_msg_handler().

This patch enhances tx_msg_lock to protect all tx_msg accesses to
solve this issue.  Then tx_msg_lock is always held around
complete() and tx_msg accesses.
Calling smp_wmb() before setting msg_done flag so that messages
completed due to flushing will not be handled as 'done' messages
while their contents are not vaild.

Signed-off-by: Lv Zheng lv.zh...@intel.com
Cc: Zhao Yakui yakui.z...@intel.com
Reviewed-by: Huang Ying ying.hu...@intel.com
---
 drivers/acpi/acpi_ipmi.c |   10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
index
b37c189..527ee43 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct
   acpi_ipmi_device *ipmi)
struct acpi_ipmi_msg *tx_msg, *temp;
int count = HZ / 10;
struct pnp_dev *pnp_dev = ipmi-pnp_dev;
+   unsigned long flags;

+   spin_lock_irqsave(ipmi-tx_msg_lock, flags);
list_for_each_entry_safe(tx_msg, temp, ipmi-tx_msg_list, 
head) {
/* wake up the sleep thread on the Tx msg */
complete(tx_msg-tx_complete);
}
+   spin_unlock_irqrestore(ipmi-tx_msg_lock, flags);

/* wait for about 100ms to flush the tx message list */
while (count--) {
@@ -268,13 +271,12 @@ static void ipmi_msg_handler(struct
   ipmi_recv_msg *msg, void *user_msg_data)
break;
}
}
-   spin_unlock_irqrestore(ipmi_device-tx_msg_lock, flags);

if (!msg_found) {
dev_warn(pnp_dev-dev,
 Unexpected response (msg id %ld) is 
returned.\n,
 msg-msgid);
-   goto out_msg;
+   goto out_lock;
}

/* copy the response data to Rx_data buffer */ @@ -286,10
+288,14 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg,
void
   *user_msg_data)
}
tx_msg-rx_len = msg-msg.data_len;
memcpy(tx_msg-data, msg-msg.data, tx_msg-rx_len);
+   /* tx_msg content must be valid before setting msg_done flag */
+   smp_wmb();

   That's suspicious.

   If you need the write barrier here, you'll most likely need a read
   barrier somewhere else.  Where's that?

  It might depend on whether the content written before the smp_wmb() is
 used or not by the other side codes under the condition set after the
 smp_wmb().

  So comment could be treated as 2 parts:
  1. do we need a paired smp_rmb().
  2. do we need a smp_wmb().

  For 1.
  If we want a paired smp_rmb(), then it will appear in this function:

  186 static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
  187 acpi_integer *value, int rem_time)
  188 {
  189 struct acpi_ipmi_buffer *buffer;
  190
  191 /*
  192  * value is also used as output parameter. It represents the
 response
  193  * IPMI message returned by IPMI command.
  194  */
  195 buffer = (struct acpi_ipmi_buffer *)value;
  196 if (!rem_time  !msg-msg_done) {
  197 buffer-status = ACPI_IPMI_TIMEOUT;
  198 return;
  199 }
  200 /*
  201  * If the flag of msg_done is not set or the recv length is 
  zero,
 it
  202  * means that the IPMI command is not executed correctly.
  203  * The status code will be ACPI_IPMI_UNKNOWN.
  204  */
  205 if (!msg-msg_done || !msg-rx_len) {
  206 buffer-status = ACPI_IPMI_UNKNOWN;
  207 return;
  208 }
  + smp_rmb();
  209 /*
  210  * If the IPMI response message is obtained correctly, the
 status code
  211  * will be ACPI_IPMI_OK
  212  */
  213 buffer-status = ACPI_IPMI_OK;
  214 buffer-length = msg-rx_len;
  215 memcpy(buffer

RE: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers

 From: Corey Minyard [mailto:tcminy...@gmail.com]
 Sent: Friday, July 26, 2013 2:13 AM
 
 On 07/25/2013 07:06 AM, Rafael J. Wysocki wrote:
  On Thursday, July 25, 2013 03:09:35 AM Zheng, Lv wrote:
  -stable according to the previous conversation.
 
  From: Rafael J. Wysocki [mailto:r...@sisk.pl]
  Sent: Thursday, July 25, 2013 7:38 AM
 
  On Tuesday, July 23, 2013 04:09:15 PM Lv Zheng wrote:
  This patch fixes races caused by unprotected ACPI IPMI transfers.
 
  We can see the following crashes may occur:
  1. There is no tx_msg_lock held for iterating tx_msg_list in
  ipmi_flush_tx_msg() while it is parellel unlinked on failure in
  acpi_ipmi_space_handler() under protection of tx_msg_lock.
  2. There is no lock held for freeing tx_msg in acpi_ipmi_space_handler()
  while it is parellel accessed in ipmi_flush_tx_msg() and
  ipmi_msg_handler().
 
  This patch enhances tx_msg_lock to protect all tx_msg accesses to
  solve this issue.  Then tx_msg_lock is always held around
  complete() and tx_msg accesses.
  Calling smp_wmb() before setting msg_done flag so that messages
  completed due to flushing will not be handled as 'done' messages
  while their contents are not vaild.
 
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Cc: Zhao Yakui yakui.z...@intel.com
  Reviewed-by: Huang Ying ying.hu...@intel.com
  ---
drivers/acpi/acpi_ipmi.c |   10 --
1 file changed, 8 insertions(+), 2 deletions(-)
 
  diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
  index
  b37c189..527ee43 100644
  --- a/drivers/acpi/acpi_ipmi.c
  +++ b/drivers/acpi/acpi_ipmi.c
  @@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct
  acpi_ipmi_device *ipmi)
   struct acpi_ipmi_msg *tx_msg, *temp;
   int count = HZ / 10;
   struct pnp_dev *pnp_dev = ipmi-pnp_dev;
  +unsigned long flags;
 
  +spin_lock_irqsave(ipmi-tx_msg_lock, flags);
   list_for_each_entry_safe(tx_msg, temp, ipmi-tx_msg_list, 
  head) {
   /* wake up the sleep thread on the Tx msg */
   complete(tx_msg-tx_complete);
   }
  +spin_unlock_irqrestore(ipmi-tx_msg_lock, flags);
 
   /* wait for about 100ms to flush the tx message list */
   while (count--) {
  @@ -268,13 +271,12 @@ static void ipmi_msg_handler(struct
  ipmi_recv_msg *msg, void *user_msg_data)
   break;
   }
   }
  -spin_unlock_irqrestore(ipmi_device-tx_msg_lock, flags);
 
   if (!msg_found) {
   dev_warn(pnp_dev-dev,
Unexpected response (msg id %ld) is 
  returned.\n,
msg-msgid);
  -goto out_msg;
  +goto out_lock;
   }
 
   /* copy the response data to Rx_data buffer */ @@ -286,10
  +288,14 @@ static void ipmi_msg_handler(struct ipmi_recv_msg *msg,
  void
  *user_msg_data)
   }
   tx_msg-rx_len = msg-msg.data_len;
   memcpy(tx_msg-data, msg-msg.data, tx_msg-rx_len);
  +/* tx_msg content must be valid before setting msg_done flag */
  +smp_wmb();
  That's suspicious.
 
  If you need the write barrier here, you'll most likely need a read
  barrier somewhere else.  Where's that?
  It might depend on whether the content written before the smp_wmb() is
 used or not by the other side codes under the condition set after the
 smp_wmb().
 
  So comment could be treated as 2 parts:
  1. do we need a paired smp_rmb().
  2. do we need a smp_wmb().
 
  For 1.
  If we want a paired smp_rmb(), then it will appear in this function:
 
  186 static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
  187 acpi_integer *value, int rem_time)
  188 {
  189 struct acpi_ipmi_buffer *buffer;
  190
  191 /*
  192  * value is also used as output parameter. It represents the
 response
  193  * IPMI message returned by IPMI command.
  194  */
  195 buffer = (struct acpi_ipmi_buffer *)value;
  196 if (!rem_time  !msg-msg_done) {
  197 buffer-status = ACPI_IPMI_TIMEOUT;
  198 return;
  199 }
  200 /*
  201  * If the flag of msg_done is not set or the recv length is 
  zero,
 it
  202  * means that the IPMI command is not executed correctly.
  203  * The status code will be ACPI_IPMI_UNKNOWN.
  204  */
  205 if (!msg-msg_done || !msg-rx_len) {
  206 buffer-status = ACPI_IPMI_UNKNOWN;
  207 return;
  208 }
  + smp_rmb();
  209 /*
  210  * If the IPMI response message is obtained correctly, the
 status code
  211  * will be ACPI_IPMI_OK
  212  */
  213 buffer-status = ACPI_IPMI_OK;
  214 buffer-length = msg-rx_len;
  215 memcpy(buffer-data, msg-rx_data, msg-rx_len);
  216 }
 
  If we don't

RE: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers

 From: linux-acpi-ow...@vger.kernel.org
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki
 Sent: Friday, July 26, 2013 3:33 AM

 On Thursday, July 25, 2013 01:12:38 PM Corey Minyard wrote:
  On 07/25/2013 07:06 AM, Rafael J. Wysocki wrote:
   On Thursday, July 25, 2013 03:09:35 AM Zheng, Lv wrote:
   -stable according to the previous conversation.

   From: Rafael J. Wysocki [mailto:r...@sisk.pl]
   Sent: Thursday, July 25, 2013 7:38 AM

   On Tuesday, July 23, 2013 04:09:15 PM Lv Zheng wrote:
   This patch fixes races caused by unprotected ACPI IPMI transfers.

   We can see the following crashes may occur:
   1. There is no tx_msg_lock held for iterating tx_msg_list in
   ipmi_flush_tx_msg() while it is parellel unlinked on failure in
   acpi_ipmi_space_handler() under protection of tx_msg_lock.
   2. There is no lock held for freeing tx_msg in 
   acpi_ipmi_space_handler()
   while it is parellel accessed in ipmi_flush_tx_msg() and
   ipmi_msg_handler().

   This patch enhances tx_msg_lock to protect all tx_msg accesses to
   solve this issue.  Then tx_msg_lock is always held around
   complete() and tx_msg accesses.
   Calling smp_wmb() before setting msg_done flag so that messages
   completed due to flushing will not be handled as 'done' messages
   while their contents are not vaild.

   Signed-off-by: Lv Zheng lv.zh...@intel.com
   Cc: Zhao Yakui yakui.z...@intel.com
   Reviewed-by: Huang Ying ying.hu...@intel.com
   ---
 drivers/acpi/acpi_ipmi.c |   10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

   diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
   index
   b37c189..527ee43 100644
   --- a/drivers/acpi/acpi_ipmi.c
   +++ b/drivers/acpi/acpi_ipmi.c
   @@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct
   acpi_ipmi_device *ipmi)
  struct acpi_ipmi_msg *tx_msg, *temp;
  int count = HZ / 10;
  struct pnp_dev *pnp_dev = ipmi-pnp_dev;
   +  unsigned long flags;

   +  spin_lock_irqsave(ipmi-tx_msg_lock, flags);
  list_for_each_entry_safe(tx_msg, temp, ipmi-tx_msg_list,
 head) {
  /* wake up the sleep thread on the Tx msg */
  complete(tx_msg-tx_complete);
  }
   +  spin_unlock_irqrestore(ipmi-tx_msg_lock, flags);

  /* wait for about 100ms to flush the tx message list */
  while (count--) {
   @@ -268,13 +271,12 @@ static void ipmi_msg_handler(struct
   ipmi_recv_msg *msg, void *user_msg_data)
  break;
  }
  }
   -  spin_unlock_irqrestore(ipmi_device-tx_msg_lock, flags);

  if (!msg_found) {
  dev_warn(pnp_dev-dev,
   Unexpected response (msg id %ld) is 
   returned.\n,
   msg-msgid);
   -  goto out_msg;
   +  goto out_lock;
  }

  /* copy the response data to Rx_data buffer */ @@ -286,10
   +288,14 @@ static void ipmi_msg_handler(struct ipmi_recv_msg
   *msg, void
   *user_msg_data)
  }
  tx_msg-rx_len = msg-msg.data_len;
  memcpy(tx_msg-data, msg-msg.data, tx_msg-rx_len);
   +  /* tx_msg content must be valid before setting msg_done flag */
   +  smp_wmb();
   That's suspicious.

   If you need the write barrier here, you'll most likely need a read
   barrier somewhere else.  Where's that?
   It might depend on whether the content written before the smp_wmb() is
 used or not by the other side codes under the condition set after the
 smp_wmb().

   So comment could be treated as 2 parts:
   1. do we need a paired smp_rmb().
   2. do we need a smp_wmb().

   For 1.
   If we want a paired smp_rmb(), then it will appear in this function:

   186 static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
   187 acpi_integer *value, int rem_time)
   188 {
   189 struct acpi_ipmi_buffer *buffer;
   190
   191 /*
   192  * value is also used as output parameter. It represents the
 response
   193  * IPMI message returned by IPMI command.
   194  */
   195 buffer = (struct acpi_ipmi_buffer *)value;
   196 if (!rem_time  !msg-msg_done) {
   197 buffer-status = ACPI_IPMI_TIMEOUT;
   198 return;
   199 }
   200 /*
   201  * If the flag of msg_done is not set or the recv length is
 zero, it
   202  * means that the IPMI command is not executed correctly.
   203  * The status code will be ACPI_IPMI_UNKNOWN.
   204  */
   205 if (!msg-msg_done || !msg-rx_len) {
   206 buffer-status = ACPI_IPMI_UNKNOWN;
   207 return;
   208 }
   + smp_rmb();
   209 /*
   210  * If the IPMI response message is obtained correctly, the
 status code
   211  * will be ACPI_IPMI_OK
   212

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers



 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Friday, July 26, 2013 4:27 AM
 
 On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
  This patch adds reference couting for ACPI operation region handlers
  to fix races caused by the ACPICA address space callback invocations.
 
  ACPICA address space callback invocation is not suitable for Linux
  CONFIG_MODULE=y execution environment.  This patch tries to protect
  the address space callbacks by invoking them under a module safe
 environment.
  The IPMI address space handler is also upgraded in this patch.
  The acpi_unregister_region() is designed to meet the following
  requirements:
  1. It acts as a barrier for operation region callbacks - no callback will
 happen after acpi_unregister_region().
  2. acpi_unregister_region() is safe to be called in moudle-exit()
 functions.
  Using reference counting rather than module referencing allows such
  benefits to be achieved even when acpi_unregister_region() is called
  in the environments other than module-exit().
  The header file of include/acpi/acpi_bus.h should contain the
  declarations that have references to some ACPICA defined types.
 
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Reviewed-by: Huang Ying ying.hu...@intel.com
  ---
   drivers/acpi/acpi_ipmi.c |   16 ++--
   drivers/acpi/osl.c   |  224
 ++
   include/acpi/acpi_bus.h  |5 ++
   3 files changed, 235 insertions(+), 10 deletions(-)
 
  diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
  5f8f495..2a09156 100644
  --- a/drivers/acpi/acpi_ipmi.c
  +++ b/drivers/acpi/acpi_ipmi.c
  @@ -539,20 +539,18 @@ out_ref:
   static int __init acpi_ipmi_init(void)  {
  int result = 0;
  -   acpi_status status;
 
  if (acpi_disabled)
  return result;
 
  mutex_init(driver_data.ipmi_lock);
 
  -   status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
  -   ACPI_ADR_SPACE_IPMI,
  -   acpi_ipmi_space_handler,
  -   NULL, NULL);
  -   if (ACPI_FAILURE(status)) {
  +   result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
  + acpi_ipmi_space_handler,
  + NULL, NULL);
  +   if (result) {
  pr_warn(Can't register IPMI opregion space handle\n);
  -   return -EINVAL;
  +   return result;
  }
 
  result = ipmi_smi_watcher_register(driver_data.bmc_events);
  @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
  }
  mutex_unlock(driver_data.ipmi_lock);
 
  -   acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
  - ACPI_ADR_SPACE_IPMI,
  - acpi_ipmi_space_handler);
  +   acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
   }
 
   module_init(acpi_ipmi_init);
  diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
  6ab2c35..8398e51 100644
  --- a/drivers/acpi/osl.c
  +++ b/drivers/acpi/osl.c
  @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;  static
  struct workqueue_struct *kacpi_notify_wq;  static struct
  workqueue_struct *kacpi_hotplug_wq;
 
  +struct acpi_region {
  +   unsigned long flags;
  +#define ACPI_REGION_DEFAULT0x01
  +#define ACPI_REGION_INSTALLED  0x02
  +#define ACPI_REGION_REGISTERED 0x04
  +#define ACPI_REGION_UNREGISTERING  0x08
  +#define ACPI_REGION_INSTALLING 0x10
 
 What about (1UL  1), (1UL  2) etc.?
 
 Also please remove the #defines out of the struct definition.

OK.

 
  +   /*
  +* NOTE: Upgrading All Region Handlers
  +* This flag is only used during the period where not all of the
  +* region handers are upgraded to the new interfaces.
  +*/
  +#define ACPI_REGION_MANAGED0x80
  +   acpi_adr_space_handler handler;
  +   acpi_adr_space_setup setup;
  +   void *context;
  +   /* Invoking references */
  +   atomic_t refcnt;
 
 Actually, why don't you use krefs?

If you take a look at other piece of my codes, you'll find there are two 
reasons:

1. I'm using while (atomic_read()  1) to implement the objects' flushing and 
there is no kref API to do so.
  I just think it is not suitable for me to introduce such an API into kref.h 
and start another argument around kref designs in this bug fix patch. :-)
  I'll start a discussion about kref design using another thread.
2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's 
kind of atomic_t coding style.
  If atomic_t is changed to struct kref, I will need to implement two API, 
__ipmi_dev_release() to take a struct kref as parameter and call 
ipmi_dev_release inside it.
  By not using kref, I needn't write codes to implement such API.

 
  +};
  +
  +static struct acpi_region acpi_regions[ACPI_NUM_PREDEFINED_REGIONS]
 = {
  +

RE: [PATCH 04/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI user

 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Friday, July 26, 2013 5:59 AM
 
 On Tuesday, July 23, 2013 04:09:26 PM Lv Zheng wrote:
  This patch uses reference counting to fix the race caused by the
  unprotected ACPI IPMI user.
 
  As the acpi_ipmi_device-user_interface check in
  acpi_ipmi_space_handler() can happen before setting user_interface to
  NULL and codes after the check in acpi_ipmi_space_handler() can happen
  after user_interface becoming NULL, then the on-going
  acpi_ipmi_space_handler() still can pass an invalid
  acpi_ipmi_device-user_interface to ipmi_request_settime().  Such race
  condition is not allowed by the IPMI layer's API design as crash will 
  happen in
 ipmi_request_settime().
  In IPMI layer, smi_gone()/new_smi() callbacks are protected by
  smi_watchers_mutex, thus their invocations are serialized.  But as a
  new smi can re-use the freed intf_num, it requires that the callback
  implementation must not use intf_num as an identification mean or it
  must ensure all references to the previous smi are all dropped before
  exiting
  smi_gone() callback.  In case of acpi_ipmi module, this means
  ipmi_flush_tx_msg() must ensure all on-going IPMI transfers are
  completed before exiting ipmi_flush_tx_msg().
 
  This patch follows ipmi_devintf.c design:
  1. Invoking ipmi_destroy_user() after the reference count of
 acpi_ipmi_device dropping to 0, this matches IPMI layer's API calling
 rule on ipmi_destroy_user() and ipmi_request_settime().
  2. References of acpi_ipmi_device dropping to 1 means tx_msg related to
 this acpi_ipmi_device are all freed, this can be used to implement the
 new flushing mechanism.  Note complete() must be retried so that the
 on-going tx_msg won't block flushing at the point to add tx_msg into
 tx_msg_list where reference of acpi_ipmi_device is held.  This matches
 the IPMI layer's callback rule on smi_gone()/new_smi() serialization.
  3. ipmi_flush_tx_msg() is performed after deleting acpi_ipmi_device from
 the list so that no new tx_msg can be created after entering flushing
 process.
  4. The flushing of tx_msg is also moved out of ipmi_lock in this patch.
 
  The forthcoming IPMI operation region handler installation changes
  also requires acpi_ipmi_device be handled in the reference counting style.
 
  Authorship is also updated due to this design change.
 
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Cc: Zhao Yakui yakui.z...@intel.com
  Reviewed-by: Huang Ying ying.hu...@intel.com
  ---
   drivers/acpi/acpi_ipmi.c |  249
  +++---
   1 file changed, 149 insertions(+), 100 deletions(-)
 
  diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
  527ee43..cbf25e0 100644
  --- a/drivers/acpi/acpi_ipmi.c
  +++ b/drivers/acpi/acpi_ipmi.c
  @@ -1,8 +1,9 @@
   /*
*  acpi_ipmi.c - ACPI IPMI opregion
*
  - *  Copyright (C) 2010 Intel Corporation
  - *  Copyright (C) 2010 Zhao Yakui yakui.z...@intel.com
  + *  Copyright (C) 2010, 2013 Intel Corporation
  + *Author: Zhao Yakui yakui.z...@intel.com
  + *Lv Zheng lv.zh...@intel.com
*
*
 
 ~~
*
  @@ -67,6 +68,7 @@ struct acpi_ipmi_device {
  long curr_msgid;
  unsigned long flags;
  struct ipmi_smi_info smi_data;
  +   atomic_t refcnt;
 
 Can you use a kref instead?

Please see my concerns in another email.

 
   };
 
   struct ipmi_driver_data {
  @@ -107,8 +109,8 @@ struct acpi_ipmi_buffer {  static void
  ipmi_register_bmc(int iface, struct device *dev);  static void
  ipmi_bmc_gone(int iface);  static void ipmi_msg_handler(struct
  ipmi_recv_msg *msg, void *user_msg_data); -static void
  acpi_add_ipmi_device(struct acpi_ipmi_device *ipmi_device); -static
  void acpi_remove_ipmi_device(struct acpi_ipmi_device *ipmi_device);
  +static int ipmi_install_space_handler(struct acpi_ipmi_device *ipmi);
  +static void ipmi_remove_space_handler(struct acpi_ipmi_device *ipmi);
 
   static struct ipmi_driver_data driver_data = {
  .ipmi_devices = LIST_HEAD_INIT(driver_data.ipmi_devices),
  @@ -122,6 +124,80 @@ static struct ipmi_driver_data driver_data = {
  },
   };
 
  +static struct acpi_ipmi_device *
  +ipmi_dev_alloc(int iface, struct ipmi_smi_info *smi_data, acpi_handle
  +handle) {
  +   struct acpi_ipmi_device *ipmi_device;
  +   int err;
  +   ipmi_user_t user;
  +
  +   ipmi_device = kzalloc(sizeof(*ipmi_device), GFP_KERNEL);
  +   if (!ipmi_device)
  +   return NULL;
  +
  +   atomic_set(ipmi_device-refcnt, 1);
  +   INIT_LIST_HEAD(ipmi_device-head);
  +   INIT_LIST_HEAD(ipmi_device-tx_msg_list);
  +   spin_lock_init(ipmi_device-tx_msg_lock);
  +
  +   ipmi_device-handle = handle;
  +   ipmi_device-pnp_dev = to_pnp_dev(get_device(smi_data-dev));
  +   memcpy(ipmi_device-smi_data, smi_data, sizeof(struct
 ipmi_smi_info));
  +   ipmi_device-ipmi_ifnum = iface;
  +
  +

RE: [PATCH 07/13] ACPI/IPMI: Add reference counting for ACPI IPMI transfers

 From: linux-acpi-ow...@vger.kernel.org
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki
 Sent: Friday, July 26, 2013 6:23 AM
 
 On Tuesday, July 23, 2013 04:09:54 PM Lv Zheng wrote:
  This patch adds reference counting for ACPI IPMI transfers to tune the
  locking granularity of tx_msg_lock.
 
  The acpi_ipmi_msg handling is re-designed using referece counting.
  1. tx_msg is always unlinked before complete(), so that:
 1.1. it is safe to put complete() out side of tx_msg_lock;
 1.2. complete() can only happen once, thus smp_wmb() is not required.
  2. Increasing the reference of tx_msg before calling
 ipmi_request_settime() and introducing tx_msg_lock protected
 ipmi_cancel_tx_msg() so that a complete() can happen in parellel with
 tx_msg unlinking in the failure cases.
  3. tx_msg holds the reference of acpi_ipmi_device so that it can be flushed
 and freed in the contexts other than acpi_ipmi_space_handler().
 
  The lockdep_chains shows all acpi_ipmi locks are leaf locks after the
  tuning:
  1. ipmi_lock is always leaf:
 irq_context: 0
 [81a943f8] smi_watchers_mutex
 [a06eca60] driver_data.ipmi_lock
 irq_context: 0
 [82767b40] buffer-mutex
 [a00a6678] s_active#103
 [a06eca60] driver_data.ipmi_lock
  2. without this patch applied, lock used by complete() is held after
 holding tx_msg_lock:
 irq_context: 0
 [82767b40] buffer-mutex
 [a00a6678] s_active#103
 [a06ecce8] (ipmi_device-tx_msg_lock)-rlock
 irq_context: 1
 [a06ecce8] (ipmi_device-tx_msg_lock)-rlock
 irq_context: 1
 [a06ecce8] (ipmi_device-tx_msg_lock)-rlock
 [a06eccf0] x-wait#25
 irq_context: 1
 [a06ecce8] (ipmi_device-tx_msg_lock)-rlock
 [a06eccf0] x-wait#25
 [81e36620] p-pi_lock
 irq_context: 1
 [a06ecce8] (ipmi_device-tx_msg_lock)-rlock
 [a06eccf0] x-wait#25
 [81e36620] p-pi_lock
 [81e5d0a8] rq-lock
  3. with this patch applied, tx_msg_lock is always leaf:
 irq_context: 0
 [82767b40] buffer-mutex
 [a00a66d8] s_active#107
 [a07ecdc8] (ipmi_device-tx_msg_lock)-rlock
 irq_context: 1
 [a07ecdc8] (ipmi_device-tx_msg_lock)-rlock
 
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Cc: Zhao Yakui yakui.z...@intel.com
  Reviewed-by: Huang Ying ying.hu...@intel.com
  ---
   drivers/acpi/acpi_ipmi.c |  107
 +-
   1 file changed, 77 insertions(+), 30 deletions(-)
 
  diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
  index 2a09156..0ee1ea6 100644
  --- a/drivers/acpi/acpi_ipmi.c
  +++ b/drivers/acpi/acpi_ipmi.c
  @@ -105,6 +105,7 @@ struct acpi_ipmi_msg {
  u8  data[ACPI_IPMI_MAX_MSG_LENGTH];
  u8  rx_len;
  struct acpi_ipmi_device *device;
  +   atomic_trefcnt;
 
 Again: kref, please?

Please see the concerns in another email.

 
   };
 
   /* IPMI request/response buffer per ACPI 4.0, sec 5.5.2.4.3.2 */
  @@ -195,22 +196,47 @@ static struct acpi_ipmi_device
 *acpi_ipmi_get_selected_smi(void)
  return ipmi_device;
   }
 
  -static struct acpi_ipmi_msg *acpi_alloc_ipmi_msg(struct acpi_ipmi_device
 *ipmi)
  +static struct acpi_ipmi_msg *ipmi_msg_alloc(void)
   {
  +   struct acpi_ipmi_device *ipmi;
  struct acpi_ipmi_msg *ipmi_msg;
  -   struct pnp_dev *pnp_dev = ipmi-pnp_dev;
 
  +   ipmi = acpi_ipmi_get_selected_smi();
  +   if (!ipmi)
  +   return NULL;
  ipmi_msg = kzalloc(sizeof(struct acpi_ipmi_msg), GFP_KERNEL);
  -   if (!ipmi_msg)  {
  -   dev_warn(pnp_dev-dev, Can't allocate memory for ipmi_msg\n);
  +   if (!ipmi_msg) {
  +   acpi_ipmi_dev_put(ipmi);
  return NULL;
  }
  +   atomic_set(ipmi_msg-refcnt, 1);
  init_completion(ipmi_msg-tx_complete);
  INIT_LIST_HEAD(ipmi_msg-head);
  ipmi_msg-device = ipmi;
  +
  return ipmi_msg;
   }
 
  +static struct acpi_ipmi_msg *
  +acpi_ipmi_msg_get(struct acpi_ipmi_msg *tx_msg)
  +{
  +   if (tx_msg)
  +   atomic_inc(tx_msg-refcnt);
  +   return tx_msg;
  +}
  +
  +static void ipmi_msg_release(struct acpi_ipmi_msg *tx_msg)
  +{
  +   acpi_ipmi_dev_put(tx_msg-device);
  +   kfree(tx_msg);
  +}
  +
  +static void acpi_ipmi_msg_put(struct acpi_ipmi_msg *tx_msg)
  +{
  +   if (tx_msg  atomic_dec_and_test(tx_msg-refcnt))
  +   ipmi_msg_release(tx_msg);
  +}
  +
   #defineIPMI_OP_RGN_NETFN(offset)   ((offset  8)  0xff)
   #defineIPMI_OP_RGN_CMD(offset) (offset  0xff)
   static int acpi_format_ipmi_request(struct acpi_ipmi_msg *tx_msg,
  @@ -300,7 +326,7 @@ static void acpi_format_ipmi_response(struct
 acpi_ipmi_msg *msg,
 
   static void ipmi_flush_tx_msg(struct acpi_ipmi_device *ipmi)
   {
  -   struct acpi_ipmi_msg *tx_msg, *temp;
  +   struct acpi_ipmi_msg

RE: [PATCH 08/13] ACPI/IPMI: Cleanup several acpi_ipmi_device members

 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Friday, July 26, 2013 6:26 AM
 
 On Tuesday, July 23, 2013 04:10:06 PM Lv Zheng wrote:
  This is a trivial patch:
  1. Deletes a member of the acpi_ipmi_device - smi_data which is not
 actually used.
  2. Updates a member of the acpi_ipmi_device - pnp_dev which is only used
 by dev_warn() invocations, so changes it to struct device.
 
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Reviewed-by: Huang Ying ying.hu...@intel.com
  ---
   drivers/acpi/acpi_ipmi.c |   30 ++
   1 file changed, 14 insertions(+), 16 deletions(-)
 
  diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
  0ee1ea6..7f93ffd 100644
  --- a/drivers/acpi/acpi_ipmi.c
  +++ b/drivers/acpi/acpi_ipmi.c
  @@ -63,11 +63,10 @@ struct acpi_ipmi_device {
  struct list_head tx_msg_list;
  spinlock_t  tx_msg_lock;
  acpi_handle handle;
  -   struct pnp_dev *pnp_dev;
  +   struct device *dev;
  ipmi_user_t user_interface;
  int ipmi_ifnum; /* IPMI interface number */
  long curr_msgid;
  -   struct ipmi_smi_info smi_data;
  atomic_t refcnt;
   };
 
  @@ -132,7 +131,7 @@ static struct ipmi_driver_data driver_data = {  };
 
   static struct acpi_ipmi_device *
  -ipmi_dev_alloc(int iface, struct ipmi_smi_info *smi_data, acpi_handle
  handle)
  +ipmi_dev_alloc(int iface, struct device *pdev, acpi_handle handle)
 
 Why is the second arg called pdev?

OK, I will change it to dev.

 
   {
  struct acpi_ipmi_device *ipmi_device;
  int err;
  @@ -148,14 +147,13 @@ ipmi_dev_alloc(int iface, struct ipmi_smi_info
 *smi_data, acpi_handle handle)
  spin_lock_init(ipmi_device-tx_msg_lock);
 
  ipmi_device-handle = handle;
  -   ipmi_device-pnp_dev = to_pnp_dev(get_device(smi_data-dev));
  -   memcpy(ipmi_device-smi_data, smi_data, sizeof(struct
 ipmi_smi_info));
  +   ipmi_device-dev = get_device(pdev);
  ipmi_device-ipmi_ifnum = iface;
 
  err = ipmi_create_user(iface, driver_data.ipmi_hndlrs,
 ipmi_device, user);
  if (err) {
  -   put_device(smi_data-dev);
  +   put_device(pdev);
  kfree(ipmi_device);
  return NULL;
  }
  @@ -175,7 +173,7 @@ acpi_ipmi_dev_get(struct acpi_ipmi_device
  *ipmi_device)  static void ipmi_dev_release(struct acpi_ipmi_device
  *ipmi_device)  {
  ipmi_destroy_user(ipmi_device-user_interface);
  -   put_device(ipmi_device-smi_data.dev);
  +   put_device(ipmi_device-dev);
  kfree(ipmi_device);
   }
 
  @@ -263,7 +261,7 @@ static int acpi_format_ipmi_request(struct
 acpi_ipmi_msg *tx_msg,
  buffer = (struct acpi_ipmi_buffer *)value;
  /* copy the tx message data */
  if (buffer-length  ACPI_IPMI_MAX_MSG_LENGTH) {
  -   dev_WARN_ONCE(tx_msg-device-pnp_dev-dev, true,
  +   dev_WARN_ONCE(tx_msg-device-dev, true,
Unexpected request (msg len %d).\n,
buffer-length);
  return -EINVAL;
  @@ -382,11 +380,11 @@ static void ipmi_msg_handler(struct
 ipmi_recv_msg *msg, void *user_msg_data)
  struct acpi_ipmi_device *ipmi_device = user_msg_data;
  int msg_found = 0;
  struct acpi_ipmi_msg *tx_msg;
  -   struct pnp_dev *pnp_dev = ipmi_device-pnp_dev;
  +   struct device *dev = ipmi_device-dev;
  unsigned long flags;
 
  if (msg-user != ipmi_device-user_interface) {
  -   dev_warn(pnp_dev-dev,
  +   dev_warn(dev,
   Unexpected response is returned. returned user %p, 
  expected
 user %p\n,
   msg-user, ipmi_device-user_interface);
  goto out_msg;
  @@ -404,7 +402,7 @@ static void ipmi_msg_handler(struct ipmi_recv_msg
 *msg, void *user_msg_data)
  spin_unlock_irqrestore(ipmi_device-tx_msg_lock, flags);
 
  if (!msg_found) {
  -   dev_warn(pnp_dev-dev,
  +   dev_warn(dev,
   Unexpected response (msg id %ld) is returned.\n,
   msg-msgid);
  goto out_msg;
  @@ -412,7 +410,7 @@ static void ipmi_msg_handler(struct ipmi_recv_msg
  *msg, void *user_msg_data)
 
  /* copy the response data to Rx_data buffer */
  if (msg-msg.data_len  ACPI_IPMI_MAX_MSG_LENGTH) {
  -   dev_WARN_ONCE(pnp_dev-dev, true,
  +   dev_WARN_ONCE(dev, true,
Unexpected response (msg len %d).\n,
msg-msg.data_len);
  goto out_comp;
  @@ -431,7 +429,7 @@ out_msg:
   static void ipmi_register_bmc(int iface, struct device *dev)  {
  struct acpi_ipmi_device *ipmi_device, *temp;
  -   struct pnp_dev *pnp_dev;
  +   struct device *pdev;
 
 And here?

The dev is the parameter of the ipmi_register_bmc(), it is not possible to name 
the struct ipmi_smi_info  as dev here for this quick fix.

Thanks
-Lv

 
  int err;
  struct ipmi_smi_info smi_data;
  acpi_handle handle;
  @@ -445,11 +443,11 @@

RE: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers

 From: Corey Minyard [mailto:tcminy...@gmail.com]
 Sent: Friday, July 26, 2013 8:48 AM
 
 On 07/25/2013 07:16 PM, Zheng, Lv wrote:
 
  If I understand this correctly, the problem would be if:
 
  rem_time = wait_for_completion_timeout(tx_msg-tx_complete,
IPMI_TIMEOUT);
 
  returns on a timeout, then checks msg_done and races with something
  setting msg_done.  If that is the case, you would need the smp_rmb()
  before checking msg_done.
 
  However, the timeout above is unnecessary.  You are using
  ipmi_request_settime(), so you can set the timeout when the IPMI
  command fails and returns a failure message.  The driver guarantees a
  return message for each request.  Just remove the timeout from the
  completion, set the timeout and retries in the ipmi request, and the
  completion should handle the barrier issues.
  It's just difficult for me to determine retry count and timeout value, maybe
 retry=0, timeout=IPMI_TIMEOUT is OK.
  The code of the timeout completion is already there, I think the quick fix 
  code
 should not introduce this logic.
  I'll add a new patch to apply your comment.
 
 Since it is a local BMC, I doubt a retry is required.  That is probably fine. 
  Or
 you could set retry=1 and timeout=IPMI_TIMEOUT/2 if you wanted to be more
 sure, but I doubt it would make a difference.  The only time you really need 
 to
 worry about retries is if you are resetting the BMC or it is being overloaded.

I think for ACPI IPMI operation region, retries can be implemented in the ASL 
codes by the BIOS.
I'll check if retry=0 is correct.

 
 
  Plus, from a quick glance at the code, it doesn't look like it will
  properly handle a situation where the timeout occurs and is handled
  then the response comes in later.
  PATCH 07 fixed this issue.
  Here we just need the smp_rmb() or holding tx_msg_lock() around the
 acpi_format_ipmi_response().
 
 If you apply the fix like I suggest, then the race goes away.  If there's no
 timeout and it just waits for the completion, things get a lot simpler.

Exactly.  I'll try to apply this in this patch, then the PATCH 07 is also need 
to be re-worked.

Thanks and best regards
-Lv


 
  Thanks for commenting.
 
 No problem, thanks for working on this.
 
 -corey

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Friday, July 26, 2013 5:29 AM
 
 On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
  This patch adds reference couting for ACPI operation region handlers to fix
  races caused by the ACPICA address space callback invocations.
 
  ACPICA address space callback invocation is not suitable for Linux
  CONFIG_MODULE=y execution environment.
 
 Actually, can you please explain to me what *exactly* the problem is?

OK.  I'll add race explanations in the next revision.

The problem is there is no lock held inside ACPICA for invoking operation 
region handlers.
Thus races happens between the acpi_remove/install_address_space_handler and 
the handler/setup callbacks.

This is correct per ACPI specification.
As if there is interpreter locks held for invoking operation region handlers, 
the timeout implemented inside the operation region handlers will make all 
locking facilities (Acquire or Sleep,...) timed out.
Please refer to ACPI specification 5.5.2 Control Method Execution:
Interpretation of a Control Method is not preemptive, but it can block. When a 
control method does block, OSPM can initiate or continue the execution of a 
different control method. A control method can only assume that access to 
global objects is exclusive for any period the control method does not block.

So it is pretty much likely that ACPI IO transfers are locked inside the 
operation region callback implementations.
Using locking facility to protect the callback invocation will risk dead locks.

Thanks
-Lv

 Rafael

RE: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path

 From: Konrad Rzeszutek Wilk [mailto:konrad.w...@oracle.com]
 Sent: Thursday, July 25, 2013 8:04 PM
 
 CC-ing some of the tboot maintainers.
  As what I've said, it's up to the others to determine if the patch is OK.
  I just need to make my concerns visible in the community. :-)
 
 If I understand your concerns you don't want the hook to depend on any
 of the bit manipulations the existing code does for the pm1 values. The
 hook should do it itself case it needs to tweak them or what not.
 
 And it also frees you from altering the ACPICA code without having to
 worry about being dependent on what the input values the hook requires?
 
 Is this what you had in mind? (not compile tested nor tested).

Actually I've drafted such a patch that had conflicts with the Xen/tboot hooks.
Then the patchset has to first delete the hooks to make test possible for 
testers.
It is here:
https://bugzilla.kernel.org/show_bug.cgi?id=54181

 
 I am not even sure if outside the drivers/acpi you can call
 acpi_hw_get_bit_register_info ..

If we want this patch to be accepted without modification, then someone can 
help to do such cleanup in the future when ACPICA change happens.

 
 And since the Xen bits would do the same exact bit manipulation it
 probably could use a library to do pm1* stuff so both tboot and Xen
 can use it.

This sounds better.
I think Xen and tboot will need such a library to atomically accessing PM 
register fields.

Thanks and best regards
-Lv

 
 diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
 index f84fe00..59570b1 100644
 --- a/arch/x86/kernel/tboot.c
 +++ b/arch/x86/kernel/tboot.c
 @@ -273,20 +273,75 @@ static void tboot_copy_fadt(const struct
 acpi_table_fadt *fadt)
   offsetof(struct acpi_table_facs, firmware_waking_vector);
  }
 
 -static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
 +static int tboot_get_pm_control(bool legacy)
 +{
 + u32 pm1a_control;
 + u32 pm1b_control;
 + u32 in_value;
 + acpi_status status;
 + struct acpi_bit_register_info *sleep_type_reg_info;
 + struct acpi_bit_register_info *sleep_enable_reg_info;
 +
 + if (!legacy)
 + return -ENOSPC;
 +
 + status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
 +pm1a_control);
 + if (ACPI_FAILURE(status)) {
 + return -EXXX /* something */;
 + }
 + sleep_type_reg_info =
 acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_TYPE);
 + sleep_enable_reg_info =
 acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_ENABLE);
 + /* Clear the SLP_EN and SLP_TYP fields */
 +
 + pm1a_control = ~(sleep_type_reg_info-access_bit_mask |
 +   sleep_enable_reg_info-access_bit_mask);
 + pm1b_control = pm1a_control;
 +
 + /* Insert the SLP_TYP bits */
 +
 + pm1a_control |=
 + (acpi_gbl_sleep_type_a  sleep_type_reg_info-bit_position);
 + pm1b_control |=
 + (acpi_gbl_sleep_type_b  sleep_type_reg_info-bit_position);
 +
 + /*
 +  * We split the writes of SLP_TYP and SLP_EN to workaround
 +  * poorly implemented hardware.
 +  */
 +
 + /* Write #1: write the SLP_TYP data to the PM1 Control registers */
 +
 + status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
 + if (ACPI_FAILURE(status)) {
 + return -EXXX /* something */;
 + }
 +
 + /* Insert the sleep enable (SLP_EN) bit */
 +
 + pm1a_control |= sleep_enable_reg_info-access_bit_mask;
 + pm1b_control |= sleep_enable_reg_info-access_bit_mask;
 + tboot-acpi_sinfo.pm1a_cnt_val = pm1a_control;
 + tboot-acpi_sinfo.pm1b_cnt_val = pm1b_control;
 + return 0;
 +}
 +static int tboot_sleep(u8 sleep_state);
  {
   static u32 acpi_shutdown_map[ACPI_S_STATE_COUNT] = {
   /* S0,1,2: */ -1, -1, -1,
   /* S3: */ TB_SHUTDOWN_S3,
   /* S4: */ TB_SHUTDOWN_S4,
   /* S5: */ TB_SHUTDOWN_S5 };
 + int rc;
 
   if (!tboot_enabled())
   return 0;
 
   tboot_copy_fadt(acpi_gbl_FADT);
 - tboot-acpi_sinfo.pm1a_cnt_val = pm1a_control;
 - tboot-acpi_sinfo.pm1b_cnt_val = pm1b_control;
 +
 + rc = tboot_get_pm_control();
 + if (rc  0)
 + return -1;
   /* we always use the 32b wakeup vector */
   tboot-acpi_sinfo.vector_width = 32;
 
 diff --git a/drivers/acpi/acpica/hwesleep.c b/drivers/acpi/acpica/hwesleep.c
 index 5e5f762..a8e98f9 100644
 --- a/drivers/acpi/acpica/hwesleep.c
 +++ b/drivers/acpi/acpica/hwesleep.c
 @@ -113,6 +113,15 @@ acpi_status acpi_hw_extended_sleep(u8
 sleep_state)
   !acpi_gbl_FADT.sleep_status.address) {
   return_ACPI_STATUS(AE_NOT_EXIST);
   }
 +   /*
 +* If using tboot or other platforms that need tweaks then
 +* do them here, and also bail out if neccessary.
 +*/
 +   status = acpi_os_prepare_sleep(sleep_state);
 +   if (ACPI_SKIP(status))
 +

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

2013-07-26 Thread Zheng, Lv

 From: linux-acpi-ow...@vger.kernel.org
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, Lv
 Sent: Friday, July 26, 2013 8:48 AM

  From: Rafael J. Wysocki [mailto:r...@sisk.pl]
  Sent: Friday, July 26, 2013 4:27 AM

  On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
   This patch adds reference couting for ACPI operation region handlers
   to fix races caused by the ACPICA address space callback invocations.

   ACPICA address space callback invocation is not suitable for Linux
   CONFIG_MODULE=y execution environment.  This patch tries to protect
   the address space callbacks by invoking them under a module safe
  environment.
   The IPMI address space handler is also upgraded in this patch.
   The acpi_unregister_region() is designed to meet the following
   requirements:
   1. It acts as a barrier for operation region callbacks - no callback will
  happen after acpi_unregister_region().
   2. acpi_unregister_region() is safe to be called in moudle-exit()
  functions.
   Using reference counting rather than module referencing allows such
   benefits to be achieved even when acpi_unregister_region() is called
   in the environments other than module-exit().
   The header file of include/acpi/acpi_bus.h should contain the
   declarations that have references to some ACPICA defined types.

   Signed-off-by: Lv Zheng lv.zh...@intel.com
   Reviewed-by: Huang Ying ying.hu...@intel.com
   ---
drivers/acpi/acpi_ipmi.c |   16 ++--
drivers/acpi/osl.c   |  224
  ++
include/acpi/acpi_bus.h  |5 ++
3 files changed, 235 insertions(+), 10 deletions(-)

   diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
   index
   5f8f495..2a09156 100644
   --- a/drivers/acpi/acpi_ipmi.c
   +++ b/drivers/acpi/acpi_ipmi.c
   @@ -539,20 +539,18 @@ out_ref:
static int __init acpi_ipmi_init(void)  {
 int result = 0;
   - acpi_status status;

 if (acpi_disabled)
 return result;

 mutex_init(driver_data.ipmi_lock);

   - status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
   - ACPI_ADR_SPACE_IPMI,
   - acpi_ipmi_space_handler,
   - NULL, NULL);
   - if (ACPI_FAILURE(status)) {
   + result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
   +   acpi_ipmi_space_handler,
   +   NULL, NULL);
   + if (result) {
 pr_warn(Can't register IPMI opregion space handle\n);
   - return -EINVAL;
   + return result;
 }

 result = ipmi_smi_watcher_register(driver_data.bmc_events);
   @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
 }
 mutex_unlock(driver_data.ipmi_lock);

   - acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
   -   ACPI_ADR_SPACE_IPMI,
   -   acpi_ipmi_space_handler);
   + acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
}

module_init(acpi_ipmi_init);
   diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
   6ab2c35..8398e51 100644
   --- a/drivers/acpi/osl.c
   +++ b/drivers/acpi/osl.c
   @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
   static struct workqueue_struct *kacpi_notify_wq;  static struct
   workqueue_struct *kacpi_hotplug_wq;

   +struct acpi_region {
   + unsigned long flags;
   +#define ACPI_REGION_DEFAULT  0x01
   +#define ACPI_REGION_INSTALLED0x02
   +#define ACPI_REGION_REGISTERED   0x04
   +#define ACPI_REGION_UNREGISTERING0x08
   +#define ACPI_REGION_INSTALLING   0x10

  What about (1UL  1), (1UL  2) etc.?

  Also please remove the #defines out of the struct definition.

 OK.

   + /*
   +  * NOTE: Upgrading All Region Handlers
   +  * This flag is only used during the period where not all of the
   +  * region handers are upgraded to the new interfaces.
   +  */
   +#define ACPI_REGION_MANAGED  0x80
   + acpi_adr_space_handler handler;
   + acpi_adr_space_setup setup;
   + void *context;
   + /* Invoking references */
   + atomic_t refcnt;

  Actually, why don't you use krefs?

 If you take a look at other piece of my codes, you'll find there are two 
 reasons:

 1. I'm using while (atomic_read()  1) to implement the objects' flushing and
 there is no kref API to do so.
   I just think it is not suitable for me to introduce such an API into kref.h 
 and
 start another argument around kref designs in this bug fix patch. :-)
   I'll start a discussion about kref design using another thread.
 2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's 
 kind
 of atomic_t coding style.
   If atomic_t is changed to struct kref, I will need to implement two API,
 __ipmi_dev_release() to take a struct kref as parameter and call

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

2013-07-26 Thread Zheng, Lv

From: linux-acpi-ow...@vger.kernel.org
[mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, Lv
Sent: Friday, July 26, 2013 9:54 AM
To: Rafael J. Wysocki
Cc: Wysocki, Rafael J; Brown, Len; linux-kernel@vger.kernel.org;
linux-a...@vger.kernel.org
Subject: RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI
operation region handlers

From: Rafael J. Wysocki [mailto:r...@sisk.pl]
Sent: Friday, July 26, 2013 5:29 AM

On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
This patch adds reference couting for ACPI operation region handlers
to fix races caused by the ACPICA address space callback invocations.

ACPICA address space callback invocation is not suitable for Linux
CONFIG_MODULE=y execution environment.

Actually, can you please explain to me what *exactly* the problem is?

OK. I'll add race explanations in the next revision.

The problem is there is no lock held inside ACPICA for invoking operation
region handlers.
Thus races happens between the acpi_remove/install_address_space_handler
and the handler/setup callbacks.

This seems not a good explanation of the intent of this patch.
I think the intent is here in the patch description:

1. It acts as a barrier for operation region callbacks - no callback will
happen after acpi_unregister_region().
2. acpi_unregister_region() is safe to be called in moudle-exit()
functions.

Hmm, maybe I need to re-order the patch description for this patch.

Thanks for commenting.

Best regards
-Lv

This is correct per ACPI specification.
As if there is interpreter locks held for invoking operation region handlers,
the
timeout implemented inside the operation region handlers will make all locking
facilities (Acquire or Sleep,...) timed out.
Please refer to ACPI specification 5.5.2 Control Method Execution:
Interpretation of a Control Method is not preemptive, but it can block. When a
control method does block, OSPM can initiate or continue the execution of a
different control method. A control method can only assume that access to
global objects is exclusive for any period the control method does not block.

So it is pretty much likely that ACPI IO transfers are locked inside the
operation
region callback implementations.
Using locking facility to protect the callback invocation will risk dead
locks.

Thanks
-Lv

Rafael
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH 08/13] ACPI/IPMI: Cleanup several acpi_ipmi_device members

 From: linux-acpi-ow...@vger.kernel.org
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki
 
 On Friday, July 26, 2013 01:25:12 AM Zheng, Lv wrote:
   From: Rafael J. Wysocki [mailto:r...@sisk.pl]
   Sent: Friday, July 26, 2013 6:26 AM
  
   On Tuesday, July 23, 2013 04:10:06 PM Lv Zheng wrote:
This is a trivial patch:
1. Deletes a member of the acpi_ipmi_device - smi_data which is not
   actually used.
2. Updates a member of the acpi_ipmi_device - pnp_dev which is only
 used
   by dev_warn() invocations, so changes it to struct device.
   
Signed-off-by: Lv Zheng lv.zh...@intel.com
Reviewed-by: Huang Ying ying.hu...@intel.com
---
 drivers/acpi/acpi_ipmi.c |   30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)
   
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
0ee1ea6..7f93ffd 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -63,11 +63,10 @@ struct acpi_ipmi_device {
struct list_head tx_msg_list;
spinlock_t  tx_msg_lock;
acpi_handle handle;
-   struct pnp_dev *pnp_dev;
+   struct device *dev;
ipmi_user_t user_interface;
int ipmi_ifnum; /* IPMI interface number */
long curr_msgid;
-   struct ipmi_smi_info smi_data;
atomic_t refcnt;
 };
   
@@ -132,7 +131,7 @@ static struct ipmi_driver_data driver_data = {  };
   
 static struct acpi_ipmi_device *
-ipmi_dev_alloc(int iface, struct ipmi_smi_info *smi_data, acpi_handle
handle)
+ipmi_dev_alloc(int iface, struct device *pdev, acpi_handle handle)
  
   Why is the second arg called pdev?
 
  OK, I will change it to dev.
 
 OK, thanks.
 
  
 {
struct acpi_ipmi_device *ipmi_device;
int err;
@@ -148,14 +147,13 @@ ipmi_dev_alloc(int iface, struct ipmi_smi_info
   *smi_data, acpi_handle handle)
spin_lock_init(ipmi_device-tx_msg_lock);
   
ipmi_device-handle = handle;
-   ipmi_device-pnp_dev = to_pnp_dev(get_device(smi_data-dev));
-   memcpy(ipmi_device-smi_data, smi_data, sizeof(struct
   ipmi_smi_info));
+   ipmi_device-dev = get_device(pdev);
ipmi_device-ipmi_ifnum = iface;
   
err = ipmi_create_user(iface, driver_data.ipmi_hndlrs,
   ipmi_device, user);
if (err) {
-   put_device(smi_data-dev);
+   put_device(pdev);
kfree(ipmi_device);
return NULL;
}
@@ -175,7 +173,7 @@ acpi_ipmi_dev_get(struct acpi_ipmi_device
*ipmi_device)  static void ipmi_dev_release(struct acpi_ipmi_device
*ipmi_device)  {
ipmi_destroy_user(ipmi_device-user_interface);
-   put_device(ipmi_device-smi_data.dev);
+   put_device(ipmi_device-dev);
kfree(ipmi_device);
 }
   
@@ -263,7 +261,7 @@ static int acpi_format_ipmi_request(struct
   acpi_ipmi_msg *tx_msg,
buffer = (struct acpi_ipmi_buffer *)value;
/* copy the tx message data */
if (buffer-length  ACPI_IPMI_MAX_MSG_LENGTH) {
-   dev_WARN_ONCE(tx_msg-device-pnp_dev-dev, true,
+   dev_WARN_ONCE(tx_msg-device-dev, true,
  Unexpected request (msg len %d).\n,
  buffer-length);
return -EINVAL;
@@ -382,11 +380,11 @@ static void ipmi_msg_handler(struct
   ipmi_recv_msg *msg, void *user_msg_data)
struct acpi_ipmi_device *ipmi_device = user_msg_data;
int msg_found = 0;
struct acpi_ipmi_msg *tx_msg;
-   struct pnp_dev *pnp_dev = ipmi_device-pnp_dev;
+   struct device *dev = ipmi_device-dev;
unsigned long flags;
   
if (msg-user != ipmi_device-user_interface) {
-   dev_warn(pnp_dev-dev,
+   dev_warn(dev,
 Unexpected response is returned. returned 
user %p,
 expected
   user %p\n,
 msg-user, ipmi_device-user_interface);
goto out_msg;
@@ -404,7 +402,7 @@ static void ipmi_msg_handler(struct
 ipmi_recv_msg
   *msg, void *user_msg_data)
spin_unlock_irqrestore(ipmi_device-tx_msg_lock, flags);
   
if (!msg_found) {
-   dev_warn(pnp_dev-dev,
+   dev_warn(dev,
 Unexpected response (msg id %ld) is 
returned.\n,
 msg-msgid);
goto out_msg;
@@ -412,7 +410,7 @@ static void ipmi_msg_handler(struct
 ipmi_recv_msg
*msg, void *user_msg_data)
   
/* copy the response data to Rx_data buffer */
if (msg-msg.data_len  ACPI_IPMI_MAX_MSG_LENGTH) {
-   dev_WARN_ONCE(pnp_dev

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

 On Friday, July 26, 2013 10:01 PM Rafael J. Wysocki wrote:
  On Friday, July 26, 2013 12:47:44 AM Zheng, Lv wrote:
 
   On Friday, July 26, 2013 4:27 AM Rafael J. Wysocki wrote:
  
   On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
This patch adds reference couting for ACPI operation region handlers
to fix races caused by the ACPICA address space callback invocations.
   
ACPICA address space callback invocation is not suitable for Linux
CONFIG_MODULE=y execution environment.  This patch tries to protect
the address space callbacks by invoking them under a module safe
   environment.
The IPMI address space handler is also upgraded in this patch.
The acpi_unregister_region() is designed to meet the following
requirements:
1. It acts as a barrier for operation region callbacks - no callback 
will
   happen after acpi_unregister_region().
2. acpi_unregister_region() is safe to be called in moudle-exit()
   functions.
Using reference counting rather than module referencing allows such
benefits to be achieved even when acpi_unregister_region() is called
in the environments other than module-exit().
The header file of include/acpi/acpi_bus.h should contain the
declarations that have references to some ACPICA defined types.
   
Signed-off-by: Lv Zheng lv.zh...@intel.com
Reviewed-by: Huang Ying ying.hu...@intel.com
---
 drivers/acpi/acpi_ipmi.c |   16 ++--
 drivers/acpi/osl.c   |  224
   ++
 include/acpi/acpi_bus.h  |5 ++
 3 files changed, 235 insertions(+), 10 deletions(-)
   
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
5f8f495..2a09156 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -539,20 +539,18 @@ out_ref:
 static int __init acpi_ipmi_init(void)  {
int result = 0;
-   acpi_status status;
   
if (acpi_disabled)
return result;
   
mutex_init(driver_data.ipmi_lock);
   
-   status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
-   ACPI_ADR_SPACE_IPMI,
-   
acpi_ipmi_space_handler,
-   NULL, NULL);
-   if (ACPI_FAILURE(status)) {
+   result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
+ acpi_ipmi_space_handler,
+ NULL, NULL);
+   if (result) {
pr_warn(Can't register IPMI opregion space handle\n);
-   return -EINVAL;
+   return result;
}
   
result = ipmi_smi_watcher_register(driver_data.bmc_events);
@@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
}
mutex_unlock(driver_data.ipmi_lock);
   
-   acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
- ACPI_ADR_SPACE_IPMI,
- acpi_ipmi_space_handler);
+   acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
 }
   
 module_init(acpi_ipmi_init);
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
6ab2c35..8398e51 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
 static
struct workqueue_struct *kacpi_notify_wq;  static struct
workqueue_struct *kacpi_hotplug_wq;
   
+struct acpi_region {
+   unsigned long flags;
+#define ACPI_REGION_DEFAULT0x01
+#define ACPI_REGION_INSTALLED  0x02
+#define ACPI_REGION_REGISTERED 0x04
+#define ACPI_REGION_UNREGISTERING  0x08
+#define ACPI_REGION_INSTALLING 0x10
  
   What about (1UL  1), (1UL  2) etc.?
  
   Also please remove the #defines out of the struct definition.
 
  OK.
 
  
+   /*
+* NOTE: Upgrading All Region Handlers
+* This flag is only used during the period where not all of the
+* region handers are upgraded to the new interfaces.
+*/
+#define ACPI_REGION_MANAGED0x80
+   acpi_adr_space_handler handler;
+   acpi_adr_space_setup setup;
+   void *context;
+   /* Invoking references */
+   atomic_t refcnt;
  
   Actually, why don't you use krefs?
 
  If you take a look at other piece of my codes, you'll find there are two
 reasons:
 
  1. I'm using while (atomic_read()  1) to implement the objects' flushing 
  and
 there is no kref API to do so.
 
 No, there's not any, but you can read kref.refcount directly, can't you?
 
 Moreover, it is not entirely clear to me that doing the while (atomic_read() 
  1)
 is actually correct.
 
I just think it is not suitable

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

 On Friday, July 26, 2013 10:49 PM Rafael J. Wysocki wrote:
  On Friday, July 26, 2013 01:54:00 AM Zheng, Lv wrote:
   On Friday, July 26, 2013 5:29 AM Rafael J. Wysocki wrote:
   On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
This patch adds reference couting for ACPI operation region
handlers to fix races caused by the ACPICA address space callback
 invocations.
   
ACPICA address space callback invocation is not suitable for Linux
CONFIG_MODULE=y execution environment.
  
   Actually, can you please explain to me what *exactly* the problem is?
 
  OK.  I'll add race explanations in the next revision.
 
  The problem is there is no lock held inside ACPICA for invoking
  operation region handlers.
  Thus races happens between the
  acpi_remove/install_address_space_handler and the handler/setup
 callbacks.
 
 I see.  Now you're trying to introduce something that would prevent those
 races from happening, right?

Yes. Let me explain this later in this email.

 
  This is correct per ACPI specification.
  As if there is interpreter locks held for invoking operation region
  handlers, the timeout implemented inside the operation region handlers
  will make all locking facilities (Acquire or Sleep,...) timed out.
  Please refer to ACPI specification 5.5.2 Control Method Execution:
  Interpretation of a Control Method is not preemptive, but it can
  block. When a control method does block, OSPM can initiate or continue
  the execution of a different control method. A control method can only
  assume that access to global objects is exclusive for any period the control
 method does not block.
 
  So it is pretty much likely that ACPI IO transfers are locked inside
  the operation region callback implementations.
  Using locking facility to protect the callback invocation will risk dead 
  locks.
 
 No.  If you use a single global lock around all invocations of operation 
 region
 handlers, it won't deadlock, but it will *serialize* things.  This means that
 there won't be two handlers executing in parallel.  That may or may not be
 bad depending on what those handlers actually do.
 
 Your concern seems to be that if one address space handler is buggy and it
 blocks indefinitely, executing it under such a lock would affect the other 
 address
 space handlers and in my opinion this is a valid concern.

It can be expressed in more detailed ways:

The interpreter runs control methods in the following style according to the 
ACPI spec.
CM1_Enter - EnterInter - CM1_Running - OpRegion1 - ExitInter - EnterInter  
  - CM1_running - ExitInter - CM1_Exit
CM2_Enter - EnterInter -   - CM2_Running 
- OpRegion1 - ExitInter - EnterInter   - CM2_running - 
ExitInter - CM2_Exit

EnterInter: Enter interpreter lock
ExitInter: Leave interpreter lock

Let me introduce two situations:

1. If we hold global mutex before EnterInter, then no second control method 
can be run NotSerialized.
If the CM1 just have some codes waiting for a hardware flag and CM2 can access 
other hardware IOs to trigger this flag, then nothing can happen any longer.
This is a practical bug as what we have already seen in NotSerialized marked 
ACPI control methods behave in the interpreter mode executed in serialized way 
- kernel parameter acpi_serialize.

2. If we hold global mutex after EnterInter and Before OpRegion1
If we do things this way, then all IO accesses are serialized, if we have 
something in an IPMI operation region failed due to timeout, then any other 
system IOs that should happen in parallel will just happen after 5 seconds.  
This is not an acceptable experience.

 
 So the idea seems to be to add wrappers around
 acpi_install_address_space_handler()
 and acpi_remove_address_space_handler (but I don't see where the latter is
 called after the change?), such that they will know when it is safe to 
 unregister
 the handler.  That is simple enough.

An obvious bug, it should be put between the while (atomic_read()  1) block 
and the final atomic_dec().

 However, I'm not sure it is needed in the context of IPMI.

I think I do this just because I need a quick fix to test IPMI bug-fix series.
The issue is highly related to ACPI interpreter design, and codes should be 
implemented inside ACPICA.
And there is not only ACPI_ROOT_OBJECT based address space handlers, but also 
non-ACPI_ROOT_OBJECT based address space handlers, this patch can't protect the 
latter ones.

 Your address space
 handler's context is NULL, so even it if is executed after
 acpi_remove_address_space_handler() has been called for it (or in parallel), 
 it
 doesn't depend on anything passed by the caller, so I don't see why the issue
 can't be addressed by a proper synchronization between
 acpi_ipmi_exit() and acpi_ipmi_space_handler().
 
 Clearly, acpi_ipmi_exit() should wait for all already running instances of
 acpi_ipmi_space_handler() to complete and all

RE: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path

We can find the maintainer's reply that the patch set will be merged.
So let me just do a top-based reply here.

The examples I showed in the previous emails are just examples that could be 
used to demonstrate the difficulties on the future ACPICA development that 
brought by the hooks.
Since all what we are talking about are the future, we can just live with the 
current approaches.
We can find a way in the future when the conflicts do happen.

Thanks and best regards
-Lv

 On Saturday, July 27, 2013 2:04 AM konrad wilk
 On 7/25/2013 10:51 PM, Zheng, Lv wrote:
  From: Konrad Rzeszutek Wilk [mailto:konrad.w...@oracle.com]
  Sent: Thursday, July 25, 2013 8:04 PM
 
  CC-ing some of the tboot maintainers.
  As what I've said, it's up to the others to determine if the patch is OK.
  I just need to make my concerns visible in the community. :-)
  If I understand your concerns you don't want the hook to depend on any
  of the bit manipulations the existing code does for the pm1 values. The
  hook should do it itself case it needs to tweak them or what not.
 
  And it also frees you from altering the ACPICA code without having to
  worry about being dependent on what the input values the hook requires?
 
  Is this what you had in mind? (not compile tested nor tested).
  Actually I've drafted such a patch that had conflicts with the Xen/tboot 
  hooks.
 
 Ok, so the idea patch is not yet what you had in mind I surmise?
 
 Looking at the patch ACPICA: Hardware: Modify sleep hardware
 interfaces from said
 bugzilla I believe you could still retain the hook. You could even add
 it as part of the
 state machine (before the values are written to the respective area).
  Then the patchset has to first delete the hooks to make test possible for
 testers.
  It is here:
  https://bugzilla.kernel.org/show_bug.cgi?id=54181
 I must be missing something obvious but the patches just say Removes
 the hook for debugging purpose.
 I think the reason you did that is b/c you were not sure were it would
 fit in your ACPICA: Hardware: Modify sleep hardware interfaces patch.
 But as I said - I am probably missing something obvious.
 
 
  I am not even sure if outside the drivers/acpi you can call
  acpi_hw_get_bit_register_info ..
  If we want this patch to be accepted without modification, then someone can
 help to do such cleanup in the future when ACPICA change happens.
 How would you expose said functions to non-ACPICA drivers?
 
  And since the Xen bits would do the same exact bit manipulation it
  probably could use a library to do pm1* stuff so both tboot and Xen
  can use it.
  This sounds better.
  I think Xen and tboot will need such a library to atomically accessing PM
 register fields.
 
 The sad part is that it would mirror the same exact code paths that the
 existing ACPICA code does. Which is why
 it was just passing in the pm* registers. But I understand that you
 prefer to have a generic API so that you don't have to worry about
 low-level implementations of platform hooks.
 
 What if this hook was allowed to be turned in the state machine? That is
 the acpi_sleep_dispatch and
 it over-wrote part of the write to PM1 state? The goal of the hook is
 to redirect the write using a different mechanism - so if the 'write to
 PM1' is accomplished then that is good.
 
  Thanks and best regards
  -Lv
 
  diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
  index f84fe00..59570b1 100644
  --- a/arch/x86/kernel/tboot.c
  +++ b/arch/x86/kernel/tboot.c
  @@ -273,20 +273,75 @@ static void tboot_copy_fadt(const struct
  acpi_table_fadt *fadt)
 offsetof(struct acpi_table_facs, firmware_waking_vector);
}
 
  -static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32
 pm1b_control)
  +static int tboot_get_pm_control(bool legacy)
  +{
  +  u32 pm1a_control;
  +  u32 pm1b_control;
  +  u32 in_value;
  +  acpi_status status;
  +  struct acpi_bit_register_info *sleep_type_reg_info;
  +  struct acpi_bit_register_info *sleep_enable_reg_info;
  +
  +  if (!legacy)
  +  return -ENOSPC;
  +
  +  status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
  + pm1a_control);
  +  if (ACPI_FAILURE(status)) {
  +  return -EXXX /* something */;
  +  }
  +  sleep_type_reg_info =
  acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_TYPE);
  +  sleep_enable_reg_info =
  acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_ENABLE);
  +  /* Clear the SLP_EN and SLP_TYP fields */
  +
  +  pm1a_control = ~(sleep_type_reg_info-access_bit_mask |
  +sleep_enable_reg_info-access_bit_mask);
  +  pm1b_control = pm1a_control;
  +
  +  /* Insert the SLP_TYP bits */
  +
  +  pm1a_control |=
  +  (acpi_gbl_sleep_type_a  sleep_type_reg_info-bit_position);
  +  pm1b_control |=
  +  (acpi_gbl_sleep_type_b  sleep_type_reg_info-bit_position);
  +
  +  /*
  +   * We split the writes of SLP_TYP and SLP_EN to workaround
  +   * poorly implemented hardware

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

2013-09-13 Thread Zheng, Lv

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Mika Westerberg
 Sent: Friday, September 13, 2013 11:15 PM
 
 GPIO operation regions is a new feature introduced in ACPI 5.0
 specification. In practise it means that now ASL code can toggle GPIOs with
 the help of the OS GPIO driver.
 
 An imaginary example of such ASL code:
 
   Device (\SB.GPIO)
   {
   // _REG is called when the operation region handler is
   // installed.
   Method (_REG)
   {
   ...
   }
 
   OperationRegion (GPOP, GeneralPurposeIo, 0, 0xc)
   Field (GPOP, ByteAcc, NoLock, Preserve)
   {
   Connection
   (
   GpioIo (Exclusive, PullDefault, 0x, 0x,
   IoRestrictionOutputOnly, \\_SB.GPIO,
   0x00, ResourceConsumer,,)
   {
   0x0005
   }
   ),
   PWR0, 1
   }
 
   ...
   }
 
   Device (\SB.DEV0)
   {
   ...
 
   Method (_PS0, 0, Serialized)
   {
   // Toggle the GPIO
   Store (1, \SB.GPIO.PWR0)
   }
 
   Method (_PS3, 0, Serialized)
   {
   Store (0, \SB.GPIO.PRW0)
   }
   }
 
 The device \SB.DEV0 is powered on by asserting \SB.GPIO.PWR0 GPIO line. So
 when the OS calls _PS0 or _PS3, that GPIO line should be set to 1 or 0 by
 the actual GPIO driver.
 
 In order to support GPIO operation regions we install ACPI address space
 handler for the device (provided that it has an ACPI handle). This handler
 uses the standard GPIO APIs to toggle the pin according to what the ASL
 code does.
 
 Since there is need to attach more data to the ACPI object, we create a new
 structure acpi_gpio_chip_data that contains data for both GPIO operation
 regions and for ACPI event handling and make the event handling code to use
 this new structure.
 
 Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
 ---
  drivers/gpio/gpiolib-acpi.c | 131 
 ++--
  1 file changed, 126 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
 index e12a08e..f52536a 100644
 --- a/drivers/gpio/gpiolib-acpi.c
 +++ b/drivers/gpio/gpiolib-acpi.c
 @@ -24,6 +24,17 @@ struct acpi_gpio_evt_pin {
   unsigned int irq;
  };
 
 +struct acpi_gpio_chip_data {
 + /*
 +  * acpi_install_address_space_handler() wants to put the connection
 +  * information at the start of the context structure we pass it, in
 +  * case we are dealing with GPIO operation regions.
 +  */
 + struct acpi_connection_info ci;
 + struct gpio_chip *chip;
 + struct list_head *evt_pins;
 +};
 +
  static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
  {
   if (!gc-dev)
 @@ -86,7 +97,7 @@ static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void 
 *data)
   return IRQ_HANDLED;
  }
 
 -static void acpi_gpio_evt_dh(acpi_handle handle, void *data)
 +static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
  {
   /* The address of this function is used as a key. */
  }
 @@ -127,12 +138,16 @@ static void acpi_gpiochip_request_interrupts(struct 
 gpio_chip *chip)
   if (ACPI_SUCCESS(status)) {
   evt_pins = kzalloc(sizeof(*evt_pins), GFP_KERNEL);
   if (evt_pins) {
 + struct acpi_gpio_chip_data *data;
 +
   INIT_LIST_HEAD(evt_pins);
 - status = acpi_attach_data(handle, acpi_gpio_evt_dh,
 -   evt_pins);
 + status = acpi_get_data(handle, acpi_gpio_chip_dh,
 +(void **)data);
   if (ACPI_FAILURE(status)) {
   kfree(evt_pins);
   evt_pins = NULL;
 + } else {
 + data-evt_pins = evt_pins;
   }
   }
   }
 @@ -293,6 +308,7 @@ static void acpi_gpiochip_free_interrupts(struct 
 gpio_chip *chip)
   acpi_status status;
   struct list_head *evt_pins;
   struct acpi_gpio_evt_pin *evt_pin, *ep;
 + struct acpi_gpio_chip_data *data;
 
   if (!chip-dev || !chip-to_irq)
   return;
 @@ -301,28 +317,133 @@ static void acpi_gpiochip_free_interrupts(struct 
 gpio_chip *chip)
   if (!handle)
   return;
 
 - status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)evt_pins);
 + status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)data);
   if (ACPI_FAILURE(status))

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

2013-09-15 Thread Zheng, Lv

 From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
 Sent: Sunday, September 15, 2013 2:52 PM
 
 On Sat, Sep 14, 2013 at 12:10:37AM +, Zheng, Lv wrote:
  Is it possible to install the handler for ACPI_ROOT_OBJECT?
  Can it be achieved by implementing a setup callback?
 
 Yes that can be done. However, that would mean that we always install the
 operation region handler even if there is no suitable GPIO driver loaded.
 With this patch we install the handler once the GPIO driver for this device
 is registered. If nothing is registered no handlers will be installed.
 
 What would be the advantage in doing what you propose?

A pseudo device may be created to access the GPIO operation region fields 
provided by one GPIO device.
The pseudo device may have other functions to access other GPIO operation 
region fields provided by other GPIO devices, or even worse to access other 
ACPI provided value-adds.
So hierarchically the pseudo device only requires CPU, thus should not be under 
the GPIO device, which means the GPIO operation regions have nothing to do with 
the GPIO devices' ACPI handle.
We cannot imply that the BIOS writers won't create such Frankenstein in the 
future.
So it's better to install address space handlers from ACPI_ROOT_OBJECT.

  Maybe you can also eliminate acpi_attach_data usages by doing so.
 
 I think we still need that for ACPI _EVT handling.

It could be good if we can find a way to eliminate all acpi_attach_data usages 
and make this function deprecated.
But that's fine. :-)

Thanks and best regards
-Lv
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

2013-09-15 Thread Zheng, Lv

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, Lv
 Sent: Monday, September 16, 2013 8:47 AM

  From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
  Sent: Sunday, September 15, 2013 2:52 PM

  On Sat, Sep 14, 2013 at 12:10:37AM +, Zheng, Lv wrote:
   Is it possible to install the handler for ACPI_ROOT_OBJECT?
   Can it be achieved by implementing a setup callback?

  Yes that can be done. However, that would mean that we always install the
  operation region handler even if there is no suitable GPIO driver loaded.
  With this patch we install the handler once the GPIO driver for this device
  is registered. If nothing is registered no handlers will be installed.

  What would be the advantage in doing what you propose?

 A pseudo device may be created to access the GPIO operation region fields 
 provided by one GPIO device.
 The pseudo device may have other functions to access other GPIO operation 
 region fields provided by other GPIO devices, or even worse to
 access other ACPI provided value-adds.
 So hierarchically the pseudo device only requires CPU, thus should not be 
 under the GPIO device, which means the GPIO operation regions
 have nothing to do with the GPIO devices' ACPI handle.

Sorry for the wording.
It's better to say the GPIO operation region users haven't strict relationship 
to the GPIO operation region providers.
As the installation is to provide GPIO operation regions to the users, it 
shouldn't relate to the providers' ACPI handle.

 We cannot imply that the BIOS writers won't create such Frankenstein in the 
 future.
 So it's better to install address space handlers from ACPI_ROOT_OBJECT.

If we didn't do this and such a pseudo device was created, then the error 
message: Region XXX has no handler would be prompted.

Thanks
-Lv

   Maybe you can also eliminate acpi_attach_data usages by doing so.

  I think we still need that for ACPI _EVT handling.

 It could be good if we can find a way to eliminate all acpi_attach_data 
 usages and make this function deprecated.
 But that's fine. :-)

 Thanks and best regards
 -Lv
 --
 To unsubscribe from this list: send the line unsubscribe linux-acpi in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

2013-09-16 Thread Zheng, Lv

 From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
 Sent: Monday, September 16, 2013 4:11 PM
 
 On Mon, Sep 16, 2013 at 01:21:53AM +, Zheng, Lv wrote:
   A pseudo device may be created to access the GPIO operation region fields 
   provided by one GPIO device.
   The pseudo device may have other functions to access other GPIO operation 
   region fields provided by other GPIO devices, or even worse
 to
   access other ACPI provided value-adds.
   So hierarchically the pseudo device only requires CPU, thus should not be 
   under the GPIO device, which means the GPIO operation
 regions
   have nothing to do with the GPIO devices' ACPI handle.
 
  Sorry for the wording.
  It's better to say the GPIO operation region users haven't strict
  relationship to the GPIO operation region providers.
  As the installation is to provide GPIO operation regions to the users, it
  shouldn't relate to the providers' ACPI handle.
 
 If I understand you correctly you mean that there might be multiple users
 (different devices) for the same GPIO operation region, right?

No, this is not what I meant.
Can one vendor device uses two or more GPIO pins from different GPIO ports?

 That shouldn't be a problem as far as I can tell.
 
 What comes to the hierarchy you refer, I'm not sure if that is a problem
 either (unless I'm missing something). The GPIO can be used anywhere in the
 ASL, it doesn't have to be descendant of the GPIO device. You only need to
 do something like this:
 
   // Assert the GPIO
   Store(1, \_SB.PCI0.SHD3)
 
 In other words, use the fully qualified name.

Yes, which means this line of code can be everywhere in the namespace.
It is not required to be under one particular GPIO device as long as there is 
an operation region defined in that scope.

The problem is the installation, the first parameter for 
acpi_install_address_space_handler() is a namespace node, the function will 
call _REG for all OperationRegions below the scope whose top most node is the 
specified node.
The nodes out of this scope are not affected.  So if an OperationRegion is 
defined out of this scope, problem happens.

 Typically when the GPIO device _REG() is called it sets some variable like
 AVBL to true which is then checked in the code that uses the GPIO:
 
   If (LEqual (\_SB.PCI0.GPO0.AVBL, One))
   {
   Store (Zero, \_SB.PCI0..SHD3)
   }
 
 So if there is no driver, this part of the code is never called.

This can trigger exceptions, which can be used to load the GPIO driver.
This seems to be another topic.

Thanks and best regards
-Lv
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 0/4] ACPICA: Stable material of ACPI executer fixes for linux-3.8.

2013-11-24 Thread Zheng, Lv

 From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
 Sent: Sunday, November 24, 2013 11:22 AM

 On Fri, Nov 01, 2013 at 02:58:16AM +, Zheng, Lv wrote:
   From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
   Sent: Thursday, October 31, 2013 8:22 PM

   On Thursday, October 31, 2013 05:08:50 AM Greg Kroah-Hartman wrote:
On Thu, Oct 31, 2013 at 12:39:21PM +0100, Rafael J. Wysocki wrote:
 On Thursday, October 31, 2013 09:07:40 AM Lv Zheng wrote:
  There are bug-fixes for AML interpreter upstreamed, fixing some 
  serious
  issues found in recent platforms.  These fixes make Linux AML 
  interpreter
  more ACPI 2.0 ASL concept compliant.  Further AML interpreter fixes 
  should
  be based on such improvements, thus they are good materials for 
  stable.

  This patch set can be safely applied to linux-3.8:
  commit 19f949f52599ba7c3f67a5897ac6be14bfcb1200 upstream.

  The patch set has passed build/boot tests on the following machines:
Dell Inspiron Mini 1010 (i386)
HP Compaq 8200 Elite SFF PC (x86-64)

  Bob Moore (4):
ACPICA: Interpreter: Fix Store() when implicit conversion is not
  possible.
ACPICA: DeRefOf operator: Update to fully resolve FieldUnit and
  BufferField refs.
ACPICA: Return error if DerefOf resolves to a null package 
  element.
ACPICA: Fix for a Store-ArgX when ArgX contains a reference to a
  field.

 Hi Greg,

 Please take patches [1-4/4] for stable.

Which stable tree?

I don't do 3.8, it's long been end-of-life, although one company is
trying to keep it alive, but that's not me.

I'm only handling 3.4, 3.10, and 3.11 stable trees right now, which
one(s) should these be applied to?

   3.10.x and 3.11.x then.

   Lv, do the original mainline commits apply to these kernels?

   Rafael

  Hi, Rafael and Greg

  I checked the back port dependencies since v3.8:
  1. [PATCH 1] belongs to v3.9.
  2. [PATCH 4] includes an empty line belonging to a coding style fix 
  affecting this series (between [PATCH 3] and [PATCH 4]).
  Thus,
  1. For v3.10:
  [PATCH 1]: It's already in the repo, so please drop it.
  [PATCH 2-4]: They can be used directly as 3.10.x stable materials.
  2. For v3.11:
  [PATCH 1]: It's already in the repo, so please drop it.
  [PATCH 2-3]: They can be used directly as 3.11.x stable materials.
  [PATCH 4]: The original commit from Linus' tree should be used instead.

   I checked the commit log since v3.4.
   There is no functional change done to the AML executer between v3.4 and 
  v3.8.
   The problem is there is a coding style fix affecting this series (between 
  v3.4 and [PATCH 1]).
   I generated the following diff block before applying [PATCH 1], and 
  obtained a successful build/boot to a v3.4 kernel with these
 patches applied.
  Thus,
  1. For v3.4:
  [PATCH 1]: You can merge this diff block to [PATCH 1] or simply modify 
  the [PATCH 1] by manually adding this white space.
  [PATCH 2-4]: They can be used directly as 3.4.x stable materials.

 Ok, I think I have this all properly queued up for 3.4, 3.10, and
 3.11-stable trees, can you please check and verify I didn't mess
 anything up?

I pulled and checked, they are all correct.

Thanks and best regards
-Lv

 thanks,

 greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/4] ACPI: Cleanup acpi/acpi_bus.h and acpi/acpi_drivers.h inclusions.

2013-11-25 Thread Zheng, Lv

Hi, Rafael

Thanks for commenting.

 From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
 Sent: Tuesday, November 26, 2013 8:06 AM

 On Saturday, November 23, 2013 07:29:08 AM Lv Zheng wrote:
  From: Lv Zheng lv.zh...@intel.com

  This patch enfoces linux/acpi.h inclusion instead of direct
  acpi/acpi_drivers.h and acpi/acpi_bus.h inclusions.

 First off, please make the changelog shorter.  Here's my version:

 Replace direct inclusions of acpi/acpi_drivers.h and acpi/acpi_bus.h,
 which are incorrect, with linux/acpi.h inclusions.

 First of all, acpi/acpi_drivers.h and acpi/acpi_bus.h should not
 be included directly from any files that are built for CONFIG_ACPI
 unset, because that generally leads to build warnings about undefined
 symbols in !CONFIG_ACPI builds.  For CONFIG_ACPI set linux/acpi.h
 includes those files and for CONFIG_ACPI it provides stub ACPI
 symbols to be used in that case.

 Second, there are ordering dependencies between those files that
 always have to be met.  Namely, it is required that acpi/acpi_bus.h
 be included prior to acpi/acpi_drivers.h so that the acpi_pci_root
 declarations the latter depends on are always there.  That also is
 taken care of including linux/acpi.h as appropriate.

OK.

 Second, please fold [3/4] into this one.  Keeping them separate is
 artificial and pointless in my opinion.  [Of course, the changelog will
 have to be modified then.]

OK.

 [...]

 Apart from the above -

  Signed-off-by: Lv Zheng lv.zh...@intel.com
  ---
[...]

  diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
  index ba5b56d..1bd3de4 100644
  --- a/drivers/acpi/ec.c
  +++ b/drivers/acpi/ec.c
  @@ -40,8 +40,7 @@
   #include linux/spinlock.h
   #include linux/slab.h
   #include asm/io.h
  -#include acpi/acpi_bus.h
  -#include acpi/acpi_drivers.h
  +#include linux/acpi.h

 - add the new #include above the asm/io.h maybe?

The patch was generated for people who even don't have too many ACPI header 
knowledge can review and determine that the modifications will not trigger 
regressions.
So it doesn't cleanup other existing inclusion issues for the affected files, 
for example:
1. empty lines.
2. asm files
3. other acpi inclusions

Do you mean I need to cleanup whole file other than what the patch description 
claimed to clean?
If so, I'll follow this comment and all the following ones to refresh the patch.

   #include linux/dmi.h

   #include internal.h

[...]

  diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
  index 54a20ff..8507034 100644
  --- a/drivers/acpi/osl.c
  +++ b/drivers/acpi/osl.c
  @@ -50,7 +50,6 @@
   #include asm/uaccess.h

   #include acpi/acpi.h
  -#include acpi/acpi_bus.h
   #include acpi/processor.h

 - I guess all of these acpi/... inclusions may go away from here?

   #include internal.h

[...]

  diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
  index 2652a61..cc5feb9 100644
  --- a/drivers/acpi/pci_link.c
  +++ b/drivers/acpi/pci_link.c
  @@ -40,8 +40,7 @@
   #include linux/mutex.h
   #include linux/slab.h

 - Remove the empty line too.

  -#include acpi/acpi_bus.h
  -#include acpi/acpi_drivers.h
  +#include linux/acpi.h

   #define PREFIX ACPI: 

  diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
  index 20360e4..dadd0f50 100644
  --- a/drivers/acpi/pci_root.c
  +++ b/drivers/acpi/pci_root.c
  @@ -35,8 +35,6 @@
   #include linux/pci-aspm.h
   #include linux/acpi.h
   #include linux/slab.h
  -#include acpi/acpi_bus.h
  -#include acpi/acpi_drivers.h
   #include acpi/apei.h

 Does acpi/apei.h have to be included directly here?  If not, please remove
 it too.

   #include internal.h

[...]

  diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
  index 6a5b152..56d30fe 100644
  --- a/drivers/acpi/proc.c
  +++ b/drivers/acpi/proc.c
  @@ -5,8 +5,7 @@
   #include linux/bcd.h
   #include asm/uaccess.h

  -#include acpi/acpi_bus.h
  -#include acpi/acpi_drivers.h
  +#include linux/acpi.h

 - Put the new line above the asm/uaccess.h

   #include sleep.h

  diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
  index b3171f3..ad88410 100644
  --- a/drivers/acpi/processor_core.c
  +++ b/drivers/acpi/processor_core.c
  @@ -11,7 +11,7 @@
   #include linux/dmi.h
   #include linux/slab.h

  -#include acpi/acpi_drivers.h
  +#include linux/acpi.h
   #include acpi/processor.h

 - Remove the useless empty line and processor.h inclusion too.

   #include internal.h
  diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
  index 644516d..a1087d8 100644
  --- a/drivers/acpi/processor_idle.c
  +++ b/drivers/acpi/processor_idle.c
  @@ -46,7 +46,6 @@
   #include asm/apic.h
   #endif

  -#include acpi/acpi_bus.h
   #include acpi/processor.h

 Is the processor.h inclusion necessary here?

   #define PREFIX ACPI: 
  diff --git a/drivers/acpi/processor_perflib.c 
  b/drivers/acpi/processor_perflib.c
  index 60a7c28..df63370 100644
  ---

RE: [UPDATE PATCH 4/4] ACPI: Add support to force header inclusion rules for linux/acpi.h.

2013-11-25 Thread Zheng, Lv

Hi, Rafael

 From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
 Sent: Tuesday, November 26, 2013 8:10 AM
 To: Lv Zheng
 Cc: Wysocki, Rafael J; Brown, Len; Zheng, Lv; linux-kernel@vger.kernel.org; 
 linux-a...@vger.kernel.org
 Subject: Re: [UPDATE PATCH 4/4] ACPI: Add support to force header inclusion 
 rules for linux/acpi.h.

 On Saturday, November 23, 2013 07:54:18 AM Lv Zheng wrote:
  From: Lv Zheng lv.zh...@intel.com

  (Update due to some typo fixes.)

  As there is only CONFIG_ACPI=n processing in the linux/acpi.h, it is not
  safe to include acpi/acpi.h, acpi/acpi_drivers.h and acpi/acpi_bus.h
  directly for source out of Linux ACPI subsystems.

  This patch adds error messaging to warn developers of such wrong
  inclusions.

  In order not to be bisected and reverted as a wrong commit, warning
  messages are carefully split into a seperate patch other than the wrong
  inclusion cleanups.

  Signed-off-by: Lv Zheng lv.zh...@intel.com

 I don't like this one to be honest.

I generate this to prevent device driver writers to do stupid things before we 
can sort acpi/acpi_bus.h and acpi/acpi_drivers.h out.

  ---
   drivers/acpi/acpica/Makefile|2 +-
   include/acpi/acpi_bus.h |5 +
   include/acpi/acpi_drivers.h |5 +
   include/acpi/platform/aclinux.h |   10 ++
   4 files changed, 21 insertions(+), 1 deletion(-)

  diff --git a/drivers/acpi/acpica/Makefile b/drivers/acpi/acpica/Makefile
  index 4383040..7738677 100644
  --- a/drivers/acpi/acpica/Makefile
  +++ b/drivers/acpi/acpica/Makefile
  @@ -2,7 +2,7 @@
   # Makefile for ACPICA Core interpreter
   #

  -ccflags-y  := -Os
  +ccflags-y  := -Os -DLINUXIZED_ACPICA

 I don't like this (the naming and the way it is done).

   ccflags-$(CONFIG_ACPI_DEBUG)   += -DACPI_DEBUG_OUTPUT

   # use acpi.o to put all files here into acpi.o modparam namespace
  diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
  index 0af9667..0b1ea80 100644
  --- a/include/acpi/acpi_bus.h
  +++ b/include/acpi/acpi_bus.h
  @@ -26,6 +26,11 @@
   #ifndef __ACPI_BUS_H__
   #define __ACPI_BUS_H__

  +/* acpi/acpi_bus.h is not safe for CONFIG_ACPI=n environment */
  +#ifndef _LINUX_ACPI_H
  +#error Please don't include acpi/acpi_bus.h directly, include 
  linux/acpi.h instead.
  +#endif

 And this should either go into all of the acpi/*.h files, not only to the 
 two
 of them you chose, or to none of them.  I prefer none.

OK, let it be done for now.  So we need to open eyes on new ACPI commits. :-)

I'll merge patch 2 and 3 and send it with your comments addressed and re-based.

Thanks and best regards
-Lv

 Thanks,
 Rafael

RE: [PATCH v2] ACPI: Cleanup acpi/acpi.h, acpi/acpi_bus.h and acpi/acpi_drivers.h inclusions.

2013-11-27 Thread Zheng, Lv

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki
 Sent: Wednesday, November 27, 2013 8:29 AM
 
 On Tuesday, November 26, 2013 03:29:05 PM Konrad Rzeszutek Wilk wrote:
  On Tue, Nov 26, 2013 at 09:29:33PM +0100, Rafael J. Wysocki wrote:
   On Tuesday, November 26, 2013 01:21:15 PM Lv Zheng wrote:
Replace direct inclusions of acpi/acpi.h, acpi/acpi_bus.h and
acpi/acpi_drivers.h, which are incorrect, with linux/acpi.h 
inclusions.
   
First of all, acpi/acpi.h, acpi/acpi_bus.h and acpi/acpi_drivers.h
should not be included directly from any files that are built for
CONFIG_ACPI unset, because that generally leads to build warnings about
undefined symbols in !CONFIG_ACPI builds.  For CONFIG_ACPI set,
linux/acpi.h includes those files and for !CONFIG_ACPI it provides 
stub
ACPI symbols to be used in that case.
   
Second, there are ordering dependencies between those files that always
have to be met.  Namely, it is required that acpi/acpi_bus.h be 
included
prior to acpi/acpi_drivers.h so that the acpi_pci_root declarations 
the
latter depends on are always there.  And acpi/acpi.h which provides
basic ACPICA type declarations should always be included prior to any 
other
ACPI headers in CONFIG_ACPI builds.  That also is taken care of 
including
linux/acpi.h as appropriate.
   
This patch also includes necessary cleanups in the affected files where
other ACPI headers is also included but not referenced.
   
  
   This looks OK to me, but it touches several other subsystems.  It's 
   better to
   CC such things to linux-kernel at least.
  
   Peter, Matthew, Tony, Konrad, Greg, Bjorn, do you have any objections 
   against this?
 
  CC-ing Boris and David here.
 
  I presume the proper compilation tests to make sure they do compile properly
  has been done.
 
 Yes and we're going to run it through the auto build machinery anyway.

Hi, Rafael

The revision 1 is written in the style that it wouldn't introduce regressions 
as:
acpi/acpi.h acpi/acpi_bus.h acpi/acpi_drivers.h is replaced by 
linux/acpi.h.  If there are 2 or more linux/acpi.h inclusions in the same 
files, then the latter ones are deleted.
And it gets building/booting tested by enabling the affected modules for 
testing on x86 and x86_64.
The checker enabled tests only showed line number differences.
For special IA64 only modules, they are tested using make dirname of 
file/basename of file.o with necessary -D passed through command line.

I the revision 2, there are asm/xxx.h order changes, it thus may introduce 
regressions if there are tricks we don't know better than the original authors.
My build tests still showed no issues, but we could feed them a coverage test.
Rafael, hope you can help me do this in your bleeding-edge branch and feel free 
to modify the commit to fix the issues caused by such tricks.

Thanks and best regards
-Lv

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

RE: [RFC part1 PATCH 5/7] ARM64 / ACPI: Introduce arm_core.c and its related head file

2013-12-03 Thread Zheng, Lv

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Hanjun Guo
 Sent: Wednesday, December 04, 2013 12:37 AM
 
 introduce arm_core.c and its related head file, after this patch,
 we can get ACPI tables from BIOS on ARM64 now.
 
 Signed-off-by: Al Stone al.st...@linaro.org
 Signed-off-by: Graeme Gregory graeme.greg...@linaro.org
 Signed-off-by: Hanjun Guo hanjun@linaro.org
 ---
  arch/arm64/include/asm/acpi.h |   57 +++
  arch/arm64/kernel/setup.c |8 ++
  drivers/acpi/Makefile |2 +
  drivers/acpi/plat/Makefile|1 +
  drivers/acpi/plat/arm-core.c  |  219 
 +
  5 files changed, 287 insertions(+)
  create mode 100644 drivers/acpi/plat/Makefile
  create mode 100644 drivers/acpi/plat/arm-core.c
 
 diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
 index c186f5b..e9444e4 100644
 --- a/arch/arm64/include/asm/acpi.h
 +++ b/arch/arm64/include/asm/acpi.h
 @@ -19,6 +19,43 @@
  #ifndef _ASM_ARM_ACPI_H
  #define _ASM_ARM_ACPI_H
 
 +#include asm/cacheflush.h
 +
 +#include linux/init.h
 +
 +#define COMPILER_DEPENDENT_INT64 long long
 +#define COMPILER_DEPENDENT_UINT64unsigned long long
 +
 +/*
 + * Calling conventions:
 + *
 + * ACPI_SYSTEM_XFACE- Interfaces to host OS (handlers, threads)
 + * ACPI_EXTERNAL_XFACE  - External ACPI interfaces
 + * ACPI_INTERNAL_XFACE  - Internal ACPI interfaces
 + * ACPI_INTERNAL_VAR_XFACE  - Internal variable-parameter list interfaces
 + */
 +#define ACPI_SYSTEM_XFACE
 +#define ACPI_EXTERNAL_XFACE
 +#define ACPI_INTERNAL_XFACE
 +#define ACPI_INTERNAL_VAR_XFACE
 +
 +/* Asm macros */
 +#define ACPI_FLUSH_CPU_CACHE() flush_cache_all()

Well, you may need to check the following environments defined in 
acpi/platform/aclinux.h is sufficient for ARM targets:
49 #define ACPI_USE_SYSTEM_CLIBRARY
50 #define ACPI_USE_DO_WHILE_0
51 #define ACPI_MUTEX_TYPE ACPI_BINARY_SEMAPHORE

70 #define ACPI_MACHINE_WIDTH  BITS_PER_LONG
Will this zap IO addresses on ARM32 platforms?

And following default settings in acpi/platform/acenv.h and acpi/acxxx.h is 
sufficient for ARM targets:
179 #if defined (__IA64__) || defined (__ia64__)
180 #define ACPI_MISALIGNMENT_NOT_SUPPORTED
181 #endif
Will this cause any exceptions on ARM by executing ACPICA name functions?

444 #if ACPI_MACHINE_WIDTH == 64
445 #define ACPI_USE_NATIVE_DIVIDE  /* Use compiler native 64-bit divide */
446 #endif
I think you may see build breakage on ARM32 as you haven't implemented the 
following ACPICA macros for ARM:
67 #define ACPI_DIV_64_BY_32(n_hi, n_lo, d32, q32, r32)
74 #define ACPI_SHIFT_RIGHT_64(n_hi, n_lo)
Have you tested this yet?

I'm not sure if all global lock code blocks are not referenced by 
ACPI_REDUCED_HARDWARE and I'm not sure what mechanism is implemented on ARM 
ACPI platforms to offer the synchronization mechanism between firmware and 
OSPM.  So you may need to implement the following synchronization protocol in 
asm/acpi.h:
58 #define ACPI_ACQUIRE_GLOBAL_LOCK(facs, Acq)
61 #define ACPI_RELEASE_GLOBAL_LOCK(facs, Acq)

I only reviewed the ACPICA stuffs in asm/acpi.h, I didn't take a look at your 
Linux ACPI stuff in asm/acpi.h.  You may need more instructions on the 
porting issues from Linux ACPI guys.

Thanks and best regards
-Lv

 +
 +/* Basic configuration for ACPI */
 +#ifdef   CONFIG_ACPI
 +extern int acpi_disabled;
 +extern int acpi_noirq;
 +extern int acpi_pci_disabled;
 +extern int acpi_strict;
 +
 +static inline void disable_acpi(void)
 +{
 + acpi_disabled = 1;
 + acpi_pci_disabled = 1;
 + acpi_noirq = 1;
 +}
 +
  static inline bool arch_has_acpi_pdc(void)
  {
   return false;   /* always false for now */
 @@ -29,4 +66,24 @@ static inline void arch_acpi_set_pdc_bits(u32 *buf)
   return;
  }
 
 +static inline void acpi_noirq_set(void) { acpi_noirq = 1; }
 +static inline void acpi_disable_pci(void)
 +{
 + acpi_pci_disabled = 1;
 + acpi_noirq_set();
 +}
 +
 +/* FIXME: this function should be moved to topology.h when it's ready */
 +void arch_fix_phys_package_id(int num, u32 slot);
 +
 +/* temperally define -1 to make acpi core compilerable */
 +#define cpu_physical_id(cpu) -1
 +
 +#else/* !CONFIG_ACPI */
 +#define acpi_disabled 1  /* ACPI sometimes enabled on ARM */
 +#define acpi_noirq 1 /* ACPI sometimes enabled on ARM */
 +#define acpi_pci_disabled 1  /* ACPI PCI sometimes enabled on ARM */
 +#define acpi_strict 1/* no ACPI spec workarounds on ARM */
 +#endif
 +
  #endif /*_ASM_ARM_ACPI_H*/
 diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
 index bd9bbd0..8199360 100644
 --- a/arch/arm64/kernel/setup.c
 +++ b/arch/arm64/kernel/setup.c
 @@ -41,6 +41,7 @@
  #include linux/memblock.h
  #include linux/of_fdt.h
  #include linux/of_platform.h
 +#include linux/acpi.h
 
  #include asm/cputype.h
  #include asm/elf.h
 @@ -225,6 +226,13 @@ void

RE: [PATCH] SFI: fix compilation warnings

2013-12-05 Thread Zheng, Lv

 From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
 Sent: Friday, December 06, 2013 6:37 AM
 
 On Thursday, December 05, 2013 07:26:37 PM Andy Shevchenko wrote:
  When build kernel with make W=1 we get the following compiler error.
 
  In file included from drivers/sfi/sfi_acpi.c:66:0:
  include/linux/sfi_acpi.h: In function ‘acpi_sfi_table_parse’:
  include/linux/sfi_acpi.h:72:2: error: implicit declaration of function 
  ‘acpi_table_parse’ [-Werror=implicit-function-declaration]
if (!acpi_table_parse(signature, handler))
^
cc1: some warnings being treated as errors
 
  The patch changes acpi/acpi.h inclusion to linux/acpi.h.
 
  Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com
 
 OK
 
 Lv, I'm dropping your patch cleaning up the inclusions, please sort that thing
 out entirely for the next cycle.

No, I think Andy's patch is wrong.
This warning is existing before merging my fixes.

Either we fix this warning or leave it as is.

To fix it, you need to add 1 line in sfi_acpi.h rather than change the line.
Let me post a fix for this.

Thanks
-Lv

 
 Thanks!
 
  ---
   drivers/sfi/sfi_acpi.c   | 2 +-
   include/linux/sfi_acpi.h | 2 +-
   2 files changed, 2 insertions(+), 2 deletions(-)
 
  diff --git a/drivers/sfi/sfi_acpi.c b/drivers/sfi/sfi_acpi.c
  index 0c9f419..d4dfcbe 100644
  --- a/drivers/sfi/sfi_acpi.c
  +++ b/drivers/sfi/sfi_acpi.c
  @@ -60,7 +60,7 @@
   #define pr_fmt(fmt) KMSG_COMPONENT :  fmt
 
   #include linux/kernel.h
  -#include acpi/acpi.h /* FIXME: inclusion should be removed */
  +#include linux/acpi.h
 
   #include linux/sfi.h
   #include linux/sfi_acpi.h
  diff --git a/include/linux/sfi_acpi.h b/include/linux/sfi_acpi.h
  index 2cfcb79..c9cab82 100644
  --- a/include/linux/sfi_acpi.h
  +++ b/include/linux/sfi_acpi.h
  @@ -60,7 +60,7 @@
   #define _LINUX_SFI_ACPI_H
 
   #ifdef CONFIG_SFI
  -#include acpi/acpi.h /* FIXME: inclusion should be removed */
  +#include linux/acpi.h
 
   extern int sfi_acpi_table_parse(char *signature, char *oem_id,
  char *oem_table_id,
 
 --
 I speak only for myself.
 Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH] SFI: fix compilation warnings

2013-12-05 Thread Zheng, Lv

 From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
 Sent: Friday, December 06, 2013 9:26 AM

 On Friday, December 06, 2013 01:08:45 AM Zheng, Lv wrote:
   From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
   Sent: Friday, December 06, 2013 6:37 AM

   On Thursday, December 05, 2013 07:26:37 PM Andy Shevchenko wrote:
When build kernel with make W=1 we get the following compiler error.

In file included from drivers/sfi/sfi_acpi.c:66:0:
include/linux/sfi_acpi.h: In function ‘acpi_sfi_table_parse’:
include/linux/sfi_acpi.h:72:2: error: implicit declaration of function 
‘acpi_table_parse’ [-Werror=implicit-function-declaration]
  if (!acpi_table_parse(signature, handler))
  ^
  cc1: some warnings being treated as errors

The patch changes acpi/acpi.h inclusion to linux/acpi.h.

Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com

   OK

   Lv, I'm dropping your patch cleaning up the inclusions, please sort that 
   thing
   out entirely for the next cycle.

  No, I think Andy's patch is wrong.

 His patch may be wrong, but the warning in his changelog surely is real. :-)

  This warning is existing before merging my fixes.

  Either we fix this warning or leave it as is.

  To fix it, you need to add 1 line in sfi_acpi.h rather than change the line.
  Let me post a fix for this.

 Please do.  And I'm putting all ACPI header cleanups on hold until all 
 warnings
 of this kind have been fixed by them.  Otherwise doing those cleanups makes a
 little sense in my opinion.

OK, not only this warning, there is another warning in sfi_acpi.c.
I doubt whether they are strictly tested before upstreamed...
They are all existing warnings.
You can apply my new patch before the cleanup series.

Thanks
-Lv

 Thanks,
 Rafael

N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH] SFI: fix compilation warnings

2013-12-05 Thread Zheng, Lv

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki
 Sent: Friday, December 06, 2013 10:01 AM
 To: Zheng, Lv
 Cc: Andy Shevchenko; Wysocki, Rafael J; linux-a...@vger.kernel.org; LKML
 Subject: Re: [PATCH] SFI: fix compilation warnings

 On Friday, December 06, 2013 01:26:07 AM Zheng, Lv wrote:
   From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
   Sent: Friday, December 06, 2013 9:26 AM

   On Friday, December 06, 2013 01:08:45 AM Zheng, Lv wrote:
 From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
 Sent: Friday, December 06, 2013 6:37 AM

 On Thursday, December 05, 2013 07:26:37 PM Andy Shevchenko wrote:
  When build kernel with make W=1 we get the following compiler error.

  In file included from drivers/sfi/sfi_acpi.c:66:0:
  include/linux/sfi_acpi.h: In function ‘acpi_sfi_table_parse’:
  include/linux/sfi_acpi.h:72:2: error: implicit declaration of 
  function ‘acpi_table_parse’ [-Werror=implicit-function-declaration]
if (!acpi_table_parse(signature, handler))
^
cc1: some warnings being treated as errors

  The patch changes acpi/acpi.h inclusion to linux/acpi.h.

  Signed-off-by: Andy Shevchenko andriy.shevche...@linux.intel.com

 OK

 Lv, I'm dropping your patch cleaning up the inclusions, please sort 
 that thing
 out entirely for the next cycle.

No, I think Andy's patch is wrong.

   His patch may be wrong, but the warning in his changelog surely is real. 
   :-)

This warning is existing before merging my fixes.

Either we fix this warning or leave it as is.

To fix it, you need to add 1 line in sfi_acpi.h rather than change the 
line.
Let me post a fix for this.

   Please do.  And I'm putting all ACPI header cleanups on hold until all 
   warnings
   of this kind have been fixed by them.  Otherwise doing those cleanups 
   makes a
   little sense in my opinion.

  OK, not only this warning, there is another warning in sfi_acpi.c.
  I doubt whether they are strictly tested before upstreamed...
  They are all existing warnings.
  You can apply my new patch before the cleanup series.

 Sorry, I'm already lost in the thick of patches. :-)

 Please resend them all as a single series.

OK, I'll sort them again.

Thanks
-Lv

 Thanks,
 Rafael

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

RE: ACPICA 20140214 auto-serialize weirds my machine...

2014-03-16 Thread Zheng, Lv

Hi,

 From: Valdis Kletnieks [mailto:valdis.kletni...@vt.edu]
 Sent: Saturday, March 15, 2014 12:05 AM
 
 Surprisingly enough, this hasn't seemed to have bitten many Linux people yet,
 Google only finds a BSD thread (where the same ACPICA code is in use):
 
 http://comments.gmane.org/gmane.os.dragonfly-bsd.user/1817
 
 I found this, but haven't tried it yet:
 
 commit 5f1cb4a92e4c4aabd139ff9ca1e11c0e2db2ac59
 Author: Lv Zheng lv.zh...@intel.com
 Date:   Wed Feb 26 10:32:01 2014 +0800
 
 ACPICA: Remove global option to serialize all control methods.
 
 With the addition of the auto-serialization feature, this option
 is obsolete and is completely removed. Lv Zheng.

This is not the right commit that is related to your issue.
This commit only removes the old facility, it is harmless.
Actually the facility is enabled by default in this commit:
https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-nextid=cd52379678785b02d7a357988cfba214fdaf92f4

commit cd52379678785b02d7a357988cfba214fdaf92f4
Subject: ACPICA: Add global option to disable method auto-serialization.

This change adds an option to disable the auto-serialization of
methods that create named objects.

And the commit you are talking about just re-used the kernel parameter in order 
to add an option to disable the new facility.

 
 This patch also updates acpi_serialize kernel parameter using the
 new auto-serialization feature - acpi_gbl_auto_serialize_methods.
 
 This looks like a user nightmare waiting to happen, because:
 
 - acpi_serialize  [HW,ACPI] force serialization of AML methods
 + acpi_serialize= [HW,ACPI] configure auto-serialization of AML methods
 
 Re-using a kernel parameter name with different semantics is just asking for 
 trouble.
 
 I get this in my dmesg, and then I end up with all sorts of ACPI-related
 weirdness - everything from 4 or 5 temperature sensors evaporating to it
 claiming that my laptop doesn't have a battery bay (it may also be related to
 an issue I'm seeing with multiple graphics monitors as well, but I haven't
 verified that yet).
 
 We're going to need a better way to deal with this before it gets to
 mainline
 
 [0.119344] ACPI: Executed 1 blocks of module-level executable AML code
 [0.135282] ACPI Error: Cannot acquire Mutex for method [SNVC], current 
 SyncLevel is too large (1) (20140214/dsmethod-362)
 [0.135288] ACPI Error: Method parse/execution failed [\SMBF] (Node 
 88012d026ec0), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.135298] ACPI Error: Method parse/execution failed [\GENS] (Node 
 88012d026e70), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.135307] ACPI Error: Method parse/execution failed [\EISC] (Node 
 88012d02d560), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.135320] ACPI Error: Method parse/execution failed 
 [\_SB_.PCI0.LPCB.ECDV.ECR1] (Node 88012d02d470),
 AE_AML_MUTEX_ORDER (20140214/psparse-536)
 [0.135328] ACPI Error: Method parse/execution failed [\ECRB] (Node 
 88012d02d5d8), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.135336] ACPI Error: Method parse/execution failed [\ECG5] (Node 
 88012d02d740), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.135343] ACPI Error: Method parse/execution failed 
 [\_SB_.PCI0.LPCB.EINI] (Node 88012d02db00), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.135351] ACPI Error: Method parse/execution failed [\EV3_] (Node 
 88012d0314c0), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.135359] ACPI Error: Method parse/execution failed [\_SB_.PCI0._INI] 
 (Node 88012d026920), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.135574] [Firmware Bug]: ACPI: BIOS _OSI(Linux) query ignored
 [0.136283] ACPI: SSDT 0xCE789598 000523 (v01 PmRef  Cpu0Cst  
 3001 INTL 20051117)
 [0.138312] ACPI: Dynamic OEM Table Load:
 [0.138316] ACPI: SSDT 0x 000523 (v01 PmRef  Cpu0Cst  
 3001 INTL 20051117)
 [0.141483] ACPI: SSDT 0xCE78AA98 000303 (v01 PmRef  ApIst
 3000 INTL 20051117)
 [0.143683] ACPI: Dynamic OEM Table Load:
 [0.143687] ACPI: SSDT 0x 000303 (v01 PmRef  ApIst
 3000 INTL 20051117)
 [0.146215] ACPI: SSDT 0xCE790C18 000119 (v01 PmRef  ApCst
 3000 INTL 20051117)
 [0.148279] ACPI: Dynamic OEM Table Load:
 [0.148282] ACPI: SSDT 0x 000119 (v01 PmRef  ApCst
 3000 INTL 20051117)
 [0.152678] ACPI Error: Cannot acquire Mutex for method [SNVC], current 
 SyncLevel is too large (1) (20140214/dsmethod-362)
 [0.152684] ACPI Error: Method parse/execution failed [\SMBF] (Node 
 88012d026ec0), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.152693] ACPI Error: Method parse/execution failed [\GENS] (Node 
 88012d026e70), AE_AML_MUTEX_ORDER
 (20140214/psparse-536)
 [0.152702] ACPI Error: Method parse/execution failed [\EISC] (Node 
 88012d02d560), AE_AML_MUTEX_ORDER

RE: [RFC PATCH 2/2] ACPI: Default disable auto-serialization.

2014-03-17 Thread Zheng, Lv

Hi, Rafael

 From: Wysocki, Rafael J
 Sent: Monday, March 17, 2014 8:09 PM

 On 3/17/2014 5:14 AM, Lv Zheng wrote:
  This feature enabled by the following commit is still under development.

 Commit: cd52379678785b02d7a357988cfba214fdaf92f4
 Subject: ACPICA: Add global option to disable method auto-serialization.
 This change adds an option to disable the auto-serialization of
 methods that create named objects.

  This patch disables it by default temporarily according to the bug reports.

 Well, it doesn't restore the 3.14 behavior of acpi_auto_serialize, as
 far as I can say, so we can't apply it.

 I still can drop the commits related to auto-serialization from my
 acpica branch.  Which commits are they?

From linux-pm/bleeding-edge branch, I found this series includes the following 
commits:

Commit: a0fd108f2da56b769472fc0095616fb370988516
Author: Lv Zheng lv.zh...@intel.com
Subject: ACPI: Rename kernel parameter acpi_serialize to 
acpi_no_auto_serialize

Commit: 5f1cb4a92e4c4aabd139ff9ca1e11c0e2db2ac59
Author: Lv Zheng lv.zh...@intel.com
Subject: ACPICA: Remove global option to serialize all control methods.

Commit: cd52379678785b02d7a357988cfba214fdaf92f4
Author: Bob Moore robert.mo...@intel.com
Subject: ACPICA: Add global option to disable method auto-serialization.

Commit: f56b05bd111b01141ef74568dc4d262c70295d03
Author: Lv Zheng lv.zh...@intel.com
Subject: ACPICA: Add additional named objects for the auto-serialize method 
scan.

Commit: 27f1899350a1d6baab117c03c87f0e37730047bd
Author: Bob Moore robert.mo...@intel.com
Subject: ACPICA: Add auto-serialization support for ill-behaved control methods.

Reverting these commits can restore original Linux behavior.
Please check.

Thanks and best regards
-Lv

  References: http://www.spinics.net/lists/linux-acpi/msg49496.html
  Reported-by: Valdis Kletnieks validis.kletni...@vt.edu
  Reported-by: Sabrina Dubroka s...@queasysnail.net
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  ---
Documentation/kernel-parameters.txt |8 
drivers/acpi/acpica/acglobal.h  |2 +-
drivers/acpi/osl.c  |   12 ++--
3 files changed, 11 insertions(+), 11 deletions(-)

  diff --git a/Documentation/kernel-parameters.txt 
  b/Documentation/kernel-parameters.txt
  index 91f0be8..a159537 100644
  --- a/Documentation/kernel-parameters.txt
  +++ b/Documentation/kernel-parameters.txt
  @@ -229,13 +229,13 @@ bytes respectively. Such letter suffixes can also be 
  entirely omitted.
  use by PCI
  Format: irq,irq...

  -   acpi_no_auto_serialize  [HW,ACPI]
  -   Disable auto-serialization of AML methods
  +   acpi_auto_serialize [HW,ACPI]
  +   Enable auto-serialization of AML methods
  AML control methods that contain the opcodes to create
  named objects will be marked as Serialized by the
  auto-serialization feature.
  -   This feature is enabled by default.
  -   This option allows to turn off the feature.
  +   This feature is disabled by default.
  +   This option allows to turn on the feature.

  acpi_no_auto_ssdt   [HW,ACPI] Disable automatic loading of SSDT

  diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h
  index 49bbc71..ea0f838 100644
  --- a/drivers/acpi/acpica/acglobal.h
  +++ b/drivers/acpi/acpica/acglobal.h
  @@ -99,7 +99,7 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_enable_interpreter_slack, 
  FALSE);
 * that create named objects are marked Serialized in order to prevent
 * possible run-time problems if they are entered by more than one thread.
 */
  -ACPI_INIT_GLOBAL(u8, acpi_gbl_auto_serialize_methods, TRUE);
  +ACPI_INIT_GLOBAL(u8, acpi_gbl_auto_serialize_methods, FALSE);

/*
 * Create the predefined _OSI method in the namespace? Default is TRUE
  diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
  index d40d6dc..928f0c2 100644
  --- a/drivers/acpi/osl.c
  +++ b/drivers/acpi/osl.c
  @@ -1538,20 +1538,20 @@ static int __init osi_setup(char *str)
__setup(acpi_osi=, osi_setup);

/*
  - * Disable the auto-serialization of named objects creation methods.
  + * Enable the auto-serialization of named objects creation methods.
 *
  - * This feature is enabled by default.  It marks the AML control methods
  + * This feature is disabled by default.  It marks the AML control methods
 * that contain the opcodes to create named objects as Serialized.
 */
  -static int __init acpi_no_auto_serialize_setup(char *str)
  +static int __init acpi_auto_serialize_setup(char *str)
{
  -   acpi_gbl_auto_serialize_methods = FALSE;
  -   pr_info(ACPI: auto-serialization disabled\n);
  +   acpi_gbl_auto_serialize_methods = TRUE;
  +   pr_info(ACPI: auto-serialization enabled\n);

  return 1;
}

RE: [RFC PATCH 1/2] ACPICA: Dispatcher: Ignore SyncLevel for auto-serialization mechanism.

2014-03-17 Thread Zheng, Lv

Hi, Sabrina and Valdis

Could you give this patch a try to see if this patch is working?

Thanks and best regards
-Lv

 From: Zheng, Lv
 Sent: Monday, March 17, 2014 12:15 PM
 
 From: Robert Moore robert.mo...@intel.com
 
 It is reported that the auto-serialization mechanism has broken some
 machine.  This patch fixes the issues by igoring the SyncLevel
 attribute of the marked Method.
 
 References: http://www.spinics.net/lists/linux-acpi/msg49496.html
 Reported-by: Valdis Kletnieks validis.kletni...@vt.edu
 Reported-by: Sabrina Dubroka s...@queasysnail.net
 Signed-off-by: Bob Moore robert.mo...@intel.com
 Signed-off-by: Lv Zheng lv.zh...@intel.com
 ---
  drivers/acpi/acpica/acobject.h |3 ++-
  drivers/acpi/acpica/dsmethod.c |   24 +++-
  2 files changed, 21 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h
 index 1a4d618..22fb644 100644
 --- a/drivers/acpi/acpica/acobject.h
 +++ b/drivers/acpi/acpica/acobject.h
 @@ -193,7 +193,8 @@ struct acpi_object_method {
  #define ACPI_METHOD_INTERNAL_ONLY   0x02 /* Method is implemented 
 internally (_OSI) */
  #define ACPI_METHOD_SERIALIZED  0x04 /* Method is serialized */
  #define ACPI_METHOD_SERIALIZED_PENDING  0x08 /* Method is to be marked 
 serialized */
 -#define ACPI_METHOD_MODIFIED_NAMESPACE  0x10 /* Method modified the 
 namespace */
 +#define ACPI_METHOD_IGNORE_SYNC_LEVEL   0x10 /* Method was auto-serialized 
 at table load time */
 +#define ACPI_METHOD_MODIFIED_NAMESPACE  0x20 /* Method modified the 
 namespace */
 
  
 /**
   *
 diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
 index 73764c7..3c7f737 100644
 --- a/drivers/acpi/acpica/dsmethod.c
 +++ b/drivers/acpi/acpica/dsmethod.c
 @@ -175,8 +175,15 @@ acpi_ds_detect_named_opcodes(struct acpi_walk_state 
 *walk_state,
* At this point, we know we have a Named object opcode.
* Mark the method as serialized. Later code will create a mutex for
* this method to enforce serialization.
 +  *
 +  * Note, ACPI_METHOD_IGNORE_SYNC_LEVEL flag means that we will ignore 
 the
 +  * Sync Level mechanism for this method, even though it is now 
 serialized.
 +  * Otherwise, there can be conflicts with existing ASL code that 
 actually
 +  * uses sync levels.
*/
 - walk_state-method_desc-method.info_flags |= ACPI_METHOD_SERIALIZED;
 + walk_state-method_desc-method.sync_level = 0;
 + walk_state-method_desc-method.info_flags |=
 + (ACPI_METHOD_SERIALIZED | ACPI_METHOD_IGNORE_SYNC_LEVEL);
 
   ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 Method serialized [%4.4s] %p - [%s] (%4.4X)\n,
 @@ -349,13 +356,19 @@ acpi_ds_begin_method_execution(struct 
 acpi_namespace_node *method_node,
   /*
* The current_sync_level (per-thread) must be less than or 
 equal to
* the sync level of the method. This mechanism provides some
 -  * deadlock prevention
 +  * deadlock prevention.
 +  *
 +  * If the method was auto-serialized, we just ignore the sync 
 level
 +  * mechanism, because auto-serialization of methods can 
 interfere
 +  * with ASL code that actually uses sync levels.
*
* Top-level method invocation has no walk state at this point
*/
   if (walk_state 
 - (walk_state-thread-current_sync_level 
 -  obj_desc-method.mutex-mutex.sync_level)) {
 + (!(obj_desc-method.
 +info_flags  ACPI_METHOD_IGNORE_SYNC_LEVEL))
 +  (walk_state-thread-current_sync_level 
 + obj_desc-method.mutex-mutex.sync_level)) {
   ACPI_ERROR((AE_INFO,
   Cannot acquire Mutex for method [%4.4s], 
 current SyncLevel is too large (%u),
   acpi_ut_get_node_name(method_node),
 @@ -800,7 +813,8 @@ acpi_ds_terminate_control_method(union 
 acpi_operand_object *method_desc,
   method_desc-method.info_flags =
   ~ACPI_METHOD_SERIALIZED_PENDING;
   method_desc-method.info_flags |=
 - ACPI_METHOD_SERIALIZED;
 + (ACPI_METHOD_SERIALIZED |
 +  ACPI_METHOD_IGNORE_SYNC_LEVEL);
   method_desc-method.sync_level = 0;
   }
 
 --
 1.7.10

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [RFC PATCH 2/2] ACPI: Default disable auto-serialization.

2014-03-17 Thread Zheng, Lv

Hi,

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki
 Sent: Tuesday, March 18, 2014 9:13 AM

 On Tuesday, March 18, 2014 12:31:54 AM Zheng, Lv wrote:
  Hi, Rafael

   From: Wysocki, Rafael J
   Sent: Monday, March 17, 2014 8:09 PM

   On 3/17/2014 5:14 AM, Lv Zheng wrote:
This feature enabled by the following commit is still under development.

   Commit: cd52379678785b02d7a357988cfba214fdaf92f4
   Subject: ACPICA: Add global option to disable method 
auto-serialization.
   This change adds an option to disable the auto-serialization of
   methods that create named objects.

This patch disables it by default temporarily according to the bug 
reports.

   Well, it doesn't restore the 3.14 behavior of acpi_auto_serialize, as
   far as I can say, so we can't apply it.

   I still can drop the commits related to auto-serialization from my
   acpica branch.  Which commits are they?

  From linux-pm/bleeding-edge branch, I found this series includes the 
  following commits:

  Commit: a0fd108f2da56b769472fc0095616fb370988516
  Author: Lv Zheng lv.zh...@intel.com
  Subject: ACPI: Rename kernel parameter acpi_serialize to 
  acpi_no_auto_serialize

  Commit: 5f1cb4a92e4c4aabd139ff9ca1e11c0e2db2ac59
  Author: Lv Zheng lv.zh...@intel.com
  Subject: ACPICA: Remove global option to serialize all control methods.

  Commit: cd52379678785b02d7a357988cfba214fdaf92f4
  Author: Bob Moore robert.mo...@intel.com
  Subject: ACPICA: Add global option to disable method auto-serialization.

  Commit: f56b05bd111b01141ef74568dc4d262c70295d03
  Author: Lv Zheng lv.zh...@intel.com
  Subject: ACPICA: Add additional named objects for the auto-serialize method 
  scan.

  Commit: 27f1899350a1d6baab117c03c87f0e37730047bd
  Author: Bob Moore robert.mo...@intel.com
  Subject: ACPICA: Add auto-serialization support for ill-behaved control 
  methods.

  Reverting these commits can restore original Linux behavior.
  Please check.

 OK, I dropped the five commits above from the acpica branch.

 Please check if the current ACPICA material in the bleeding-edge branch of
 linux-pm.git looks OK (and let me know either way).

It took me some time to confirm.
I just checked ACPICA code using the ACPICA divergence script.
And checked the Linux side reversal in osl.c/kernel-parameter.txt.
All seem to be OK.

Best regards
-Lv

 --
 I speak only for myself.
 Rafael J. Wysocki, Intel Open Source Technology Center.
 --
 To unsubscribe from this list: send the line unsubscribe linux-acpi in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH V2] Change ACPI IPMI support to default y

2014-02-23 Thread Zheng, Lv

Hi,

From: linux-acpi-ow...@vger.kernel.org
[mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Matthew Garrett
Sent: Saturday, February 22, 2014 12:13 AM

On Fri, 2014-02-21 at 02:17 +, Zheng, Lv wrote:

In fact there is a workaround solution I've posted here:
https://patchwork.kernel.org/patch/2831851/
The updated version of this patch can be found at:
https://bugzilla.kernel.org/attachment.cgi?id=112611
It is the acpi-ipmi13.patch file.

I don't think that solves the underlying problem? We're still left with
no guarantee of ordering between ACPI IPMI operation region support
being available and the loading of a driver that may rely on it.

Yes. As it is a bit far beyond the bug report, I didn't work on it.
But actually, I have solution to achieve this.

I think there should be relationship between ACPI region and Linux kernel
modules.
In fact on the well-known operating system, _REG is always invoked at the
end of the IRP_PNP_START_DEVICE completions.
But we may still be able to return -EPROBE_DEFER in the power meter driver
when it detects failures caused by the readiness state
of the region handlers.

We don't have a guarantee that it'll happen at probe time. The
capabilities list may be static, and so we'll get our first failure on
an attempted userspace access instead.

This is the issue related to the IPMI operation region, and it can be solved by
a request_module() call.
The -EPROBE_DEFER I mentioned is about the quality of acpi power meter driver
itself.

It may be that Corey's suggestion satisfies the concerns Russ raised.
Building IPMI in but only probing by default on systems that declare a
BMC in ACPI or DMI should avoid boot-time delays while still ensuring
that the functionality is available on systems where there's a real
chance that the firmware expects the OS to provide support.

If you take a look at this patch (https://patchwork.kernel.org/patch/2831851/),
you'll see it is possible to add request_module() in
acpi_region_default_handler().
The callback is invoked when the first IPMI region access happens and it is in
the process context without any locking.
I'd rather to embed acpi_ipmi into ipmi_si module with this fix:
https://bugzilla.kernel.org/attachment.cgi?id=112611 (acpi-ipmi14.patch).
After that, a request_module() invocation of ipmi_si in
acpi_region_default_handler() can solve what you are worrying about.

What's your opinion?

Thanks and best regards
-Lv

--
Matthew Garrett matthew.garr...@nebula.com
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH 2/4] ACPICA: Introduce new acpi_os_physical_table_add OS callback

2014-03-02 Thread Zheng, Lv

Hi, Thomas

I have a patch series that can cleanup the ACPICA table manager, and change the 
acpi_load_table into the following style:

acpi_status acpi_install_table(acpi_physical_address address, char *signature, 
u8 flags, bool override);

For the flags parameter, it will be:
ACPI_TABLE_ORIGIN_EXTERNAL_PHYSICAL (renamed from ACPI_TABLE_ORIGIN_UNKNOWN)
ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL (renamed from ACPI_TABLE_ORIGIN_OVERRIDE)
ACPI_TABLE_ORIGIN_INTERNAL_PHYSICAL (renamed from ACPI_TABLE_ORIGIN_MAPPED)
ACPI_TABLE_ORIGIN_INTERNAL_LOGICAL (renamed from ACPI_TABLE_ORIGIN_ALLOCATED)

So that we can have the following advantages:

1. the table signature can be validate in one single function.
2. the table checksum can always be validated before installing it to 
acpi_gbl_root_table_list.
3. acpi_os_table_override() and acpi_os_phsycal_table_override() can be merged
4. for acpi_load_table (renamed to acpi_install_table()) invocations, if it is 
invoked by OSPM, override can be avoided.
5. part of the acpi_tb_parse_root_table() code can be merged with the 
acpi_load_table() by invoking same installing API with flags specified.

And with this API, you don't have to introduce code into ACPICA to achieve 
acpi_os_physical_table_add().
You only need to invoke an OSPM only initialization step after 
acpi_initialize_tables(), inside of which, you can invoke acpi_install_table() 
to install new arbitrary tables serving for the purposes of OSPM.
For your cases, I think you can implement the code in this way:

Int __init acpi_table_init(void)
{
Status = acpi_initialize_tables()
/* add all tables, if they haven't been overridden, they will be newly 
installed */
for (no = 0; no  no_of_overridden_tables; no++) {
acpi_install_table(the_phsycail_address, NULL, 
ACPI_TABLE_ORIGIN_EXTERNAL_PHYSICAL, false);
}
}

This function also has been carefully redesigned to ensure:
1. override can avoided for the newly added table.
2. checksum can be verified before adding it to the acpi_gbl_root_table_list, 
and it will only be verified once per table.
3. No need to save the table no that has already been physically overridden, it 
will be taken care by the new interfaces.

I can send out the cleanup series for you to confirm.

Thanks and best regards
-Lv


 -Original Message-
 From: Thomas Renninger [mailto:tr...@suse.de]
 Sent: Saturday, March 01, 2014 1:24 AM
 To: Moore, Robert; Zheng, Lv; Box, David E; Wysocki, Rafael J
 Cc: Thomas Renninger; h...@zytor.com; t...@linutronix.de; 
 c...@conrad-kostecki.de; linux-kernel@vger.kernel.org; x...@kernel.org;
 mi...@redhat.com; r...@rjwysocki.net; de...@acpica.org
 Subject: [PATCH 2/4] ACPICA: Introduce new acpi_os_physical_table_add OS 
 callback
 
 This one allows OS to add arbitrary ACPI tables.
 
 ToDo: It should get checked whether a table with the same signature already
 exists and if this is the case, adding should not happen.
 
 Signed-off-by: Thomas Renninger tr...@suse.de
 CC: h...@zytor.com
 CC: t...@linutronix.de
 CC: c...@conrad-kostecki.de
 CC: linux-kernel@vger.kernel.org
 CC: x...@kernel.org
 CC: mi...@redhat.com
 CC: r...@rjwysocki.net
 CC: de...@acpica.org
 ---
  drivers/acpi/acpica/tbutils.c |   36 
  include/acpi/acpiosxf.h   |6 ++
  2 files changed, 42 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
 index 6412d3c..a819d198 100644
 --- a/drivers/acpi/acpica/tbutils.c
 +++ b/drivers/acpi/acpica/tbutils.c
 @@ -453,6 +453,8 @@ static acpi_status 
 acpi_tb_validate_xsdt(acpi_physical_address xsdt_address)
   *
   
 **/
 
 +#define ACPI_MAX_TABLE_ADD 64
 +
  acpi_status __init acpi_tb_parse_root_table(acpi_physical_address 
 rsdp_address)
  {
   struct acpi_table_rsdp *rsdp;
 @@ -623,5 +625,39 @@ acpi_status __init 
 acpi_tb_parse_root_table(acpi_physical_address rsdp_address)
   }
   }
 
 + /*
 +  * ACPI Table Add:
 +  * Allow the OS to add additional tables to the global root table list
 +  */
 + for (i = 0; i  ACPI_MAX_TABLE_ADD; i++) {
 + int tmp, k;
 + table_entry_size = 0;
 + address = 0;
 + status = acpi_os_physical_table_add(address,
 + table_entry_size);
 + if (status == AE_OK  table_entry_size  address) {
 + table = acpi_os_map_memory(address, table_entry_size);
 + for (k = 2; k  
 acpi_gbl_root_table_list.current_table_count; k++) {
 + /*
 +   Always add SSDTs. Only allow adding of other
 +   tables if none of such a signature already
 +   exists. Use the override interface instead
 +   in such a case

RE: [PATCH 2/4] ACPICA: Introduce new acpi_os_physical_table_add OS callback

2014-03-03 Thread Zheng, Lv

Hi,  Thomas

 From: Thomas Renninger [mailto:tr...@suse.de]
 Sent: Monday, March 03, 2014 8:42 PM
 
 Hi Lv,
 
 On Monday, March 03, 2014 01:20:31 AM Zheng, Lv wrote:
  Hi, Thomas
 
  I have a patch series that can cleanup the ACPICA table manager, and change
  the acpi_load_table into the following style:
 
 Ok. I suggest that:
 1) If Thomas (Gleixner) or whoever wants to try out or needs it urgently, he
can (must) use a recent kernel with my patches applied.
 2) You continue to get your changes into ACPICA.
Eventually or best would be if you add whatever is needed to
allow adding of tables as well (which will be there automatically if
I understood the description of your changes correctly).
 3) Either you give it a try yourself or give me short description for
what I have to look out for and I can re-post the Linux patches
based on your ACPICA changes, once they show up in the Linux kernel.
Best give me a ping as soon as I should look at it.

That sounds good.

Or it can be more efficient for productions:
Linux can merge your patches and ACPICA just stop to take them.
This will leave us divergences.
After the table manager cleanups are tested and shipped in the ACPICA repo, the 
new facilities will automatically be rolled into Linux branches.
Then I can help to reduce the divergences using the new ACPICA facilities.
At that time I may ask whoever that can test to offer help to review the 
cleanup patch.

Thanks and best regards
-Lv
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/4] ACPICA: Introduce new acpi_os_physical_table_add OS callback

2014-03-04 Thread Zheng, Lv

Hi, Thomas

 From: Thomas Renninger [mailto:tr...@suse.de]
 Sent: Tuesday, March 04, 2014 7:55 PM

 On Tuesday, March 04, 2014 12:31:57 AM Zheng, Lv wrote:
  Hi,  Thomas

   From: Thomas Renninger [mailto:tr...@suse.de]
   Sent: Monday, March 03, 2014 8:42 PM

   Hi Lv,

   On Monday, March 03, 2014 01:20:31 AM Zheng, Lv wrote:
Hi, Thomas

I have a patch series that can cleanup the ACPICA table manager, and
change

the acpi_load_table into the following style:
   Ok. I suggest that:
   1) If Thomas (Gleixner) or whoever wants to try out or needs it urgently,
   he
  can (must) use a recent kernel with my patches applied.

   2) You continue to get your changes into ACPICA.

  Eventually or best would be if you add whatever is needed to
  allow adding of tables as well (which will be there automatically if
  I understood the description of your changes correctly).

   3) Either you give it a try yourself or give me short description for

  what I have to look out for and I can re-post the Linux patches
  based on your ACPICA changes, once they show up in the Linux kernel.
  Best give me a ping as soon as I should look at it.

  That sounds good.

  Or it can be more efficient for productions:
  Linux can merge your patches and ACPICA just stop to take them.
 You mean add my stuff to drivers/acpi/acpica in Linux kernel without
 pushing them into the ACPICA repository?

 I cannot remember that this ever happened (beside small important fixes)
 and I doubt Rafael is willing to do that.
 If it would be super critical, but I cannot see that it is.

OK.

  This will leave us divergences.
 Yes, that would be bad.

Yes.

  After the table manager cleanups are tested and shipped in the ACPICA repo,
  the new facilities will automatically be rolled into Linux branches.
 I'd suggest to just wait for that.
 Best already try to integrate the ACPI table override/add part as you think
 it should work without additional changes in drivers/acpi/acpica.

 If this happened and things are submitted to get integrated into the Linux
 kernel, please add me to CC or point me to the patchset.

Fortunately, there is a kernel bugzilla entry requires this series.
I've posted it on the kernel bugzilla.
Here is the patchset:
https://bugzilla.kernel.org/show_bug.cgi?id=69711
You need to apply 9 patches:
attachment 128061 - attachment 128141
The last patch has introduced an early boot API:
acpi_install_table.
This can be used to enhance this series.

  Then I
  can help to reduce the divergences using the new ACPICA facilities. At that
  time I may ask whoever that can test to offer help to review the cleanup
  patch.

 I can then give your new patcheset some testing and try to get the Linux
 (drivers/acpi/osl.c) parts (re-)implemented based on your stuff.
 There might still be the one or other minor fix needed that has to go
 into acpica as well, but that should not be that hard to manage and
 might end up in acpica and Linux kernel in parallel without much
 extra overhead.

If you found any issues in using this series, let me know.

Thanks and best regards
-Lv

  Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/2] ACPICA: acpidump: Remove translation protection on integer types.

2014-02-17 Thread Zheng, Lv

Hi, Tony

 From: Luck, Tony
 Sent: Friday, February 14, 2014 7:35 AM
 
 All definitions are equal except ACPI_UINT64_MAX for CONFIG_IA64.  It
 is changed from sizeof(unsigned long) to sizeof(unsigned long long).
 By investigation, 64bit Linux kernel build is LP64 compliant, i.e.,
 sizeof(long) and (pointer) are 64.  As sizeof(unsigned long) equals to
 sizeof(unsigned long long) on IA64 platform where CONFIG_64BIT cannot be
 disabled, this change actually will not affect the value of
 ACPI_UINT64_MAX on IA64 platforms.
 
 This all looks correct to me - it really shouldn't make any difference
 to ia64 whether we use long or long long ... both are 8-byte entities.
 The compiler would complain in some places if you mixed  matched
 incorrectly (e.g. printk(val = %ld\n, val); will give a warning if val has
 been switched from long to long long and the format would need
 to change to %lld).  But it looks like nothing like that happens as a
 result of this patch. All my ia64 configs build with no new warnings.
 
 Boots OK too.

It looked scary to me that without applying this patch, Linux ACPICA was using 
the u64 defined by Linux but defining ACPI_UINT64_MAX to the value that could 
be different from sizeof(u64).
This patch can make them converged.  I was worrying about the regressions that 
could be triggered by this fix if there was something already dependent on this 
divergence.
Great to hear that there is no such dependency. :-)

Thanks and best regards
-Lv
 
 
 -Tony
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V2] Change ACPI IPMI support to default y

2014-02-20 Thread Zheng, Lv

Hi,

Sorry for interrupting you.
I have some information that may be helpful for your discussion.
Please find them in the inlined replies.
Well, I don't want to join the fight, just for your informations. :-)

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Russ Anderson
 Sent: Friday, February 21, 2014 7:59 AM
 
 On Thu, Feb 20, 2014 at 11:09:42PM +, Matthew Garrett wrote:
  On Thu, 2014-02-20 at 16:45 -0600, Russ Anderson wrote:
   On Thu, Feb 20, 2014 at 10:26:45PM +, Matthew Garrett wrote:
 
Because I'm trying to ensure that the default behaviour of the kernel is
to *work*. Defaulting to having IPMI be modular means that the default
behaviour of the kernel, as far as the ACPI spec goes, is to be broken.
  
   The ACPI spec requires IPMI functionality before a module loads at
   boot time?  And the kernel is *broken* if it does not support ACIP IPMI
   functionality before module load time?  Really?
 
  There's no mechanism to ensure that IPMI support will be loaded before
  ACPI calls attempt to access IPMI operation regions. Really.
 
 And no mechanism can be added to ensure that ACPI call are
 not attempted before IPMI is initialized?  A flag or lock
 or exported symbol indicating IPMI support is ready.

In fact there is a workaround solution I've posted here:
https://patchwork.kernel.org/patch/2831851/
The updated version of this patch can be found at:
https://bugzilla.kernel.org/attachment.cgi?id=112611
It is the acpi-ipmi13.patch file.

This solution may change the meaning of ACPI spec defined _REG.
So we may need a better solution.

But after merging this patch, it is safe to unload acpi_ipmi at users' wishes.
Without solutions to solve region handler uninstallation races, it is not safe 
to unload acpi_ipmi module.
You can see crashes in the description of this patch.

The ipmi_si module is using a different way to unload itself which has not been 
tested by me.
You can find it in Documentation/IPMI.txt by searching hot and remove of 
interfaces in this file.

 
ACPI 4.0 includes support for IPMI operation regions. Modular IPMI means
that the kernel will spend a significant amount of time (potentially
until a user manually loads a driver) failing to implement part of the
IPMI specification. That's a problem, and the correct fix is to ensure
that the kernel always implements IPMI support.
  
   The ACPI spec says ipmi_si cannot be a driver?  Really?
   What is the real problem you are trying to solve?
 
  The most straightforward case is that of an ACPI power meter.
 
 So it is just a matter of making sure ipmi_si modules loads before
 the ACPI power meter module loads, right?  module dependency issue.

I agree.
I think there should be relationship between ACPI region and Linux kernel 
modules.
In fact on the well-known operating system, _REG is always invoked at the end 
of the IRP_PNP_START_DEVICE completions.
But we may still be able to return -EPROBE_DEFER in the power meter driver when 
it detects failures caused by the readiness state of the region handlers.

I didn't work further on this issue when solving the reported bug:
https://bugzilla.kernel.org/show_bug.cgi?id=46741

 
 Several
  vendors implement this with an IPMI operation region. Calling any of the
  power meter functions will trigger access to that IPMI operation region,
  which will fail. This may result in driver initialisation failing. There
  is no express dependency between the power meter driver and ipmi_si,
  because the spec envisages IPMI support as basic kernel functionality.
  It's meant to be there before you start loading any other drivers.
 
 The spec envisages?  I get there is a dependency, that IPMI driver
 needs to be loaded before ACIP power meter.  This isn't the first
 case of a driver being dependent on another driver.  That doesn't
 mean IPMI driver must be built into the kernel.
 
Now, you've described some other problems. I don't disagree that those
are problems. The correct thing for us to do with those problems is to
fix them, not to simply change the kernel defaults such that it's
possible for users to choose between two differently broken states. I'm
absolutely willing to help, as long as you're willing to put some
reasonable amount of effort into describing them.
  
   How about ACPI IPMI functionality starts when the ipmi_si
   module loads at boot time.

Actually, I have a solution implemented this.
You can find it at:
https://bugzilla.kernel.org/attachment.cgi?id=112611
It is the acpi-ipmi14.patch file.

The patch will hand the maintenance-ship of acpi_ipmi to IPMI community.
I'm not sure it is proper to merge it by Linux upstreams.

Thanks and best regards
-Lv

 
  I've repeatedly asked for you to provide detailed descriptions of the
  problems you've seen because I have a genuine interest in fixing them.
  If you're just

RE: [Devel] [PATCH 1/4] ACPI: Provide support for ACPI table adding via OS

2014-02-20 Thread Zheng, Lv

Hi, Thomas

 From: Devel [mailto:devel-boun...@acpica.org] On Behalf Of Thomas Renninger
 Sent: Wednesday, February 19, 2014 7:23 PM
 
 On Tuesday, February 18, 2014 12:51:07 PM H. Peter Anvin wrote:
  On 02/18/2014 10:44 AM, Thomas Renninger wrote:
   On Tuesday, February 18, 2014 10:27:23 AM H. Peter Anvin wrote:
   Why can't you add SSDTs? It would be particularly useful.
  
   There are 2 ways how ACPI tables get added:
  - Via pointer from a root table (XSDT or RSDT iirc)
  - Via load statement inside of ACPI context when ACPI BIOS
  
code gets executed (iirc the physical address is passed).
  
   The latter is only for SSDTs.
   The problem is that you if you add an SSDT early, it might
   have been intended for overriding when an SSDT gets dynamically
   loaded later when the system is up which is particular useful as
   well if you want to debug this specific BIOS table.
  
   This could be workarounded via a boot param:
   acpi=allow_ssdt_adding
   But this is not nice. Maybe someone has a more elegant idea.
   Something could still be added if someone is really needing this.
 
  Since adding SSDTs is one of the things I really can imagine one would
  do, I think we need to figure out how to do that.  I would think that
  overriding would be the exception case.
 
 You can always paste new code into the DSDT. It is effectively the same
 than adding a new SSDT table.
 The other way around, modifying or deleting broken code in a BIOS
 provided SSDT needs overriding.
 
 So in fact adding SSDTs is not necessary at all.
 It would be a nice add-on, but it's not even worth introducing
 an extra boot param or whatever confusion.
 A hint in Documentation that adding additional ASL (ACPI Source Language)
 code to the DSDT would be the same than creating and adding a new
 SSDT table which is not possible should be enough.

IMO, we need to load tables from RSDT/XSDT, they look like static tables.
Given that we can live in the environment where table reloading is properly 
implemented by ACPICA, we won't face issues.
And the reloading feature is also required by ACPI specification.
If a table contains same signature, oem_id, oem_table_id, it could be able to 
replace the old loaded one if the revision field is greater than the old one.

Actually I'm currently working on the parallel reloading support and all 
functionalities have been done.
This report is a bit hurry than I expected.
I'll try to prepare fixes (which seems to be dozens of patches) for the testers 
to validate.

Fortunately, specific to this bug, I don't think the reload requirement must be 
implemented as the new one doesn't contain a greater revision number.
So there might just be dead lock issues for this bug.

Thanks and best regards
-Lv

 
 The whole thing will always taint the kernel and is meant for
 debugging only anyway.
 
Thomas
 ___
 Devel mailing list
 de...@acpica.org
 https://lists.acpica.org/mailman/listinfo/devel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [Devel] [PATCH 1/4] ACPI: Provide support for ACPI table adding via OS

2014-02-20 Thread Zheng, Lv

Hi, Thomas

 From: Devel [mailto:devel-boun...@acpica.org] On Behalf Of Thomas Renninger
 Sent: Wednesday, February 19, 2014 2:44 AM
 
 On Tuesday, February 18, 2014 10:27:23 AM H. Peter Anvin wrote:
  Why can't you add SSDTs? It would be particularly useful.
 
 There are 2 ways how ACPI tables get added:
- Via pointer from a root table (XSDT or RSDT iirc)
- Via load statement inside of ACPI context when ACPI BIOS
  code gets executed (iirc the physical address is passed).
 
 The latter is only for SSDTs.
 The problem is that you if you add an SSDT early, it might
 have been intended for overriding when an SSDT gets dynamically
 loaded later when the system is up which is particular useful as
 well if you want to debug this specific BIOS table.
 
 This could be workarounded via a boot param:
 acpi=allow_ssdt_adding
 But this is not nice. Maybe someone has a more elegant idea.
 Something could still be added if someone is really needing this.

I'm not sure if you are talking about the issue that:
If a system booted using customized DSDT embedded with SSDT, it also requires 
dynamic SSDT loading be prevented in ACPICA.

Thanks and best regards
-Lv


 
  Thomas
 ___
 Devel mailing list
 de...@acpica.org
 https://lists.acpica.org/mailman/listinfo/devel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 2/2] ACPICA: acpidump: Remove translation protection on integer types.

2014-02-12 Thread Zheng, Lv

Hi, Tony and Fenghua

I need your opinions on this patch?
Will it affect the functionality of IA64 kernel behavior?
Can you help to confirm?

Thanks in advance.

Best regards
-Lv


 -Original Message-
 From: Zheng, Lv
 Sent: Tuesday, February 11, 2014 10:52 AM
 To: Wysocki, Rafael J; Brown, Len
 Cc: Zheng, Lv; Lv Zheng; linux-kernel@vger.kernel.org; 
 linux-a...@vger.kernel.org; Luck, Tony; Yu, Fenghua
 Subject: [PATCH 2/2] ACPICA: acpidump: Remove translation protection on 
 integer types.
 
 Remove translation protection for applications as Linux tools folder will
 start to use such types.
 
 In Linux kernel source tree, after removing this translation protection,
 the u8/u16/u32/u64/s32/s64 typedefs are exposed for both __KERNEL__ builds
 and !__KERNEL__ builds (tools/power/acpi) and the original definitions of
 ACPI_UINT8/16/32/64_MAX are changed.
 
 For !__KERNEL__ builds, this kind of defintions should already been tested
 by the distribution vendors that are distributing binary ACPICA package and
 we've achieved the successful built/run test result in the kernel source
 tree.
 
 For __KERNEL__ builds, there are 2 things affected:
 1. u8/u16/u32/u64/s32/s64 type definitions:
Since Linux has already type defined u8/u16/u32/u64/s32/s64 in
include/uapi/asm-generic/int-ll64.h for __KERNEL__.  In order not to
introduce build regressions where the 2 typedefs are differed,
ACPI_USE_SYSTEM_INTTYPES is introduced to mask out ACPICA's typedefs.
It must be defined for Linux __KERNEL__ builds.
 2. ACPI_UINT8/16/32/64_MAX definitions:
Before applying this change:
  ACPI_UINT8_MAX: sizeof (UINT8)
   UINT8: unsigned char
  ACPI_UINT16_MAX: sizeof (UINT16)
   UINT16: unsigned short
  ACPI_UINT32_MAX: sizeof (UINT32)
   INT32: int
   UINT32: unsigned int
  ACPI_UINT64_MAX: sizeof (UINT64)
   INT64: COMPILER_DEPENDENT_INT64
COMPILER_DEPENDENT_INT64: signed long (IA64) or
  signed long long (IA32)
   UINT64: COMPILER_DEPENDENT_UINT64
COMPILER_DEPENDENT_UINT64: unsigned long (IA64) or
   unsigned long long (IA32)
After applying this change:
  ACPI_UINT8_MAX: sizeof (u8)
   u8: unsigned char
   UINT8: (removed from actypes.h)
  ACPI_UINT16_MAX: sizeof (u16)
   u16: unsigned short
   UINT16: (removed from actypes.h)
  ACPI_UINT32_MAX: sizeof (u32)
   INT32/UINT32: (removed from actypes.h)
   s32: signed int
   u32: unsigned int
  ACPI_UINT64_MAX: sizeof (u64)
   INT64/UINT64: (removed from actypes.h)
   u64: unsigned long long
   s64: signed long long
   COMPILER_DEPENDENT_INT64: signed long (IA64) (not used any more)
 signed long long (IA32) (not used any more)
   COMPILER_DEPENDENT_UINT64: unsigned long (IA64) (not used any more)
  unsigned long long (IA32) (not used any more)
All definitions are equal except ACPI_UINT64_MAX for CONFIG_IA64.  It
is changed from sizeof(unsigned long) to sizeof(unsigned long long).
By investigation, 64bit Linux kernel build is LP64 compliant, i.e.,
sizeof(long) and (pointer) are 64.  As sizeof(unsigned long) equals to
sizeof(unsigned long long) on IA64 platform where CONFIG_64BIT cannot be
disabled, this change actually will not affect the value of
ACPI_UINT64_MAX on IA64 platforms.
 
 This patch is generated as linuxized acpidump is failed to be compiled due
 to lacking in u8/u16/u32/u64/s32/s64 definitions.  Lv Zheng.
 
 Signed-off-by: Lv Zheng lv.zh...@intel.com
 Cc: Tony Luck tony.l...@intel.com
 Cc: Fenghua Yu fenghua...@intel.com
 ---
  include/acpi/actypes.h  |   64 
 +++
  include/acpi/platform/aclinux.h |2 ++
  2 files changed, 34 insertions(+), 32 deletions(-)
 
 diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
 index 68a3ada..86e9f13 100644
 --- a/include/acpi/actypes.h
 +++ b/include/acpi/actypes.h
 @@ -55,18 +55,16 @@
  #error ACPI_MACHINE_WIDTH not defined
  #endif
 
 -/*! [Begin] no source code translation */
 -
  /*
   * Data type ranges
   * Note: These macros are designed to be compiler independent as well as
   * working around problems that some 32-bit compilers have with 64-bit
   * constants.
   */
 -#define ACPI_UINT8_MAX  (UINT8) (~((UINT8)  0))  /* 0xFF 
   */
 -#define ACPI_UINT16_MAX (UINT16)(~((UINT16) 0))  /* 
 0x */
 -#define ACPI_UINT32_MAX (UINT32)(~((UINT32) 0))  /* 
 0x */
 -#define ACPI_UINT64_MAX (UINT64)(~((UINT64) 0))  /* 
 0x */
 +#define ACPI_UINT8_MAX  (u8) (~((u8)  0))/* 0xFF 
   */
 +#define ACPI_UINT16_MAX (u16)(~((u16) 0))/* 0x   
   */
 +#define ACPI_UINT32_MAX

RE: [PATCH 0/4] ACPICA: Stable material of ACPI executer fixes for linux-3.8.

2013-10-31 Thread Zheng, Lv

 From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
 Sent: Thursday, October 31, 2013 8:22 PM
 
 On Thursday, October 31, 2013 05:08:50 AM Greg Kroah-Hartman wrote:
  On Thu, Oct 31, 2013 at 12:39:21PM +0100, Rafael J. Wysocki wrote:
   On Thursday, October 31, 2013 09:07:40 AM Lv Zheng wrote:
There are bug-fixes for AML interpreter upstreamed, fixing some serious
issues found in recent platforms.  These fixes make Linux AML 
interpreter
more ACPI 2.0 ASL concept compliant.  Further AML interpreter fixes 
should
be based on such improvements, thus they are good materials for stable.
   
This patch set can be safely applied to linux-3.8:
commit 19f949f52599ba7c3f67a5897ac6be14bfcb1200 upstream.
   
The patch set has passed build/boot tests on the following machines:
  Dell Inspiron Mini 1010 (i386)
  HP Compaq 8200 Elite SFF PC (x86-64)
   
Bob Moore (4):
  ACPICA: Interpreter: Fix Store() when implicit conversion is not
possible.
  ACPICA: DeRefOf operator: Update to fully resolve FieldUnit and
BufferField refs.
  ACPICA: Return error if DerefOf resolves to a null package element.
  ACPICA: Fix for a Store-ArgX when ArgX contains a reference to a
field.
  
   Hi Greg,
  
   Please take patches [1-4/4] for stable.
 
  Which stable tree?
 
  I don't do 3.8, it's long been end-of-life, although one company is
  trying to keep it alive, but that's not me.
 
  I'm only handling 3.4, 3.10, and 3.11 stable trees right now, which
  one(s) should these be applied to?
 
 3.10.x and 3.11.x then.
 
 Lv, do the original mainline commits apply to these kernels?
 
 Rafael

Hi, Rafael and Greg

I checked the back port dependencies since v3.8:
1. [PATCH 1] belongs to v3.9.
2. [PATCH 4] includes an empty line belonging to a coding style fix affecting 
this series (between [PATCH 3] and [PATCH 4]).
Thus,
1. For v3.10:
[PATCH 1]: It's already in the repo, so please drop it.
[PATCH 2-4]: They can be used directly as 3.10.x stable materials.
2. For v3.11:
[PATCH 1]: It's already in the repo, so please drop it.
[PATCH 2-3]: They can be used directly as 3.11.x stable materials.
[PATCH 4]: The original commit from Linus' tree should be used instead.

 I checked the commit log since v3.4.
 There is no functional change done to the AML executer between v3.4 and v3.8.
 The problem is there is a coding style fix affecting this series (between v3.4 
and [PATCH 1]).
 I generated the following diff block before applying [PATCH 1], and obtained a 
successful build/boot to a v3.4 kernel with these patches applied.
Thus,
1. For v3.4:
[PATCH 1]: You can merge this diff block to [PATCH 1] or simply modify the 
[PATCH 1] by manually adding this white space.
[PATCH 2-4]: They can be used directly as 3.4.x stable materials.

---
Index: linux-acpica/drivers/acpi/acpica/exstore.c
===
--- linux-acpica.orig/drivers/acpi/acpica/exstore.c
+++ linux-acpica/drivers/acpi/acpica/exstore.c
@@ -491,7 +491,7 @@ acpi_ex_store_object_to_node(union acpi_
  acpi_ut_get_object_type_name(source_desc),
  source_desc, node));
 
-   /* No conversions for all other types.  Just attach the source 
object */
+   /* No conversions for all other types. Just attach the source 
object */
 
status = acpi_ns_attach_object(node, source_desc,
   source_desc-common.type);
---

Thanks and best regards
-Lv
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH 2/2] gpio / ACPI: add support for GPIO operation regions

2013-09-23 Thread Zheng, Lv

 From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
 Sent: Tuesday, September 17, 2013 4:37 PM

 On Mon, Sep 16, 2013 at 11:35:56PM +, Zheng, Lv wrote:
   From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
   Sent: Monday, September 16, 2013 4:11 PM

   On Mon, Sep 16, 2013 at 01:21:53AM +, Zheng, Lv wrote:
 A pseudo device may be created to access the GPIO operation region 
 fields provided by one GPIO device.
 The pseudo device may have other functions to access other GPIO 
 operation region fields provided by other GPIO devices, or even
 worse
   to
 access other ACPI provided value-adds.
 So hierarchically the pseudo device only requires CPU, thus should 
 not be under the GPIO device, which means the GPIO operation
   regions
 have nothing to do with the GPIO devices' ACPI handle.

Sorry for the wording.
It's better to say the GPIO operation region users haven't strict
relationship to the GPIO operation region providers.
As the installation is to provide GPIO operation regions to the users, 
it
shouldn't relate to the providers' ACPI handle.

   If I understand you correctly you mean that there might be multiple users
   (different devices) for the same GPIO operation region, right?

  No, this is not what I meant.
  Can one vendor device uses two or more GPIO pins from different GPIO ports?

 Yes.

So how can such a device under one of these GPIO ports?
It can only be under one particular GPIO port device, right?

Thus I believe such device will appear below other devices rather than below a 
GPIO port device in the ACPI namespace.

   That shouldn't be a problem as far as I can tell.

   What comes to the hierarchy you refer, I'm not sure if that is a problem
   either (unless I'm missing something). The GPIO can be used anywhere in 
   the
   ASL, it doesn't have to be descendant of the GPIO device. You only need to
   do something like this:

 // Assert the GPIO
 Store(1, \_SB.PCI0.SHD3)

   In other words, use the fully qualified name.

  Yes, which means this line of code can be everywhere in the namespace.
  It is not required to be under one particular GPIO device as long as there 
  is an operation region defined in that scope.

  The problem is the installation, the first parameter for
  acpi_install_address_space_handler() is a namespace node, the function
  will call _REG for all OperationRegions below the scope whose top most
  node is the specified node.
  The nodes out of this scope are not affected.  So if an OperationRegion
  is defined out of this scope, problem happens.

 I suppose for that operation region another GPIO device should be
 introduced then, no?

I believe the answer is no.

 So if we don't have a GPIO driver for the given operation region, what can
 we do? We don't want the ASL code to erroneusly think that there is an
 operation region available when it is not.

I think GPIO address_space_handler should be provided by an ACPI GPIO address 
space module rather than provided by a particular GPIO driver.

Actually, without the readiness of the GPIO driver, currently ASL code can 
still access the GPIO fields, though it just results in handler not found 
ACPICA error message.
If the setup callback is implemented, then the proper GPIO driver can be 
matched in this setup callback.
If the GPIO driver hasn't been loaded by Linux and thus not matched in the 
setup callback, from users' point of view, the result is just changing the 
error message from handler not found to driver not found.

Thanks and best regards
-Lv
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow

2013-07-23 Thread Zheng, Lv

 From: linux-acpi-ow...@vger.kernel.org
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Greg KH
 Sent: Tuesday, July 23, 2013 10:54 PM
 
 On Tue, Jul 23, 2013 at 04:08:59PM +0800, Lv Zheng wrote:
  This patch enhances sanity checks on message size to avoid potential
  buffer overflow.
 
  The kernel IPMI message size is IPMI_MAX_MSG_LENGTH(272 bytes) while
  the ACPI specification defined IPMI message size is 64 bytes.  The
  difference is not handled by the original codes.  This may cause crash
  in the response handling codes.
  This patch fixes this gap and also combines rx_data/tx_data to use
  single data/len pair since they need not be seperated.
 
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Reviewed-by: Huang Ying ying.hu...@intel.com
  ---
   drivers/acpi/acpi_ipmi.c |  100
  --
   1 file changed, 61 insertions(+), 39 deletions(-)
 
 formletter
 
 This is not the correct way to submit patches for inclusion in the stable 
 kernel
 tree.  Please read Documentation/stable_kernel_rules.txt
 for how to do this properly.
 
 /formletter
 
 Same goes for the other patches you sent in this thread...

OK, I'll add prerequisites for each that want to be accepted by the stable 
queue and re-send them (PATCH 01-06).

Thanks and best regards
-Lv

 --
 To unsubscribe from this list: send the line unsubscribe linux-acpi in the 
 body
 of a message to majord...@vger.kernel.org More majordomo info at
 http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 01/13] ACPI/IPMI: Fix potential response buffer overflow

2013-07-23 Thread Zheng, Lv

 From: Zheng, Lv
 Sent: Wednesday, July 24, 2013 8:22 AM

  From: linux-acpi-ow...@vger.kernel.org
  [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Greg KH
  Sent: Tuesday, July 23, 2013 10:54 PM

  On Tue, Jul 23, 2013 at 04:08:59PM +0800, Lv Zheng wrote:
   This patch enhances sanity checks on message size to avoid potential
   buffer overflow.

   The kernel IPMI message size is IPMI_MAX_MSG_LENGTH(272 bytes) while
   the ACPI specification defined IPMI message size is 64 bytes.  The
   difference is not handled by the original codes.  This may cause
   crash in the response handling codes.
   This patch fixes this gap and also combines rx_data/tx_data to use
   single data/len pair since they need not be seperated.

   Signed-off-by: Lv Zheng lv.zh...@intel.com
   Reviewed-by: Huang Ying ying.hu...@intel.com
   ---
drivers/acpi/acpi_ipmi.c |  100
   --
1 file changed, 61 insertions(+), 39 deletions(-)

  formletter

  This is not the correct way to submit patches for inclusion in the
  stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
  for how to do this properly.

  /formletter

  Same goes for the other patches you sent in this thread...

 OK, I'll add prerequisites for each that want to be accepted by the stable 
 queue
 and re-send them (PATCH 01-06).

Maybe I shouldn't.
I looks it is not possible to add commit ID prerequisites for patch series that 
has not been accepted by the mainline.
As the patches haven't been merged by the mainline, it is likely that the 
commit IDs in this series will be changed.
Please ignore [PATCH 01-06] that have been sent to the stable mailing list.
I'll just let ACPI maintainers know which patches I think that can go for 
stable tree and let they make the decision after the mainline acceptance.

Thanks and best regards
-Lv

 Thanks and best regards
 -Lv

  --
  To unsubscribe from this list: send the line unsubscribe linux-acpi
  in the body of a message to majord...@vger.kernel.org More majordomo
  info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path

Hi,

Sorry for the delayed response.

 From: Ben Guthro [mailto:benjamin.gut...@citrix.com]
 Sent: Tuesday, July 02, 2013 7:43 PM
 
 
 On 07/02/2013 02:19 AM, Zheng, Lv wrote:
  Thanks for your efforts!
 
  I wonder if it is possible to remove the argument - u8 extended and 
  convert
 pm1a_control, pm1b_control into some u8 values that are equivalent to
 acpi_gbl_sleep_type_a, acpi_gbl_sleep_type_b in the legacy sleep path.
  It can also simplify Xen codes.
 
 Thanks for your time to review this.
 
 I'm not sure that this simplifies things. I think that, in fact, it would 
 make them
 quite a bit more complicated, but perhaps I misunderstand.
 
 Is it not preferred to use the reduced hardware sleep, over the old method?
 While these register definitions may be equivalent below, doing the 
 translation
 in linux, only to translate them back again at a lower layer seems 
 unnecessary.
 

Yes, it would require tboot layer to be able to be aware of how such fields 
locate in the PM registers.
So I think you can pass the register address of the field and the field 
name/value pair to the tboot, this could simplify things, no lower layer effort 
will be needed.
Please don't worry about the case that a register field could be split into 
PM1a and PM1b, it could be a hardware design issue.
IMO, one field should always be in one register, either PM1a or PM1b.
Or there could be hardware issues cannot be addressed by the ACPICA 
architecture (something like natural atomicity).
But maybe I'm wrong.

Thanks and best regards
-Lv

 The hypervisor knows how to deal with both the reduced hardware sleep as
 well as the legacy sleep path - it merely need to distinguish these two paths,
 when performing the hypercall.
 
 Since there are two paths through the higher level ACPICA code - that in
 hwsleep.c, and hwesleep.c - there needs to be some distinction between the
 two paths, when calling through to the lower level
 acpi_os_prepare_sleep() call.
 
 An alternate method would be to create another interface named
 acpi_os_prepare_esleep() which would do the equivalent of this patch series,
 with an extended parameter hidden from upper level interfaces.
 
 This, however, would also add another function to include/acpi/acpiosxf.h -
 which, I thought was undesirable, in the impression that I got from Bob Moore,
 and Rafael Wysocki (though, please correct me on this point, if I have
 misunderstood)
 
 Best Regards
 
 Ben
 
 
  As in ACPI specification, the bit definitions between the legacy sleep 
  registers
 and the extended sleep registers are equivalent.
 
  The legacy sleep register definition:
  Table 4-16 PM1 Status Registers Fixed Hardware Feature Status Bits -
  WAK_STS(bit 15) Table 4-18 PM1 Control Registers Fixed Hardware
  Feature Control Bits - SLP_TYPx (bit 10-12), SLP_EN (bit 13)
 
  The extended sleep register definition:
  Table 4-24 Sleep Control Register - SLP_TYPx (3 bits from offset 2), SLP_EN 
  (1
 bit from offset 5), here 10-8 = 2, and 13-8 = 5, this definition is 
 equivalent to
 Table 4-18.
  Table 4-25 Sleep Status Register - WAK_STS (1 bit 7), 15-8 = 7, this 
  definition is
 equivalent to Table 4-16.
 
  Thanks and best regards
  -Lv
 
  -Original Message-
  From: linux-acpi-ow...@vger.kernel.org
  [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Ben Guthro
  Sent: Wednesday, June 26, 2013 10:06 PM
  To: Konrad Rzeszutek Wilk; Jan Beulich; Rafaell J . Wysocki;
  linux-kernel@vger.kernel.org; linux-a...@vger.kernel.org;
  xen-de...@lists.xen.org
  Cc: Ben Guthro; Moore, Robert
  Subject: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in
  reduced hardware sleep path
 
  In version 3.4 acpi_os_prepare_sleep() got introduced in parallel
  with reduced hardware sleep support, and the two changes didn't get
  synchronized: The new code doesn't call the hook function (if so
  requested). Fix this, requiring a parameter to be added to the hook
  function to distinguish extended from legacy sleep.
 
  Signed-off-by: Ben Guthro benjamin.gut...@citrix.com
  Signed-off-by: Jan Beulich jbeul...@suse.com
  Cc: Bob Moore robert.mo...@intel.com
  Cc: Rafaell J. Wysocki r...@sisk.pl
  Cc: linux-a...@vger.kernel.org
  ---
drivers/acpi/acpica/hwesleep.c |8 
drivers/acpi/acpica/hwsleep.c  |2 +-
drivers/acpi/osl.c |   16 
include/linux/acpi.h   |   10 +-
4 files changed, 22 insertions(+), 14 deletions(-)
 
  diff --git a/drivers/acpi/acpica/hwesleep.c
  b/drivers/acpi/acpica/hwesleep.c index 5e5f762..6834dd7 100644
  --- a/drivers/acpi/acpica/hwesleep.c
  +++ b/drivers/acpi/acpica/hwesleep.c
  @@ -43,6 +43,7 @@
 */
 
#include acpi/acpi.h
  +#include linux/acpi.h
#include accommon.h
 
#define _COMPONENT  ACPI_HARDWARE
  @@ -128,6 +129,13 @@ acpi_status acpi_hw_extended_sleep(u8
  sleep_state)
 
 ACPI_FLUSH_CPU_CACHE();
 
  +  status = acpi_os_prepare_sleep(sleep_state, acpi_gbl_sleep_type_a

RE: [PATCH] ACPICA: Revert ACPICA: Add option to favor 32-bit FADT addresses.

2014-05-11 Thread Zheng, Lv

 From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
 Sent: Monday, May 12, 2014 8:12 AM
 
 From: Rafael J. Wysocki rafael.j.wyso...@intel.com
 
 Revert commit 0249ed2444d6 (ACPICA: Add option to favor 32-bit FADT
 addresses.) that breaks resume from system suspend on the Intel DP45SG
 board.

This commit doesn't look like the root cause.
[0.00] ACPI BIOS Warning (bug): Incorrect checksum in table [XSDT] - 
0xA0, should be 0xC9 (20131218/tbprint-214)
[0.00] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT: 
0xCF661F40/0xCF667E40, using 64-bit address (20131218/tbfadt-271)
It seems before using the FADT, ACPICA have already detected that the XSDT is 
not correct.

Please revert it for now.  I'll take a look at this bug.

Thanks and best regards
-Lv

 
 Fixes: 0249ed2444d6 (ACPICA: Add option to favor 32-bit FADT addresses.)
 References: https://bugzilla.kernel.org/show_bug.cgi?id=74021
 Reported-and-tested-by: Oswald Buddenhagen o...@kde.org
 Cc: 3.14+ sta...@vger.kernel.org # 3.14+
 Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
 ---
  drivers/acpi/acpica/acglobal.h |  10 --
  drivers/acpi/acpica/tbfadt.c   | 335 
 +++--
  include/acpi/acpixf.h  |   1 -
  3 files changed, 152 insertions(+), 194 deletions(-)
 
 diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h
 index 49bbc71fad54..72578a4b8212 100644
 --- a/drivers/acpi/acpica/acglobal.h
 +++ b/drivers/acpi/acpica/acglobal.h
 @@ -136,16 +136,6 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_copy_dsdt_locally, FALSE);
  ACPI_INIT_GLOBAL(u8, acpi_gbl_do_not_use_xsdt, FALSE);
 
  /*
 - * Optionally use 32-bit FADT addresses if and when there is a conflict
 - * (address mismatch) between the 32-bit and 64-bit versions of the
 - * address. Although ACPICA adheres to the ACPI specification which
 - * requires the use of the corresponding 64-bit address if it is non-zero,
 - * some machines have been found to have a corrupted non-zero 64-bit
 - * address. Default is FALSE, do not favor the 32-bit addresses.
 - */
 -ACPI_INIT_GLOBAL(u8, acpi_gbl_use32_bit_fadt_addresses, FALSE);
 -
 -/*
   * Optionally truncate I/O addresses to 16 bits. Provides compatibility
   * with other ACPI implementations. NOTE: During ACPICA initialization,
   * this value is set to TRUE if any Windows OSI strings have been
 diff --git a/drivers/acpi/acpica/tbfadt.c b/drivers/acpi/acpica/tbfadt.c
 index ec14588254d4..6dd5c590e0bb 100644
 --- a/drivers/acpi/acpica/tbfadt.c
 +++ b/drivers/acpi/acpica/tbfadt.c
 @@ -56,10 +56,9 @@ acpi_tb_init_generic_address(struct acpi_generic_address 
 *generic_address,
 
  static void acpi_tb_convert_fadt(void);
 
 -static void acpi_tb_setup_fadt_registers(void);
 +static void acpi_tb_validate_fadt(void);
 
 -static u64
 -acpi_tb_select_address(char *register_name, u32 address32, u64 address64);
 +static void acpi_tb_setup_fadt_registers(void);
 
  /* Table for conversion of FADT to common internal format and FADT 
 validation */
 
 @@ -176,7 +175,6 @@ static struct acpi_fadt_pm_info fadt_pm_info_table[] = {
   *  space_id- ACPI Space ID for this register
   *  byte_width  - Width of this register
   *  address - Address of the register
 - *  register_name   - ASCII name of the ACPI register
   *
   * RETURN:  None
   *
 @@ -222,68 +220,6 @@ acpi_tb_init_generic_address(struct acpi_generic_address 
 *generic_address,
 
  
 /***
   *
 - * FUNCTION:acpi_tb_select_address
 - *
 - * PARAMETERS:  register_name   - ASCII name of the ACPI register
 - *  address32   - 32-bit address of the register
 - *  address64   - 64-bit address of the register
 - *
 - * RETURN:  The resolved 64-bit address
 - *
 - * DESCRIPTION: Select between 32-bit and 64-bit versions of addresses within
 - *  the FADT. Used for the FACS and DSDT addresses.
 - *
 - * NOTES:
 - *
 - * Check for FACS and DSDT address mismatches. An address mismatch between
 - * the 32-bit and 64-bit address fields (FIRMWARE_CTRL/X_FIRMWARE_CTRL and
 - * DSDT/X_DSDT) could be a corrupted address field or it might indicate
 - * the presence of two FACS or two DSDT tables.
 - *
 - * November 2013:
 - * By default, as per the ACPICA specification, a valid 64-bit address is
 - * used regardless of the value of the 32-bit address. However, this
 - * behavior can be overridden via the acpi_gbl_use32_bit_fadt_addresses flag.
 - *
 - 
 **/
 -
 -static u64
 -acpi_tb_select_address(char *register_name, u32 address32, u64 address64)
 -{
 -
 - if (!address64) {
 -
 - /* 64-bit address is zero, use 32-bit address */
 -
 - return ((u64)address32);
 - }
 -
 - if (address32  (address64 !=

RE: [PATCH v3 1/2] ACPI: Cleanup to convert acpi.no_static_ssdt into a compile-time configurable.

2014-05-19 Thread Zheng, Lv

Hi, Rafael

 From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
 Sent: Tuesday, May 20, 2014 6:52 AM
 Subject: Re: [PATCH v3 1/2] ACPI: Cleanup to convert acpi.no_static_ssdt into 
 a compile-time configurable.

 On Tuesday, May 20, 2014 12:49:04 AM Rafael J. Wysocki wrote:
  On Monday, May 12, 2014 03:49:59 PM Lv Zheng wrote:
   User can specify a DSDT with SSDT embedded, in which case, no_static_ssdt
   must be enforced during compile-time.  If we don't do that and forget to
   specify acpi.no_static_ssdt as a boot parameter, then:
   1. The AML executable will be executed twice, the second execution is
  running under a changed environment.
   2. The namespace object conflicts will result in an AE_ALREADY_EXISTS
  exception;
   3. The namespace objects that are deleted from the original SSDT will be
  restored by the auto loading of the original SSDT.

   Note that:
   1. The DSDT customization is a compile-time feature, thus the SSDT
  inclusion indication of the DSDT customization should also be
  implemented as a compile-time configurable.
   2. According to the commit log:
   Commit: 67effe8fff32f60bdf51cba484766ba6003005bb
   Subject: ACPI: add acpi_no_auto_ssdt bootparam
  The acpi.no_static_ssdt (originally acpi.no_auto_ssdt) was introduced 
   to
  be used for this use case.  And over a long time, there is no other
  situation than this requires this boot parameter.
   This patch thus converts the runtime kernel parameter into the 
   compile-time
   configuration item.

   References: https://bugzilla.kernel.org/show_bug.cgi?id=3774
   Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=69711
   Original-by: Enrico Etxe Arte goitizena.gene...@gmail.com
   Signed-off-by: Lv Zheng lv.zh...@intel.com
   ---
Documentation/kernel-parameters.txt |   10 --
drivers/acpi/Kconfig|   32 
   ++--
drivers/acpi/internal.h |5 +
drivers/acpi/osl.c  |8 +++-
drivers/acpi/tables.c   |2 ++
5 files changed, 36 insertions(+), 21 deletions(-)

   diff --git a/Documentation/kernel-parameters.txt 
   b/Documentation/kernel-parameters.txt
   index 831fb1e..ceba0d6 100644
   --- a/Documentation/kernel-parameters.txt
   +++ b/Documentation/kernel-parameters.txt
   @@ -237,16 +237,6 @@ bytes respectively. Such letter suffixes can also be 
   entirely omitted.
 This feature is enabled by default.
 This option allows to turn off the feature.

   - acpi_no_static_ssdt [HW,ACPI]
   - Disable installation of static SSDTs at early boot time
   - By default, SSDTs contained in the RSDT/XSDT will be
   - installed automatically and they will appear under
   - /sys/firmware/acpi/tables.
   - This option turns off this feature.
   - Note that specifying this option does not affect
   - dynamic table installation which will install SSDT
   - tables to /sys/firmware/acpi/tables/dynamic.
   -
 acpica_no_return_repair [HW, ACPI]
 Disable AML predefined validation mechanism
 This mechanism can repair the evaluation result to make
   diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
   index c0160bb..51dc3b8 100644
   --- a/drivers/acpi/Kconfig
   +++ b/drivers/acpi/Kconfig
   @@ -216,22 +216,40 @@ config ACPI_NUMA
 depends on (X86 || IA64)
 default y if IA64_GENERIC || IA64_SGI_SN2

   -config ACPI_CUSTOM_DSDT_FILE
   - string Custom DSDT Table file to include
   - default 
   +menu Table override
   +
   +menuconfig ACPI_CUSTOM_DSDT

  I would make this a choice, like the Kernel compression mode (in 
  init/Kconfig).

  Then ACPI_CUSTOM_DSDT_ONLY and ACPI_CUSTOM_DSDT_WITH_SSDT may be the options
  and they both would select ACPI_CUSTOM_DSDT (otherwise invisible to the 
  user),

 With the third (and default) option NO_ACPI_CUSTOM_DSDT that would not
 select ACPI_CUSTOM_DSDT.

OK.

Thanks
-Lv

 Thanks!

 --
 I speak only for myself.
 Rafael J. Wysocki, Intel Open Source Technology Center.
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH] ACPICA: Revert ACPICA: Add option to favor 32-bit FADT addresses.

2014-05-12 Thread Zheng, Lv

Hi, Rafael

I checked the bug.

The dmesg of the kernel without the bisected commit:
[0.00] ACPI BIOS Warning (bug): Incorrect checksum in table [XSDT] - 
0xA0, should be 0xC9 (20140214/tbprint-218)
[0.00] ACPI Warning: 32/64 FACS address mismatch in FADT - two FACS 
tables! (20140214/tbfadt-395)
[0.00] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT - 
0xCF661F40/0xCF667E40, using 32 (20140214/tbfadt-522)

The dmesg of the kernel with the bisected commit:
[0.00] ACPI BIOS Warning (bug): Incorrect checksum in table [XSDT] - 
0xA0, should be 0xC9 (20131218/tbprint-214)
[0.00] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT: 
0xCF661F40/0xCF667E40, using 64-bit address (20131218/tbfadt-271)

This is the purpose of the bisected commit.
According to the link below:
http://bugs.acpica.org/show_bug.cgi?id=885
And Windows documentation:
http://download.microsoft.com/download/5/b/9/5b97017b-e28a-4bae-ba48-174cf47d23cd/CPA002_WH06.ppt
We believe 64-bit addresses should be used by default so that new features can 
be enabled according to the public knowledge of Windows Vista+ behavior.
For old Windows, it's hard for us to guess, we should wait for the reports and 
add quirks for them.

Thus this commit is not wrong, it shouldn't be reverted.
Though this platform is newer than vista, we still should offer a quirk 
mechanism for it as a quick fix:
Release Date: 01/21/2010

What's your opinion?

Thanks and best regards
-Lv

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, Lv
 Sent: Monday, May 12, 2014 9:10 AM
 
  From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
  Sent: Monday, May 12, 2014 8:12 AM
 
  From: Rafael J. Wysocki rafael.j.wyso...@intel.com
 
  Revert commit 0249ed2444d6 (ACPICA: Add option to favor 32-bit FADT
  addresses.) that breaks resume from system suspend on the Intel DP45SG
  board.
 
 This commit doesn't look like the root cause.
 [0.00] ACPI BIOS Warning (bug): Incorrect checksum in table [XSDT] - 
 0xA0, should be 0xC9 (20131218/tbprint-214)
 [0.00] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT: 
 0xCF661F40/0xCF667E40, using 64-bit address
 (20131218/tbfadt-271)
 It seems before using the FADT, ACPICA have already detected that the XSDT is 
 not correct.
 
 Please revert it for now.  I'll take a look at this bug.
 
 Thanks and best regards
 -Lv
 
 
  Fixes: 0249ed2444d6 (ACPICA: Add option to favor 32-bit FADT addresses.)
  References: https://bugzilla.kernel.org/show_bug.cgi?id=74021
  Reported-and-tested-by: Oswald Buddenhagen o...@kde.org
  Cc: 3.14+ sta...@vger.kernel.org # 3.14+
  Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
  ---
   drivers/acpi/acpica/acglobal.h |  10 --
   drivers/acpi/acpica/tbfadt.c   | 335 
  +++--
   include/acpi/acpixf.h  |   1 -
   3 files changed, 152 insertions(+), 194 deletions(-)
 
  diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h
  index 49bbc71fad54..72578a4b8212 100644
  --- a/drivers/acpi/acpica/acglobal.h
  +++ b/drivers/acpi/acpica/acglobal.h
  @@ -136,16 +136,6 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_copy_dsdt_locally, 
  FALSE);
   ACPI_INIT_GLOBAL(u8, acpi_gbl_do_not_use_xsdt, FALSE);
 
   /*
  - * Optionally use 32-bit FADT addresses if and when there is a conflict
  - * (address mismatch) between the 32-bit and 64-bit versions of the
  - * address. Although ACPICA adheres to the ACPI specification which
  - * requires the use of the corresponding 64-bit address if it is non-zero,
  - * some machines have been found to have a corrupted non-zero 64-bit
  - * address. Default is FALSE, do not favor the 32-bit addresses.
  - */
  -ACPI_INIT_GLOBAL(u8, acpi_gbl_use32_bit_fadt_addresses, FALSE);
  -
  -/*
* Optionally truncate I/O addresses to 16 bits. Provides compatibility
* with other ACPI implementations. NOTE: During ACPICA initialization,
* this value is set to TRUE if any Windows OSI strings have been
  diff --git a/drivers/acpi/acpica/tbfadt.c b/drivers/acpi/acpica/tbfadt.c
  index ec14588254d4..6dd5c590e0bb 100644
  --- a/drivers/acpi/acpica/tbfadt.c
  +++ b/drivers/acpi/acpica/tbfadt.c
  @@ -56,10 +56,9 @@ acpi_tb_init_generic_address(struct acpi_generic_address 
  *generic_address,
 
   static void acpi_tb_convert_fadt(void);
 
  -static void acpi_tb_setup_fadt_registers(void);
  +static void acpi_tb_validate_fadt(void);
 
  -static u64
  -acpi_tb_select_address(char *register_name, u32 address32, u64 address64);
  +static void acpi_tb_setup_fadt_registers(void);
 
   /* Table for conversion of FADT to common internal format and FADT 
  validation */
 
  @@ -176,7 +175,6 @@ static struct acpi_fadt_pm_info fadt_pm_info_table[] = {
*  space_id- ACPI Space ID for this register
*  byte_width  - Width of this register

RE: [PATCH] ACPICA: Revert ACPICA: Add option to favor 32-bit FADT addresses.

2014-05-12 Thread Zheng, Lv

Hi,

From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
Sent: Tuesday, May 13, 2014 8:09 AM

On Monday, May 12, 2014 08:51:36 AM Zheng, Lv wrote:
Hi, Rafael

I checked the bug.

The dmesg of the kernel without the bisected commit:
[0.00] ACPI BIOS Warning (bug): Incorrect checksum in table [XSDT]
- 0xA0, should be 0xC9 (20140214/tbprint-218)
[0.00] ACPI Warning: 32/64 FACS address mismatch in FADT - two FACS
tables! (20140214/tbfadt-395)
[0.00] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in
FADT - 0xCF661F40/0xCF667E40, using 32
(20140214/tbfadt-522)

The dmesg of the kernel with the bisected commit:
[0.00] ACPI BIOS Warning (bug): Incorrect checksum in table [XSDT]
- 0xA0, should be 0xC9 (20131218/tbprint-214)
[0.00] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in
FADT: 0xCF661F40/0xCF667E40, using 64-bit
address (20131218/tbfadt-271)

This is the purpose of the bisected commit.
According to the link below:
http://bugs.acpica.org/show_bug.cgi?id=885
And Windows documentation:
http://download.microsoft.com/download/5/b/9/5b97017b-e28a-4bae-ba48-174cf47d23cd/CPA002_WH06.ppt
We believe 64-bit addresses should be used by default so that new features
can be enabled according to the public knowledge of
Windows Vista+ behavior.
For old Windows, it's hard for us to guess, we should wait for the reports
and add quirks for them.

Thus this commit is not wrong, it shouldn't be reverted.

It is wrong, because it breaks a system that worked without it.

It's *that* simple.

For this commit, we knew there would be systems broken.
And was prepared to add quirks for them.
The quirks are not there just because we rely on end users to report.

And either you have a fix for that (which is not a quirk, because there may be
more machines like that), or we have to revert it.

Though this platform is newer than vista, we still should offer a quirk
mechanism
for it as a quick fix:

We didn't need a quirk for it before, though.

But according to BZ885, we need more quirks for other machines before.
For example, ThinkPad 40e and ThinkPad 51e reported in the BZ885.

So really, I'm reverting it.

OK.
I'll first try to figure out the cause of the issue that is happening to Intel
DP45SG.
And then try this approach again in a smarter way that is more tolerant torward
the possible regressions.

Thanks and best regards
-Lv

Thanks!

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH] ACPICA: Revert ACPICA: Add option to favor 32-bit FADT addresses.

2014-05-12 Thread Zheng, Lv

Hi, Rafael

From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
Sent: Tuesday, May 13, 2014 9:31 AM
To: Zheng, Lv
Cc: robertbmo...@compuserve.com; Thomas Renninger (tr...@suse.de); Oswald
Buddenhagen; ACPI Devel Maling List; Linux
Kernel Mailing List
Subject: Re: [PATCH] ACPICA: Revert ACPICA: Add option to favor 32-bit FADT
addresses.

On Tuesday, May 13, 2014 01:05:59 AM Zheng, Lv wrote:
Hi,

From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
Sent: Tuesday, May 13, 2014 8:09 AM

On Monday, May 12, 2014 08:51:36 AM Zheng, Lv wrote:
Hi, Rafael

I checked the bug.

The dmesg of the kernel without the bisected commit:
[0.00] ACPI BIOS Warning (bug): Incorrect checksum in table
[XSDT] - 0xA0, should be 0xC9 (20140214/tbprint-218)
[0.00] ACPI Warning: 32/64 FACS address mismatch in FADT - two
FACS tables! (20140214/tbfadt-395)
[0.00] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in
FADT - 0xCF661F40/0xCF667E40, using 32
(20140214/tbfadt-522)

The dmesg of the kernel with the bisected commit:
[0.00] ACPI BIOS Warning (bug): Incorrect checksum in table
[XSDT] - 0xA0, should be 0xC9 (20131218/tbprint-214)
[0.00] ACPI BIOS Warning (bug): 32/64X FACS address mismatch in
FADT: 0xCF661F40/0xCF667E40, using 64-bit
address (20131218/tbfadt-271)

This is the purpose of the bisected commit.
According to the link below:
http://bugs.acpica.org/show_bug.cgi?id=885
And Windows documentation:
http://download.microsoft.com/download/5/b/9/5b97017b-e28a-4bae-ba48-174cf47d23cd/CPA002_WH06.ppt
We believe 64-bit addresses should be used by default so that new
features can be enabled according to the public knowledge
of
Windows Vista+ behavior.
For old Windows, it's hard for us to guess, we should wait for the
reports and add quirks for them.

Thus this commit is not wrong, it shouldn't be reverted.

It is wrong, because it breaks a system that worked without it.

It's *that* simple.

For this commit, we knew there would be systems broken.
And was prepared to add quirks for them.
The quirks are not there just because we rely on end users to report.

And either you have a fix for that (which is not a quirk, because there
may be
more machines like that), or we have to revert it.

Though this platform is newer than vista, we still should offer a quirk
mechanism
for it as a quick fix:

We didn't need a quirk for it before, though.

But according to BZ885, we need more quirks for other machines before.
For example, ThinkPad 40e and ThinkPad 51e reported in the BZ885.

So really, I'm reverting it.

OK.
I'll first try to figure out the cause of the issue that is happening to
Intel DP45SG.
And then try this approach again in a smarter way that is more tolerant
torward the possible regressions.

Great, thanks!

If you have troubles in reverting things.
I can offer one line patch to revert the functionality of the bisected commit
back to the original behavior.
In acpixf.h, make acpi_gbl_use32_bit_fadt_address to TRUE can be a fix.
I'll ask for the test on the bugzilla.

Thanks and best regards
-Lv

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH] ACPI/Battery: Retry to get Battery information if failed during probing

2014-06-15 Thread Zheng, Lv

Hi,

 From: linux-acpi-ow...@vger.kernel.org 
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Lan Tianyu
 Sent: Monday, June 16, 2014 10:12 AM
 To: David Rientjes
 Cc: r...@rjwysocki.net; l...@kernel.org; nas...@ya.ru; 
 linux-a...@vger.kernel.org; linux-kernel@vger.kernel.org
 Subject: Re: [PATCH] ACPI/Battery: Retry to get Battery information if failed 
 during probing
 
 On 2014年06月14日 05:46, David Rientjes wrote:
  On Fri, 13 Jun 2014, Lan Tianyu wrote:
 
  How about this?
 
  -   result = acpi_battery_update(battery, false);
  -   if (result)
  +
  +   /*
  +* Some machines'(E,G Lenovo Z480) ECs are not stable

Just a reminder.

This statement may not be true.
The issue may be caused by the EC driver itself.
So we need to investigate.

  +* during boot up and this causes battery driver fails to be
  +* probed due to failure of getting battery information
  +* from EC sometimes. After several retries, the operation
  +* may work. So add retry code here and 20ms sleep between
  +* every retries.
  +*/
  +   while (acpi_battery_update(battery, false)  retry--)

If EC hardware is stable, why we need to do retry here?

Thanks and best regards
-Lv

  +   msleep(20);
  +   if (!retry) {
  +   result = -ENODEV;
  goto fail;
  +   }
  +
 
  I think you want --retry and not retry--.
 
 My original purpose is to retry 5 times after the first try fails.
 If use --retry here, it just retries 4 times.
 
   Otherwise it's possible for the
  final call to acpi_battery_update() to succeed and now it's returning
  -ENODEV.
 
 
 Yes, it maybe and I will change code like the following.
 
 while ((result = acpi_battery_update(battery, false))  retry--)
msleep(20);
 if (result)
goto fail;
 
 
 --
 Best regards
 Tianyu Lan
 --
 To unsubscribe from this list: send the line unsubscribe linux-acpi in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/7] ACPICA: Only include ACPI asm files if ACPI is enabled

Hi, Lee

 From: Lee Jones [mailto:lee.jo...@linaro.org]
 Sent: Wednesday, June 04, 2014 8:10 PM
 
 Any drivers which support ACPI and Device Tree probing need to include
 both respective header files.  Without this patch, if a driver is being
 used on a platform which does not support ACPI and subsequently does not
 have the config option enabled, but includes linux/acpi.h the build
 breaks with:
 
   In file included from ../include/acpi/platform/acenv.h:150:0,
from ../include/acpi/acpi.h:56,
from ../include/linux/match.h:2,
from ../drivers/i2c/i2c-core.c:43:
   ../include/acpi/platform/aclinux.h:73:23:
fatal error: asm/acenv.h: No such file or directory
#include asm/acenv.h
^
 Cc: Lv Zheng lv.zh...@intel.com
 Cc: Rafael J. Wysocki rafael.j.wyso...@intel.com
 Cc: linux-a...@vger.kernel.org
 Cc: de...@acpica.org
 Signed-off-by: Lee Jones lee.jo...@linaro.org
 ---
  include/acpi/platform/aclinux.h | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
 
 diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
 index cd1f052..fdf7663 100644
 --- a/include/acpi/platform/aclinux.h
 +++ b/include/acpi/platform/aclinux.h
 @@ -70,9 +70,10 @@
  #ifdef EXPORT_ACPI_INTERFACES
  #include linux/export.h
  #endif
 -#include asm/acenv.h
 
 -#ifndef CONFIG_ACPI
 +#ifdef CONFIG_ACPI
 +#include asm/acenv.h
 +#else

This is exactly what I want to do in the next step.
But you are a bit faster here.
I believe:
The miss-ordered inclusions of asm/acpi.h is the culprit of all of the 
miss-ordered inclusions in arch/x86/include/asm.
You should have noted that asm/acpi.h was originally unexpected included by 
some x86 specific headers.
Simply doing asm/acenv.h exlusion in this way might be able to fix your issue 
for your architecture, but it could be very likely breaking x86 builds.
You might be able to find another way to solve your build issue - for example, 
creating an empty asm/acenv.h for arch/arm.

Thanks and best regards
-Lv

 
  /* External globals for __KERNEL__, stubs is needed */
 
 --
 1.8.3.2

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/7] ACPICA: Only include ACPI asm files if ACPI is enabled

Hi,

 From: linux-i2c-ow...@vger.kernel.org 
 [mailto:linux-i2c-ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki
 Sent: Thursday, June 05, 2014 5:30 AM
 
 On Wednesday, June 04, 2014 01:51:37 PM Lee Jones wrote:
  On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:
 
   On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
Any drivers which support ACPI and Device Tree probing need to include
both respective header files.  Without this patch, if a driver is being
used on a platform which does not support ACPI and subsequently does not
have the config option enabled, but includes linux/acpi.h the build
breaks with:
   
  In file included from ../include/acpi/platform/acenv.h:150:0,
   from ../include/acpi/acpi.h:56,
   from ../include/linux/match.h:2,
   from ../drivers/i2c/i2c-core.c:43:
  ../include/acpi/platform/aclinux.h:73:23:
   fatal error: asm/acenv.h: No such file or directory
   #include asm/acenv.h
   ^
  
   Which kernel does this happen with?
 
  a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)
Add linux-next specific files for 20140602
 
 It looks like the problem is with include/linux/match.h that should not
 include acpi/acpi.h directly.

This is another example that many mis-ordered inclusions are caused by the 
mis-ordered asm/acpi.h inclusion.

 
 But I can't find this file in the Linus' next branch even, so I guess it's
 on its way to that branch?
 

I guess,
In their tree, they have CONFIG_ACPI enabled for ARM, but we've changed to make:
1. asm/acenv.h the architecture specific layer for ACPICA, and
2. asm/acpi.h is now the architecture specific layer for Linux ACPI.
So they need to follow this.

Thanks and best regards
-Lv

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

RE: [PATCH 1/7] ACPICA: Only include ACPI asm files if ACPI is enabled

Hi, Lee

 From: Lee Jones [mailto:lee.jo...@linaro.org]
 Sent: Wednesday, June 04, 2014 8:52 PM
 To: Rafael J. Wysocki

 On Wed, 04 Jun 2014, Rafael J. Wysocki wrote:

  On Wednesday, June 04, 2014 01:09:50 PM Lee Jones wrote:
   Any drivers which support ACPI and Device Tree probing need to include
   both respective header files.  Without this patch, if a driver is being
   used on a platform which does not support ACPI and subsequently does not
   have the config option enabled, but includes linux/acpi.h the build
   breaks with:

 In file included from ../include/acpi/platform/acenv.h:150:0,
  from ../include/acpi/acpi.h:56,
  from ../include/linux/match.h:2,
  from ../drivers/i2c/i2c-core.c:43:
 ../include/acpi/platform/aclinux.h:73:23:
  fatal error: asm/acenv.h: No such file or directory
  #include asm/acenv.h
  ^

Note that:
In our tree:
asm/acenv.h is only included by acpi/acpi.h.
And acpi/acpi.h is only included by
1. linux/acpi.h when CONFIG_ACPI enabled
2. linux/sfi_acpi.h - this is x86 specific, we'll clean it up by implementing 
stubs for all ACPI external interfaces.
So there is no case we need to exclude asm/acenv.h when CONFIG_ACPI is not 
enabled.

I cannot find linux/match.h here.
If linux/match.h want to include ACPI features, it shouldn't include 
acpi/acpi.h, but should include linux/acpi.h.
Please refer to:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8b48463f
And stop including acpi/acpi.h directly in any cases.

Thanks and best regards
-Lv

  Which kernel does this happen with?

 a0a962d (tag: refs/tags/next-20140602, refs/remotes/next/master)
   Add linux-next specific files for 20140602

   Cc: Lv Zheng lv.zh...@intel.com
   Cc: Rafael J. Wysocki rafael.j.wyso...@intel.com
   Cc: linux-a...@vger.kernel.org
   Cc: de...@acpica.org
   Signed-off-by: Lee Jones lee.jo...@linaro.org
   ---
include/acpi/platform/aclinux.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

   diff --git a/include/acpi/platform/aclinux.h 
   b/include/acpi/platform/aclinux.h
   index cd1f052..fdf7663 100644
   --- a/include/acpi/platform/aclinux.h
   +++ b/include/acpi/platform/aclinux.h
   @@ -70,9 +70,10 @@
#ifdef EXPORT_ACPI_INTERFACES
#include linux/export.h
#endif
   -#include asm/acenv.h

   -#ifndef CONFIG_ACPI
   +#ifdef CONFIG_ACPI
   +#include asm/acenv.h
   +#else

/* External globals for __KERNEL__, stubs is needed */

 --
 Lee Jones
 Linaro STMicroelectronics Landing Team Lead
 Linaro.org │ Open source software for ARM SoCs
 Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH 1/7] ACPICA: Only include ACPI asm files if ACPI is enabled