* [PATCH] acpi: Use access_width over register_width for system memory accesses @ 2023-09-25 18:05 Jarred White 2023-10-03 18:50 ` Rafael J. Wysocki 0 siblings, 1 reply; 5+ messages in thread From: Jarred White @ 2023-09-25 18:05 UTC (permalink / raw) To: Rafael J. Wysocki, Len Brown, open list:ACPI, open list 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(-) 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; -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] acpi: Use access_width over register_width for system memory accesses 2023-09-25 18:05 [PATCH] acpi: Use access_width over register_width for system memory accesses Jarred White @ 2023-10-03 18:50 ` Rafael J. Wysocki 2023-10-11 19:56 ` Jarred White 0 siblings, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2023-10-03 18:50 UTC (permalink / raw) To: Jarred White; +Cc: Rafael J. Wysocki, Len Brown, open list:ACPI, open list 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] acpi: Use access_width over register_width for system memory accesses 2023-10-03 18:50 ` Rafael J. Wysocki @ 2023-10-11 19:56 ` Jarred White 2023-10-26 21:15 ` [PATCH v2] " Jarred White 0 siblings, 1 reply; 5+ messages in thread From: Jarred White @ 2023-10-11 19:56 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Len Brown, open list:ACPI, open list 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] acpi: Use access_width over register_width for system memory accesses 2023-10-11 19:56 ` Jarred White @ 2023-10-26 21:15 ` Jarred White 2023-12-01 23:52 ` Jarred White 0 siblings, 1 reply; 5+ messages in thread From: Jarred White @ 2023-10-26 21:15 UTC (permalink / raw) To: jarredwhite; +Cc: lenb, linux-acpi, linux-kernel, rafael 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 bits we 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> --- changelog: v1-->v2: 1. Fixed coding style errors 2. Backwards compatibility with ioremapping of address still an open question. Suggestions are welcomed. drivers/acpi/cppc_acpi.c | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 7ff269a78c20..fb37e1727bf8 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -163,6 +163,13 @@ show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq); show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf); show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time); +/* Use access_width to determine the total number of bits */ +#define ACCESS_WIDTH_TO_BITS(reg) 8 << ((reg)->access_width - 1) + +/* Shift and apply the mask for CPC reads/writes */ +#define MASK_VAL(val) (((val) >> reg->bit_offset) & \ + GENMASK((reg->bit_width), 0)) + static ssize_t show_feedback_ctrs(struct kobject *kobj, struct kobj_attribute *attr, char *buf) { @@ -777,6 +784,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 +792,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 = ACCESS_WIDTH_TO_BITS(gas_t) / 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 +989,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; @@ -991,7 +1001,7 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) *val = 0; if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { - u32 width = 8 << (reg->access_width - 1); + u32 width = ACCESS_WIDTH_TO_BITS(reg); u32 val_u32; acpi_status status; @@ -1015,7 +1025,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 = ACCESS_WIDTH_TO_BITS(reg); + else + size = reg->bit_width; + + switch (size) { case 8: *val = readb_relaxed(vaddr); break; @@ -1034,18 +1049,22 @@ 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 = MASK_VAL(*val); + 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; if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { - u32 width = 8 << (reg->access_width - 1); + u32 width = ACCESS_WIDTH_TO_BITS(reg); acpi_status status; status = acpi_os_write_port((acpi_io_address)reg->address, @@ -1067,7 +1086,14 @@ 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 = ACCESS_WIDTH_TO_BITS(reg); + val = MASK_VAL(val); + } else { + size = reg->bit_width; + } + + switch (size) { case 8: writeb_relaxed(val, vaddr); break; -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] acpi: Use access_width over register_width for system memory accesses 2023-10-26 21:15 ` [PATCH v2] " Jarred White @ 2023-12-01 23:52 ` Jarred White 0 siblings, 0 replies; 5+ messages in thread From: Jarred White @ 2023-12-01 23:52 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: lenb, linux-acpi, linux-kernel Hello, On 10/26/2023 2:15 PM, Jarred White 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 > bits we 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> > --- > changelog: > v1-->v2: > 1. Fixed coding style errors > 2. Backwards compatibility with ioremapping of address still an > open question. Suggestions are welcomed. > > drivers/acpi/cppc_acpi.c | 36 +++++++++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 7ff269a78c20..fb37e1727bf8 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -163,6 +163,13 @@ show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq); > show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf); > show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time); > > +/* Use access_width to determine the total number of bits */ > +#define ACCESS_WIDTH_TO_BITS(reg) 8 << ((reg)->access_width - 1) > + > +/* Shift and apply the mask for CPC reads/writes */ > +#define MASK_VAL(val) (((val) >> reg->bit_offset) & \ > + GENMASK((reg->bit_width), 0)) > + > static ssize_t show_feedback_ctrs(struct kobject *kobj, > struct kobj_attribute *attr, char *buf) > { > @@ -777,6 +784,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 +792,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 = ACCESS_WIDTH_TO_BITS(gas_t) / 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 +989,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; > > @@ -991,7 +1001,7 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) > *val = 0; > > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > - u32 width = 8 << (reg->access_width - 1); > + u32 width = ACCESS_WIDTH_TO_BITS(reg); > u32 val_u32; > acpi_status status; > > @@ -1015,7 +1025,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 = ACCESS_WIDTH_TO_BITS(reg); > + else > + size = reg->bit_width; > + > + switch (size) { > case 8: > *val = readb_relaxed(vaddr); > break; > @@ -1034,18 +1049,22 @@ 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 = MASK_VAL(*val); > + > 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; > > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > - u32 width = 8 << (reg->access_width - 1); > + u32 width = ACCESS_WIDTH_TO_BITS(reg); > acpi_status status; > > status = acpi_os_write_port((acpi_io_address)reg->address, > @@ -1067,7 +1086,14 @@ 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 = ACCESS_WIDTH_TO_BITS(reg); > + val = MASK_VAL(val); > + } else { > + size = reg->bit_width; > + } > + > + switch (size) { > case 8: > writeb_relaxed(val, vaddr); > break; I just wanted to follow-up on the V2 patch and discuss the open we have regarding the backwards compatibility with ioremapping of the address. Thanks, Jarred ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-12-01 23:52 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-25 18:05 [PATCH] acpi: Use access_width over register_width for system memory accesses Jarred White 2023-10-03 18:50 ` Rafael J. Wysocki 2023-10-11 19:56 ` Jarred White 2023-10-26 21:15 ` [PATCH v2] " Jarred White 2023-12-01 23:52 ` Jarred White
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox