Changeset View
Standalone View
sys/dev/acpica/acpi_ec.c
Show First 20 Lines • Show All 322 Lines • ▼ Show 20 Lines | /*count*/1); | ||||
params->gpe_handle = NULL; | params->gpe_handle = NULL; | ||||
params->gpe_bit = ecdt->Gpe; | params->gpe_bit = ecdt->Gpe; | ||||
params->uid = ecdt->Uid; | params->uid = ecdt->Uid; | ||||
acpi_GetInteger(h, "_GLK", ¶ms->glk); | acpi_GetInteger(h, "_GLK", ¶ms->glk); | ||||
acpi_set_private(child, params); | acpi_set_private(child, params); | ||||
/* Finish the attach process. */ | /* Finish the attach process. */ | ||||
if (device_probe_and_attach(child) != 0) | if (device_probe_and_attach(child) != 0) | ||||
device_delete_child(parent, child); | device_delete_child(parent, child); | ||||
jhb: You don't need this line because adding a device with a set name (the BUS_ADD_CHILD) already… | |||||
} | } | ||||
Done Inline ActionsSo I don't know how I quite feel about bypassing device_probe() in this case. The other suggestion I had made was to add a 'device_has_fixed_devclass' or some such and using that to make acpi_ec_probe fail unless either it had a valid ID or the fixed devclass. That would be a much smaller patch overall. There might be some other places in the future where 'device_has_fixed_devclass' might also be useful. jhb: So I don't know how I quite feel about bypassing device_probe() in this case. The other… | |||||
Done Inline ActionsI intentionally ignored your advice earlier because when I tried to implement it, it seemed impossible to ever support more than a single EC that way. Perhaps I mangled what you meant though. In my interpretation, you meant to add a check in the probe to find a fixedclass EC device, if found, skip the probe. If not found, continue on with probe. This solution I think always works when there is a single EC. If you ever want to support multiple ECs, it no longer works. Perhaps that case never will exist, or we should only bother when it does? I'm fine with changing it if you prefer, I just wanted to make my intention clear. Please advise. bwidawsk: I intentionally ignored your advice earlier because when I tried to implement it, it seemed… | |||||
Done Inline ActionsThe change as described won't prevent multiple ECs, though the current code already tries to avoid duplicate ECs. I'd be surprised if there was a system with more than one EC. If we encounter one we can revise the current code that tries to handle duplicates. My suggestion was to use the check to avoid rejecting ECDT devices for which the ACPI_ID_PROBE check would fail. That is, if we had 'device_has_fixed_devclass()' then the entire change in acpi_ec.c would be to make the first few lines in acpi_ec_probe() this: /* Check that this is a device and that EC is not disabled. */ if (acpi_get_type(dev) != ACPI_TYPE_DEVICE || acpi_disabled("ec") || (!device_has_fixed_devclass(dev) && ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids) != 0)) return (ENXIO); Those lines are a bit long though so I might structure it as something like: if (acpi_get_type(dev) != ACPI_TYPE_DEVICE || acpi_disabled("ec")) return (ENXIO); if (device_has_fixed_devclass(dev)) { ecdt = 1; params = acpi_get_private(dev); if (params != NULL) ret = 0; else ret = ENXIO; } else { ret = ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids); if (ret > 0) return (ret); params = malloc(...); /* Existing non-ECDT code */ } This also ensures that the invariant about parameters only existing for ECDT devices still holds. jhb: The change as described won't prevent multiple ECs, though the current code already tries to… | |||||
Done Inline ActionsWon't this fail if another ACPI device has a fixed class type, and private data? It seems to just be kicking the can down the road unless I don't understand quite right. bwidawsk: Won't this fail if another ACPI device has a fixed class type, and private data? It seems to… | |||||
Done Inline ActionsNo, because if it has a fixed device class then it can only be probed by drivers whose name matches the fixed devclass. Thus, in this case, only drivers that have the "acpi_ec" name (of which there is only this driver). We do have some device classes (names) that have multiple drivers (e.g. "pci" and "pcib"), but "acpi_ec" only has a single driver, so this test is sufficient. jhb: No, because if it has a fixed device class then it can only be probed by drivers whose name… | |||||
static int | static int | ||||
acpi_ec_probe(device_t dev) | acpi_ec_probe(device_t dev) | ||||
{ | { | ||||
ACPI_BUFFER buf; | ACPI_BUFFER buf; | ||||
ACPI_HANDLE h; | ACPI_HANDLE h; | ||||
ACPI_OBJECT *obj; | ACPI_OBJECT *obj; | ||||
ACPI_STATUS status; | ACPI_STATUS status; | ||||
device_t peer; | device_t peer; | ||||
char desc[64]; | char desc[64]; | ||||
int ecdt; | int ecdt; | ||||
int ret; | int ret; | ||||
struct acpi_ec_params *params; | struct acpi_ec_params *params; | ||||
static char *ec_ids[] = { "PNP0C09", NULL }; | static char *ec_ids[] = { "PNP0C09", NULL }; | ||||
/* Check that this is a device and that EC is not disabled. */ | /* Check that this is a device and that EC is not disabled. */ | ||||
if (acpi_get_type(dev) != ACPI_TYPE_DEVICE || acpi_disabled("ec")) | if (acpi_get_type(dev) != ACPI_TYPE_DEVICE || acpi_disabled("ec") || | ||||
!ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids)) | |||||
jhbUnsubmitted Done Inline ActionsThis breaks the case that a device was added via ECDT. On systems that only enumerate the EC via ECDT without a corresponding Device() node, the device_t is added above in acpi_ec_ecdt_probe() and it won't have a valid _HID or _CID so would always fail here. A better way is to check to see if the device has a "fixed" device class and using that below instead of the 'params != NULL' check. The DF_FIXEDCLASS flag is what you want, but we don't currently expose that out of sys/kern/subr_bus.c. We could maybe add a 'device_is_fixedclass()' accessor. jhb: This breaks the case that a device was added via ECDT. On systems that only enumerate the EC… | |||||
bwidawskAuthorUnsubmitted Done Inline ActionsYou're right. I will respin to add the accessor and use that flag. bwidawsk: You're right. I will respin to add the accessor and use that flag. | |||||
bwidawskAuthorUnsubmitted Done Inline ActionsOn looking a bit closer, won't we pretty much always get a FIXEDCLASS device? The only case we do not is when there is no ECDT, or what am I missing here? bwidawsk: On looking a bit closer, won't we pretty much always get a FIXEDCLASS device? The only case we… | |||||
jhbUnsubmitted Done Inline ActionsYes, if there is no ECDT but the EC is still enumerated as a Device() with a EC _HID. That is the 'params == NULL' case. jhb: Yes, if there is no ECDT but the EC is still enumerated as a Device() with a EC _HID. That is… | |||||
bwidawskAuthorUnsubmitted Done Inline ActionsOkay, just seemed a bit weird that the most common case is probably there is an ECDT and EC Device(), which we're calling "FIXEDCLASS"... I'm fine with that, just making sure I understood first. bwidawsk: Okay, just seemed a bit weird that the most common case is probably there is an ECDT and EC… | |||||
jhbUnsubmitted Done Inline ActionsYes, it is odd. :( I think there were older systems that only used ECDT or only used Device(). I guess we have something in place to not create two device_t objects for the EC if you have both, but I'd have to look some more to see how that works. Actually, my ivy bridge desktop doesn't have an ECDT at all, only a Device(). Checking the 6.2 spec it seems that ECDT is optional, and it wasn't present in ACPI 1.0. I actually don't know what we do on a system with the ECDT present currently. It seems like we might create two acpi_ec devices. I wonder if the second one (via Device()) fails to attach because it's resources are already allocated? jhb: Yes, it is odd. :( I think there were older systems that only used ECDT or only used Device(). | |||||
return (ENXIO); | return (ENXIO); | ||||
/* | /* | ||||
* If probed via ECDT, set description and continue. Otherwise, | * If probed via ECDT, set description and continue. Otherwise, | ||||
* we can access the namespace and make sure this is not a | * we can access the namespace and make sure this is not a | ||||
* duplicate probe. | * duplicate probe. | ||||
*/ | */ | ||||
ret = ENXIO; | ret = ENXIO; | ||||
ecdt = 0; | ecdt = 0; | ||||
buf.Pointer = NULL; | buf.Pointer = NULL; | ||||
buf.Length = ACPI_ALLOCATE_BUFFER; | buf.Length = ACPI_ALLOCATE_BUFFER; | ||||
params = acpi_get_private(dev); | params = acpi_get_private(dev); | ||||
if (params != NULL) { | if (params != NULL) { | ||||
ecdt = 1; | ecdt = 1; | ||||
ret = 0; | ret = 0; | ||||
} else if (ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids)) { | } else { | ||||
params = malloc(sizeof(struct acpi_ec_params), M_TEMP, | params = malloc(sizeof(struct acpi_ec_params), M_TEMP, | ||||
M_WAITOK | M_ZERO); | M_WAITOK | M_ZERO); | ||||
h = acpi_get_handle(dev); | h = acpi_get_handle(dev); | ||||
/* | /* | ||||
* Read the unit ID to check for duplicate attach and the | * Read the unit ID to check for duplicate attach and the | ||||
* global lock value to see if we should acquire it when | * global lock value to see if we should acquire it when | ||||
* accessing the EC. | * accessing the EC. | ||||
▲ Show 20 Lines • Show All 653 Lines • Show Last 20 Lines |
You don't need this line because adding a device with a set name (the BUS_ADD_CHILD) already sets the devclass.