| Message ID | ZLm3G5G6/gmLZwkn@tora (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | platform/chrome: cros_ec_lpc: Add DMI definition for post-Skylake machines running custom Coreboot builds | expand |
On Fri, Jul 21, 2023 at 12:37:15AM +0200, Alicja Michalska wrote: > ChromeOS EC LPC lacks DMI match for newer machines, which > use "Google" DMI_SYS_VENDOR as opposed to "GOOGLE" in older models. I'm confused about the sentence as it looks irrelevant to the patch. > This patch adds DMI definition for MrChomebox's custom Coreboots builds, > which we (Chrultrabook Project) are using. s/This patch adds/Add/. Search "imperative mood" in [1]. Looks like a typo: s/MrChomebox/MrChromebox/. If you get chance to send next version, please shorten the commit title. I guess "platform/chrome: cros_ec_lpc: Add DMI match for MrChromebox" should be quite explicit. [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html > + /* DMI doesn't match modern machines running custom firmware */ Remove the line. > + { > + /* MrChromebox's firmware */ > + .matches = { > + DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"), > + DMI_MATCH(DMI_BIOS_VERSION, "MrChromebox-"), > + }, > + }, Put the block after "A small number of non-Chromebook/box machines also use the ChromeOS EC"[2]. [2]: https://elixir.bootlin.com/linux/v6.4/source/drivers/platform/chrome/cros_ec_lpc.c#L533
Thank you for your feedback. I've explained the reason behind adding this patch, but we'll go with different approach next time around. Since we're discussing this, I would like to suggest removal of DMI matches for EOL machines from lines 503...535 (Link, Samus, Peppy, Glimmer). Those machines aren't supported by Google anymore. Patch I suggested will match DMI while running custom firmware. If maintainers are okay with it, I will submit a patch removing DMI matches for stock firmware running on those machines since it's not needed anymore. On 21/07/2023 08:39, Tzung-Bi Shih wrote: > On Fri, Jul 21, 2023 at 12:37:15AM +0200, Alicja Michalska wrote: >> ChromeOS EC LPC lacks DMI match for newer machines, which >> use "Google" DMI_SYS_VENDOR as opposed to "GOOGLE" in older models. > > I'm confused about the sentence as it looks irrelevant to the patch. > >> This patch adds DMI definition for MrChomebox's custom Coreboots builds, >> which we (Chrultrabook Project) are using. > > s/This patch adds/Add/. Search "imperative mood" in [1]. > > Looks like a typo: s/MrChomebox/MrChromebox/. > > If you get chance to send next version, please shorten the commit title. > I guess "platform/chrome: cros_ec_lpc: Add DMI match for MrChromebox" should > be quite explicit. > > [1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html > >> + /* DMI doesn't match modern machines running custom firmware */ > > Remove the line. > >> + { >> + /* MrChromebox's firmware */ >> + .matches = { >> + DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"), >> + DMI_MATCH(DMI_BIOS_VERSION, "MrChromebox-"), >> + }, >> + }, > > Put the block after "A small number of non-Chromebook/box machines also use > the ChromeOS EC"[2]. > > [2]: https://elixir.bootlin.com/linux/v6.4/source/drivers/platform/chrome/cros_ec_lpc.c#L533
On Fri, Jul 21, 2023 at 10:16 AM Alicja Michalska <ahplka19@gmail.com> wrote: > I've explained the reason behind adding this patch, but we'll go with > different approach next time around. FWIW, I'm also confused about your first sentence the same way Tzung-Bi is. If two people are confused by parts of your description, then maybe it needs improvement :) > Since we're discussing this, I would like to suggest removal of DMI > matches for EOL machines from lines 503...535 (Link, Samus, Peppy, Glimmer). > > Those machines aren't supported by Google anymore. Patch I suggested > will match DMI while running custom firmware. > > If maintainers are okay with it, I will submit a patch removing DMI > matches for stock firmware running on those machines since it's not > needed anymore. That seems actively harmful. These devices continue to work just fine with their stock BIOS, even if Google no longer supports updating the Google-built OS. That doesn't mean people can't boot other OS'es (e.g., their own ChromiumOS builds; or other Linux distros) on them. Brian
That's understandable :) I spoke with Matt (MrChromebox). It seems like he tried to upstream similar patch to what I suggested here few years ago, but it never got merged. Reason for this is that pre-Skylake machines had different DMI vendor name. For example, that's my DMI information from ELDRID (Google/Volteer baseboard) running MrChromebox's firmware and mainline kernel: [...] Vendor: coreboot Manufacturer: Google [...] There's a match for coreboot/GOOGLE, but not for coreboot/Google - which is the case for all modern-ish machines made past 2018 (reef, octopus, hatch, volteer, brya and so on). As for the concerns regarding removal of outdated DMIs - it's understandable, but mainline doesn't work correctly on stock firmware. In order to get mainline Linux running correctly on any ChromeOS device, user has to flash firmware that contains our patches. We currently support over 100 machines, starting with first machines that used Coreboot (SandyBridge) up to AlderLake (at the moment). Flashing those machines (pre-CR50) is as simple as removing Write Protect screw and running MrChromebox's script from ChromeOS shell. Once that's done, Chromebooks behave in the same exact way as UEFI systems (because we're using EDK2 as Coreboot payload in our builds) with correct ACPI tables and other numerous fixes that are missing from stock firmware. https://chrultrabook.github.io/docs/docs/getting-started.html On 21/07/2023 19:28, Brian Norris wrote: > On Fri, Jul 21, 2023 at 10:16 AM Alicja Michalska <ahplka19@gmail.com> wrote: >> I've explained the reason behind adding this patch, but we'll go with >> different approach next time around. > > FWIW, I'm also confused about your first sentence the same way > Tzung-Bi is. If two people are confused by parts of your description, > then maybe it needs improvement :) > >> Since we're discussing this, I would like to suggest removal of DMI >> matches for EOL machines from lines 503...535 (Link, Samus, Peppy, Glimmer). >> >> Those machines aren't supported by Google anymore. Patch I suggested >> will match DMI while running custom firmware. >> >> If maintainers are okay with it, I will submit a patch removing DMI >> matches for stock firmware running on those machines since it's not >> needed anymore. > > That seems actively harmful. These devices continue to work just fine > with their stock BIOS, even if Google no longer supports updating the > Google-built OS. That doesn't mean people can't boot other OS'es > (e.g., their own ChromiumOS builds; or other Linux distros) on them. > > Brian
On Thu, Jul 20, 2023 at 4:37 PM Alicja Michalska <ahplka19@gmail.com> wrote: > > ChromeOS EC LPC lacks DMI match for newer machines, which > use "Google" DMI_SYS_VENDOR as opposed to "GOOGLE" in older models. > This patch adds DMI definition for MrChomebox's custom Coreboots builds, > which we (Chrultrabook Project) are using. > > Signed-off-by: Alicja Michalska <ahplka19@gmail.com> > --- > drivers/platform/chrome/cros_ec_lpc.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c > index 500a61b093e4..6ac993be4eb1 100644 > --- a/drivers/platform/chrome/cros_ec_lpc.c > +++ b/drivers/platform/chrome/cros_ec_lpc.c > @@ -533,6 +533,14 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = { > DMI_MATCH(DMI_PRODUCT_NAME, "Glimmer"), > }, > }, > + /* DMI doesn't match modern machines running custom firmware */ > + { > + /* MrChromebox's firmware */ > + .matches = { > + DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"), > + DMI_MATCH(DMI_BIOS_VERSION, "MrChromebox-"), > + }, > + }, > /* A small number of non-Chromebook/box machines also use the ChromeOS EC */ > { > /* the Framework Laptop */ > -- > 2.41.0 + Mrchromebox Hrmm, it looks like this table is only used if the GOOG0004 ACPI device wasn't found. Is the MrChromebox fw missing this ACPI device?
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c index 500a61b093e4..6ac993be4eb1 100644 --- a/drivers/platform/chrome/cros_ec_lpc.c +++ b/drivers/platform/chrome/cros_ec_lpc.c @@ -533,6 +533,14 @@ static const struct dmi_system_id cros_ec_lpc_dmi_table[] __initconst = { DMI_MATCH(DMI_PRODUCT_NAME, "Glimmer"), }, }, + /* DMI doesn't match modern machines running custom firmware */ + { + /* MrChromebox's firmware */ + .matches = { + DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"), + DMI_MATCH(DMI_BIOS_VERSION, "MrChromebox-"), + }, + }, /* A small number of non-Chromebook/box machines also use the ChromeOS EC */ { /* the Framework Laptop */
ChromeOS EC LPC lacks DMI match for newer machines, which use "Google" DMI_SYS_VENDOR as opposed to "GOOGLE" in older models. This patch adds DMI definition for MrChomebox's custom Coreboots builds, which we (Chrultrabook Project) are using. Signed-off-by: Alicja Michalska <ahplka19@gmail.com> --- drivers/platform/chrome/cros_ec_lpc.c | 8 ++++++++ 1 file changed, 8 insertions(+)