[PATCH v2] Introduce AcpiOs(Read|Write)Cmos and enable these in AcpiExCmosSpaceHandler
by Adam Goode
This introduces a generic OSL interface for reading and writing
SystemCMOS spaces. It is needed on Linux for registering a generic
handler for SystemCMOS because some firmware accesses CMOS in _INI.
(The first time I sent a somewhat corrupted patch file. This one should
be better).
---
source/components/events/evhandler.c | 1 +
source/components/executer/exregion.c | 23 +++++++++
source/include/acconfig.h | 2 +-
source/include/acpiosxf.h | 18 +++++++
source/os_specific/service_layers/osunixxf.c | 64 +++++++++++++++++++++++
source/os_specific/service_layers/oswinxf.c | 64 +++++++++++++++++++++++
tests/aapits/atosxfctrl.c | 2 +
tests/aapits/atosxfctrl.h | 3 ++
tests/aapits/atosxfwrap.c | 76 ++++++++++++++++++++++++++++
tests/aapits/atosxfwrap.h | 16 ++++++
10 files changed, 268 insertions(+), 1 deletion(-)
diff --git a/source/components/events/evhandler.c b/source/components/events/evhandler.c
index d02ea53..634c18e 100644
--- a/source/components/events/evhandler.c
+++ b/source/components/events/evhandler.c
@@ -139,6 +139,7 @@ UINT8 AcpiGbl_DefaultAddressSpaces[ACPI_NUM_DEFAULT_SPACES] =
ACPI_ADR_SPACE_SYSTEM_MEMORY,
ACPI_ADR_SPACE_SYSTEM_IO,
ACPI_ADR_SPACE_PCI_CONFIG,
+ ACPI_ADR_SPACE_CMOS,
ACPI_ADR_SPACE_DATA_TABLE
};
diff --git a/source/components/executer/exregion.c b/source/components/executer/exregion.c
index 31bbb60..bd9addd 100644
--- a/source/components/executer/exregion.c
+++ b/source/components/executer/exregion.c
@@ -551,6 +551,29 @@ AcpiExCmosSpaceHandler (
ACPI_FUNCTION_TRACE (ExCmosSpaceHandler);
+ ACPI_DEBUG_PRINT ((ACPI_DB_INFO,
+ "System-CMOS (width %u) R/W %u Address=%8.8X%8.8X\n",
+ BitWidth, Function, ACPI_FORMAT_UINT64(Address)));
+
+ switch (Function)
+ {
+ case ACPI_READ:
+
+ *Value = 0;
+ Status = AcpiOsReadCmos(Address, Value, BitWidth);
+ break;
+
+ case ACPI_WRITE:
+
+ Status = AcpiOsWriteCmos(Address, *Value, BitWidth);
+ break;
+
+ default:
+
+ Status = AE_BAD_PARAMETER;
+ break;
+ }
+
return_ACPI_STATUS (Status);
}
diff --git a/source/include/acconfig.h b/source/include/acconfig.h
index 41fa33d..dcecfa7 100644
--- a/source/include/acconfig.h
+++ b/source/include/acconfig.h
@@ -270,7 +270,7 @@
/* Maximum SpaceIds for Operation Regions */
#define ACPI_MAX_ADDRESS_SPACE 255
-#define ACPI_NUM_DEFAULT_SPACES 4
+#define ACPI_NUM_DEFAULT_SPACES 5
/* Array sizes. Used for range checking also */
diff --git a/source/include/acpiosxf.h b/source/include/acpiosxf.h
index 68d79c7..31523ea 100644
--- a/source/include/acpiosxf.h
+++ b/source/include/acpiosxf.h
@@ -452,6 +452,24 @@ AcpiOsWritePort (
UINT32 Width);
#endif
+/*
+ * Platform and hardware-independent CMOS memory interfaces
+ */
+#ifndef ACPI_USE_ALTERNATE_PROTOTYPE_AcpiOsReadCmos
+ACPI_STATUS
+AcpiOsReadCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 *Value,
+ UINT32 Width);
+#endif
+
+#ifndef ACPI_USE_ALTERNATE_PROTOTYPE_AcpiOsWriteCmos
+ACPI_STATUS
+AcpiOsWriteCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 Value,
+ UINT32 Width);
+#endif
/*
* Platform and hardware-independent physical memory interfaces
diff --git a/source/os_specific/service_layers/osunixxf.c b/source/os_specific/service_layers/osunixxf.c
index c48f581..b763ddc 100644
--- a/source/os_specific/service_layers/osunixxf.c
+++ b/source/os_specific/service_layers/osunixxf.c
@@ -1361,6 +1361,70 @@ AcpiOsWritePort (
}
+/*****************************************************************************
+ *
+ * FUNCTION: AcpiOsReadCmos
+ *
+ * PARAMETERS: Address - Address of CMOS memory to read
+ * Value - Where value is placed
+ * Width - Number of bits
+ *
+ * RETURN: Value read from CMOS memory
+ *
+ * DESCRIPTION: Read data from CMOS memory
+ *
+ *****************************************************************************/
+
+ACPI_STATUS
+AcpiOsReadCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 *Value,
+ UINT32 Width)
+{
+
+ switch (Width)
+ {
+ case 8:
+ case 16:
+ case 32:
+ case 64:
+
+ *Value = 0;
+ break;
+
+ default:
+
+ return (AE_BAD_PARAMETER);
+ }
+ return (AE_OK);
+}
+
+
+/******************************************************************************
+ *
+ * FUNCTION: AcpiOsWriteCmos
+ *
+ * PARAMETERS: Address - Address of CMOS memory to write
+ * Value - Value to write
+ * Width - Number of bits
+ *
+ * RETURN: None
+ *
+ * DESCRIPTION: Write data to CMOS memory
+ *
+ *****************************************************************************/
+
+ACPI_STATUS
+AcpiOsWriteCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 Value,
+ UINT32 Width)
+{
+
+ return (AE_OK);
+}
+
+
/******************************************************************************
*
* FUNCTION: AcpiOsReadMemory
diff --git a/source/os_specific/service_layers/oswinxf.c b/source/os_specific/service_layers/oswinxf.c
index 66ce8c8..80086fd 100644
--- a/source/os_specific/service_layers/oswinxf.c
+++ b/source/os_specific/service_layers/oswinxf.c
@@ -1356,6 +1356,70 @@ AcpiOsWritePort (
}
+/*****************************************************************************
+ *
+ * FUNCTION: AcpiOsReadCmos
+ *
+ * PARAMETERS: Address - Address of CMOS memory to read
+ * Value - Where value is placed
+ * Width - Number of bits
+ *
+ * RETURN: Value read from CMOS memory
+ *
+ * DESCRIPTION: Read data from CMOS memory
+ *
+ *****************************************************************************/
+
+ACPI_STATUS
+AcpiOsReadCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 *Value,
+ UINT32 Width)
+{
+
+ switch (Width)
+ {
+ case 8:
+ case 16:
+ case 32:
+ case 64:
+
+ *Value = 0;
+ break;
+
+ default:
+
+ return (AE_BAD_PARAMETER);
+ }
+ return (AE_OK);
+}
+
+
+/******************************************************************************
+ *
+ * FUNCTION: AcpiOsWriteCmos
+ *
+ * PARAMETERS: Address - Address of CMOS memory to write
+ * Value - Value to write
+ * Width - Number of bits
+ *
+ * RETURN: None
+ *
+ * DESCRIPTION: Write data to CMOS memory
+ *
+ *****************************************************************************/
+
+ACPI_STATUS
+AcpiOsWriteCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 Value,
+ UINT32 Width)
+{
+
+ return (AE_OK);
+}
+
+
/******************************************************************************
*
* FUNCTION: AcpiOsReadMemory
diff --git a/tests/aapits/atosxfctrl.c b/tests/aapits/atosxfctrl.c
index fe8b562..9a6b41c 100644
--- a/tests/aapits/atosxfctrl.c
+++ b/tests/aapits/atosxfctrl.c
@@ -160,6 +160,8 @@ const char *OsxfNames[] = {
"AcpiOsDerivePciId",
"AcpiOsReadPort",
"AcpiOsWritePort",
+ "AcpiOsReadCmos",
+ "AcpiOsWriteCmos",
"AcpiOsReadMemory",
"AcpiOsWriteMemory",
"AcpiOsSignal"};
diff --git a/tests/aapits/atosxfctrl.h b/tests/aapits/atosxfctrl.h
index 66a361d..2c8b9e5 100644
--- a/tests/aapits/atosxfctrl.h
+++ b/tests/aapits/atosxfctrl.h
@@ -162,6 +162,8 @@ typedef enum
AcpiOsDerivePciIdC,
AcpiOsReadPortC,
AcpiOsWritePortC,
+ AcpiOsReadCmosC,
+ AcpiOsWriteCmosC,
AcpiOsReadMemoryC,
AcpiOsWriteMemoryC,
AcpiOsSignalC,
@@ -229,6 +231,7 @@ typedef struct acpi_os_emul_reg
*/
#define EMUL_REG_SYS 0x01
#define EMUL_REG_IO 0x02
+#define EMUL_REG_CMOS 0x03
/*
* Fixed ACPI h/w emulated registers numbers
diff --git a/tests/aapits/atosxfwrap.c b/tests/aapits/atosxfwrap.c
index 7c5de39..0088212 100644
--- a/tests/aapits/atosxfwrap.c
+++ b/tests/aapits/atosxfwrap.c
@@ -1272,6 +1272,82 @@ AcpiOsWritePort (
}
+/*****************************************************************************
+ *
+ * FUNCTION: AcpiOsReadCmos
+ *
+ * PARAMETERS: Address - Address of CMOS memory to read
+ * Value - Where value is placed
+ * Width - Number of bits
+ *
+ * RETURN: Value read from CMOS memory
+ *
+ * DESCRIPTION: Read data from CMOS memory
+ *
+ *****************************************************************************/
+
+ACPI_STATUS
+AcpiOsReadCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 *Value,
+ UINT32 Width)
+{
+ AT_CTRL_DECL(AcpiOsReadCmos);
+
+ AT_CHCK_RET_STATUS(AcpiOsReadCmos);
+
+ if (!EMUL_REG_MODE) {
+ Status = AcpiOsActualReadCmos(Address, Value, Width);
+ }
+ else
+ {
+ Status = OsxfCtrlReadReg(EMUL_REG_CMOS, Address, Value, Width);
+ }
+
+ AT_CTRL_SUCCESS(AcpiOsReadCmos);
+
+ return (Status);
+}
+
+
+/******************************************************************************
+ *
+ * FUNCTION: AcpiOsWriteCmos
+ *
+ * PARAMETERS: Address - Address of CMOS memory to write
+ * Value - Value to write
+ * Width - Number of bits
+ *
+ * RETURN: None
+ *
+ * DESCRIPTION: Write data to CMOS memory
+ *
+ *****************************************************************************/
+
+ACPI_STATUS
+AcpiOsWriteCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 Value,
+ UINT32 Width)
+{
+ AT_CTRL_DECL(AcpiOsWriteCmos);
+
+ AT_CHCK_RET_STATUS(AcpiOsWriteCmos);
+
+ if (!EMUL_REG_MODE) {
+ Status = AcpiOsActualWriteCmos(Address, Value, Width);
+ }
+ else
+ {
+ Status = OsxfCtrlWriteReg(EMUL_REG_CMOS, Address, Value, Width);
+ }
+
+ AT_CTRL_SUCCESS(AcpiOsWriteCmos);
+
+ return (Status);
+}
+
+
/******************************************************************************
*
* FUNCTION: AcpiOsReadMemory
diff --git a/tests/aapits/atosxfwrap.h b/tests/aapits/atosxfwrap.h
index 27224d0..48e2100 100644
--- a/tests/aapits/atosxfwrap.h
+++ b/tests/aapits/atosxfwrap.h
@@ -324,6 +324,22 @@ AcpiOsActualWritePort (
/*
+ * Platform and hardware-independent CMOS memory interfaces
+ */
+ACPI_STATUS
+AcpiOsActualReadCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 *Value,
+ UINT32 Width);
+
+ACPI_STATUS
+AcpiOsActualWriteCmos (
+ ACPI_PHYSICAL_ADDRESS Address,
+ UINT64 Value,
+ UINT32 Width);
+
+
+/*
* Platform and hardware-independent physical memory interfaces
*/
ACPI_STATUS
--
2.3.7
6 years, 1 month
ACPICA version 20151218 released
by Moore, Robert
18 December 2015. Summary of changes for version 20151218:
This release is available at https://acpica.org/downloads
1) ACPICA kernel-resident subsystem:
Implemented per-AML-table execution of "module-level code" as individual ACPI tables are loaded into the namespace during ACPICA initialization. In other words, any module-level code within an AML table is executed immediately after the table is loaded, instead of batched and executed after all of the tables have been loaded. This provides compatibility with other ACPI implementations. ACPICA BZ 1219. Bob Moore, Lv Zheng, David Box.
To fully support the feature above, the default operation region handlers for the SystemMemory, SystemIO, and PCI_Config address spaces are now installed before any ACPI tables are loaded. This enables module-level code to access these address spaces during the table load and module-level code execution phase. ACPICA BZ 1220. Bob Moore, Lv Zheng, David Box.
Implemented several changes to the internal _REG support in conjunction with the changes above. Also, changes to the AcpiExec/AcpiNames/Examples utilities for the changes above. Although these tools were changed, host operating systems that simply use the default handlers for SystemMemory, SystemIO, and PCI_Config spaces should not require any update. Lv Zheng.
For example, in the code below, DEV1 is conditionally added to the namespace by the DSDT via module-level code that accesses an operation region. The SSDT references DEV1 via the Scope operator. DEV1 must be created immediately after the DSDT is loaded in order for the SSDT to successfully reference DEV1. Previously, this code would cause an AE_NOT_EXIST exception during the load of the SSDT. Now, this code is fully supported by ACPICA.
DefinitionBlock ("", "DSDT", 2, "Intel", "DSDT1", 1)
{
OperationRegion (OPR1, SystemMemory, 0x400, 32)
Field (OPR1, AnyAcc, NoLock, Preserve)
{
FLD1, 1
}
If (FLD1)
{
Device (\DEV1)
{
}
}
}
DefinitionBlock ("", "SSDT", 2, "Intel", "SSDT1", 1)
{
External (\DEV1, DeviceObj)
Scope (\DEV1)
{
}
}
Fixed an AML interpreter problem where control method invocations were not handled correctly when the invocation was itself a SuperName argument to another ASL operator. In these cases, the method was not invoked. ACPICA BZ 1002. Affects the following ASL operators that have a SuperName argument:
Store
Acquire, Wait
CondRefOf, RefOf
Decrement, Increment
Load, Unload
Notify
Signal, Release, Reset
SizeOf
Implemented automatic String-to-ObjectReference conversion support for packages returned by predefined names (such as _DEP). A common BIOS error is to add double quotes around an ObjectReference namepath, which turns the reference into an unexpected string object. This support detects the problem and corrects it before the package is returned to the caller that invoked the method. Lv Zheng.
Implemented extensions to the Concatenate operator. Concatenate now accepts any type of object, it is not restricted to simply Integer/String/Buffer. For objects other than these 3 basic data types, the argument is treated as a string containing the name of the object type. This expands the utility of Concatenate and the Printf/Fprintf macros. ACPICA BZ 1222.
Cleaned up the output of the ASL Debug object. The timer() value is now optional and no longer emitted by default. Also, the basic data types of Integer/String/Buffer are simply emitted as their values, without a data type string -- since the data type is obvious from the output. ACPICA BZ 1221.
Example Code and Data Size: These are the sizes for the OS-independent acpica.lib produced by the Microsoft Visual C++ 9.0 32-bit compiler. The debug version of the code includes the debug output trace mechanism and has a much larger code and data size.
Current Release:
Non-Debug Version: 102.6K Code, 28.4K Data, 131.0K Total
Debug Version: 200.3K Code, 81.9K Data, 282.3K Total
Previous Release:
Non-Debug Version: 102.0K Code, 28.3K Data, 130.3K Total
Debug Version: 199.6K Code, 81.8K Data, 281.4K Total
2) iASL Compiler/Disassembler and Tools:
iASL: Fixed some issues with the ASL Include() operator. This operator was incorrectly defined in the iASL parser rules, causing a new scope to be opened for the code within the include file. This could lead to several issues, including allowing ASL code that is technically illegal and not supported by AML interpreters. Note, this does not affect the related #include preprocessor operator. ACPICA BZ 1212.
iASL/Disassembler: Implemented support for the ASL ElseIf operator. This operator is essentially an ASL macro since there is no AML opcode associated with it. The code emitted by the iASL compiler for ElseIf is an Else opcode followed immediately by an If opcode. The disassembler will now emit an ElseIf if it finds an Else immediately followed by an If. This simplifies the decoded ASL, especially for deeply nested If..Else and large Switch constructs. Thus, the disassembled code more closely follows the original source ASL. ACPICA BZ 1211. Example:
Old disassembly:
Else
{
If (Arg0 == 0x02)
{
Local0 = 0x05
}
}
New disassembly:
ElseIf (Arg0 == 0x02)
{
Local0 = 0x05
}
AcpiExec: Added support for the new module level code behavior and the early region installation. This required a small change to the initialization, since AcpiExec must install its own operation region handlers.
AcpiExec: Added support to make the debug object timer optional. Default is timer disabled. This cleans up the debug object output -- the timer data is rarely used.
AcpiExec: Multiple ACPI tables are now loaded in the order that they appear on the command line. This can be important when there are interdependencies/references between the tables.
iASL/Templates. Add support to generate template files with multiple SSDTs within a single output file. Also added ommand line support to specify the number of SSDTs (in addition to a single DSDT). ACPICA BZ 1223, 1225.
6 years, 5 months
ACPICA version 20151124 released
by Moore, Robert
24 November 2015. Summary of changes for version 20151124:
This release is available at https://acpica.org/downloads
1) ACPICA kernel-resident subsystem:
Fixed a possible regression for a previous update to FADT handling. The FADT no longer has a fixed table ID, causing some issues with code that was hardwired to a specific ID. Lv Zheng.
Fixed a problem where the method auto-serialization could interfere with the current SyncLevel. This change makes the auto-serialization support transparent to the SyncLevel support and management.
Removed support for the _SUB predefined name in AcpiGetObjectInfo. This interface is intended for early access to the namespace during the initial namespace device discovery walk. The _SUB method has been seen to access operation regions in some cases, causing errors because the operation regions are not fully initialized.
AML Debugger: Fixed some issues with the terminate/quit/exit commands that can cause faults. Lv Zheng.
AML Debugger: Add thread ID support so that single-step mode only applies to the AML Debugger thread. This prevents runtime errors within some kernels. Lv Zheng.
Eliminated extraneous warnings from AcpiGetSleepTypeData. Since the _Sx methods that are invoked by this interface are optional, removed warnings emitted for the case where one or more of these methods do not exist. ACPICA BZ 1208, original change by Prarit Bhargava.
Made a major pass through the entire ACPICA source code base to standardize formatting that has diverged a bit over time. There are no functional changes, but this will of course cause quite a few code differences from the previous ACPICA release.
Example Code and Data Size: These are the sizes for the OS-independent acpica.lib produced by the Microsoft Visual C++ 9.0 32-bit compiler. The debug version of the code includes the debug output trace mechanism and has a much larger code and data size.
Current Release:
Non-Debug Version: 102.0K Code, 28.3K Data, 130.3K Total
Debug Version: 199.6K Code, 81.8K Data, 281.4K Total
Previous Release:
Non-Debug Version: 101.7K Code, 27.9K Data, 129.6K Total
Debug Version: 199.3K Code, 81.4K Data, 280.7K Total
2) iASL Compiler/Disassembler and Tools:
iASL/acpiexec/acpixtract/disassembler: Added support to allow multiple definition blocks within a single ASL file and the resulting AML file. Support for this type of file was also added to the various tools that use binary AML files: acpiexec, acpixtract, and the AML disassembler. The example code below shows two definition blocks within the same file:
DefinitionBlock ("dsdt.aml", "DSDT", 2, "Intel", "Template", 0x12345678)
{
}
DefinitionBlock ("", "SSDT", 2, "Intel", "Template", 0xABCDEF01)
{
}
iASL: Enhanced typechecking for the Name() operator. All expressions for the value of the named object must be reduced/folded to a single constant at compile time, as per the ACPI specification (the AML definition of Name()).
iASL: Fixed some code indentation issues for the -ic and -ia options (C and assembly headers). Now all emitted code correctly begins in column 1.
iASL: Added an error message for an attempt to open a Scope() on an object defined in an SSDT. The DSDT is always loaded into the namespace first, so any attempt to open a Scope on an SSDT object will fail at runtime.
6 years, 5 months
Re: [Devel] [PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping
by Lukas Wunner
Hi Rafael,
a gentle ping, not sure if you just haven't had time to look at this
thread or if it's fallen between the cracks.
(Further comments from me below.)
On Wed, Dec 02, 2015 at 04:56:47PM +0800, Hanjun Guo wrote:
> On 2015/12/1 21:08, Lukas Wunner wrote:
> > Hi,
> >
> > On Mon, Nov 30, 2015 at 04:14:32PM +0800, Hanjun Guo wrote:
> >> On 2015/11/30 14:27, Hanjun Guo wrote:
> >>> On 2015/11/26 4:19, Lukas Wunner wrote:
> >>>> acpi_device_add() allocates and adds an element to acpi_bus_id_list
> >>>> (or increments the instance count if the device's HID is already
> >>>> present in the list), but the element is never deleted from the list
> >>>> nor freed. Fix it.
> >>> Hmm, I didn't get it here. Seems the device's ID already freed in device core:
> >>>
> >>> In acpi_add_single_object(), acpi_device_release() registered as a callback,
> >>> ...
> >>> result = acpi_device_add(device, acpi_device_release);
> >>> ...
> >>>
> >>> And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the
> >>> IDs, did I miss some something?
> >> Sorry, I misread the code, I thought it was the pnn ids connect to the ACPI device,
> >> actually you are referring to HIDs connecting to acpi_bus_id_list, sorry for the noise.
> > Yes, I should have been more clear about this in the first place:
> >
> > When the bus is scanned and acpi_device_add() is called for each device,
> > not only do we initialize a struct acpi_device and attach it to the
> > device tree, but we also add an element to acpi_bus_id_list.
> >
> > Hence there are two ways to detect the presence of a HID: By traversing
> > the device tree or by iterating over the list. I chose the latter because
> > it is obviously cheaper and requires less code.
> >
> > However elements only ever get added to the list, never deleted. I'm not
> > sure if hotpluggable ACPI devices exist but if they do, this is a bug
> > which is fixed by this patch.
>
> ACPI devices can be hotpluggable :) , but it will have no memory leak I think, it
> only increase the instance number for ACPI devices removed and hot-added later,
> I don't know if it make sense to do that, for example, if you remove device A and
> B with same HID (such as ACPI0007) with your patch added:
>
> remove processor 0, the sysfs for device A /sys/bus/acpi/devices/ACPI0007:00 will be
> removed;
>
> remove processer 1, the sysfs for device B /sys/bus/acpi/devices/ACPI0007:01 will be
> removed;
>
> But if we add it in reverse with your patch:
>
> Add dprocesser 1, the sysfs /sys/bus/acpi/devices/ACPI0007:00 will be created,
>
> add processor 0...
>
> I'm not sure it will confuse user space or not.
>
> Rafael, what's your opinion here?
Yes, with this patch the instance number is no longer strictly increasing
and if devices are unplugged and replaced, instance numbers are recycled
for the newly plugged devices. I have no idea if this can break things.
If this patch is not acceptable, the semantics of patch [v2 2/2] change
in that acpi_dev_present() doesn't return the presence of a HID at the
time of invocation, but rather whether such a device has ever been present
since boot. It wouldn't be a problem because acpi_dev_present() is
meant for presence detection of devices which are built into the system
(same goes for the PCI counterpart pci_dev_present(), see the kerneldoc
in drivers/pci/search.c). I'd have to adjust the kerneldoc though to
reflect the changed semantics.
The other option would be to traverse the acpi_device tree instead of
iterating over acpi_bus_id_list, then it would be possible to retain
the semantics of detecting presence at the moment of invocation.
Please let me know which of these options makes the most sense to you
so that I can adjust the patches.
Thank you,
Lukas
>
> Thanks
> Hanjun
>
6 years, 5 months
Re: [Devel] [PATCH v2 1/2] ACPI / scan: Fix acpi_bus_id_list bookkeeping
by Lukas Wunner
Hi,
On Mon, Nov 30, 2015 at 04:14:32PM +0800, Hanjun Guo wrote:
> On 2015/11/30 14:27, Hanjun Guo wrote:
> > On 2015/11/26 4:19, Lukas Wunner wrote:
> >> acpi_device_add() allocates and adds an element to acpi_bus_id_list
> >> (or increments the instance count if the device's HID is already
> >> present in the list), but the element is never deleted from the list
> >> nor freed. Fix it.
> > Hmm, I didn't get it here. Seems the device's ID already freed in device core:
> >
> > In acpi_add_single_object(), acpi_device_release() registered as a callback,
> > ...
> > result = acpi_device_add(device, acpi_device_release);
> > ...
> >
> > And in acpi_device_release(), it will call acpi_free_pnp_ids() to free the
> > IDs, did I miss some something?
>
> Sorry, I misread the code, I thought it was the pnn ids connect to the ACPI device,
> actually you are referring to HIDs connecting to acpi_bus_id_list, sorry for the noise.
Yes, I should have been more clear about this in the first place:
When the bus is scanned and acpi_device_add() is called for each device,
not only do we initialize a struct acpi_device and attach it to the
device tree, but we also add an element to acpi_bus_id_list.
Hence there are two ways to detect the presence of a HID: By traversing
the device tree or by iterating over the list. I chose the latter because
it is obviously cheaper and requires less code.
However elements only ever get added to the list, never deleted. I'm not
sure if hotpluggable ACPI devices exist but if they do, this is a bug
which is fixed by this patch.
The list was added with e49bd2dd5a50 ("ACPI: use PNPID:instance_no as
bus_id of ACPI device") 9 years ago and the author even mentioned that
the list elements are never freed:
"NOTE: Now I don't take the memory free work in charge.
If necessary, I can add a reference count in
struct acpi_device_bus_id, and check the reference and
when unregister a device, i.e. memory is freed when
the reference count of a given PNPID is 0."
Best regards,
Lukas
6 years, 5 months
Re: [Devel] [PATCH v2 2/2] ACPI / utils: Add acpi_dev_present()
by Lukas Wunner
Hi Hanjun Guo,
thank you for taking a look at the patches.
On Mon, Nov 30, 2015 at 04:27:23PM +0800, Hanjun Guo wrote:
> If the driver can't pass the "dev" as the argument form acpi device or
> platform device, this is the way to match HIDs.
>
> But if we get the pointer to acpi device or platform device structure
> before calling this function, I think acpi_match_device_ids() can match
> what you need, and we don't need to invent another function.
Of the 7 users,
* 4 detect the presence of a particular HID to activate platform-
specific quirks (acer-wmi.c, eeepc-wmi.c, thinkpad_helper.c,
cht_bsw_max98090_ti.c). The drivers never obtain a pointer to the
struct acpi_device. This is also the reason why I need to detect
the presence of apple-gmux in several DRM drivers, to activate
quirks specific to MacBook Pros with dual GPUs, and the one thing
they have in common is the presence of the gmux controller.
The DRM drivers likewise never have a need to hold a pointer to
apple-gmux' struct acpi_device.
* 2 detect the presence of a particular HID and *afterwards*
instantiate a platform_device (atom/sst/sst_acpi.c and
common/sst-acpi.c). So we can't use acpi_match_device_ids()
there either.
* 1 is a driver for a platform_device (cht_bsw_rt5645.c) which was
instantiated by atom/sst/sst_acpi.c. The driver is responsible
for two chips and differentiates between the two by detecting the
presence of a particular HID. It would be possible to refactor the
code so that atom/sst/sst_acpi.c passes down the matched HID to
cht_bsw_rt5645.c, then it wouldn't be necessary to match for a
second time. Also, the only difference between the two chipsets
seems to be a minute change in struct snd_soc_dapm_route, so I'm
wondering if it's necessary to differentiate between the chipsets
at all. Mark Brown may want to poke the developers of the driver to
refactor the code.
TL;DR: 6 of the 7 users can't use acpi_match_device_ids() and the
7th could be refactored so that it doesn't have to detect the presence
of a specific HID at all.
Best regards,
Lukas
6 years, 5 months
