| Message ID | 20240411212306.1359466-1-vanshikonda@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] ACPI: CPPC: Fix access width used for PCC registers | expand |
On 4/11/2024 2:23 PM, Vanshidhar Konda wrote: > commit 2f4a4d63a193be6fd530d180bb13c3592052904c modified > cpc_read/cpc_write to use access_width to read CPC registers. For PCC > registers the access width field in the ACPI register macro specifies > the PCC subspace id. For non-zero PCC subspace id the access width is > incorrectly treated as access width. This causes errors when reading > from PCC registers in the CPPC driver. > > For PCC registers base the size of read/write on the bit width field. > The debug message in cpc_read/cpc_write is updated to print relevant > information for the address space type used to read the register. > > Signed-off-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com> > Tested-by: Jarred White <jarredwhite@linux.microsoft.com> > Reviewed-by: Jarred White <jarredwhite@linux.microsoft.com> > Cc: 5.15+ <stable@vger.kernel.org> # 5.15+ > --- > > When testing v6.9-rc1 kernel on AmpereOne system dmesg showed that > cpufreq policy had failed to initialize on some cores during boot because > cpufreq->get() always returned 0. On this system CPPC registers are in PCC > subspace index 2 that are 32 bits wide. With this patch the CPPC driver > interpreted the access width field as 16 bits, causing the register read > to roll over too quickly to provide valid values during frequency > computation. > > v2: > - Use size variable in debug print message > - Use size instead of reg->bit_width for acpi_os_read_memory and > acpi_os_write_memory > > drivers/acpi/cppc_acpi.c | 53 ++++++++++++++++++++++++++++------------ > 1 file changed, 37 insertions(+), 16 deletions(-) Thanks for adding the CC: stable tag. Couple of nits, assuming those are fixed: Reviewed-by: Easwar Hariharan <eahariha@linux.microsoft.com> > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 4bfbe55553f4..a037e9d15f48 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -1002,14 +1002,14 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) > } > > *val = 0; > + size = GET_BIT_WIDTH(reg); > > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > - u32 width = GET_BIT_WIDTH(reg); > u32 val_u32; > acpi_status status; > > status = acpi_os_read_port((acpi_io_address)reg->address, > - &val_u32, width); > + &val_u32, size); > if (ACPI_FAILURE(status)) { > pr_debug("Error: Failed to read SystemIO port %llx\n", > reg->address); > @@ -1018,17 +1018,22 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) > > *val = val_u32; > return 0; > - } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) > + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) { > + /* > + * For registers in PCC space, the register size is determined > + * by the bit width field; the access size is used to indicate > + * the PCC subspace id. > + */ > + size = reg->bit_width; > vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id); > + } > else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) > vaddr = reg_res->sys_mem_vaddr; > else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) > return cpc_read_ffh(cpu, reg, val); > else > return acpi_os_read_memory((acpi_physical_address)reg->address, > - val, reg->bit_width); > - > - size = GET_BIT_WIDTH(reg); > + val, size); > > switch (size) { > case 8: > @@ -1044,8 +1049,13 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) > *val = readq_relaxed(vaddr); > break; > default: > - pr_debug("Error: Cannot read %u bit width from PCC for ss: %d\n", > - reg->bit_width, pcc_ss_id); > + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) { > + pr_debug("Error: Cannot read %u width from for system memory: 0x%llx\n", > + size, reg->address); Nit: from for? There might be a missing word there, or just an extra. Ditto for cpc_write() below. > + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) { > + pr_debug("Error: Cannot read %u bit width to PCC for ss: %d\n", > + size, pcc_ss_id); > + } > return -EFAULT; > } > > @@ -1063,12 +1073,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); > struct cpc_reg *reg = ®_res->cpc_entry.reg; > > + size = GET_BIT_WIDTH(reg); > + > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { > - u32 width = GET_BIT_WIDTH(reg); > acpi_status status; > > status = acpi_os_write_port((acpi_io_address)reg->address, > - (u32)val, width); > + (u32)val, size); > if (ACPI_FAILURE(status)) { > pr_debug("Error: Failed to write SystemIO port %llx\n", > reg->address); > @@ -1076,17 +1087,22 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > } > > return 0; > - } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) > + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) { > + /* > + * For registers in PCC space, the register size is determined > + * by the bit width field; the access size is used to indicate > + * the PCC subspace id. > + */ > + size = reg->bit_width; > vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id); > + } > else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) > vaddr = reg_res->sys_mem_vaddr; > else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) > return cpc_write_ffh(cpu, reg, val); > else > return acpi_os_write_memory((acpi_physical_address)reg->address, > - val, reg->bit_width); > - > - size = GET_BIT_WIDTH(reg); > + val, size); > > if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) > val = MASK_VAL(reg, val); > @@ -1105,8 +1121,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) > writeq_relaxed(val, vaddr); > break; > default: > - pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n", > - reg->bit_width, pcc_ss_id); > + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) { > + pr_debug("Error: Cannot write %u width from for system memory: 0x%llx\n", > + size, reg->address); > + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) { > + pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n", > + size, pcc_ss_id); > + } > ret_val = -EFAULT; > break; > }
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 4bfbe55553f4..a037e9d15f48 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -1002,14 +1002,14 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) } *val = 0; + size = GET_BIT_WIDTH(reg); if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { - u32 width = GET_BIT_WIDTH(reg); u32 val_u32; acpi_status status; status = acpi_os_read_port((acpi_io_address)reg->address, - &val_u32, width); + &val_u32, size); if (ACPI_FAILURE(status)) { pr_debug("Error: Failed to read SystemIO port %llx\n", reg->address); @@ -1018,17 +1018,22 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) *val = val_u32; return 0; - } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) { + /* + * For registers in PCC space, the register size is determined + * by the bit width field; the access size is used to indicate + * the PCC subspace id. + */ + size = reg->bit_width; vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id); + } else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) vaddr = reg_res->sys_mem_vaddr; else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) return cpc_read_ffh(cpu, reg, val); else return acpi_os_read_memory((acpi_physical_address)reg->address, - val, reg->bit_width); - - size = GET_BIT_WIDTH(reg); + val, size); switch (size) { case 8: @@ -1044,8 +1049,13 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val) *val = readq_relaxed(vaddr); break; default: - pr_debug("Error: Cannot read %u bit width from PCC for ss: %d\n", - reg->bit_width, pcc_ss_id); + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) { + pr_debug("Error: Cannot read %u width from for system memory: 0x%llx\n", + size, reg->address); + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) { + pr_debug("Error: Cannot read %u bit width to PCC for ss: %d\n", + size, pcc_ss_id); + } return -EFAULT; } @@ -1063,12 +1073,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu); struct cpc_reg *reg = ®_res->cpc_entry.reg; + size = GET_BIT_WIDTH(reg); + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_IO) { - u32 width = GET_BIT_WIDTH(reg); acpi_status status; status = acpi_os_write_port((acpi_io_address)reg->address, - (u32)val, width); + (u32)val, size); if (ACPI_FAILURE(status)) { pr_debug("Error: Failed to write SystemIO port %llx\n", reg->address); @@ -1076,17 +1087,22 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) } return 0; - } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM && pcc_ss_id >= 0) { + /* + * For registers in PCC space, the register size is determined + * by the bit width field; the access size is used to indicate + * the PCC subspace id. + */ + size = reg->bit_width; vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id); + } else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) vaddr = reg_res->sys_mem_vaddr; else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) return cpc_write_ffh(cpu, reg, val); else return acpi_os_write_memory((acpi_physical_address)reg->address, - val, reg->bit_width); - - size = GET_BIT_WIDTH(reg); + val, size); if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) val = MASK_VAL(reg, val); @@ -1105,8 +1121,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val) writeq_relaxed(val, vaddr); break; default: - pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n", - reg->bit_width, pcc_ss_id); + if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) { + pr_debug("Error: Cannot write %u width from for system memory: 0x%llx\n", + size, reg->address); + } else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) { + pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n", + size, pcc_ss_id); + } ret_val = -EFAULT; break; }