On Wed, Jun 4, 2025 at 4:36 AM Chen Yu <yu.c.chen@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> ---
> 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!
