* [PATCH] ACPI: pfr_update: Add more debug information when firmware update failed
@ 2025-06-04 2:29 Chen Yu
2025-06-30 20:10 ` Rafael J. Wysocki
0 siblings, 1 reply; 2+ messages in thread
From: Chen Yu @ 2025-06-04 2:29 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown
Cc: linux-acpi, linux-kernel, Chen Yu, Govindarajulu, Hariganesh
Users reported insufficient error information for debugging during
firmware update failures on certain platforms. Add verbose error logs
in the error code path to enhance debuggability.
Reported-by: "Govindarajulu, Hariganesh" <hariganesh.govindarajulu@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
drivers/acpi/pfr_update.c | 63 +++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/drivers/acpi/pfr_update.c b/drivers/acpi/pfr_update.c
index 031d1ba81b86..318683744ed1 100644
--- a/drivers/acpi/pfr_update.c
+++ b/drivers/acpi/pfr_update.c
@@ -127,8 +127,11 @@ static int query_capability(struct pfru_update_cap_info *cap_hdr,
pfru_dev->rev_id,
PFRU_FUNC_QUERY_UPDATE_CAP,
NULL, ACPI_TYPE_PACKAGE);
- if (!out_obj)
+ if (!out_obj) {
+ dev_dbg(pfru_dev->parent_dev,
+ "Query cap failed with no object\n");
return ret;
+ }
if (out_obj->package.count < CAP_NR_IDX ||
out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER ||
@@ -141,13 +144,17 @@ static int query_capability(struct pfru_update_cap_info *cap_hdr,
out_obj->package.elements[CAP_DRV_SVN_IDX].type != ACPI_TYPE_INTEGER ||
out_obj->package.elements[CAP_PLAT_ID_IDX].type != ACPI_TYPE_BUFFER ||
out_obj->package.elements[CAP_OEM_ID_IDX].type != ACPI_TYPE_BUFFER ||
- out_obj->package.elements[CAP_OEM_INFO_IDX].type != ACPI_TYPE_BUFFER)
+ out_obj->package.elements[CAP_OEM_INFO_IDX].type != ACPI_TYPE_BUFFER) {
+ dev_dbg(pfru_dev->parent_dev,
+ "Query cap failed with invalid package count/type\n");
goto free_acpi_buffer;
+ }
cap_hdr->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
if (cap_hdr->status != DSM_SUCCEED) {
ret = -EBUSY;
- dev_dbg(pfru_dev->parent_dev, "Error Status:%d\n", cap_hdr->status);
+ dev_dbg(pfru_dev->parent_dev, "Query cap Error Status:%d\n",
+ cap_hdr->status);
goto free_acpi_buffer;
}
@@ -193,24 +200,32 @@ static int query_buffer(struct pfru_com_buf_info *info,
out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
pfru_dev->rev_id, PFRU_FUNC_QUERY_BUF,
NULL, ACPI_TYPE_PACKAGE);
- if (!out_obj)
+ if (!out_obj) {
+ dev_dbg(pfru_dev->parent_dev,
+ "Query buf failed with no object\n");
return ret;
+ }
if (out_obj->package.count < BUF_NR_IDX ||
out_obj->package.elements[BUF_STATUS_IDX].type != ACPI_TYPE_INTEGER ||
out_obj->package.elements[BUF_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER ||
out_obj->package.elements[BUF_ADDR_LOW_IDX].type != ACPI_TYPE_INTEGER ||
out_obj->package.elements[BUF_ADDR_HI_IDX].type != ACPI_TYPE_INTEGER ||
- out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER)
+ out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER) {
+ dev_dbg(pfru_dev->parent_dev,
+ "Query buf failed with invalid package count/type\n");
goto free_acpi_buffer;
+ }
info->status = out_obj->package.elements[BUF_STATUS_IDX].integer.value;
info->ext_status =
out_obj->package.elements[BUF_EXT_STATUS_IDX].integer.value;
if (info->status != DSM_SUCCEED) {
ret = -EBUSY;
- dev_dbg(pfru_dev->parent_dev, "Error Status:%d\n", info->status);
- dev_dbg(pfru_dev->parent_dev, "Error Extended Status:%d\n", info->ext_status);
+ dev_dbg(pfru_dev->parent_dev,
+ "Query buf failed with Error Status:%d\n", info->status);
+ dev_dbg(pfru_dev->parent_dev,
+ "Query buf failed with Error Extended Status:%d\n", info->ext_status);
goto free_acpi_buffer;
}
@@ -295,12 +310,16 @@ static bool applicable_image(const void *data, struct pfru_update_cap_info *cap,
m_img_hdr = data + size;
type = get_image_type(m_img_hdr, pfru_dev);
- if (type < 0)
+ if (type < 0) {
+ dev_dbg(pfru_dev->parent_dev, "Invalid image type\n");
return false;
+ }
size = adjust_efi_size(m_img_hdr, size);
- if (size < 0)
+ if (size < 0) {
+ dev_dbg(pfru_dev->parent_dev, "Invalid image size\n");
return false;
+ }
auth = data + size;
size += sizeof(u64) + auth->auth_info.hdr.len;
@@ -346,8 +365,11 @@ static int start_update(int action, struct pfru_device *pfru_dev)
out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
pfru_dev->rev_id, PFRU_FUNC_START,
&in_obj, ACPI_TYPE_PACKAGE);
- if (!out_obj)
+ if (!out_obj) {
+ dev_dbg(pfru_dev->parent_dev,
+ "Update failed to start with no object\n");
return ret;
+ }
if (out_obj->package.count < UPDATE_NR_IDX ||
out_obj->package.elements[UPDATE_STATUS_IDX].type != ACPI_TYPE_INTEGER ||
@@ -355,8 +377,11 @@ static int start_update(int action, struct pfru_device *pfru_dev)
out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER ||
out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].type != ACPI_TYPE_INTEGER ||
out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER ||
- out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
+ out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER) {
+ dev_dbg(pfru_dev->parent_dev,
+ "Update failed with invalid package count/type\n");
goto free_acpi_buffer;
+ }
update_result.status =
out_obj->package.elements[UPDATE_STATUS_IDX].integer.value;
@@ -365,8 +390,10 @@ static int start_update(int action, struct pfru_device *pfru_dev)
if (update_result.status != DSM_SUCCEED) {
ret = -EBUSY;
- dev_dbg(pfru_dev->parent_dev, "Error Status:%d\n", update_result.status);
- dev_dbg(pfru_dev->parent_dev, "Error Extended Status:%d\n",
+ dev_dbg(pfru_dev->parent_dev,
+ "Update failed with Error Status:%d\n", update_result.status);
+ dev_dbg(pfru_dev->parent_dev,
+ "Update failed with Error Extended Status:%d\n",
update_result.ext_status);
goto free_acpi_buffer;
@@ -450,8 +477,10 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
if (ret)
return ret;
- if (len > buf_info.buf_size)
+ if (len > buf_info.buf_size) {
+ dev_dbg(pfru_dev->parent_dev, "Capsule image size too large\n");
return -EINVAL;
+ }
iov.iov_base = (void __user *)buf;
iov.iov_len = len;
@@ -460,10 +489,14 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
/* map the communication buffer */
phy_addr = (phys_addr_t)((buf_info.addr_hi << 32) | buf_info.addr_lo);
buf_ptr = memremap(phy_addr, buf_info.buf_size, MEMREMAP_WB);
- if (!buf_ptr)
+ if (!buf_ptr) {
+ dev_dbg(pfru_dev->parent_dev, "Failed to remap the buffer\n");
return -ENOMEM;
+ }
if (!copy_from_iter_full(buf_ptr, len, &iter)) {
+ dev_dbg(pfru_dev->parent_dev,
+ "Failed to copy the data from the user space buffer\n");
ret = -EINVAL;
goto unmap;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] ACPI: pfr_update: Add more debug information when firmware update failed
2025-06-04 2:29 [PATCH] ACPI: pfr_update: Add more debug information when firmware update failed Chen Yu
@ 2025-06-30 20:10 ` Rafael J. Wysocki
0 siblings, 0 replies; 2+ messages in thread
From: Rafael J. Wysocki @ 2025-06-30 20:10 UTC (permalink / raw)
To: Chen Yu
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel,
Govindarajulu, Hariganesh
On Wed, Jun 4, 2025 at 4:36 AM Chen Yu <yu.c.chen@intel.com> wrote:
>
> Users reported insufficient error information for debugging during
> firmware update failures on certain platforms. Add verbose error logs
> in the error code path to enhance debuggability.
>
> Reported-by: "Govindarajulu, Hariganesh" <hariganesh.govindarajulu@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> drivers/acpi/pfr_update.c | 63 +++++++++++++++++++++++++++++----------
> 1 file changed, 48 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/acpi/pfr_update.c b/drivers/acpi/pfr_update.c
> index 031d1ba81b86..318683744ed1 100644
> --- a/drivers/acpi/pfr_update.c
> +++ b/drivers/acpi/pfr_update.c
> @@ -127,8 +127,11 @@ static int query_capability(struct pfru_update_cap_info *cap_hdr,
> pfru_dev->rev_id,
> PFRU_FUNC_QUERY_UPDATE_CAP,
> NULL, ACPI_TYPE_PACKAGE);
> - if (!out_obj)
> + if (!out_obj) {
> + dev_dbg(pfru_dev->parent_dev,
> + "Query cap failed with no object\n");
> return ret;
> + }
>
> if (out_obj->package.count < CAP_NR_IDX ||
> out_obj->package.elements[CAP_STATUS_IDX].type != ACPI_TYPE_INTEGER ||
> @@ -141,13 +144,17 @@ static int query_capability(struct pfru_update_cap_info *cap_hdr,
> out_obj->package.elements[CAP_DRV_SVN_IDX].type != ACPI_TYPE_INTEGER ||
> out_obj->package.elements[CAP_PLAT_ID_IDX].type != ACPI_TYPE_BUFFER ||
> out_obj->package.elements[CAP_OEM_ID_IDX].type != ACPI_TYPE_BUFFER ||
> - out_obj->package.elements[CAP_OEM_INFO_IDX].type != ACPI_TYPE_BUFFER)
> + out_obj->package.elements[CAP_OEM_INFO_IDX].type != ACPI_TYPE_BUFFER) {
> + dev_dbg(pfru_dev->parent_dev,
> + "Query cap failed with invalid package count/type\n");
> goto free_acpi_buffer;
> + }
>
> cap_hdr->status = out_obj->package.elements[CAP_STATUS_IDX].integer.value;
> if (cap_hdr->status != DSM_SUCCEED) {
> ret = -EBUSY;
> - dev_dbg(pfru_dev->parent_dev, "Error Status:%d\n", cap_hdr->status);
> + dev_dbg(pfru_dev->parent_dev, "Query cap Error Status:%d\n",
> + cap_hdr->status);
> goto free_acpi_buffer;
> }
>
> @@ -193,24 +200,32 @@ static int query_buffer(struct pfru_com_buf_info *info,
> out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> pfru_dev->rev_id, PFRU_FUNC_QUERY_BUF,
> NULL, ACPI_TYPE_PACKAGE);
> - if (!out_obj)
> + if (!out_obj) {
> + dev_dbg(pfru_dev->parent_dev,
> + "Query buf failed with no object\n");
> return ret;
> + }
>
> if (out_obj->package.count < BUF_NR_IDX ||
> out_obj->package.elements[BUF_STATUS_IDX].type != ACPI_TYPE_INTEGER ||
> out_obj->package.elements[BUF_EXT_STATUS_IDX].type != ACPI_TYPE_INTEGER ||
> out_obj->package.elements[BUF_ADDR_LOW_IDX].type != ACPI_TYPE_INTEGER ||
> out_obj->package.elements[BUF_ADDR_HI_IDX].type != ACPI_TYPE_INTEGER ||
> - out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER)
> + out_obj->package.elements[BUF_SIZE_IDX].type != ACPI_TYPE_INTEGER) {
> + dev_dbg(pfru_dev->parent_dev,
> + "Query buf failed with invalid package count/type\n");
> goto free_acpi_buffer;
> + }
>
> info->status = out_obj->package.elements[BUF_STATUS_IDX].integer.value;
> info->ext_status =
> out_obj->package.elements[BUF_EXT_STATUS_IDX].integer.value;
> if (info->status != DSM_SUCCEED) {
> ret = -EBUSY;
> - dev_dbg(pfru_dev->parent_dev, "Error Status:%d\n", info->status);
> - dev_dbg(pfru_dev->parent_dev, "Error Extended Status:%d\n", info->ext_status);
> + dev_dbg(pfru_dev->parent_dev,
> + "Query buf failed with Error Status:%d\n", info->status);
> + dev_dbg(pfru_dev->parent_dev,
> + "Query buf failed with Error Extended Status:%d\n", info->ext_status);
>
> goto free_acpi_buffer;
> }
> @@ -295,12 +310,16 @@ static bool applicable_image(const void *data, struct pfru_update_cap_info *cap,
> m_img_hdr = data + size;
>
> type = get_image_type(m_img_hdr, pfru_dev);
> - if (type < 0)
> + if (type < 0) {
> + dev_dbg(pfru_dev->parent_dev, "Invalid image type\n");
> return false;
> + }
>
> size = adjust_efi_size(m_img_hdr, size);
> - if (size < 0)
> + if (size < 0) {
> + dev_dbg(pfru_dev->parent_dev, "Invalid image size\n");
> return false;
> + }
>
> auth = data + size;
> size += sizeof(u64) + auth->auth_info.hdr.len;
> @@ -346,8 +365,11 @@ static int start_update(int action, struct pfru_device *pfru_dev)
> out_obj = acpi_evaluate_dsm_typed(handle, &pfru_guid,
> pfru_dev->rev_id, PFRU_FUNC_START,
> &in_obj, ACPI_TYPE_PACKAGE);
> - if (!out_obj)
> + if (!out_obj) {
> + dev_dbg(pfru_dev->parent_dev,
> + "Update failed to start with no object\n");
> return ret;
> + }
>
> if (out_obj->package.count < UPDATE_NR_IDX ||
> out_obj->package.elements[UPDATE_STATUS_IDX].type != ACPI_TYPE_INTEGER ||
> @@ -355,8 +377,11 @@ static int start_update(int action, struct pfru_device *pfru_dev)
> out_obj->package.elements[UPDATE_AUTH_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER ||
> out_obj->package.elements[UPDATE_AUTH_TIME_HI_IDX].type != ACPI_TYPE_INTEGER ||
> out_obj->package.elements[UPDATE_EXEC_TIME_LOW_IDX].type != ACPI_TYPE_INTEGER ||
> - out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER)
> + out_obj->package.elements[UPDATE_EXEC_TIME_HI_IDX].type != ACPI_TYPE_INTEGER) {
> + dev_dbg(pfru_dev->parent_dev,
> + "Update failed with invalid package count/type\n");
> goto free_acpi_buffer;
> + }
>
> update_result.status =
> out_obj->package.elements[UPDATE_STATUS_IDX].integer.value;
> @@ -365,8 +390,10 @@ static int start_update(int action, struct pfru_device *pfru_dev)
>
> if (update_result.status != DSM_SUCCEED) {
> ret = -EBUSY;
> - dev_dbg(pfru_dev->parent_dev, "Error Status:%d\n", update_result.status);
> - dev_dbg(pfru_dev->parent_dev, "Error Extended Status:%d\n",
> + dev_dbg(pfru_dev->parent_dev,
> + "Update failed with Error Status:%d\n", update_result.status);
> + dev_dbg(pfru_dev->parent_dev,
> + "Update failed with Error Extended Status:%d\n",
> update_result.ext_status);
>
> goto free_acpi_buffer;
> @@ -450,8 +477,10 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
> if (ret)
> return ret;
>
> - if (len > buf_info.buf_size)
> + if (len > buf_info.buf_size) {
> + dev_dbg(pfru_dev->parent_dev, "Capsule image size too large\n");
> return -EINVAL;
> + }
>
> iov.iov_base = (void __user *)buf;
> iov.iov_len = len;
> @@ -460,10 +489,14 @@ static ssize_t pfru_write(struct file *file, const char __user *buf,
> /* map the communication buffer */
> phy_addr = (phys_addr_t)((buf_info.addr_hi << 32) | buf_info.addr_lo);
> buf_ptr = memremap(phy_addr, buf_info.buf_size, MEMREMAP_WB);
> - if (!buf_ptr)
> + if (!buf_ptr) {
> + dev_dbg(pfru_dev->parent_dev, "Failed to remap the buffer\n");
> return -ENOMEM;
> + }
>
> if (!copy_from_iter_full(buf_ptr, len, &iter)) {
> + dev_dbg(pfru_dev->parent_dev,
> + "Failed to copy the data from the user space buffer\n");
> ret = -EINVAL;
> goto unmap;
> }
> --
Applied as 6.17 material, thanks!
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-06-30 20:11 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-04 2:29 [PATCH] ACPI: pfr_update: Add more debug information when firmware update failed Chen Yu
2025-06-30 20:10 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).