[PATCH] platform/chrome: cros_ec_lpc: Add DMI definition for post-Skylake machines running custom Coreboot builds

[PATCH] platform/chrome: cros_ec_lpc: Add DMI definition for post-Skylake machines running custom Coreboot builds

[Alicja Michalska]

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Add DMI definition for post-Skylake machines running custom Coreboot builds

[Tzung-Bi Shih]

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Add DMI definition for post-Skylake machines running custom Coreboot builds

[Alicja Michalska]

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Add DMI definition for post-Skylake machines running custom Coreboot builds

[Brian Norris]

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Add DMI definition for post-Skylake machines running custom Coreboot builds

[Alicja Michalska]

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Add DMI definition for post-Skylake machines running custom Coreboot builds

[Raul Rangel]

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?