* [PATCH] ACPI: property: Fix fwnode string properties matching
@ 2021-02-11 18:30 Rafael J. Wysocki
2021-02-12 9:57 ` Sakari Ailus
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2021-02-11 18:30 UTC (permalink / raw)
To: Andy Shevchenko, Linux ACPI
Cc: Mika Westerberg, LKML, Calvin Johnson, Sakari Ailus
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Property matching does not work for ACPI fwnodes if the value of the
given property is not represented as a package in the _DSD package
containing it. For example, the "compatible" property in the _DSD
below
Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
}
})
will not be found by fwnode_property_match_string(), because the ACPI
code handling device properties does not regard the single value as a
"list" in that case.
Namely, fwnode_property_match_string() invoked to match a given
string property value first calls fwnode_property_read_string_array()
with the last two arguments equal to NULL and 0, respectively, in
order to count the items in the value of the given property, with the
assumption that this value may be an array. For ACPI fwnodes, that
operation is carried out by acpi_node_prop_read() which calls
acpi_data_prop_read() for this purpose. However, when the return
(val) pointer is NULL, that function only looks for a property whose
value is a package without checking the single-value case at all.
To fix that, make acpi_data_prop_read() check the single-value case
regardless of the return pointer value if its return pointer argument
is NULL and modify acpi_data_prop_read_single() handling that case to
attempt to read the value of the property if the return pointer is
NULL and return 1 if that succeeds.
Fixes: 3708184afc77 ("device property: Move FW type specific functionality to FW specific files")
Reported-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
Cc: 4.13+ <stable@vger.kernel.org> # 4.13+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/property.c | 44 +++++++++++++++++++++++++++++++++-----------
1 file changed, 33 insertions(+), 11 deletions(-)
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -787,9 +787,6 @@ static int acpi_data_prop_read_single(co
const union acpi_object *obj;
int ret;
- if (!val)
- return -EINVAL;
-
if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) {
ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj);
if (ret)
@@ -799,28 +796,43 @@ static int acpi_data_prop_read_single(co
case DEV_PROP_U8:
if (obj->integer.value > U8_MAX)
return -EOVERFLOW;
- *(u8 *)val = obj->integer.value;
+
+ if (val)
+ *(u8 *)val = obj->integer.value;
+
break;
case DEV_PROP_U16:
if (obj->integer.value > U16_MAX)
return -EOVERFLOW;
- *(u16 *)val = obj->integer.value;
+
+ if (val)
+ *(u16 *)val = obj->integer.value;
+
break;
case DEV_PROP_U32:
if (obj->integer.value > U32_MAX)
return -EOVERFLOW;
- *(u32 *)val = obj->integer.value;
+
+ if (val)
+ *(u32 *)val = obj->integer.value;
+
break;
default:
- *(u64 *)val = obj->integer.value;
+ if (val)
+ *(u64 *)val = obj->integer.value;
+
break;
}
+
+ if (!val)
+ return 1;
} else if (proptype == DEV_PROP_STRING) {
ret = acpi_data_get_property(data, propname, ACPI_TYPE_STRING, &obj);
if (ret)
return ret;
- *(char **)val = obj->string.pointer;
+ if (val)
+ *(char **)val = obj->string.pointer;
return 1;
} else {
@@ -834,7 +846,7 @@ int acpi_dev_prop_read_single(struct acp
{
int ret;
- if (!adev)
+ if (!adev || !val)
return -EINVAL;
ret = acpi_data_prop_read_single(&adev->data, propname, proptype, val);
@@ -928,10 +940,20 @@ static int acpi_data_prop_read(const str
const union acpi_object *items;
int ret;
- if (val && nval == 1) {
+ if (nval == 1 || !val) {
ret = acpi_data_prop_read_single(data, propname, proptype, val);
- if (ret >= 0)
+ /*
+ * The overflow error means that the property is there and it is
+ * single-value, but its type does not match, so return.
+ */
+ if (ret >= 0 || ret == -EOVERFLOW)
return ret;
+
+ /*
+ * Reading this property as a single-value one failed, but its
+ * value may still be represented as one-element array, so
+ * continue.
+ */
}
ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj);
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ACPI: property: Fix fwnode string properties matching
2021-02-11 18:30 [PATCH] ACPI: property: Fix fwnode string properties matching Rafael J. Wysocki
@ 2021-02-12 9:57 ` Sakari Ailus
2021-02-12 10:28 ` Mika Westerberg
2021-02-12 10:32 ` Andy Shevchenko
2 siblings, 0 replies; 4+ messages in thread
From: Sakari Ailus @ 2021-02-12 9:57 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Andy Shevchenko, Linux ACPI, Mika Westerberg, LKML, Calvin Johnson
Hi Rafael,
On Thu, Feb 11, 2021 at 07:30:01PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Property matching does not work for ACPI fwnodes if the value of the
> given property is not represented as a package in the _DSD package
> containing it. For example, the "compatible" property in the _DSD
> below
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
> }
> })
>
> will not be found by fwnode_property_match_string(), because the ACPI
> code handling device properties does not regard the single value as a
> "list" in that case.
>
> Namely, fwnode_property_match_string() invoked to match a given
> string property value first calls fwnode_property_read_string_array()
> with the last two arguments equal to NULL and 0, respectively, in
> order to count the items in the value of the given property, with the
> assumption that this value may be an array. For ACPI fwnodes, that
> operation is carried out by acpi_node_prop_read() which calls
> acpi_data_prop_read() for this purpose. However, when the return
> (val) pointer is NULL, that function only looks for a property whose
> value is a package without checking the single-value case at all.
>
> To fix that, make acpi_data_prop_read() check the single-value case
> regardless of the return pointer value if its return pointer argument
> is NULL and modify acpi_data_prop_read_single() handling that case to
> attempt to read the value of the property if the return pointer is
> NULL and return 1 if that succeeds.
>
> Fixes: 3708184afc77 ("device property: Move FW type specific functionality to FW specific files")
> Reported-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> Cc: 4.13+ <stable@vger.kernel.org> # 4.13+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Thanks for addressing this.
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
--
Sakari Ailus
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ACPI: property: Fix fwnode string properties matching
2021-02-11 18:30 [PATCH] ACPI: property: Fix fwnode string properties matching Rafael J. Wysocki
2021-02-12 9:57 ` Sakari Ailus
@ 2021-02-12 10:28 ` Mika Westerberg
2021-02-12 10:32 ` Andy Shevchenko
2 siblings, 0 replies; 4+ messages in thread
From: Mika Westerberg @ 2021-02-12 10:28 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Andy Shevchenko, Linux ACPI, LKML, Calvin Johnson, Sakari Ailus
On Thu, Feb 11, 2021 at 07:30:01PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Property matching does not work for ACPI fwnodes if the value of the
> given property is not represented as a package in the _DSD package
> containing it. For example, the "compatible" property in the _DSD
> below
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
> }
> })
>
> will not be found by fwnode_property_match_string(), because the ACPI
> code handling device properties does not regard the single value as a
> "list" in that case.
>
> Namely, fwnode_property_match_string() invoked to match a given
> string property value first calls fwnode_property_read_string_array()
> with the last two arguments equal to NULL and 0, respectively, in
> order to count the items in the value of the given property, with the
> assumption that this value may be an array. For ACPI fwnodes, that
> operation is carried out by acpi_node_prop_read() which calls
> acpi_data_prop_read() for this purpose. However, when the return
> (val) pointer is NULL, that function only looks for a property whose
> value is a package without checking the single-value case at all.
>
> To fix that, make acpi_data_prop_read() check the single-value case
> regardless of the return pointer value if its return pointer argument
> is NULL and modify acpi_data_prop_read_single() handling that case to
> attempt to read the value of the property if the return pointer is
> NULL and return 1 if that succeeds.
>
> Fixes: 3708184afc77 ("device property: Move FW type specific functionality to FW specific files")
> Reported-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> Cc: 4.13+ <stable@vger.kernel.org> # 4.13+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ACPI: property: Fix fwnode string properties matching
2021-02-11 18:30 [PATCH] ACPI: property: Fix fwnode string properties matching Rafael J. Wysocki
2021-02-12 9:57 ` Sakari Ailus
2021-02-12 10:28 ` Mika Westerberg
@ 2021-02-12 10:32 ` Andy Shevchenko
2 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2021-02-12 10:32 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux ACPI, Mika Westerberg, LKML, Calvin Johnson, Sakari Ailus
On Thu, Feb 11, 2021 at 07:30:01PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Property matching does not work for ACPI fwnodes if the value of the
> given property is not represented as a package in the _DSD package
> containing it. For example, the "compatible" property in the _DSD
> below
>
> Name (_DSD, Package () {
> ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> Package () {
> Package () {"compatible", "ethernet-phy-ieee802.3-c45"}
> }
> })
>
> will not be found by fwnode_property_match_string(), because the ACPI
> code handling device properties does not regard the single value as a
> "list" in that case.
>
> Namely, fwnode_property_match_string() invoked to match a given
> string property value first calls fwnode_property_read_string_array()
> with the last two arguments equal to NULL and 0, respectively, in
> order to count the items in the value of the given property, with the
> assumption that this value may be an array. For ACPI fwnodes, that
> operation is carried out by acpi_node_prop_read() which calls
> acpi_data_prop_read() for this purpose. However, when the return
> (val) pointer is NULL, that function only looks for a property whose
> value is a package without checking the single-value case at all.
>
> To fix that, make acpi_data_prop_read() check the single-value case
> regardless of the return pointer value if its return pointer argument
> is NULL and modify acpi_data_prop_read_single() handling that case to
> attempt to read the value of the property if the return pointer is
> NULL and return 1 if that succeeds.
Thanks, fine with me.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
I'll rebase the rest I have on to of this.
> Fixes: 3708184afc77 ("device property: Move FW type specific functionality to FW specific files")
> Reported-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> Cc: 4.13+ <stable@vger.kernel.org> # 4.13+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/acpi/property.c | 44 +++++++++++++++++++++++++++++++++-----------
> 1 file changed, 33 insertions(+), 11 deletions(-)
>
> Index: linux-pm/drivers/acpi/property.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/property.c
> +++ linux-pm/drivers/acpi/property.c
> @@ -787,9 +787,6 @@ static int acpi_data_prop_read_single(co
> const union acpi_object *obj;
> int ret;
>
> - if (!val)
> - return -EINVAL;
> -
> if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) {
> ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj);
> if (ret)
> @@ -799,28 +796,43 @@ static int acpi_data_prop_read_single(co
> case DEV_PROP_U8:
> if (obj->integer.value > U8_MAX)
> return -EOVERFLOW;
> - *(u8 *)val = obj->integer.value;
> +
> + if (val)
> + *(u8 *)val = obj->integer.value;
> +
> break;
> case DEV_PROP_U16:
> if (obj->integer.value > U16_MAX)
> return -EOVERFLOW;
> - *(u16 *)val = obj->integer.value;
> +
> + if (val)
> + *(u16 *)val = obj->integer.value;
> +
> break;
> case DEV_PROP_U32:
> if (obj->integer.value > U32_MAX)
> return -EOVERFLOW;
> - *(u32 *)val = obj->integer.value;
> +
> + if (val)
> + *(u32 *)val = obj->integer.value;
> +
> break;
> default:
> - *(u64 *)val = obj->integer.value;
> + if (val)
> + *(u64 *)val = obj->integer.value;
> +
> break;
> }
> +
> + if (!val)
> + return 1;
> } else if (proptype == DEV_PROP_STRING) {
> ret = acpi_data_get_property(data, propname, ACPI_TYPE_STRING, &obj);
> if (ret)
> return ret;
>
> - *(char **)val = obj->string.pointer;
> + if (val)
> + *(char **)val = obj->string.pointer;
>
> return 1;
> } else {
> @@ -834,7 +846,7 @@ int acpi_dev_prop_read_single(struct acp
> {
> int ret;
>
> - if (!adev)
> + if (!adev || !val)
> return -EINVAL;
>
> ret = acpi_data_prop_read_single(&adev->data, propname, proptype, val);
> @@ -928,10 +940,20 @@ static int acpi_data_prop_read(const str
> const union acpi_object *items;
> int ret;
>
> - if (val && nval == 1) {
> + if (nval == 1 || !val) {
> ret = acpi_data_prop_read_single(data, propname, proptype, val);
> - if (ret >= 0)
> + /*
> + * The overflow error means that the property is there and it is
> + * single-value, but its type does not match, so return.
> + */
> + if (ret >= 0 || ret == -EOVERFLOW)
> return ret;
> +
> + /*
> + * Reading this property as a single-value one failed, but its
> + * value may still be represented as one-element array, so
> + * continue.
> + */
> }
>
> ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj);
>
>
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-12 10:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-11 18:30 [PATCH] ACPI: property: Fix fwnode string properties matching Rafael J. Wysocki
2021-02-12 9:57 ` Sakari Ailus
2021-02-12 10:28 ` Mika Westerberg
2021-02-12 10:32 ` Andy Shevchenko
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).