diff mbox series

[v1,1/1] ACPI: CPPC: Fix access width used for PCC registers

Message ID 20240329220054.1205596-1-vanshikonda@os.amperecomputing.com
State Superseded
Headers show
Series [v1,1/1] ACPI: CPPC: Fix access width used for PCC registers | expand

Commit Message

Vanshidhar Konda March 29, 2024, 10 p.m. UTC
Commit 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for
system memory accesses") 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.

Fixes: 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for system memory accesses")
Signed-off-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
---

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.

 drivers/acpi/cppc_acpi.c | 45 +++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 12 deletions(-)

Comments

Easwar Hariharan April 1, 2024, 4:48 p.m. UTC | #1
Hi Vanshi,

Thanks for testing and catching this. One comment below, but Jarred is OOF for a couple days so
we'll get back again after testing on our platform.

On 3/29/2024 3:00 PM, Vanshidhar Konda wrote:
> Commit 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for
> system memory accesses") 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.
> 
> Fixes: 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for system memory accesses")
> Signed-off-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
> ---
> 
> 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.
> 
>  drivers/acpi/cppc_acpi.c | 45 +++++++++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 4bfbe55553f4..23d16a1ee878 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,8 +1018,15 @@ 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)
> @@ -1028,8 +1035,6 @@ 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);
>  
> -	size = GET_BIT_WIDTH(reg);
> -
>  	switch (size) {
>  	case 8:
>  		*val = readb_relaxed(vaddr);
> @@ -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",
> +		if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +			pr_debug("Error: Cannot read %u width from for system memory: 0x%llx\n",
> +				reg->bit_width, 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",
>  			 reg->bit_width, 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 = &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,8 +1087,15 @@ 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)
> @@ -1086,8 +1104,6 @@ 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);
>  
> -	size = GET_BIT_WIDTH(reg);
> -
>  	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",
> +		if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +			pr_debug("Error: Cannot write %u width from for system memory: 0x%llx\n",
> +				reg->bit_width, reg->address);
                                ^^^^^^^^^^^^^^
Shouldn't this be size = GET_BIT_WIDTH(reg) instead of reg->bit_width? I think this is falling back to the
previous behavior where it assumes access_width was not provided by the platform firmware.

> +		} else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
> +			pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n",
>  			 reg->bit_width, pcc_ss_id);
> +		}
>  		ret_val = -EFAULT;
>  		break;
>  	}


Thanks,
Easwar
Vanshidhar Konda April 1, 2024, 5:45 p.m. UTC | #2
On Mon, Apr 01, 2024 at 09:48:28AM -0700, Easwar Hariharan wrote:
>Hi Vanshi,
>
>Thanks for testing and catching this. One comment below, but Jarred is OOF for a couple days so
>we'll get back again after testing on our platform.
>
>On 3/29/2024 3:00 PM, Vanshidhar Konda wrote:
>> Commit 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for
>> system memory accesses") 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.
>>
>> Fixes: 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for system memory accesses")
>> Signed-off-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
>> ---
>>
>> 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.
>>
>>  drivers/acpi/cppc_acpi.c | 45 +++++++++++++++++++++++++++++-----------
>>  1 file changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 4bfbe55553f4..23d16a1ee878 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,8 +1018,15 @@ 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)
>> @@ -1028,8 +1035,6 @@ 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);
>>
>> -	size = GET_BIT_WIDTH(reg);
>> -
>>  	switch (size) {
>>  	case 8:
>>  		*val = readb_relaxed(vaddr);
>> @@ -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",
>> +		if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> +			pr_debug("Error: Cannot read %u width from for system memory: 0x%llx\n",
>> +				reg->bit_width, 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",
>>  			 reg->bit_width, 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 = &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,8 +1087,15 @@ 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)
>> @@ -1086,8 +1104,6 @@ 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);
>>
>> -	size = GET_BIT_WIDTH(reg);
>> -
>>  	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",
>> +		if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
>> +			pr_debug("Error: Cannot write %u width from for system memory: 0x%llx\n",
>> +				reg->bit_width, reg->address);
>                                ^^^^^^^^^^^^^^
>Shouldn't this be size = GET_BIT_WIDTH(reg) instead of reg->bit_width? I think this is falling back to the
>previous behavior where it assumes access_width was not provided by the platform firmware.
>

That makes sense. I'll replace this in both read/write for the
SPACE_SYSTEM_MEMORY in the next version. I can send the next version
after you've a chance to test this.

Thanks,
Vanshi

>> +		} else if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
>> +			pr_debug("Error: Cannot write %u bit width to PCC for ss: %d\n",
>>  			 reg->bit_width, pcc_ss_id);
>> +		}
>>  		ret_val = -EFAULT;
>>  		break;
>>  	}
>
>
>Thanks,
>Easwar
Jarred White April 8, 2024, 8:19 p.m. UTC | #3
On 4/1/2024 10:45 AM, Vanshidhar Konda wrote:
> On Mon, Apr 01, 2024 at 09:48:28AM -0700, Easwar Hariharan wrote:
>> Hi Vanshi,
>>
>> Thanks for testing and catching this. One comment below, but Jarred is 
>> OOF for a couple days so
>> we'll get back again after testing on our platform.
>>
>> On 3/29/2024 3:00 PM, Vanshidhar Konda wrote:
>>> Commit 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for
>>> system memory accesses") 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.
>>>
>>> Fixes: 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for 
>>> system memory accesses")
>>> Signed-off-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
>>> ---

Hi Vanshi,

The patch is good.

You can add,
Reviewed-by: Jarred White <jarredwhite@linux.microsoft.com>
Tested-by: Jarred White <jarredwhite@linux.microsoft.com>

We also found another bug in the process of testing, which we will 
submitting a patch for.


Thanks,
Jarred
Easwar Hariharan April 9, 2024, 5:28 a.m. UTC | #4
On 4/8/2024 1:19 PM, Jarred White wrote:
> On 4/1/2024 10:45 AM, Vanshidhar Konda wrote:
>> On Mon, Apr 01, 2024 at 09:48:28AM -0700, Easwar Hariharan wrote:
>>> Hi Vanshi,
>>>
>>> Thanks for testing and catching this. One comment below, but Jarred is OOF for a couple days so
>>> we'll get back again after testing on our platform.
>>>
>>> On 3/29/2024 3:00 PM, Vanshidhar Konda wrote:
>>>> Commit 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for
>>>> system memory accesses") 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.
>>>>
>>>> Fixes: 2f4a4d63a193 ("ACPI: CPPC: Use access_width over bit_width for system memory accesses")
>>>> Signed-off-by: Vanshidhar Konda <vanshikonda@os.amperecomputing.com>
>>>> ---

Hi Vanshi,

When you send v2 for the SystemMemory space fixes, could you add CC: stable@vger.kernel.org # 5.15+
to your commit message, since your patch fixes 2f4a4d63a193 that was marked for stable?

Thanks,
Easwar
diff mbox series

Patch

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 4bfbe55553f4..23d16a1ee878 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,8 +1018,15 @@  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)
@@ -1028,8 +1035,6 @@  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);
 
-	size = GET_BIT_WIDTH(reg);
-
 	switch (size) {
 	case 8:
 		*val = readb_relaxed(vaddr);
@@ -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",
+		if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+			pr_debug("Error: Cannot read %u width from for system memory: 0x%llx\n",
+				reg->bit_width, 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",
 			 reg->bit_width, 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 = &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,8 +1087,15 @@  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)
@@ -1086,8 +1104,6 @@  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);
 
-	size = GET_BIT_WIDTH(reg);
-
 	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",
+		if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+			pr_debug("Error: Cannot write %u width from for system memory: 0x%llx\n",
+				reg->bit_width, 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",
 			 reg->bit_width, pcc_ss_id);
+		}
 		ret_val = -EFAULT;
 		break;
 	}