| Message ID | 20240515055631.5775-6-ben@jubnut.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix MEC concurrency problems for Framework Laptop | expand |
On Wed, May 15, 2024 at 06:56:30AM +0100, Ben Walsh wrote: > Framework Laptops' ACPI exposes the EC as name "PNP0C09". Use this to > find the device. This makes it easy to find the AML mutex via the > ACPI_COMPANION device. > > The name "PNP0C09" is part of the ACPI standard, not Chrome-specific, > so only recognise the device if the DMI data is recognised too. I don't quite understand the statement. Why it needs DMI data? > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c [...] > -/* True if ACPI device is present */ > -static bool cros_ec_lpc_acpi_device_found; > +/* > + * Index into cros_ec_lpc_acpi_device_ids of ACPI device, > + * negative for ACPI device not found. > + */ > +static int cros_ec_lpc_acpi_device_found; Rename it to `cros_ec_lpc_acpi_device_index` for clarity? > static int __init cros_ec_lpc_init(void) > { > int ret; > - acpi_status status; > const struct dmi_system_id *dmi_match; > > - status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device, > - &cros_ec_lpc_acpi_device_found, NULL); > - if (ACPI_FAILURE(status)) > - pr_warn(DRV_NAME ": Looking for %s failed\n", ACPI_DRV_NAME); > + cros_ec_lpc_acpi_device_found = cros_ec_lpc_find_acpi_dev( > + cros_ec_lpc_driver.driver.acpi_match_table); Or just use `cros_ec_lpc_acpi_device_ids`. > - } else if (!cros_ec_lpc_acpi_device_found) { > + } else if (cros_ec_lpc_acpi_device_found <= 0) { > + /* Standard EC "PNP0C09" not supported without DMI data */ Asked the same question above, why PNP0C09 needs DMI data? Also the way is a bit confusing as "PNP0C09" must be at index 0 in the acpi_device_id.
Tzung-Bi Shih <tzungbi@kernel.org> writes: > On Wed, May 15, 2024 at 06:56:30AM +0100, Ben Walsh wrote: >> Framework Laptops' ACPI exposes the EC as name "PNP0C09". Use this to >> find the device. This makes it easy to find the AML mutex via the >> ACPI_COMPANION device. >> >> The name "PNP0C09" is part of the ACPI standard, not Chrome-specific, >> so only recognise the device if the DMI data is recognised too. > > I don't quite understand the statement. Why it needs DMI data? There are lots of computers with EC chips with ACPI name "PNP0C09" because it's part of the ACPI standard (for example I have an Intel NUC with one of these). Most of them don't support the cros_ec protocol, so the cros_ec driver should ignore these chips. The Framework EC is unusual in that it's called "PNP0C09" and supports the cros_ec protocol. Before these patches, the cros_ec code just ignored PNP0C09 because it wasn't in the match table. The cros_ec_lpc_init logic looked like: * dmi_match => ok * acpi_name == "GOOG0004" => ok * otherwise fail. After the patch, cros_ec_lpc_init still has this behaviour. We have "PNP0C09" in the match table so the driver gets hooked up correctly with the right "ACPI_COMPANION" device, but we don't allow the match to proceed unless we have the DMI data indicating it's a Framework EC. >> - } else if (!cros_ec_lpc_acpi_device_found) { >> + } else if (cros_ec_lpc_acpi_device_found <= 0) { >> + /* Standard EC "PNP0C09" not supported without DMI data */ > > Also the way is a bit confusing as "PNP0C09" must be at index 0 in the > acpi_device_id. I need some way of saying "will we match PNP0C09?" The table index seems a simple way of doing it. I could use a strcmp on the table match instead? Regarding your other emails, I agree with all your suggestions. Thanks for reviewing!
On Thu, May 23, 2024 at 07:42:00PM +0100, Ben Walsh wrote: > Tzung-Bi Shih <tzungbi@kernel.org> writes: > > > On Wed, May 15, 2024 at 06:56:30AM +0100, Ben Walsh wrote: > >> Framework Laptops' ACPI exposes the EC as name "PNP0C09". Use this to > >> find the device. This makes it easy to find the AML mutex via the > >> ACPI_COMPANION device. > >> > >> The name "PNP0C09" is part of the ACPI standard, not Chrome-specific, > >> so only recognise the device if the DMI data is recognised too. > > > > I don't quite understand the statement. Why it needs DMI data? > > There are lots of computers with EC chips with ACPI name "PNP0C09" > because it's part of the ACPI standard (for example I have an Intel NUC > with one of these). Most of them don't support the cros_ec protocol, so > the cros_ec driver should ignore these chips. The Framework EC is > unusual in that it's called "PNP0C09" and supports the cros_ec protocol. > > Before these patches, the cros_ec code just ignored PNP0C09 because it > wasn't in the match table. The cros_ec_lpc_init logic looked like: > > * dmi_match => ok > * acpi_name == "GOOG0004" => ok > * otherwise fail. > > After the patch, cros_ec_lpc_init still has this behaviour. We have > "PNP0C09" in the match table so the driver gets hooked up correctly > with the right "ACPI_COMPANION" device, but we don't allow the match > to proceed unless we have the DMI data indicating it's a Framework EC. From the context you provided, instead of matching "PNP0C09" in the driver, it makes more sense to me (for Framework EC): * Mainly use DMI match. * Add a quirk for looking up (acpi_get_devices()?) and binding (e.g. ACPI_COMPANION_SET()) the `adev` in cros_ec_lpc_probe().
Tzung-Bi Shih <tzungbi@kernel.org> writes: > From the context you provided, instead of matching "PNP0C09" in the driver, > it makes more sense to me (for Framework EC): > > * Mainly use DMI match. > * Add a quirk for looking up (acpi_get_devices()?) and binding > (e.g. ACPI_COMPANION_SET()) the `adev` in cros_ec_lpc_probe(). Sorry, I don't think I provided enough context. There is already a platform device /sys/bus/platform/devices/PNP0C09:00 with a companion acpi device /sys/bus/acpi/devices/PNP0C09:00. I think it makes sense to bind the driver to the existing platform device. I could add a new quirk which provides an alternative ACPI match table to be used instead of the default. In the default case the match_table will contain only "GOOG0004" as before. But in the Framework EC case the match table will be "PNP0C09".
On Fri, May 24, 2024 at 1:35 PM Ben Walsh <ben@jubnut.com> wrote: > > I could add a new quirk which provides an alternative ACPI match table > to be used instead of the default. In the default case the match_table > will contain only "GOOG0004" as before. But in the Framework EC case the > match table will be "PNP0C09". My biggest concern with putting PNP0C09 in the direct match table is that it would cause cros_ec_lpcs to be loaded for *all* devices with an ACPI-compatible embedded controller; it would likely print an error and bail out early on, but it would still be unnecessary on 99% of platforms. This is my misunderstanding the driver model, but is it possible to add a second companion device? d
Dustin Howett <dustin@howett.net> writes: > On Fri, May 24, 2024 at 1:35 PM Ben Walsh <ben@jubnut.com> wrote: >> >> I could add a new quirk which provides an alternative ACPI match table >> to be used instead of the default. In the default case the match_table >> will contain only "GOOG0004" as before. But in the Framework EC case the >> match table will be "PNP0C09". > > My biggest concern with putting PNP0C09 in the direct match table is > that it would cause cros_ec_lpcs to be loaded for *all* devices with > an ACPI-compatible embedded controller; it would likely print an error > and bail out early on, but it would still be unnecessary on 99% of > platforms. That's exactly what we're talking about: how to *avoid* putting PNP0C09 in the match table. My original idea was to put it in the match table, but only allow a match to proceed if Framework EC is detected by DMI. My new idea is to not to put it in the match table *at all*, but to provide a new match table if Framework EC is detected by DMI.
On Fri, May 24, 2024 at 07:35:22PM +0100, Ben Walsh wrote: > Tzung-Bi Shih <tzungbi@kernel.org> writes: > > > From the context you provided, instead of matching "PNP0C09" in the driver, > > it makes more sense to me (for Framework EC): > > > > * Mainly use DMI match. > > * Add a quirk for looking up (acpi_get_devices()?) and binding > > (e.g. ACPI_COMPANION_SET()) the `adev` in cros_ec_lpc_probe(). > > Sorry, I don't think I provided enough context. There is already a > platform device /sys/bus/platform/devices/PNP0C09:00 with a companion > acpi device /sys/bus/acpi/devices/PNP0C09:00. I think it makes sense to > bind the driver to the existing platform device. > > I could add a new quirk which provides an alternative ACPI match table > to be used instead of the default. In the default case the match_table > will contain only "GOOG0004" as before. But in the Framework EC case the > match table will be "PNP0C09". I think it doesn't work as the current quirk is handling in cros_ec_lpc_probe() which is after matching. My original idea: would it be possible to get the `adev` in cros_ec_lpc_probe() via any lookup API? If yes, it could still use DMI match and get `adev` if required.
Tzung-Bi Shih <tzungbi@kernel.org> writes: > On Fri, May 24, 2024 at 07:35:22PM +0100, Ben Walsh wrote: >> I could add a new quirk which provides an alternative ACPI match table >> to be used instead of the default. In the default case the match_table >> will contain only "GOOG0004" as before. But in the Framework EC case the >> match table will be "PNP0C09". > > I think it doesn't work as the current quirk is handling in > cros_ec_lpc_probe() which is after matching. I was thinking of a new quirk called CROS_EC_LPC_QUIRK_ACPI_MATCH, and putting it in cros_ec_lpc_init(), not cros_ec_lpc_probe(). Do we have to do all quirk handling in cros_ec_lpc_probe()? > My original idea: would it be possible to get the `adev` in cros_ec_lpc_probe() > via any lookup API? If yes, it could still use DMI match and get `adev` if > required. That works; I've tested it. In this scenario we're not using the existing PNP0C09 platform device, which means I can't look at /sys/bus/acpi/devices/PNP0C09\:00/physical_node/driver and see the driver. Is this OK? (Note that ACPI_COMPANION_SET() doesn't fix this. You can use acpi_bind_one() but that seems more like internal plumbing).
On Mon, May 27, 2024 at 07:06:40PM +0100, Ben Walsh wrote: > Tzung-Bi Shih <tzungbi@kernel.org> writes: > > > On Fri, May 24, 2024 at 07:35:22PM +0100, Ben Walsh wrote: > > >> I could add a new quirk which provides an alternative ACPI match table > >> to be used instead of the default. In the default case the match_table > >> will contain only "GOOG0004" as before. But in the Framework EC case the > >> match table will be "PNP0C09". > > > > I think it doesn't work as the current quirk is handling in > > cros_ec_lpc_probe() which is after matching. > > I was thinking of a new quirk called CROS_EC_LPC_QUIRK_ACPI_MATCH, and > putting it in cros_ec_lpc_init(), not cros_ec_lpc_probe(). Do we have to > do all quirk handling in cros_ec_lpc_probe()? No, but there is already code in cros_ec_lpc_probe() for handling quirks and we would like to reuse them if we could. Also a possible issue: MODULE_DEVICE_TABLE() wouldn't work if the table changes during runtime. > > > My original idea: would it be possible to get the `adev` in cros_ec_lpc_probe() > > via any lookup API? If yes, it could still use DMI match and get `adev` if > > required. > > That works; I've tested it. > > In this scenario we're not using the existing PNP0C09 platform device, > which means I can't look at > /sys/bus/acpi/devices/PNP0C09\:00/physical_node/driver and see the > driver. Is this OK? > > (Note that ACPI_COMPANION_SET() doesn't fix this. You can use > acpi_bind_one() but that seems more like internal plumbing). As long as ACPI_COMPANION() in the driver can get the correct `adev`, I guess it's fine. Otherwise, put the `adev` to somewhere device specific (e.g. struct cros_ec_lpc) should also work.
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c index ab7779b57a13..a9200f0a1a37 100644 --- a/drivers/platform/chrome/cros_ec_lpc.c +++ b/drivers/platform/chrome/cros_ec_lpc.c @@ -29,10 +29,12 @@ #include "cros_ec_lpc_mec.h" #define DRV_NAME "cros_ec_lpcs" -#define ACPI_DRV_NAME "GOOG0004" -/* True if ACPI device is present */ -static bool cros_ec_lpc_acpi_device_found; +/* + * Index into cros_ec_lpc_acpi_device_ids of ACPI device, + * negative for ACPI device not found. + */ +static int cros_ec_lpc_acpi_device_found; /* * Indicates that lpc_driver_data.quirk_mmio_memory_base should @@ -598,7 +600,8 @@ static void cros_ec_lpc_remove(struct platform_device *pdev) } static const struct acpi_device_id cros_ec_lpc_acpi_device_ids[] = { - { ACPI_DRV_NAME, 0 }, + { "PNP0C09", 0 }, /* Standard ACPI EC name. Must be first in list */ + { "GOOG0004", 0 }, { } }; MODULE_DEVICE_TABLE(acpi, cros_ec_lpc_acpi_device_ids); @@ -737,30 +740,33 @@ static struct platform_device cros_ec_lpc_device = { .name = DRV_NAME }; -static acpi_status cros_ec_lpc_parse_device(acpi_handle handle, u32 level, - void *context, void **retval) +static int cros_ec_lpc_find_acpi_dev(const struct acpi_device_id *acpi_ids) { - *(bool *)context = true; - return AE_CTRL_TERMINATE; + int i; + + for (i = 0; acpi_ids[i].id[0]; ++i) { + if (acpi_dev_present(acpi_ids[i].id, NULL, -1)) + return i; + } + + return -1; } static int __init cros_ec_lpc_init(void) { int ret; - acpi_status status; const struct dmi_system_id *dmi_match; - status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device, - &cros_ec_lpc_acpi_device_found, NULL); - if (ACPI_FAILURE(status)) - pr_warn(DRV_NAME ": Looking for %s failed\n", ACPI_DRV_NAME); + cros_ec_lpc_acpi_device_found = cros_ec_lpc_find_acpi_dev( + cros_ec_lpc_driver.driver.acpi_match_table); dmi_match = dmi_first_match(cros_ec_lpc_dmi_table); if (dmi_match) { /* Pass the DMI match's driver data down to the platform device */ cros_ec_lpc_driver_data = dmi_match->driver_data; - } else if (!cros_ec_lpc_acpi_device_found) { + } else if (cros_ec_lpc_acpi_device_found <= 0) { + /* Standard EC "PNP0C09" not supported without DMI data */ pr_err(DRV_NAME ": unsupported system.\n"); return -ENODEV; } @@ -772,7 +778,7 @@ static int __init cros_ec_lpc_init(void) return ret; } - if (!cros_ec_lpc_acpi_device_found) { + if (cros_ec_lpc_acpi_device_found < 0) { /* Register the device, and it'll get hooked up automatically */ ret = platform_device_register(&cros_ec_lpc_device); if (ret) { @@ -786,7 +792,7 @@ static int __init cros_ec_lpc_init(void) static void __exit cros_ec_lpc_exit(void) { - if (!cros_ec_lpc_acpi_device_found) + if (cros_ec_lpc_acpi_device_found < 0) platform_device_unregister(&cros_ec_lpc_device); platform_driver_unregister(&cros_ec_lpc_driver); }
Framework Laptops' ACPI exposes the EC as name "PNP0C09". Use this to find the device. This makes it easy to find the AML mutex via the ACPI_COMPANION device. The name "PNP0C09" is part of the ACPI standard, not Chrome-specific, so only recognise the device if the DMI data is recognised too. Signed-off-by: Ben Walsh <ben@jubnut.com> --- drivers/platform/chrome/cros_ec_lpc.c | 38 ++++++++++++++++----------- 1 file changed, 22 insertions(+), 16 deletions(-)