lkml.org 
[lkml]   [2023]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] acpi: Use access_width over register_width for system memory accesses
    On Mon, Sep 25, 2023 at 8:06 PM Jarred White
    <jarredwhite@linux.microsoft.com> wrote:
    >
    > To align with ACPI 6.3+, since bit_width can be any 8-bit value, we cannot
    > depend on it being always on a clean 8b boundary. Instead, use access_width
    > to determine the size and use the offset and width to shift and mask the
    > bit swe want to read/write out. Make sure to add a check for system memory
    > since pcc redefines the access_width to subspace id.

    This is fine, but what if there are systems in the field where
    bit_width is invalid, but they just happen to work because of the way
    it is currently handled?

    > Signed-off-by: Jarred White <jarredwhite@linux.microsoft.com>
    > ---
    > drivers/acpi/cppc_acpi.c | 24 +++++++++++++++++++++---
    > 1 file changed, 21 insertions(+), 3 deletions(-)
    >
    > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
    > index 7ff269a78c20..07619b36c056 100644
    > --- a/drivers/acpi/cppc_acpi.c
    > +++ b/drivers/acpi/cppc_acpi.c
    > @@ -777,6 +777,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
    > } else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
    > if (gas_t->address) {
    > void __iomem *addr;
    > + size_t access_width;
    >
    > if (!osc_cpc_flexible_adr_space_confirmed) {
    > pr_debug("Flexible address space capability not supported\n");
    > @@ -784,7 +785,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
    > goto out_free;
    > }
    >
    > - addr = ioremap(gas_t->address, gas_t->bit_width/8);
    > + access_width = ((8 << (gas_t->access_width - 1)) / 8);

    The 8 << (gas_t->access_width - 1) is duplicated twice below. There
    could be an inline function doing that computation.

    And the outer parens above are not needed AFAICS.

    > + addr = ioremap(gas_t->address, access_width);
    > if (!addr)
    > goto out_free;
    > cpc_ptr->cpc_regs[i-2].sys_mem_vaddr = addr;
    > @@ -980,6 +982,7 @@ int __weak cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
    > static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
    > {
    > void __iomem *vaddr = NULL;
    > + int size;
    > int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
    > struct cpc_reg *reg = &reg_res->cpc_entry.reg;
    >
    > @@ -1015,7 +1018,12 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
    > return acpi_os_read_memory((acpi_physical_address)reg->address,
    > val, reg->bit_width);
    >
    > - switch (reg->bit_width) {
    > + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
    > + size = (8 << (reg->access_width - 1));

    Code duplication, outer perens not needed.

    > + else
    > + size = reg->bit_width;
    > +
    > + switch (size) {
    > case 8:
    > *val = readb_relaxed(vaddr);
    > break;
    > @@ -1034,12 +1042,16 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
    > return -EFAULT;
    > }
    >
    > + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
    > + *val = (*val >> reg->bit_offset) & GENMASK((reg->bit_width) - 1, 0);

    The formula on the right-hand side of this is duplicated below.
    Again, there could be an inline function doing this.

    > +
    > return 0;
    > }
    >
    > static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
    > {
    > int ret_val = 0;
    > + int size;
    > void __iomem *vaddr = NULL;
    > int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
    > struct cpc_reg *reg = &reg_res->cpc_entry.reg;
    > @@ -1067,7 +1079,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
    > return acpi_os_write_memory((acpi_physical_address)reg->address,
    > val, reg->bit_width);
    >
    > - switch (reg->bit_width) {
    > + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
    > + size = (8 << (reg->access_width - 1));
    > + val = (val >> reg->bit_offset) & GENMASK((reg->bit_width) - 1, 0);
    > + } else
    > + size = reg->bit_width;

    Missing braces (as per the kernel coding style).

    > +
    > + switch (size) {
    > case 8:
    > writeb_relaxed(val, vaddr);
    > break;
    > --
    > 2.34.1
    >

    \
     
     \ /
      Last update: 2023-10-03 20:51    [W:2.770 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site