On Sat, Dec 16, 2023 at 1:13 AM Jarred White <jarredwhite@xxxxxxxxxxxxxxxxxxx> 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. So presumably, there are systems out there in which the platform firmware uses bit_width that is not on a clean 8-bit boundary. It would be good to mention them here. > 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. And it would be good to add a comment to the code for it too. > > Signed-off-by: Jarred White <jarredwhite@xxxxxxxxxxxxxxxxxxx> > --- > Original thread > https://lore.kernel.org/all/20230925180552.76071-1-jarredwhite@xxxxxxxxxxxxxxxxxxx/ > --- > 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) Consider adding parens around the entire expression, or you may get unexpected results. > + > +/* Shift and apply the mask for CPC reads/writes */ > +#define MASK_VAL(val) (((val) >> reg->bit_offset) & \ > + GENMASK((reg->bit_width), 0)) 'reg' should be an argument of the macro or it will be prone to subtle mistakes. > + > 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; What would happen if the platform firmware incorrectly provided reg->access_witdh == 0? > + 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; > --
