[PATCH v2] ACPI: Fix acpi_evaluate_object() return value check

[Yijing Wang]

Fix acpi_evaluate_object() return value check,
shoud acpi_status not int.

Signed-off-by: Yijing Wang <wangy...@huawei.com>
---

v1->v2: Add CC to the related subsystem MAINTAINERS.

---
drivers/gpu/drm/i915/intel_acpi.c | 13 +++++++------
drivers/gpu/drm/nouveau/core/subdev/mxm/base.c | 6 +++---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 13 +++++++------
drivers/pci/pci-label.c | 6 +++---
4 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
index dfff090..7ea00e5 100644
--- a/drivers/gpu/drm/i915/intel_acpi.c
+++ b/drivers/gpu/drm/i915/intel_acpi.c
@@ -35,7 +35,7 @@ static int intel_dsm(acpi_handle handle, int func)
union acpi_object params[4];
union acpi_object *obj;
u32 result;
- int ret = 0;
+ acpi_status status;

input.count = 4;
input.pointer = params;
@@ -50,8 +50,8 @@ static int intel_dsm(acpi_handle handle, int func)
params[3].package.count = 0;
params[3].package.elements = NULL;

- ret = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (ret) {
+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {
DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
return ret;
}
@@ -141,7 +141,8 @@ static void intel_dsm_platform_mux_info(void)
struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *pkg;
- int i, ret;
+ acpi_status status;
+ int i;

input.count = 4;
input.pointer = params;
@@ -156,9 +157,9 @@ static void intel_dsm_platform_mux_info(void)
params[3].package.count = 0;
params[3].package.elements = NULL;

- ret = acpi_evaluate_object(intel_dsm_priv.dhandle, "_DSM", &input,
+ acpi_status = acpi_evaluate_object(intel_dsm_priv.dhandle, "_DSM", &input,
&output);
- if (ret) {
+ if (ACPI_FAILURE(status)) {
DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
goto out;
}
diff --git a/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c b/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
index 1291204..3920943 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
@@ -114,14 +114,14 @@ mxm_shadow_dsm(struct nouveau_mxm *mxm, u8 version)
struct acpi_buffer retn = { ACPI_ALLOCATE_BUFFER, NULL };
union acpi_object *obj;
acpi_handle handle;
- int ret;
+ acpi_status status;

handle = ACPI_HANDLE(&device->pdev->dev);
if (!handle)
return false;

- ret = acpi_evaluate_object(handle, "_DSM", &list, &retn);
- if (ret) {
+ status = acpi_evaluate_object(handle, "_DSM", &list, &retn);
+ if (ACPI_FAILURE(status)) {
nv_debug(mxm, "DSM MXMS failed: %d\n", ret);
return false;
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index ba0183f..6f810f2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -82,7 +82,8 @@ static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg, uint32_t *
struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *obj;
- int i, err;
+ acpi_status status;
+ int i;
char args_buff[4];

input.count = 4;
@@ -101,8 +102,8 @@ static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg, uint32_t *
args_buff[i] = (arg >> i * 8) & 0xFF;
params[3].buffer.pointer = args_buff;

- err = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (err) {
+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {
printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
return err;
}
@@ -134,7 +135,7 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)
struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *obj;
- int err;
+ acpi_status status;

input.count = 4;
input.pointer = params;
@@ -148,8 +149,8 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)
params[3].type = ACPI_TYPE_INTEGER;
params[3].integer.value = arg;

- err = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (err) {
+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {
printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
return err;
}
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index d51f45a..3c21f1b 100644
--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -213,7 +213,7 @@ dsm_get_label(acpi_handle handle, int func,
union acpi_object *obj;
int len = 0;

- int err;
+ acpi_status status;

input.count = 4;
input.pointer = params;
@@ -228,8 +228,8 @@ dsm_get_label(acpi_handle handle, int func,
params[3].package.count = 0;
params[3].package.elements = NULL;

- err = acpi_evaluate_object(handle, "_DSM", &input, output);
- if (err)
+ status = acpi_evaluate_object(handle, "_DSM", &input, output);
+ if (ACPI_FAILURE(status))
return -1;

obj = (union acpi_object *)output->pointer;
--
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Hanjun Guo]

On 2014-1-17 9:29, Yijing Wang wrote:
> Fix acpi_evaluate_object() return value check,
> shoud acpi_status not int.
>
> Signed-off-by: Yijing Wang <wangy...@huawei.com>
> ---
>
> v1->v2: Add CC to the related subsystem MAINTAINERS.
>
> ---
> drivers/gpu/drm/i915/intel_acpi.c | 13 +++++++------
> drivers/gpu/drm/nouveau/core/subdev/mxm/base.c | 6 +++---
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 13 +++++++------
> drivers/pci/pci-label.c | 6 +++---
> 4 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> index dfff090..7ea00e5 100644
> --- a/drivers/gpu/drm/i915/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> @@ -35,7 +35,7 @@ static int intel_dsm(acpi_handle handle, int func)
> union acpi_object params[4];
> union acpi_object *obj;
> u32 result;
> - int ret = 0;

The 'ret' is removed, but

> + acpi_status status;
>
> input.count = 4;
> input.pointer = params;
> @@ -50,8 +50,8 @@ static int intel_dsm(acpi_handle handle, int func)
> params[3].package.count = 0;
> params[3].package.elements = NULL;
>
> - ret = acpi_evaluate_object(handle, "_DSM", &input, &output);
> - if (ret) {
> + status = acpi_evaluate_object(handle, "_DSM", &input, &output);
> + if (ACPI_FAILURE(status)) {
> DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
> return ret;

you still use it here, so you should -EINVAL or something else here.

here too.

> }
> @@ -134,7 +135,7 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)
> struct acpi_object_list input;
> union acpi_object params[4];
> union acpi_object *obj;
> - int err;
> + acpi_status status;
>
> input.count = 4;
> input.pointer = params;
> @@ -148,8 +149,8 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)
> params[3].type = ACPI_TYPE_INTEGER;
> params[3].integer.value = arg;
>
> - err = acpi_evaluate_object(handle, "_DSM", &input, &output);
> - if (err) {
> + status = acpi_evaluate_object(handle, "_DSM", &input, &output);
> + if (ACPI_FAILURE(status)) {
> printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
> return err;

and here.

> }
> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
> index d51f45a..3c21f1b 100644
> --- a/drivers/pci/pci-label.c
> +++ b/drivers/pci/pci-label.c
> @@ -213,7 +213,7 @@ dsm_get_label(acpi_handle handle, int func,
> union acpi_object *obj;
> int len = 0;
>
> - int err;
> + acpi_status status;
>
> input.count = 4;
> input.pointer = params;
> @@ -228,8 +228,8 @@ dsm_get_label(acpi_handle handle, int func,
> params[3].package.count = 0;
> params[3].package.elements = NULL;
>
> - err = acpi_evaluate_object(handle, "_DSM", &input, output);
> - if (err)
> + status = acpi_evaluate_object(handle, "_DSM", &input, output);
> + if (ACPI_FAILURE(status))
> return -1;

can we return specific error such as -EINVAL instead of hard code?

Thanks
Hanjun

[Yijing Wang]

>> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
>> index dfff090..7ea00e5 100644
>> --- a/drivers/gpu/drm/i915/intel_acpi.c
>> +++ b/drivers/gpu/drm/i915/intel_acpi.c
>> @@ -35,7 +35,7 @@ static int intel_dsm(acpi_handle handle, int func)
>> union acpi_object params[4];
>> union acpi_object *obj;
>> u32 result;
>> - int ret = 0;
>
> The 'ret' is removed, but

Ah, it's my mistake, will updata it right now, thanks!

>
>> + acpi_status status;
>>
>> input.count = 4;
>> input.pointer = params;
>> @@ -50,8 +50,8 @@ static int intel_dsm(acpi_handle handle, int func)
>> params[3].package.count = 0;
>> params[3].package.elements = NULL;
>>
>> - ret = acpi_evaluate_object(handle, "_DSM", &input, &output);
>> - if (ret) {
>> + status = acpi_evaluate_object(handle, "_DSM", &input, &output);
>> + if (ACPI_FAILURE(status)) {
>> DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
>> return ret;
>
> you still use it here, so you should -EINVAL or something else here.

OK

>
>> }
>> @@ -141,7 +141,8 @@ static void intel_dsm_platform_mux_info(void)
>> struct acpi_object_list input;
>> union acpi_object params[4];
>> union acpi_object *pkg;
>> - int i, ret;
>> + acpi_status status;
>>

>> - err = acpi_evaluate_object(handle, "_DSM", &input, &output);
>> - if (err) {
>> + status = acpi_evaluate_object(handle, "_DSM", &input, &output);
>> + if (ACPI_FAILURE(status)) {
>> printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
>> return err;
>
> here too.

OK, thanks.

>
>> }
>> @@ -134,7 +135,7 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)
>> struct acpi_object_list input;
>> union acpi_object params[4];
>> union acpi_object *obj;
>> - int err;
>> + acpi_status status;
>>
>> input.count = 4;
>> input.pointer = params;
>> @@ -148,8 +149,8 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)
>> params[3].type = ACPI_TYPE_INTEGER;
>> params[3].integer.value = arg;
>>
>> - err = acpi_evaluate_object(handle, "_DSM", &input, &output);
>> - if (err) {
>> + status = acpi_evaluate_object(handle, "_DSM", &input, &output);
>> + if (ACPI_FAILURE(status)) {
>> printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
>> return err;
>
> and here.

thanks.

>
>> }
>> diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
>> index d51f45a..3c21f1b 100644
>> --- a/drivers/pci/pci-label.c
>> +++ b/drivers/pci/pci-label.c
>> @@ -213,7 +213,7 @@ dsm_get_label(acpi_handle handle, int func,
>> union acpi_object *obj;
>> int len = 0;
>>
>> - int err;
>> + acpi_status status;
>>
>> input.count = 4;
>> input.pointer = params;
>> @@ -228,8 +228,8 @@ dsm_get_label(acpi_handle handle, int func,
>> params[3].package.count = 0;
>> params[3].package.elements = NULL;
>>
>> - err = acpi_evaluate_object(handle, "_DSM", &input, output);
>> - if (err)
>> + status = acpi_evaluate_object(handle, "_DSM", &input, output);
>> + if (ACPI_FAILURE(status))
>> return -1;
>
> can we return specific error such as -EINVAL instead of hard code?

I will try to add some more useful debug info here. thanks!

>
> Thanks
> Hanjun
>
> .
>


--
Thanks!
Yijing

[Yijing Wang]

Fix acpi_evaluate_object() return value check,
shoud acpi_status not int.

Signed-off-by: Yijing Wang <wangy...@huawei.com>
---

v2->v3: Fix compile error pointed out by Hanjun.
v1->v2: Add CC to related subsystem MAINTAINERS
---
drivers/gpu/drm/i915/intel_acpi.c | 24 ++++++++++++++----------
drivers/gpu/drm/nouveau/core/subdev/mxm/base.c | 9 +++++----
drivers/gpu/drm/nouveau/nouveau_acpi.c | 23 +++++++++++++----------
drivers/pci/pci-label.c | 9 ++++++---
4 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
index dfff090..87e8f74 100644
--- a/drivers/gpu/drm/i915/intel_acpi.c
+++ b/drivers/gpu/drm/i915/intel_acpi.c
@@ -35,7 +35,8 @@ static int intel_dsm(acpi_handle handle, int func)

union acpi_object params[4];
union acpi_object *obj;
u32 result;
- int ret = 0;

+ acpi_status status;
+ int ret;

input.count = 4;
input.pointer = params;

@@ -50,10 +51,11 @@ static int intel_dsm(acpi_handle handle, int func)

params[3].package.count = 0;
params[3].package.elements = NULL;

- ret = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (ret) {

- DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
- return ret;

+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {

+ DRM_DEBUG_DRIVER("failed to evaluate _DSM: %s\n",
+ acpi_format_exception(status));
+ return -EINVAL;
}

obj = (union acpi_object *)output.pointer;
@@ -141,7 +143,8 @@ static void intel_dsm_platform_mux_info(void)

struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *pkg;
- int i, ret;
+ acpi_status status;

+ int i;

input.count = 4;
input.pointer = params;

@@ -156,10 +159,11 @@ static void intel_dsm_platform_mux_info(void)

params[3].package.count = 0;
params[3].package.elements = NULL;

- ret = acpi_evaluate_object(intel_dsm_priv.dhandle, "_DSM", &input,
- &output);
- if (ret) {
- DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
+ acpi_status = acpi_evaluate_object(intel_dsm_priv.dhandle,
+ "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {
+ DRM_DEBUG_DRIVER("failed to evaluate _DSM: %s\n",
+ acpi_format_exception(status));

goto out;
}

diff --git a/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c b/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c

index 1291204..c5e7a2b 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
@@ -114,15 +114,16 @@ mxm_shadow_dsm(struct nouveau_mxm *mxm, u8 version)

struct acpi_buffer retn = { ACPI_ALLOCATE_BUFFER, NULL };
union acpi_object *obj;
acpi_handle handle;
- int ret;
+ acpi_status status;

handle = ACPI_HANDLE(&device->pdev->dev);
if (!handle)
return false;

- ret = acpi_evaluate_object(handle, "_DSM", &list, &retn);
- if (ret) {

- nv_debug(mxm, "DSM MXMS failed: %d\n", ret);

+ status = acpi_evaluate_object(handle, "_DSM", &list, &retn);
+ if (ACPI_FAILURE(status)) {

+ nv_debug(mxm, "DSM MXMS failed: %s\n",
+ acpi_format_exception(status));

return false;
}

diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c

index ba0183f..de3068b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -82,7 +82,8 @@ static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg, uint32_t *

struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *obj;

- int i, err;
+ acpi_status status;
+ int i;
char args_buff[4];

input.count = 4;

@@ -101,10 +102,11 @@ static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg, uint32_t *

args_buff[i] = (arg >> i * 8) & 0xFF;
params[3].buffer.pointer = args_buff;

- err = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (err) {

- printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
- return err;

+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {

+ pr_info("failed to evaluate _DSM: %s\n",
+ acpi_format_exception(status));
+ return -EINVAL;
}

obj = (union acpi_object *)output.pointer;
@@ -134,7 +136,7 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)

struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *obj;
- int err;
+ acpi_status status;

input.count = 4;
input.pointer = params;

@@ -148,10 +150,11 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)

params[3].type = ACPI_TYPE_INTEGER;
params[3].integer.value = arg;

- err = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (err) {

- printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
- return err;

+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {

+ pr_info("failed to evaluate _DSM: %s\n",
+ acpi_format_exception(status));
+ return -EINVAL;
}

obj = (union acpi_object *)output.pointer;
diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index d51f45a..7ba4de6 100644

--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -213,7 +213,7 @@ dsm_get_label(acpi_handle handle, int func,
union acpi_object *obj;
int len = 0;

- int err;
+ acpi_status status;

input.count = 4;
input.pointer = params;

@@ -228,9 +228,12 @@ dsm_get_label(acpi_handle handle, int func,

params[3].package.count = 0;
params[3].package.elements = NULL;

- err = acpi_evaluate_object(handle, "_DSM", &input, output);
- if (err)
+ status = acpi_evaluate_object(handle, "_DSM", &input, output);

+ if (ACPI_FAILURE(status)) {
+ pr_info("failed to evaluate _DSM: %s\n",
+ acpi_format_exception(status));
return -1;
+ }

obj = (union acpi_object *)output->pointer;

--
1.7.1

[Jani Nikula]

On Fri, 17 Jan 2014, Yijing Wang <wangy...@huawei.com> wrote:
> Fix acpi_evaluate_object() return value check,
> shoud acpi_status not int.

Please spellcheck.

In the two hunks above, one of the error paths calls
kfree(output.pointer), the other doesn't. Which one is wrong?

The fix for that should probably be a follow-up patch; this patch is

Reviewed-by: Jani Nikula <jani....@intel.com>

Jani Nikula, Intel Open Source Technology Center

[Yijing Wang]

Since acpi_evaluate_object() returns acpi_status and not plain int,
ACPI_FAILURE() should be used for checking its return value.

Reviewed-by: Jani Nikula <jani....@intel.com>

Signed-off-by: Yijing Wang <wangy...@huawei.com>
---

v3->v4: Fix spell error, add Jani Nikula reviewed-by.

[Bjorn Helgaas]

On Mon, Jan 20, 2014 at 7:46 PM, Yijing Wang <wangy...@huawei.com> wrote:
> Since acpi_evaluate_object() returns acpi_status and not plain int,
> ACPI_FAILURE() should be used for checking its return value.
>
> Reviewed-by: Jani Nikula <jani....@intel.com>
> Signed-off-by: Yijing Wang <wangy...@huawei.com>
> ---
> v3->v4: Fix spell error, add Jani Nikula reviewed-by.
> v2->v3: Fix compile error pointed out by Hanjun.
> v1->v2: Add CC to related subsystem MAINTAINERS
> ---
> drivers/gpu/drm/i915/intel_acpi.c | 24 ++++++++++++++----------
> drivers/gpu/drm/nouveau/core/subdev/mxm/base.c | 9 +++++----
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 23 +++++++++++++----------
> drivers/pci/pci-label.c | 9 ++++++---

For the drivers/pci/pci-label.c part,

Acked-by: Bjorn Helgaas <bhel...@google.com>

> + status = acpi_evaluate_object(handle, "_DSM", &input, &output);
> + if (ACPI_FAILURE(status)) {
> + DRM_DEBUG_DRIVER("failed to evaluate _DSM: %s\n",
> + acpi_format_exception(status));

It's too bad there isn't an easy way to produce more informative error
messages, e.g., by including a namespace path or something. A message
like:

failed to evaluate _DSM: A requested entity is not found

is only useful if there's enough context to figure out what's going on.

Bjorn

[Konrad Rzeszutek Wilk]

Yijing Wang <wangy...@huawei.com> wrote:
>Fix acpi_evaluate_object() return value check,
>shoud acpi_status not int.

Should be?
Your mailer also ate the word 'to' .

[Yijing Wang]

On 2014/1/23 5:37, Bjorn Helgaas wrote:
> On Mon, Jan 20, 2014 at 7:46 PM, Yijing Wang <wangy...@huawei.com> wrote:
>> Since acpi_evaluate_object() returns acpi_status and not plain int,
>> ACPI_FAILURE() should be used for checking its return value.
>>
>> Reviewed-by: Jani Nikula <jani....@intel.com>
>> Signed-off-by: Yijing Wang <wangy...@huawei.com>
>> ---
>> v3->v4: Fix spell error, add Jani Nikula reviewed-by.
>> v2->v3: Fix compile error pointed out by Hanjun.
>> v1->v2: Add CC to related subsystem MAINTAINERS
>> ---
>> drivers/gpu/drm/i915/intel_acpi.c | 24 ++++++++++++++----------
>> drivers/gpu/drm/nouveau/core/subdev/mxm/base.c | 9 +++++----
>> drivers/gpu/drm/nouveau/nouveau_acpi.c | 23 +++++++++++++----------
>> drivers/pci/pci-label.c | 9 ++++++---
>
> For the drivers/pci/pci-label.c part,
>
> Acked-by: Bjorn Helgaas <bhel...@google.com>

Thanks.

>
>> + status = acpi_evaluate_object(handle, "_DSM", &input, &output);
>> + if (ACPI_FAILURE(status)) {
>> + DRM_DEBUG_DRIVER("failed to evaluate _DSM: %s\n",
>> + acpi_format_exception(status));
>
> It's too bad there isn't an easy way to produce more informative error
> messages, e.g., by including a namespace path or something. A message
> like:
>
> failed to evaluate _DSM: A requested entity is not found
>
> is only useful if there's enough context to figure out what's going on.

Yes, I will add the namespace path into the debug info, thanks!

>
> Bjorn
>
> .
>


--
Thanks!
Yijing

[Yijing Wang]

Since acpi_evaluate_object() returns acpi_status and not plain int,

ACPI_FAILURE() should be used for checking its return value. Also
add some detailed debug info when acpi_evaluate_object() failed.

Reviewed-by: Jani Nikula <jani....@intel.com>
Acked-by: Bjorn Helgaas <bhel...@google.com>

Signed-off-by: Yijing Wang <wangy...@huawei.com>
---

v4->v5: Add some detailed debug info for acpi_evaluate_object()
failure suggested by Bjorn.

v3->v4: Fix spell error, add Jani Nikula reviewed-by.
v2->v3: Fix compile error pointed out by Hanjun.
v1->v2: Add CC to related subsystem MAINTAINERS
---

drivers/gpu/drm/i915/intel_acpi.c | 33 ++++++++++++++++-------
drivers/gpu/drm/nouveau/core/subdev/mxm/base.c | 13 ++++++---
drivers/gpu/drm/nouveau/nouveau_acpi.c | 25 +++++++++++-------
drivers/pci/pci-label.c | 10 +++++--
4 files changed, 54 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
index dfff090..e7b526b 100644
--- a/drivers/gpu/drm/i915/intel_acpi.c
+++ b/drivers/gpu/drm/i915/intel_acpi.c
@@ -31,11 +31,13 @@ static const u8 intel_dsm_guid[] = {

static int intel_dsm(acpi_handle handle, int func)

{
struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+ struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };

struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *obj;

u32 result;
- int ret = 0;
+ acpi_status status;

+ int ret;

input.count = 4;
input.pointer = params;

@@ -50,10 +52,14 @@ static int intel_dsm(acpi_handle handle, int func)

params[3].package.count = 0;
params[3].package.elements = NULL;

- ret = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (ret) {
- DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
- return ret;

+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {

+ acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
+ DRM_DEBUG_DRIVER(
+ "failed to evaluate _DSM for %s, exit status %u\n",
+ (char *)string.pointer, (unsigned int)status);
+ kfree(string.pointer);

+ return -EINVAL;
}

obj = (union acpi_object *)output.pointer;

@@ -138,10 +144,12 @@ static char *intel_dsm_mux_type(u8 type)
static void intel_dsm_platform_mux_info(void)
{
struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
+ struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };

struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *pkg;
- int i, ret;
+ acpi_status status;
+ int i;

input.count = 4;
input.pointer = params;

@@ -156,10 +164,15 @@ static void intel_dsm_platform_mux_info(void)

params[3].package.count = 0;
params[3].package.elements = NULL;

- ret = acpi_evaluate_object(intel_dsm_priv.dhandle, "_DSM", &input,

- &output);
- if (ret) {
- DRM_DEBUG_DRIVER("failed to evaluate _DSM: %d\n", ret);
+ acpi_status = acpi_evaluate_object(intel_dsm_priv.dhandle,

+ "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {
+ acpi_get_name(intel_dsm_priv.dhandle,
+ ACPI_FULL_PATHNAME, &string);
+ DRM_DEBUG_DRIVER(
+ "failed to evaluate _DSM for %s, exit status %u\n",
+ (char *)string.pointer, (unsigned int)status);
+ kfree(string.pointer);

goto out;
}

diff --git a/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c b/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c

index 1291204..c30ee88 100644
--- a/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
+++ b/drivers/gpu/drm/nouveau/core/subdev/mxm/base.c
@@ -112,17 +112,22 @@ mxm_shadow_dsm(struct nouveau_mxm *mxm, u8 version)
};
struct acpi_object_list list = { ARRAY_SIZE(args), args };

struct acpi_buffer retn = { ACPI_ALLOCATE_BUFFER, NULL };

+ struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };

union acpi_object *obj;
acpi_handle handle;
- int ret;
+ acpi_status status;

handle = ACPI_HANDLE(&device->pdev->dev);
if (!handle)
return false;

- ret = acpi_evaluate_object(handle, "_DSM", &list, &retn);
- if (ret) {

- nv_debug(mxm, "DSM MXMS failed: %d\n", ret);

+ status = acpi_evaluate_object(handle, "_DSM", &list, &retn);
+ if (ACPI_FAILURE(status)) {

+ acpi_get_name(handle, ACPI_FULL_PATHNAME, &string);
+ nv_debug(mxm, "DSM MXMS failed for %s: exit status %u\n",
+ (char *)string.pointer,
+ (unsigned int)status);
+ kfree(string.pointer);

return false;
}

diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c

index ba0183f..53d24a7 100644

--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -82,7 +82,8 @@ static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg, uint32_t *
struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *obj;
- int i, err;
+ acpi_status status;
+ int i;
char args_buff[4];

input.count = 4;

@@ -101,10 +102,12 @@ static int nouveau_optimus_dsm(acpi_handle handle, int func, int arg, uint32_t *

args_buff[i] = (arg >> i * 8) & 0xFF;
params[3].buffer.pointer = args_buff;

- err = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (err) {
- printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
- return err;

+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {

+ acpi_handle_info(handle,
+ "failed to evaluate _DSM, exit status %u\n",
+ (unsigned int)status);

+ return -EINVAL;
}

obj = (union acpi_object *)output.pointer;

@@ -134,7 +137,7 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)

struct acpi_object_list input;
union acpi_object params[4];
union acpi_object *obj;
- int err;
+ acpi_status status;

input.count = 4;
input.pointer = params;

@@ -148,10 +151,12 @@ static int nouveau_dsm(acpi_handle handle, int func, int arg, uint32_t *result)

params[3].type = ACPI_TYPE_INTEGER;
params[3].integer.value = arg;

- err = acpi_evaluate_object(handle, "_DSM", &input, &output);
- if (err) {
- printk(KERN_INFO "failed to evaluate _DSM: %d\n", err);
- return err;

+ status = acpi_evaluate_object(handle, "_DSM", &input, &output);
+ if (ACPI_FAILURE(status)) {

+ acpi_handle_info(handle,
+ "failed to evaluate _DSM, exit status %u\n",
+ (unsigned int)status);

+ return -EINVAL;
}

obj = (union acpi_object *)output.pointer;

diff --git a/drivers/pci/pci-label.c b/drivers/pci/pci-label.c
index d51f45a..0216094 100644

--- a/drivers/pci/pci-label.c
+++ b/drivers/pci/pci-label.c
@@ -213,7 +213,7 @@ dsm_get_label(acpi_handle handle, int func,
union acpi_object *obj;
int len = 0;

- int err;
+ acpi_status status;

input.count = 4;
input.pointer = params;

@@ -228,9 +228,13 @@ dsm_get_label(acpi_handle handle, int func,

params[3].package.count = 0;
params[3].package.elements = NULL;

- err = acpi_evaluate_object(handle, "_DSM", &input, output);
- if (err)
+ status = acpi_evaluate_object(handle, "_DSM", &input, output);
+ if (ACPI_FAILURE(status)) {

+ acpi_handle_info(handle,
+ "failed to evaluate _DSM, exit status %u\n",
+ (unsigned int)status);
return -1;
+ }

obj = (union acpi_object *)output->pointer;

--

1.7.1

[Bjorn Helgaas]

I said "too bad there isn't an *easy* way" to include more
information. IMHO this is too ugly and error-prone to use
consistently. And if you are going to add more information, why did
you only do it for some of the calls and not others?

I considered adding a %p extension to print the pathname; I don't know
if that's worthwhile or not. I think it would be ideal if we had a
struct device and could use dev_info(), and then a way to connect the
struct device with an ACPI path, like maybe a dmesg note when we
create the struct device corresponding to an ACPI Device node.

Bjorn

[Rafael J. Wysocki]

Well, we can generally print something like that from pci_acpi_setup().

What about the below? Wouldn't it generate too much output on some systems?

Rafael


---
drivers/pci/pci-acpi.c | 2 ++
1 file changed, 2 insertions(+)

Index: linux-pm/drivers/pci/pci-acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-acpi.c
+++ linux-pm/drivers/pci/pci-acpi.c
@@ -330,6 +330,8 @@ static void pci_acpi_setup(struct device
if (!adev)
return;

+ acpi_handle_info(adev->handle, "bound to %s\n", dev_name(dev));
+
pci_acpi_add_pm_notifier(adev, pci_dev);
if (!adev->wakeup.flags.valid)
return;

[Bjorn Helgaas]

Yeah, that probably would generate an awful lot of output. I was just
hoping to avoid treating ACPI pathnames as first-class objects. What
do you think about a %p extension? I played with that once, but I
seem to have lost the patch.

[Bjorn Helgaas]

On Fri, Jan 24, 2014 at 8:36 AM, Rafael J. Wysocki <r...@rjwysocki.net> wrote:

> Well, it may be worth doing. However, that information is readily available from
> sysfs anyway, you only need to follow the firmware_node link in the PCI device's
> sysfs directory and read the path attribute from there. For example, on my
> system:
>
> $ cat /sys/devices/pci0000:00/0000:00:1c.4/0000:0b:00.0/firmware_node/path
> \_SB_.PCI0.RP05.PXSX

That's perfect. If we had a struct device, we could just use
dev_info() for these messages. But I have no idea how hard it would
be to get at the struct device.

Bjorn

[Rafael J. Wysocki]

Well, it may be worth doing. However, that information is readily available from
sysfs anyway, you only need to follow the firmware_node link in the PCI device's
sysfs directory and read the path attribute from there. For example, on my
system:

$ cat /sys/devices/pci0000:00/0000:00:1c.4/0000:0b:00.0/firmware_node/path
\_SB_.PCI0.RP05.PXSX

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[Rafael J. Wysocki]

From the pci_dev side that is trivial: use ACPI_COMPANION(). The other way
around is rather more difficult as browsing a list would be involved.

Rafael

--