Message ID | 20230925180552.76071-1-jarredwhite@linux.microsoft.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | acpi: Use access_width over register_width for system memory accesses | expand |
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 = ®_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 = ®_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 >
On 10/3/2023 11:50 AM, Rafael J. Wysocki wrote: > 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? For the kernel coding style issues, I will clean up for the v2 patch. On the invalid bit_width for systems out there in the field, do you have any suggestions on how to handle this particular scenario? Would it be appropriate to add a kernel parameter flag that can revert back to the previous implementation? P.S. Sorry for the HTML email. Thanks, Jarred
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); + 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 = ®_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)); + 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); + 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 = ®_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; + + switch (size) { case 8: writeb_relaxed(val, vaddr); break;
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. Signed-off-by: Jarred White <jarredwhite@linux.microsoft.com> --- drivers/acpi/cppc_acpi.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)