After the commit 3a7a2ab839ad couple of functions became a dead code.
Moreover, for all these years nobody used them. Remove.
Fixes: 3a7a2ab839ad ("ACPI / property: Extend fwnode_property_* to data-only subnodes")
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/property.c | 20 --------------------
include/linux/acpi.h | 21 ---------------------
2 files changed, 41 deletions(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 24e87b630573..d9f3132466b5 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -829,20 +829,6 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
return ret;
}
-int acpi_dev_prop_read_single(struct acpi_device *adev, const char *propname,
- enum dev_prop_type proptype, void *val)
-{
- int ret;
-
- if (!adev)
- return -EINVAL;
-
- ret = acpi_data_prop_read_single(&adev->data, propname, proptype, val);
- if (ret < 0 || proptype != ACPI_TYPE_STRING)
- return ret;
- return 0;
-}
-
static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val,
size_t nval)
{
@@ -973,12 +959,6 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
return ret;
}
-int acpi_dev_prop_read(const struct acpi_device *adev, const char *propname,
- enum dev_prop_type proptype, void *val, size_t nval)
-{
- return adev ? acpi_data_prop_read(&adev->data, propname, proptype, val, nval) : -EINVAL;
-}
-
/**
* acpi_node_prop_read - retrieve the value of an ACPI property with given name.
* @fwnode: Firmware node to get the property from.
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index ea296289a94c..14ac25165ae1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1121,14 +1121,9 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
void **valptr);
-int acpi_dev_prop_read_single(struct acpi_device *adev,
- const char *propname, enum dev_prop_type proptype,
- void *val);
int acpi_node_prop_read(const struct fwnode_handle *fwnode,
const char *propname, enum dev_prop_type proptype,
void *val, size_t nval);
-int acpi_dev_prop_read(const struct acpi_device *adev, const char *propname,
- enum dev_prop_type proptype, void *val, size_t nval);
struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
struct fwnode_handle *child);
@@ -1230,14 +1225,6 @@ static inline int acpi_node_prop_get(const struct fwnode_handle *fwnode,
return -ENXIO;
}
-static inline int acpi_dev_prop_read_single(const struct acpi_device *adev,
- const char *propname,
- enum dev_prop_type proptype,
- void *val)
-{
- return -ENXIO;
-}
-
static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode,
const char *propname,
enum dev_prop_type proptype,
@@ -1246,14 +1233,6 @@ static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode,
return -ENXIO;
}
-static inline int acpi_dev_prop_read(const struct acpi_device *adev,
- const char *propname,
- enum dev_prop_type proptype,
- void *val, size_t nval)
-{
- return -ENXIO;
-}
-
static inline struct fwnode_handle *
acpi_get_next_subnode(const struct fwnode_handle *fwnode,
struct fwnode_handle *child)
--
2.30.0
There is no users outside of property.c. No need to export
acpi_node_prop_read(), hence make it static.
Fixes: 3708184afc77 ("device property: Move FW type specific functionality to FW specific files")
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/property.c | 6 +++---
include/linux/acpi.h | 11 -----------
2 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index d9f3132466b5..9304c88ce446 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -971,9 +971,9 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
* of the property. Otherwise, read at most @nval values to the array at the
* location pointed to by @val.
*/
-int acpi_node_prop_read(const struct fwnode_handle *fwnode,
- const char *propname, enum dev_prop_type proptype,
- void *val, size_t nval)
+static int acpi_node_prop_read(const struct fwnode_handle *fwnode,
+ const char *propname, enum dev_prop_type proptype,
+ void *val, size_t nval)
{
return acpi_data_prop_read(acpi_device_data_of_node(fwnode),
propname, proptype, val, nval);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 14ac25165ae1..3c5757d539ab 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1121,9 +1121,6 @@ acpi_data_add_props(struct acpi_device_data *data, const guid_t *guid,
int acpi_node_prop_get(const struct fwnode_handle *fwnode, const char *propname,
void **valptr);
-int acpi_node_prop_read(const struct fwnode_handle *fwnode,
- const char *propname, enum dev_prop_type proptype,
- void *val, size_t nval);
struct fwnode_handle *acpi_get_next_subnode(const struct fwnode_handle *fwnode,
struct fwnode_handle *child);
@@ -1225,14 +1222,6 @@ static inline int acpi_node_prop_get(const struct fwnode_handle *fwnode,
return -ENXIO;
}
-static inline int acpi_node_prop_read(const struct fwnode_handle *fwnode,
- const char *propname,
- enum dev_prop_type proptype,
- void *val, size_t nval)
-{
- return -ENXIO;
-}
-
static inline struct fwnode_handle *
acpi_get_next_subnode(const struct fwnode_handle *fwnode,
struct fwnode_handle *child)
--
2.30.0
Refactor acpi_data_prop_read_single() for less LOCs and better maintenance.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/property.c | 59 +++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 167338fe9b04..f2386ef32ff1 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -785,47 +785,44 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
enum dev_prop_type proptype, void *val)
{
const union acpi_object *obj;
- int ret;
+ int ret = 0;
if (!val)
return -EINVAL;
- if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) {
+ if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64)
ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj);
- if (ret)
- return ret;
-
- switch (proptype) {
- case DEV_PROP_U8:
- if (obj->integer.value > U8_MAX)
- return -EOVERFLOW;
- *(u8 *)val = obj->integer.value;
- break;
- case DEV_PROP_U16:
- if (obj->integer.value > U16_MAX)
- return -EOVERFLOW;
- *(u16 *)val = obj->integer.value;
- break;
- case DEV_PROP_U32:
- if (obj->integer.value > U32_MAX)
- return -EOVERFLOW;
- *(u32 *)val = obj->integer.value;
- break;
- default:
- *(u64 *)val = obj->integer.value;
- break;
- }
- } else if (proptype == DEV_PROP_STRING) {
+ else if (proptype == DEV_PROP_STRING)
ret = acpi_data_get_property(data, propname, ACPI_TYPE_STRING, &obj);
- if (ret)
- return ret;
+ if (ret)
+ return ret;
+ switch (proptype) {
+ case DEV_PROP_U8:
+ if (obj->integer.value > U8_MAX)
+ return -EOVERFLOW;
+ *(u8 *)val = obj->integer.value;
+ break;
+ case DEV_PROP_U16:
+ if (obj->integer.value > U16_MAX)
+ return -EOVERFLOW;
+ *(u16 *)val = obj->integer.value;
+ break;
+ case DEV_PROP_U32:
+ if (obj->integer.value > U32_MAX)
+ return -EOVERFLOW;
+ *(u32 *)val = obj->integer.value;
+ break;
+ case DEV_PROP_U64:
+ *(u64 *)val = obj->integer.value;
+ break;
+ case DEV_PROP_STRING:
*(char **)val = obj->string.pointer;
-
return 1;
- } else {
- ret = -EINVAL;
+ default:
+ return -EINVAL;
}
+
return ret;
}
--
2.30.0
acpi_data_prop_read_single() doesn't accept a NULL parameter for
the value. Let's modify it to accept NULL pointer in order to validate
the single value. This will be needed for the further changes.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/property.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index f2386ef32ff1..236316ee0e25 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -787,9 +787,6 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
const union acpi_object *obj;
int ret = 0;
- if (!val)
- return -EINVAL;
-
if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64)
ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj);
else if (proptype == DEV_PROP_STRING)
@@ -801,23 +798,28 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data,
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;
case DEV_PROP_U64:
- *(u64 *)val = obj->integer.value;
+ if (val)
+ *(u64 *)val = obj->integer.value;
break;
case DEV_PROP_STRING:
- *(char **)val = obj->string.pointer;
+ if (val)
+ *(char **)val = obj->string.pointer;
return 1;
default:
return -EINVAL;
--
2.30.0
We allow to read the single value as a first element in the array.
Unfortunately the counting doesn't work in this case and we can't
call fwnode_property_count_*() API without getting an error.
Modify acpi_data_prop_read() to always try the single value read
and thus allow counting the single value as an array of 1 element.
Reported-by: Calvin Johnson <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/property.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 236316ee0e25..d6100585fceb 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -913,12 +913,14 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
const union acpi_object *items;
int ret;
- if (val && nval == 1) {
+ /* Try to read as a single value first */
+ if (!val || nval == 1) {
ret = acpi_data_prop_read_single(data, propname, proptype, val);
if (ret >= 0)
- return ret;
+ return val ? ret : 1;
}
+ /* It's not the single value, get an array instead */
ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj);
if (ret)
return ret;
--
2.30.0
CHECK drivers/acpi/property.c
warning: Function parameter or member 'data' not described in 'acpi_data_get_property_array'
warning: Excess function parameter 'adev' description in 'acpi_data_get_property_array'
Fixes: 3a7a2ab839ad ("ACPI / property: Extend fwnode_property_* to data-only subnodes")
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/acpi/property.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 9304c88ce446..a6b096575fc8 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -564,7 +564,7 @@ int acpi_node_prop_get(const struct fwnode_handle *fwnode,
/**
* acpi_data_get_property_array - return an ACPI array property with given name
- * @adev: ACPI data object to get the property from
+ * @data: ACPI data object to get the property from
* @name: Name of the property
* @type: Expected type of array elements
* @obj: Location to store a pointer to the property value (if not NULL)
--
2.30.0
On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
<[email protected]> wrote:
>
> We allow to read the single value as a first element in the array.
> Unfortunately the counting doesn't work in this case and we can't
> call fwnode_property_count_*() API without getting an error.
It would be good to mention what the symptom of the issue is here.
> Modify acpi_data_prop_read() to always try the single value read
> and thus allow counting the single value as an array of 1 element.
>
> Reported-by: Calvin Johnson <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
This is a bug fix, so it should go in before the cleanups in this series IMO.
Also it looks like stable@vger material.
> ---
> drivers/acpi/property.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 236316ee0e25..d6100585fceb 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -913,12 +913,14 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
> const union acpi_object *items;
> int ret;
>
> - if (val && nval == 1) {
> + /* Try to read as a single value first */
> + if (!val || nval == 1) {
> ret = acpi_data_prop_read_single(data, propname, proptype, val);
This returns -EINVAL if val is NULL.
> if (ret >= 0)
> - return ret;
> + return val ? ret : 1;
So val cannot be NULL here.
> }
>
> + /* It's not the single value, get an array instead */
> ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj);
> if (ret)
> return ret;
> --
To me, acpi_fwnode_property_read_string_array() needs to special-case
val == NULL and nval == 0.
On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > We allow to read the single value as a first element in the array.
> > Unfortunately the counting doesn't work in this case and we can't
> > call fwnode_property_count_*() API without getting an error.
>
> It would be good to mention what the symptom of the issue is here.
>
> > Modify acpi_data_prop_read() to always try the single value read
> > and thus allow counting the single value as an array of 1 element.
> >
> > Reported-by: Calvin Johnson <[email protected]>
> > Signed-off-by: Andy Shevchenko <[email protected]>
>
> This is a bug fix, so it should go in before the cleanups in this series IMO.
>
> Also it looks like stable@vger material.
>
> > ---
> > drivers/acpi/property.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > index 236316ee0e25..d6100585fceb 100644
> > --- a/drivers/acpi/property.c
> > +++ b/drivers/acpi/property.c
> > @@ -913,12 +913,14 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
> > const union acpi_object *items;
> > int ret;
> >
> > - if (val && nval == 1) {
> > + /* Try to read as a single value first */
> > + if (!val || nval == 1) {
> > ret = acpi_data_prop_read_single(data, propname, proptype, val);
>
> This returns -EINVAL if val is NULL.
>
> > if (ret >= 0)
> > - return ret;
> > + return val ? ret : 1;
>
> So val cannot be NULL here.
>
> > }
> >
> > + /* It's not the single value, get an array instead */
> > ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj);
> > if (ret)
> > return ret;
> > --
>
> To me, acpi_fwnode_property_read_string_array() needs to special-case
> val == NULL and nval == 0.
Well, scratch this.
Something like the patch below (untested) should be sufficient to address this
if I'm not mistaken.
---
drivers/acpi/property.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -787,14 +787,14 @@ 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)
return ret;
+ if (!val)
+ return 1;
+
switch (proptype) {
case DEV_PROP_U8:
if (obj->integer.value > U8_MAX)
@@ -820,7 +820,8 @@ static int acpi_data_prop_read_single(co
if (ret)
return ret;
- *(char **)val = obj->string.pointer;
+ if (val)
+ *(char **)val = obj->string.pointer;
return 1;
} else {
@@ -928,10 +929,16 @@ static int acpi_data_prop_read(const str
const union acpi_object *items;
int ret;
- if (val && nval == 1) {
+ if (nval == 1) {
ret = acpi_data_prop_read_single(data, propname, proptype, val);
if (ret >= 0)
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);
On Wednesday, February 10, 2021 2:31:48 PM CET Rafael J. Wysocki wrote:
> On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> > On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> > <[email protected]> wrote:
> > >
> > > We allow to read the single value as a first element in the array.
> > > Unfortunately the counting doesn't work in this case and we can't
> > > call fwnode_property_count_*() API without getting an error.
> >
> > It would be good to mention what the symptom of the issue is here.
> >
> > > Modify acpi_data_prop_read() to always try the single value read
> > > and thus allow counting the single value as an array of 1 element.
> > >
> > > Reported-by: Calvin Johnson <[email protected]>
> > > Signed-off-by: Andy Shevchenko <[email protected]>
> >
> > This is a bug fix, so it should go in before the cleanups in this series IMO.
> >
> > Also it looks like stable@vger material.
> >
> > > ---
> > > drivers/acpi/property.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index 236316ee0e25..d6100585fceb 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -913,12 +913,14 @@ static int acpi_data_prop_read(const struct acpi_device_data *data,
> > > const union acpi_object *items;
> > > int ret;
> > >
> > > - if (val && nval == 1) {
> > > + /* Try to read as a single value first */
> > > + if (!val || nval == 1) {
> > > ret = acpi_data_prop_read_single(data, propname, proptype, val);
> >
> > This returns -EINVAL if val is NULL.
> >
> > > if (ret >= 0)
> > > - return ret;
> > > + return val ? ret : 1;
> >
> > So val cannot be NULL here.
> >
> > > }
> > >
> > > + /* It's not the single value, get an array instead */
> > > ret = acpi_data_get_property_array(data, propname, ACPI_TYPE_ANY, &obj);
> > > if (ret)
> > > return ret;
> > > --
> >
> > To me, acpi_fwnode_property_read_string_array() needs to special-case
> > val == NULL and nval == 0.
>
> Well, scratch this.
>
> Something like the patch below (untested) should be sufficient to address this
> if I'm not mistaken.
Well, I am mistaken ->
>
> ---
> drivers/acpi/property.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> Index: linux-pm/drivers/acpi/property.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/property.c
> +++ linux-pm/drivers/acpi/property.c
> @@ -787,14 +787,14 @@ 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)
> return ret;
>
> + if (!val)
> + return 1;
> +
> switch (proptype) {
> case DEV_PROP_U8:
> if (obj->integer.value > U8_MAX)
> @@ -820,7 +820,8 @@ static int acpi_data_prop_read_single(co
> if (ret)
> return ret;
>
> - *(char **)val = obj->string.pointer;
> + if (val)
> + *(char **)val = obj->string.pointer;
>
> return 1;
> } else {
> @@ -928,10 +929,16 @@ static int acpi_data_prop_read(const str
> const union acpi_object *items;
> int ret;
>
> - if (val && nval == 1) {
> + if (nval == 1) {
-> because this would miss the nval == 0 case.
So appended again with this fixed.
---
drivers/acpi/property.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/acpi/property.c
===================================================================
--- linux-pm.orig/drivers/acpi/property.c
+++ linux-pm/drivers/acpi/property.c
@@ -787,14 +787,14 @@ 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)
return ret;
+ if (!val)
+ return 1;
+
switch (proptype) {
case DEV_PROP_U8:
if (obj->integer.value > U8_MAX)
@@ -820,7 +820,8 @@ static int acpi_data_prop_read_single(co
if (ret)
return ret;
- *(char **)val = obj->string.pointer;
+ if (val)
+ *(char **)val = obj->string.pointer;
return 1;
} else {
@@ -928,10 +929,16 @@ static int acpi_data_prop_read(const str
const union acpi_object *items;
int ret;
- if (val && nval == 1) {
+ if (nval <= 1) {
ret = acpi_data_prop_read_single(data, propname, proptype, val);
if (ret >= 0)
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);
On Wed, Feb 10, 2021 at 02:48:09PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 10, 2021 2:31:48 PM CET Rafael J. Wysocki wrote:
> > On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> > > On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> > > <[email protected]> wrote:
Rafael, thanks for the review, my answers below.
> > > > We allow to read the single value as a first element in the array.
> > > > Unfortunately the counting doesn't work in this case and we can't
> > > > call fwnode_property_count_*() API without getting an error.
> > >
> > > It would be good to mention what the symptom of the issue is here.
fwnode_property_match_string() is not working as reported by Calvin.
> > > > Modify acpi_data_prop_read() to always try the single value read
> > > > and thus allow counting the single value as an array of 1 element.
> > > >
> > > > Reported-by: Calvin Johnson <[email protected]>
> > > > Signed-off-by: Andy Shevchenko <[email protected]>
> > >
> > > This is a bug fix, so it should go in before the cleanups in this series IMO.
Seems it was never worked, hence neither Fixes tag nor...
> > > Also it looks like stable@vger material.
...Cc to stable@.
> > > > - if (val && nval == 1) {
> > > > + /* Try to read as a single value first */
> > > > + if (!val || nval == 1) {
> > > > ret = acpi_data_prop_read_single(data, propname, proptype, val);
> > >
> > > This returns -EINVAL if val is NULL.
Nope. That's why it's a patch 7. Patch 6 solves this.
> > > > if (ret >= 0)
> > > > - return ret;
> > > > + return val ? ret : 1;
> > >
> > > So val cannot be NULL here.
Why not? I have changed conditional.
> > > > }
> > > To me, acpi_fwnode_property_read_string_array() needs to special-case
> > > val == NULL and nval == 0.
nval can be anything in the case of val==NULL. So far neither of your proposals
conform this.
--
With Best Regards,
Andy Shevchenko
On Wed, Feb 10, 2021 at 3:48 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Feb 10, 2021 at 02:48:09PM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, February 10, 2021 2:31:48 PM CET Rafael J. Wysocki wrote:
> > > On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> > > > On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> > > > <[email protected]> wrote:
>
> Rafael, thanks for the review, my answers below.
>
> > > > > We allow to read the single value as a first element in the array.
> > > > > Unfortunately the counting doesn't work in this case and we can't
> > > > > call fwnode_property_count_*() API without getting an error.
> > > >
> > > > It would be good to mention what the symptom of the issue is here.
>
> fwnode_property_match_string() is not working as reported by Calvin.
>
> > > > > Modify acpi_data_prop_read() to always try the single value read
> > > > > and thus allow counting the single value as an array of 1 element.
> > > > >
> > > > > Reported-by: Calvin Johnson <[email protected]>
> > > > > Signed-off-by: Andy Shevchenko <[email protected]>
> > > >
> > > > This is a bug fix, so it should go in before the cleanups in this series IMO.
>
> Seems it was never worked, hence neither Fixes tag nor...
>
> > > > Also it looks like stable@vger material.
>
> ...Cc to stable@.
>
> > > > > - if (val && nval == 1) {
> > > > > + /* Try to read as a single value first */
> > > > > + if (!val || nval == 1) {
> > > > > ret = acpi_data_prop_read_single(data, propname, proptype, val);
> > > >
> > > > This returns -EINVAL if val is NULL.
>
> Nope. That's why it's a patch 7. Patch 6 solves this.
That's my point. Patch 7 should be the first one in the series.
> > > > > if (ret >= 0)
> > > > > - return ret;
> > > > > + return val ? ret : 1;
> > > >
> > > > So val cannot be NULL here.
>
> Why not? I have changed conditional.
>
> > > > > }
>
> > > > To me, acpi_fwnode_property_read_string_array() needs to special-case
> > > > val == NULL and nval == 0.
>
> nval can be anything in the case of val==NULL. So far neither of your proposals
> conform this.
That is if !val and nval != 0 is regarded as a valid combination of
arguments, but is it?
If that is the case, the check in acpi_data_prop_read() in the last
patch that I posted needs to be (!val || nval == 1), but that would be
it, no?
On Wed, Feb 10, 2021 at 04:01:16PM +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 10, 2021 at 3:48 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, Feb 10, 2021 at 02:48:09PM +0100, Rafael J. Wysocki wrote:
> > > On Wednesday, February 10, 2021 2:31:48 PM CET Rafael J. Wysocki wrote:
> > > > On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> > > > > On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> > > > > <[email protected]> wrote:
...
> > > > > > - if (val && nval == 1) {
> > > > > > + /* Try to read as a single value first */
> > > > > > + if (!val || nval == 1) {
> > > > > > ret = acpi_data_prop_read_single(data, propname, proptype, val);
> > > > >
> > > > > This returns -EINVAL if val is NULL.
> >
> > Nope. That's why it's a patch 7. Patch 6 solves this.
>
> That's my point. Patch 7 should be the first one in the series.
Ah, okay. Since you want this let me rebase.
> > > > > > if (ret >= 0)
> > > > > > - return ret;
> > > > > > + return val ? ret : 1;
> > > > >
> > > > > So val cannot be NULL here.
> >
> > Why not? I have changed conditional.
> >
> > > > > > }
> >
> > > > > To me, acpi_fwnode_property_read_string_array() needs to special-case
> > > > > val == NULL and nval == 0.
> >
> > nval can be anything in the case of val==NULL. So far neither of your proposals
> > conform this.
>
> That is if !val and nval != 0 is regarded as a valid combination of
> arguments, but is it?
I believe nobody tested that.
> If that is the case, the check in acpi_data_prop_read() in the last
> patch that I posted needs to be (!val || nval == 1), but that would be
> it, no?
I think it also needs the conditional at return as in my patch.
--
With Best Regards,
Andy Shevchenko
On Wed, Feb 10, 2021 at 4:42 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Feb 10, 2021 at 04:01:16PM +0100, Rafael J. Wysocki wrote:
> > On Wed, Feb 10, 2021 at 3:48 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Wed, Feb 10, 2021 at 02:48:09PM +0100, Rafael J. Wysocki wrote:
> > > > On Wednesday, February 10, 2021 2:31:48 PM CET Rafael J. Wysocki wrote:
> > > > > On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> > > > > > On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> > > > > > <[email protected]> wrote:
>
> ...
>
> > > > > > > - if (val && nval == 1) {
> > > > > > > + /* Try to read as a single value first */
> > > > > > > + if (!val || nval == 1) {
> > > > > > > ret = acpi_data_prop_read_single(data, propname, proptype, val);
> > > > > >
> > > > > > This returns -EINVAL if val is NULL.
> > >
> > > Nope. That's why it's a patch 7. Patch 6 solves this.
> >
> > That's my point. Patch 7 should be the first one in the series.
>
> Ah, okay. Since you want this let me rebase.
Thanks!
> > > > > > > if (ret >= 0)
> > > > > > > - return ret;
> > > > > > > + return val ? ret : 1;
> > > > > >
> > > > > > So val cannot be NULL here.
> > >
> > > Why not? I have changed conditional.
> > >
> > > > > > > }
> > >
> > > > > > To me, acpi_fwnode_property_read_string_array() needs to special-case
> > > > > > val == NULL and nval == 0.
> > >
> > > nval can be anything in the case of val==NULL. So far neither of your proposals
> > > conform this.
> >
> > That is if !val and nval != 0 is regarded as a valid combination of
> > arguments, but is it?
>
> I believe nobody tested that.
>
> > If that is the case, the check in acpi_data_prop_read() in the last
> > patch that I posted needs to be (!val || nval == 1), but that would be
> > it, no?
>
> I think it also needs the conditional at return as in my patch.
I'm not sure why.
acpi_data_prop_read_single() would return 1 for !val if it finds the
property with a single value and that should be sufficient, shouldn't
it?
On Wed, Feb 10, 2021 at 04:44:34PM +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 10, 2021 at 4:42 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Wed, Feb 10, 2021 at 04:01:16PM +0100, Rafael J. Wysocki wrote:
> > > On Wed, Feb 10, 2021 at 3:48 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Wed, Feb 10, 2021 at 02:48:09PM +0100, Rafael J. Wysocki wrote:
> > > > > On Wednesday, February 10, 2021 2:31:48 PM CET Rafael J. Wysocki wrote:
> > > > > > On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> > > > > > > On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> > > > > > > <[email protected]> wrote:
...
> > > > > > > > - if (val && nval == 1) {
> > > > > > > > + /* Try to read as a single value first */
> > > > > > > > + if (!val || nval == 1) {
> > > > > > > > ret = acpi_data_prop_read_single(data, propname, proptype, val);
> > > > > > >
> > > > > > > This returns -EINVAL if val is NULL.
> > > >
> > > > Nope. That's why it's a patch 7. Patch 6 solves this.
> > >
> > > That's my point. Patch 7 should be the first one in the series.
> >
> > Ah, okay. Since you want this let me rebase.
>
> Thanks!
I started rebasing and realised that your approach has swapped the error codes,
i.e. if it's a single-value and it is, e.g., 16-bit wide, the u8 read will
return 1, while it has to return -EOVERFLOW.
If you prefer, I can move two patches to the beginning, so one will be a good
prerequisite for this fix. And I'm still unsure about stable (Fixes tag is okay
to me), because the counting never worked from the day 1.
--
With Best Regards,
Andy Shevchenko
On Thu, Feb 11, 2021 at 4:42 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Feb 10, 2021 at 04:44:34PM +0100, Rafael J. Wysocki wrote:
> > On Wed, Feb 10, 2021 at 4:42 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Wed, Feb 10, 2021 at 04:01:16PM +0100, Rafael J. Wysocki wrote:
> > > > On Wed, Feb 10, 2021 at 3:48 PM Andy Shevchenko
> > > > <[email protected]> wrote:
> > > > > On Wed, Feb 10, 2021 at 02:48:09PM +0100, Rafael J. Wysocki wrote:
> > > > > > On Wednesday, February 10, 2021 2:31:48 PM CET Rafael J. Wysocki wrote:
> > > > > > > On Wednesday, February 10, 2021 1:36:00 PM CET Rafael J. Wysocki wrote:
> > > > > > > > On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko
> > > > > > > > <[email protected]> wrote:
>
> ...
>
> > > > > > > > > - if (val && nval == 1) {
> > > > > > > > > + /* Try to read as a single value first */
> > > > > > > > > + if (!val || nval == 1) {
> > > > > > > > > ret = acpi_data_prop_read_single(data, propname, proptype, val);
> > > > > > > >
> > > > > > > > This returns -EINVAL if val is NULL.
> > > > >
> > > > > Nope. That's why it's a patch 7. Patch 6 solves this.
> > > >
> > > > That's my point. Patch 7 should be the first one in the series.
> > >
> > > Ah, okay. Since you want this let me rebase.
> >
> > Thanks!
>
> I started rebasing and realised that your approach has swapped the error codes,
> i.e. if it's a single-value and it is, e.g., 16-bit wide, the u8 read will
> return 1, while it has to return -EOVERFLOW.
Well, that's a bug in my patch.
I thought that you would reorder the series to put the fix into the
front of it, but I didn't really mean to rebase it on top of my patch.
Sorry for the confusion.
However, not that you have started to do it apparently, let me post
that patch properly with all of the issues addressed.
> If you prefer, I can move two patches to the beginning, so one will be a good
> prerequisite for this fix. And I'm still unsure about stable (Fixes tag is okay
> to me), because the counting never worked from the day 1.
Well, the code has never worked as intended, so why don't we make
"stable" work as intended too?