Message-ID: <CAHmME9rPd8Nu8Q0+R6B4Bz-caKpq5SSvLaiKdwXcyNHY8Ebfqg@mail.gmail.com>
Date:   Sun, 27 Feb 2022 11:03:50 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Len Brown <lenb@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexander Graf <graf@...zon.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v5 2/3] ACPI: allow longer device IDs

On 2/27/22, Ard Biesheuvel <ardb@...nel.org> wrote:
> On Sat, 26 Feb 2022 at 23:07, Jason A. Donenfeld <Jason@...c4.com> wrote:
>>
>> From: Alexander Graf <graf@...zon.com>
>>
>
> Please don't invent patch authors like that. Alex's patch that started
> this discussion was completely different.

Considering the investigative side ("why won't the _CID match?") and
most the commit message were Alex's, and that those things comprise
95% of what this patch is, and that the code change itself isn't even
part of anything Turing complete, I most certainly did not feel
comfortable stripping Alex's authorship. Instead I added myself as a
co-author at the bottom. When in doubt, err on the side of crediting
others. Alex also took a look at this patch, I am under the impression
of at least, before it went out. Let's minimize the paperwork
policing, okay? I think it'd make for a much more pleasant space here.
If Alex objects he can just simply say, "hey feel free to remove me as
author," and it'll be simple as that, and again doesn't involve your
policing.

>
>> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
>> entries of the respective devices. However, we squeeze them into struct
>> acpi_device_id, which only has 9 bytes space to store the identifier. It
>> originally had 16 bytes, but was changed to only have 9 in 6543becf26ff
>> ("mod/file2alias: make modalias generation safe for cross compiling"),
>> presumably on the theory that it would match the ACPI spec so it didn't
>> matter.
>>
>
> Please clarify that this applies to the module metadata side of
> things. The ACPI subsystem already captures and exposes _HIDs and
> _CIDs that are longer than 8 characters, which is why simply
> increasing the size of this field is sufficient to create modules that
> can match devices that expose a CID that is longer than 8 bytes.

Good point for strengthening the argument here. Will do.

>
>> Unfortunately, while most people adhere to the ACPI specs, Microsoft
>> decided that its VM Generation Counter device [1] should only be
>> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
>> than 9 characters.
>>
>> To allow device drivers to match identifiers that exceed the 9 byte
>> limit, this simply ups the length to 16, just like it was before the
>> aforementioned commit. Empirical testing indicates that this
>> doesn't actually increase vmlinux size, because the ulong in the same
>> struct caused there to be 7 bytes of padding anyway.
>>
>
> The padding situation only applies to struct acpi_device_id, whereas
> ACPI_ID_LEN is used in other places as well. Also, the size of vmlinux
> only covers statically allocated instances in the core kernel, and
> most of the ACPI_ID_LEN uses are probably in drivers. So whether
> vmlinux changes size or not is not that relevant.

I actually looked at every usage in the tree (there aren't that many)
and couldn't find a single one where behavior changed, performance
changed, or memory usage changed. I thought we looked together on IRC
so I'm surprised to see you mention this, but maybe I misunderstood
you. Anyway, I can't see the size increase impacting anything at all.
If you see a case, this would be the time to mention that you see
something. I didn't find anything though.

> Patch 6543becf26ff was wrong to change ACPI_ID_LEN, because it failed
> to take into account any other uses of ACPI_ID_LEN, and did not bother
> to explain why the change was necessary in the context of what it was
> trying to achieve.

I'm not sure there really were other usages back then. The commit
message seems descriptive enough to me too. This was about cross
compiling, so padding. But it certainly did seem to limit future
drivers in an unintended way, as you wrote:

> So, given that we need more than 8 characters to match drivers to
> devices exposed by Hyper-V (or other VMMs adhering to the VMGENID
> spec), I think this change is necessary and correct.

Right, that's the idea.


>
> So, with the authorship/signoff corrected, and the commit log clarified,
>
> Reviewed-by: Ard Biesheuvel <ardb@...nel.org>

Thanks.

Hopefully we'll hear from Rafael this week.

Jason

Powered by blists - more mailing lists