Re: [PATCH v7 15/20] ACPI: CPPC: Keep Performance Limited clearable on NVIDIA T41

[Sumit Gupta]

On 17/09/26 18:53, Rafael J. Wysocki (Intel) wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, Sep 17, 2026 at 2:59 PM Christian Loehle
> <christian.loehle@xxxxxxx> wrote:
> > On 9/16/26 17:28, Christian Loehle wrote:
> > > From: Sumit Gupta <sumitg@xxxxxxxxxx>
> > >
> > > NVIDIA T41 firmware describes Performance Limited as a two-bit field at
> > > offset zero in a DWord SystemMemory access unit. Generic CPPC code must
> > > keep such a field read-only because preserving the remainder when clearing
> > > it requires a read-modify-write which cannot be interlocked with platform
> > > updates.
> > >
> > > The remaining bits of this access unit are unimplemented on T41: they read
> > > as zero, writes have no side effects, and no other register uses them. The
> > > Performance Limited register therefore owns the complete access unit, but
> > > shipped firmware does not describe that property accurately.
> > >
> > > Add a CPPC platform-quirk table keyed by the DSDT header and carry quirk
> > > behavior through explicit flags. Cache a successful table lookup, copy each
> > > GAS into the driver's private descriptor, and apply fixups before layout
> > > validation, mapping and overlap registration.
> > >
> > > Distinguish a genuine non-match from a table-header lookup failure in
> > > acpi_match_platform_list(). Propagate lookup errors from CPPC probe without
> > > caching them, so a transient mapping failure cannot disable the workaround
> > > for every later processor. Existing matcher callers still treat all
> > > negative results as no match.
> > >
> > > For the known T41 layout only, widen a two-bit Performance Limited field at
> > > offset zero to its 32-bit access width. Clearing both status bits can then
> > > be issued as one DWord write of zero without a stale read. Corrected
> > > firmware which reports the full width is unchanged. The workaround
> > > therefore lapses automatically when corrected firmware ships.
> > >
> > > Link: https://lore.kernel.org/lkml/d5f1ea9b-53b7-4db2-983a-b5be8e71a371@xxxxxxx/
> > > Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx>
> > > [ Rework quirk matching and fixup placement; propagate lookup failures
> > >    without caching them. ]
> > > Signed-off-by: Christian Loehle <christian.loehle@xxxxxxx>
> > > ---
> > >   drivers/acpi/cppc_acpi.c | 72 ++++++++++++++++++++++++++++++++++++++++
> > >   drivers/acpi/utils.c     | 13 ++++++--
> > >   2 files changed, 82 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> > > index 6d130381245e..0e218f2be0fe 100644
> > > --- a/drivers/acpi/cppc_acpi.c
> > > +++ b/drivers/acpi/cppc_acpi.c
> > > @@ -330,6 +330,68 @@ static unsigned int cpc_reg_access_width(const struct cpc_reg *reg)
> > >        return reg->bit_width;
> > >   }
> > >
> > > +enum cpc_platform_quirk {
> > > +     CPC_QUIRK_PERF_LIMITED_OWNS_UNIT = BIT(0),
> > > +};
> > > +
> > > +static const struct acpi_platform_list cpc_platform_quirk_list[] = {
> > > +     {
> > > +             .oem_id = "NVIDIA",
> > > +             .oem_table_id = "T41",
> >
> > Sashiko:
> > "Will this quirk successfully match a standard ACPI table header?
> > The ACPI specification requires the OEM Table ID to be exactly 8 bytes long,
> > typically padded with trailing spaces by compliant firmware (e.g.,
> > "T41     ").
> > Looking at acpi_match_platform_list(), it compares the IDs using:
> > strncmp(plat->oem_table_id, hdr.oem_table_id, ACPI_OEM_TABLE_ID_SIZE)
> > Because "T41" is a null-terminated 3-character string, strncmp() will
> > compare the 4th character ('\0' from the quirk definition vs ' ' from the
> > ACPI table header) and immediately report a mismatch, causing the quirk to
> > silently fail on compliant firmware.
> > Should this be padded with spaces (e.g., "T41     ") to ensure it matches
> > the firmware's table correctly?"
> >
> > non-padded "T41" matches exactly what Sumit proposed and was discussed in v6:
> > https://lore.kernel.org/lkml/55a5c9fa-cfd3-4000-b3cc-52c343841c9f@xxxxxxxxxx/
> > So once Sumit adds Tested-by: this should be fine.
>
> Sure, thanks!
>
> Sumit, any concerns?

Hi Rafael,

No concerns.
"T41" without space padding is correct because this firmware NUL pads
the eight-byte field:

 # od -An -tc -j 16 -N 8 /sys/firmware/acpi/tables/DSDT
   T   4   1  \0  \0  \0  \0  \0

The quirk matched, producing the following message during boot:

 ] ACPI CPPC: firmware quirk: Performance Limited owns its access unit, 
using Bit Width 32

Thanks,
Sumit