Bug 142561

Comment 1 Mark Linimon 2010-01-10 02:08:01 UTC

Responsible Changed
From-To: freebsd-bugs->avg

Over to committer of the change in question.

The patch (to be applied in reverse):

===================================================================
Index: acpi_cpu.c
===================================================================
--- acpi_cpu.c	(revision 199942)
+++ acpi_cpu.c	(revision 199984)
@@ -255,7 +255,7 @@
 
     /* Mark this processor as in-use and save our derived id for attach. */
     cpu_softc[cpu_id] = (void *)1;
-    acpi_set_magic(dev, cpu_id);
+    acpi_set_private(dev, (void*)(intptr_t)cpu_id);
     device_set_desc(dev, "ACPI CPU");
 
     return (0);
@@ -286,7 +286,7 @@
     sc = device_get_softc(dev);
     sc->cpu_dev = dev;
     sc->cpu_handle = acpi_get_handle(dev);
-    cpu_id = acpi_get_magic(dev);
+    cpu_id = (int)(intptr_t)acpi_get_private(dev);
     cpu_softc[cpu_id] = sc;
     pcpu_data = pcpu_find(cpu_id);
     pcpu_data->pc_device = dev;
Index: acpi_hpet.c
===================================================================
--- acpi_hpet.c	(revision 199942)
+++ acpi_hpet.c	(revision 199984)
@@ -61,8 +61,6 @@
 
 static char *hpet_ids[] = { "PNP0103", NULL };
 
-#define DEV_HPET(x)	(acpi_get_magic(x) == (uintptr_t)&acpi_hpet_devclass)
-
 struct timecounter hpet_timecounter = {
 	.tc_get_timecount =	hpet_get_timecount,
 	.tc_counter_mask =	~0u,
@@ -133,8 +131,6 @@
 		return;
 	}
 
-	/* Record a magic value so we can detect this device later. */
-	acpi_set_magic(child, (uintptr_t)&acpi_hpet_devclass);
 	bus_set_resource(child, SYS_RES_MEMORY, 0, hpet->Address.Address,
 	    HPET_MEM_WIDTH);
 }
@@ -146,7 +142,7 @@
 
 	if (acpi_disabled("hpet"))
 		return (ENXIO);
-	if (!DEV_HPET(dev) &&
+	if (acpi_get_handle(dev) != NULL &&
 	    (ACPI_ID_PROBE(device_get_parent(dev), dev, hpet_ids) == NULL ||
 	    device_get_unit(dev) != 0))
 		return (ENXIO);
Index: acpi_ec.c
===================================================================
--- acpi_ec.c	(revision 199942)
+++ acpi_ec.c	(revision 199984)
@@ -129,9 +129,6 @@
     int		uid;
 };
 
-/* Indicate that this device has already been probed via ECDT. */
-#define DEV_ECDT(x)	(acpi_get_magic(x) == (uintptr_t)&acpi_ec_devclass)
-
 /*
  * Driver softc.
  */
@@ -332,7 +329,6 @@
     params->uid = ecdt->Uid;
     acpi_GetInteger(h, "_GLK", &params->glk);
     acpi_set_private(child, params);
-    acpi_set_magic(child, (uintptr_t)&acpi_ec_devclass);
 
     /* Finish the attach process. */
     if (device_probe_and_attach(child) != 0)
@@ -348,6 +344,7 @@
     ACPI_STATUS status;
     device_t	peer;
     char	desc[64];
+    int		ecdt;
     int		ret;
     struct acpi_ec_params *params;
     static char *ec_ids[] = { "PNP0C09", NULL };
@@ -362,11 +359,12 @@
      * duplicate probe.
      */
     ret = ENXIO;
-    params = NULL;
+    ecdt = 0;
     buf.Pointer = NULL;
     buf.Length = ACPI_ALLOCATE_BUFFER;
-    if (DEV_ECDT(dev)) {
-	params = acpi_get_private(dev);
+    params = acpi_get_private(dev);
+    if (params != NULL) {
+	ecdt = 1;
 	ret = 0;
     } else if (!acpi_disabled("ec") &&
 	ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids)) {
@@ -439,7 +437,7 @@
     if (ret == 0) {
 	snprintf(desc, sizeof(desc), "Embedded Controller: GPE %#x%s%s",
 		 params->gpe_bit, (params->glk) ? ", GLK" : "",
-		 DEV_ECDT(dev) ? ", ECDT" : "");
+		 ecdt ? ", ECDT" : "");
 	device_set_desc_copy(dev, desc);
     }
=====================================================================

-- Alex -- alex-goncharov@comcast.net --

Hi Alex,
can you compare the dmesg with and without the patch and check if the
processing order changed?

Joerg

,--- You/Joerg (Tue, 12 Jan 2010 15:47:36 +0100) ----*
| can you compare the dmesg with and without the patch and check if the
| processing order changed?

I would -- if only without the patch (i.e. with the official as of
last Sunday code) the system didn't panic and froze.  If you think
that rebuilding the kernel to get a panicking version and read the
console output is worth it, why, I can do it -- but I didn't see much
on the console and am hesitating to make the effort without an
assurance that it will be helpful to somebody.

-- Alex -- alex-goncharov@comcast.net --

Comment 5 Andriy Gapon 2010-01-13 08:06:25 UTC

on 13/01/2010 02:10 Alex Goncharov said the following:
>  
>  I would -- if only without the patch (i.e. with the official as of
>  last Sunday code) the system didn't panic and froze.  If you think
>  that rebuilding the kernel to get a panicking version and read the
>  console output is worth it, why, I can do it -- but I didn't see much
>  on the console and am hesitating to make the effort without an
>  assurance that it will be helpful to somebody.

Please rest assured that the information would be useful.
Actually the following should be sufficient for a start (but the more the better):
1. full verbose dmesg with a kernel that doesn't panic (either patched or some
older version - please specify which in this case);
2. picture of a screen when a panic happens with an unpatched kernel; boot
should be verbose, I'd like to see full panic report and boot messages before it.

Thanks!

P.S. your mail client sent the message that I am replying to as if it was
addressed to me individually, i.e. neither Joerg nor bug-followup@ were in To:
or Cc: headers, please don't do that.

-- 
Andriy Gapon

,--- You/Andriy (Wed, 13 Jan 2010 10:06:25 +0200) ----*
| P.S. your mail client sent the message that I am replying to as if it was
| addressed to me individually, i.e. neither Joerg nor bug-followup@ were in To:
| or Cc: headers, please don't do that.
`-----------------------------------------------------*

These are the headers that I see in the copy I got:

  Date: Tue, 12 Jan 2010 19:09:51 -0500
  From: Alex Goncharov <alex-goncharov@comcast.net>
  To: Joerg Sonnenberger <joerg@britannica.bec.de>
  CC: bug-followup@FreeBSD.org, alex-goncharov@comcast.net

Don't see your name here.  Don't see anything wrong here.  You do?

,--- You/Andriy (Wed, 13 Jan 2010 10:12:53 +0200) ----*
| Oh, looks like you were replying to bug-followup@ only.
`-----------------------------------------------------*

Nope (see above).

Going technical now:

,--- You/Andriy (Wed, 13 Jan 2010 10:06:25 +0200) ----*
| Please rest assured that the information would be useful.
| Actually the following should be sufficient for a start (but the more the better):
| 1. full verbose dmesg with a kernel that doesn't panic (either patched or some
| older version - please specify which in this case);
| 2. picture of a screen when a panic happens with an unpatched kernel; boot
| should be verbose, I'd like to see full panic report and boot messages before it.
`-----------------------------------------------------*

Will do but not immediately -- probably will have to wait till the
weekend.

,--- You/Andriy (Wed, 13 Jan 2010 10:09:19 +0200) ----*
| just in case, you may find make installkernel INSTKERNNAME=... and nextboot(8)
| useful for this kind of testing.
`-----------------------------------------------------*

Thank you -- this is helpful.

-- Alex -- alex-goncharov@comcast.net --

On Tue, Jan 12, 2010 at 07:09:51PM -0500, Alex Goncharov wrote:
> ,--- You/Joerg (Tue, 12 Jan 2010 15:47:36 +0100) ----*
> | can you compare the dmesg with and without the patch and check if the
> | processing order changed?
> 
> I would -- if only without the patch (i.e. with the official as of
> last Sunday code) the system didn't panic and froze.  If you think
> that rebuilding the kernel to get a panicking version and read the
> console output is worth it, why, I can do it -- but I didn't see much
> on the console and am hesitating to make the effort without an
> assurance that it will be helpful to somebody.

As I said on the NetBSD list, I can think of only reason why the patch
works. That reason is a change in the init order. E.g. print out the
dmesg of the working kernel and compare the attach order of the devices
up to ACPI EC.

Joerg

Comment 8 Andriy Gapon 2010-01-15 17:13:00 UTC

My current analysis of the issue.

Apparently, the primary trigger for the issue is inability to resolve
\_PR.CPU0._PPC when EC0._REG is called in acpi_ec_attach.
I think that \_PR.CPU0._PPC should come from SSDT.  Currently I am not sure why
that doesn't happen - either it is genuinely not present in SSDT, or there is (or
was) some ACPI-CA issues with bringing in SSDT or, at least, with doing that on
time.  I said that, because the second attach attempt appears to work fine, so it
looks like _REG is executed properly at that time.

Alex,
I would like to see acpidump -dt output for this system (using FreeBSD acpidump).

FreeBSD acpi driver performs two bus_generic_attach() passes for a good reason.
It seems that there is insufficient cleanup in acpi_ec_attach after the first
attach failure which leads to a crash on the second probe/attach pass.

Alex,
would you be able to test the following patch and report back the results?
(please don't forget to clean up any local changes/fixes/patches/rollbacks)

--- a/sys/dev/acpica/acpi_ec.c
+++ b/sys/dev/acpica/acpi_ec.c
@@ -470,6 +470,7 @@ acpi_ec_attach(device_t dev)
     sc->ec_uid = params->uid;
     sc->ec_suspending = FALSE;
     free(params, M_TEMP);
+    acpi_set_private(dev, NULL);

     /* Attach bus resources for data and command/status ports. */
     sc->ec_data_rid = 0;

Thanks!
-- 
Andriy Gapon

Found some interesting links:
http://lkml.org/lkml/2009/12/20/146
http://bugzilla.kernel.org/show_bug.cgi?id=14824
http://patchwork.kernel.org/patch/69039/

Apparently this is what happens here too, acpi_attachment fails when it is done
early, but works fine after acpi_cpu attaches, which does evaluate _PDC.
We would probably have to do something similar to what Linux is trying to do.

-- 
Andriy Gapon

A patch to try for the early EC attachment (not tested):

--- a/sys/dev/acpica/acpi_cpu.c
+++ b/sys/dev/acpica/acpi_cpu.c
@@ -141,6 +141,7 @@ static int		 cpu_ndevices;
 static struct acpi_cpu_softc **cpu_softc;
 ACPI_SERIAL_DECL(cpu, "ACPI CPU");

+static void	acpi_cpu_identify(driver_t *driver, device_t parent);
 static int	acpi_cpu_probe(device_t dev);
 static int	acpi_cpu_attach(device_t dev);
 static int	acpi_cpu_suspend(device_t dev);
@@ -169,6 +170,7 @@ static int	acpi_cpu_global_cx_lowest_sysctl(SYSCTL_HANDLER_ARGS);

 static device_method_t acpi_cpu_methods[] = {
     /* Device interface */
+    DEVMETHOD(device_identify,	acpi_cpu_identify),
     DEVMETHOD(device_probe,	acpi_cpu_probe),
     DEVMETHOD(device_attach,	acpi_cpu_attach),
     DEVMETHOD(device_detach,	bus_generic_detach),
@@ -203,6 +205,70 @@ static devclass_t acpi_cpu_devclass;
 DRIVER_MODULE(cpu, acpi, acpi_cpu_driver, acpi_cpu_devclass, 0, 0);
 MODULE_DEPEND(cpu, acpi, 1, 1, 1);

+
+static ACPI_STATUS
+acpi_cpu_early_init_pdc(ACPI_HANDLE handle, UINT32 level, void *context,
+    void **status)
+{
+    ACPI_OBJECT_LIST	   arglist;
+    ACPI_OBJECT		   arg[1];
+    uint32_t		   cap_set[3];
+    int			   cpu_features;
+
+    /*
+     * CPU capabilities are specified as a buffer of 32-bit integers:
+     * revision, count, and one or more capabilities.  The revision of
+     * "1" is not specified anywhere but seems to match Linux.
+     */
+    cpu_features = (intptr_t)context;
+    if (cpu_features) {
+	arglist.Pointer = arg;
+	arglist.Count = 1;
+	arg[0].Type = ACPI_TYPE_BUFFER;
+	arg[0].Buffer.Length = sizeof(cap_set);
+	arg[0].Buffer.Pointer = (uint8_t *)cap_set;
+	cap_set[0] = 1; /* revision */
+	cap_set[1] = 1; /* number of capabilities integers */
+	cap_set[2] = cpu_features;
+	AcpiEvaluateObject(handle, "_PDC", &arglist, NULL);
+    }
+
+    return_ACPI_STATUS (AE_OK);
+}
+
+/*
+ * Dummy identify routine for early evaluation of _PDC.
+ * On some systems evaluation of _PDC dynamically loads additional
+ * SSDT(s) that contain definitions needed for successful EC initialization,
+ * which is also done early, because other devices may depend on it.
+ */
+static void
+acpi_cpu_identify(driver_t *driver, device_t parent)
+{
+    driver_t 		 **drivers;
+    u_int		   features;
+    int			   cpu_features;
+    int			   drv_count, i;
+
+    /*
+     * Before calling any CPU methods, collect child driver feature hints
+     * and notify ACPI of them.  We support unified SMP power control
+     * so advertise this ourselves.  Note this is not the same as independent
+     * SMP control where each CPU can have different settings.
+     */
+    cpu_features = ACPI_CAP_SMP_SAME | ACPI_CAP_SMP_SAME_C3;
+    if (devclass_get_drivers(acpi_cpu_devclass, &drivers, &drv_count) == 0) {
+	for (i = 0; i < drv_count; i++) {
+	    if (ACPI_GET_FEATURES(drivers[i], &features) == 0)
+		cpu_features |= features;
+	}
+	free(drivers, M_TEMP);
+    }
+
+    AcpiWalkNamespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT, 100,
+        acpi_cpu_early_init_pdc, (void*)(intptr_t)cpu_features, NULL);
+}
+
 static int
 acpi_cpu_probe(device_t dev)
 {

-- 
Andriy Gapon

Comment 11 dfilter service 2010-01-18 10:30:25 UTC

Author: avg
Date: Mon Jan 18 10:30:11 2010
New Revision: 202558
URL: http://svn.freebsd.org/changeset/base/202558

Log:
  acpi_ec: clean up 'private' ivar when freeing memory to which it points
  
  This is not only a prudent thing to do, but also makes sure that probe
  method is not confused by non-NULL 'private', if the previous attach
  attempt fails for any reason.
  
  PR:		kern/142561
  Tested by:	Alex Goncharov <alex-goncharov@comcast.net>
  MFC after:	4 days

Modified:
  head/sys/dev/acpica/acpi_ec.c

Modified: head/sys/dev/acpica/acpi_ec.c
==============================================================================
--- head/sys/dev/acpica/acpi_ec.c	Mon Jan 18 10:29:04 2010	(r202557)
+++ head/sys/dev/acpica/acpi_ec.c	Mon Jan 18 10:30:11 2010	(r202558)
@@ -469,6 +469,7 @@ acpi_ec_attach(device_t dev)
     sc->ec_gpehandle = params->gpe_handle;
     sc->ec_uid = params->uid;
     sc->ec_suspending = FALSE;
+    acpi_set_private(dev, NULL);
     free(params, M_TEMP);
 
     /* Attach bus resources for data and command/status ports. */
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"

Comment 12 Andriy Gapon 2010-02-01 11:14:32 UTC

State Changed
From-To: open->closed

Original issue is resolved now. 
Late loading of Processor methods (via _OSC/_PDC) should 
be tracked separately.

Comment 13 dfilter service 2010-02-11 08:50:42 UTC

Author: avg
Date: Thu Feb 11 08:50:21 2010
New Revision: 203776
URL: http://svn.freebsd.org/changeset/base/203776

Log:
  acpi cpu: probe+attach before all other enumerated children on acpi bus
  
  Some current systems dynamically load SSDT(s) when _PDC/_OSC method
  of Processor is evaluated.  Other devices in ACPI namespace may access
  objects defined in the dynamic SSDT.  Drivers for such devices might
  have to have a rather high priority, because of other dependencies.
  Good example is acpi_ec driver for EC.
  Thus we attach to Processors as early as possible to load the SSDTs
  before any other drivers may try to evaluate control methods.
  It also seems to be a natural order for a processor in a device
  hierarchy.
  
  On the other hand, some child devices on acpi cpu bus need to access
  other system resources like PCI configuration space of chipset devices,
  so they need to be probed and attached rather late.
  For this reason we probe and attach the cpu bus at
  SI_SUB_CONFIGURE:SI_ORDER_MIDDLE SYSINIT level.
  In the future this could be done more elegantly via multipass.
  
  Please note that acpi drivers that might access ACPI namespace from
  device_identify will do that before _PDC/_OSC of Processors are evaluated.
  
  Legacy cpu driver is not affected by this change.
  
  PR:		kern/142561 (in part)
  Reviewed by:	jhb
  Silence from:	acpi@
  MFC after:	5 weeks

Modified:
  head/sys/dev/acpica/acpi.c
  head/sys/dev/acpica/acpi_cpu.c

Modified: head/sys/dev/acpica/acpi.c
==============================================================================
--- head/sys/dev/acpica/acpi.c	Thu Feb 11 08:34:41 2010	(r203775)
+++ head/sys/dev/acpica/acpi.c	Thu Feb 11 08:50:21 2010	(r203776)
@@ -1685,14 +1685,14 @@ acpi_probe_order(ACPI_HANDLE handle, int
      * 100000. CPUs
      */
     AcpiGetType(handle, &type);
-    if (acpi_MatchHid(handle, "PNP0C01") || acpi_MatchHid(handle, "PNP0C02"))
+    if (type == ACPI_TYPE_PROCESSOR)
 	*order = 1;
-    else if (acpi_MatchHid(handle, "PNP0C09"))
+    else if (acpi_MatchHid(handle, "PNP0C01") || acpi_MatchHid(handle, "PNP0C02"))
 	*order = 2;
-    else if (acpi_MatchHid(handle, "PNP0C0F"))
+    else if (acpi_MatchHid(handle, "PNP0C09"))
 	*order = 3;
-    else if (type == ACPI_TYPE_PROCESSOR)
-	*order = 100000;
+    else if (acpi_MatchHid(handle, "PNP0C0F"))
+	*order = 4;
 }
 
 /*

Modified: head/sys/dev/acpica/acpi_cpu.c
==============================================================================
--- head/sys/dev/acpica/acpi_cpu.c	Thu Feb 11 08:34:41 2010	(r203775)
+++ head/sys/dev/acpica/acpi_cpu.c	Thu Feb 11 08:50:21 2010	(r203776)
@@ -384,13 +384,31 @@ acpi_cpu_attach(device_t dev)
     /* Probe for Cx state support. */
     acpi_cpu_cx_probe(sc);
 
-    /* Finally,  call identify and probe/attach for child devices. */
-    bus_generic_probe(dev);
-    bus_generic_attach(dev);
-
     return (0);
 }
 
+static void
+acpi_cpu_postattach(void *unused __unused)
+{
+    device_t *devices;
+    int err;
+    int i, n;
+
+    err = devclass_get_devices(acpi_cpu_devclass, &devices, &n);
+    if (err != 0) {
+	printf("devclass_get_devices(acpi_cpu_devclass) failed\n");
+	return;
+    }
+    for (i = 0; i < n; i++)
+	bus_generic_probe(devices[i]);
+    for (i = 0; i < n; i++)
+	bus_generic_attach(devices[i]);
+    free(devices, M_TEMP);
+}
+
+SYSINIT(acpi_cpu, SI_SUB_CONFIGURE, SI_ORDER_MIDDLE,
+    acpi_cpu_postattach, NULL);
+
 /*
  * Disable any entry to the idle function during suspend and re-enable it
  * during resume.
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"