Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751751AbbFZBjs (ORCPT ); Thu, 25 Jun 2015 21:39:48 -0400 Received: from mga03.intel.com ([134.134.136.65]:19232 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750920AbbFZBji (ORCPT ); Thu, 25 Jun 2015 21:39:38 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,681,1427785200"; d="scan'208";a="514423182" From: "Zheng, Lv" To: "Rafael J. Wysocki" CC: "Wysocki, Rafael J" , "Brown, Len" , Lv Zheng , "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "Moore, Robert" , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , "Luck, Tony" , "Yu, Fenghua" , "linux-ia64@vger.kernel.org" Subject: RE: [PATCH v2 03/28] ACPICA: Hardware: Enable 64-bit firmware waking vector for selected FACS. Thread-Topic: [PATCH v2 03/28] ACPICA: Hardware: Enable 64-bit firmware waking vector for selected FACS. Thread-Index: AQHQroNAAnvPf13OZUmHfFXX9mQIhZ27xp2AgACXNeCAARuogIAAh0vw Date: Fri, 26 Jun 2015 01:39:22 +0000 Message-ID: <1AE640813FDE7649BE1B193DEA596E88027340D8@SHSMSX101.ccr.corp.intel.com> References: <3959851.uJ0udqUY0b@vostro.rjw.lan> <1AE640813FDE7649BE1B193DEA596E8802733C14@SHSMSX101.ccr.corp.intel.com> <1649895.CEu8Ovje9D@vostro.rjw.lan> In-Reply-To: <1649895.CEu8Ovje9D@vostro.rjw.lan> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t5Q1dt2C026924 Content-Length: 3765 Lines: 87 Hi, Rafael > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > Sent: Friday, June 26, 2015 9:21 AM > > On Thursday, June 25, 2015 12:29:02 AM Zheng, Lv wrote: > > Hi, Rafael > > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > > > Sent: Thursday, June 25, 2015 7:24 AM > > > To: Zheng, Lv > > [cut] > > > > > > > In fact, I don't see why we need to redefine the symbols at all. > > > > > > Couldn't acpi_set_firmware_waking_vector() be defined to take u32 and u64 so > > > we could just pass acpi_wakeup_address (as already defined) as the first argument > > > and 0 as the second argument to it? The back-and-forth type casts from and > > > to acpi_physical_address don't look entirely clean to me. > > > > > > Moreover, I don't really see a functional difference between the old and the > > > new code. > > > > > > The old code does "set the 32-bit waking vector and clear the 64-bit waking > > > vector if present". The new code does "set the 32-bit waking vector and > > > either clear the 64-bit one if present, or assign the second function argument > > > to it", but we always pass 0 as the second argument (which is *extremely* > > > obfuscated in your patch), so I really don't see the difference here. > > > > > > Am I missing anything? > > > > The story is: > > According to the test, if both 32-bit waking vector and 64-bit waking vector is > > set by the OSPM, > > The current code in Linux never does that. > > It never calls acpi_set_firmware_waking_vector64() and the acpi_set_firmware_waking_vector() > (before your patch) explicitly clears the 64-bit vector address. In ACPICA upstream, this is an issue before applying this patch. acpi_set_firmware_waking_vector64() and acpi_set_firmware_waking_vector() depends on ACPI_MACHINE_WIDTH definition, thus cannot be used for this purpose. > > > BIOSes only support 32-bit resume environment will jump to the 32-bit waking > > vector address and BIOSes support 64-bit resume environment will jump to > > 64-bit waking vector. > > Which doesn't matter for Linux one whit. The bug report is against the 64-bit address favor mechanism. But if the OSPM can support resuming from 64-bit waking vector, the 64-bit address favor mechanism doesn't seem to be buggy. > > > So they can be set by the OSPMs simultaneously to indicate that the OSPM can > > support both resume environments. That's why ACPICA interface is changed. > > It shouldn't. It just forces host OSes to make pointless changes to their > non-ACPICA code. > > As I said elsewhere, the old acpi_set_firmware_waking_vector() should still be > available to the OSes that don't care about the 64-bit waking vector and a *new* > interface should be added for those OSes that do care about it. And *internally* > acpi_set_firmware_waking_vector() can be defined in terms of the new interface, > but there's no reason at all for a host OS to care about that. OK, we can refine the interface inside of ACPICA. > > So the $subject patch is entirely poitless. It doesn't fix anything and it > doesn't even change the way the code works today in Linux. It just adds > complexity and pointlessly redefines some stuff. > It doesn't fix any functionality problem right here in this patch. But it fixes the code logic problem that acpi_set_firmware_waking_vector64()/acpi_set_firmware_waking_vector() depends on ACPI_MACHINE_WIDTH definition. And it facilitates the OSPMs with the capability to support both 32-bit/64-bit resuming environment. > So I'm not going to apply it. OK, I'll go back to refine the interface change. Thanks and best regards -Lv ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?