| Message ID | 20210210114320.3478-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/7] ACPI: property: Remove dead code | expand |
On Wed, Feb 10, 2021 at 12:51 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 <calvin.johnson@oss.nxp.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 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 > > > <andriy.shevchenko@linux.intel.com> 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 <calvin.johnson@oss.nxp.com> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > 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.
On Wed, Feb 10, 2021 at 4:42 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 > > <andriy.shevchenko@linux.intel.com> 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 > > > > > > <andriy.shevchenko@linux.intel.com> 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 > <andriy.shevchenko@linux.intel.com> 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 > > > <andriy.shevchenko@linux.intel.com> 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 > > > > > > > <andriy.shevchenko@linux.intel.com> 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 <andriy.shevchenko@linux.intel.com> 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 > > <andriy.shevchenko@linux.intel.com> 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 > > > > <andriy.shevchenko@linux.intel.com> 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 > > > > > > > > <andriy.shevchenko@linux.intel.com> 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?
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)
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 <andriy.shevchenko@linux.intel.com> --- drivers/acpi/property.c | 20 -------------------- include/linux/acpi.h | 21 --------------------- 2 files changed, 41 deletions(-)