| Message ID | 20241218091622.914266-1-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/1] ACPI: property: Consider data nodes as being available | expand |
On Wed, Dec 18, 2024 at 10:16 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Years after fwnode_device_is_available() was introduced, new functions > making use of the function on data nodes have been added, such as > fwnode_for_each_available_child_node(), it becomes apparent that returning > "false" for all child nodes on ACPI wasn't a workable option. Can you describe the problem in a bit more detail? > On DT side most access functions, even those without "_available" part, > did only operate on available nodes. That wasn't the case on ACPI where > only device node availability is known explicitly. > > Thus from now on, return true from fwnode_device_is_available() on all > ACPI data nodes. > > Fixes: 2294b3af05e9 ("device property: Introduce fwnode_device_is_available()") Do you want people to backport this patch? > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > 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 80a52a4e66dd..1ee81e771ae6 100644 > --- a/drivers/acpi/property.c > +++ b/drivers/acpi/property.c > @@ -1492,7 +1492,7 @@ acpi_graph_get_remote_endpoint(const struct fwnode_handle *__fwnode) > static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode) > { > if (!is_acpi_device_node(fwnode)) > - return false; > + return true; > > return acpi_device_is_present(to_acpi_device_node(fwnode)); > } > > base-commit: 7fa366f1b6e376c38966faa42da7f0f2e013fdab > -- > 2.39.5 >
Hi Rafael, Thanks for the review. On Wed, Dec 18, 2024 at 12:07:52PM +0100, Rafael J. Wysocki wrote: > On Wed, Dec 18, 2024 at 10:16 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Years after fwnode_device_is_available() was introduced, new functions > > making use of the function on data nodes have been added, such as > > fwnode_for_each_available_child_node(), it becomes apparent that returning > > "false" for all child nodes on ACPI wasn't a workable option. > > Can you describe the problem in a bit more detail? How about: Years after fwnode_device_is_available() was introduced, new functions making use of the data node availability information have been added, such as fwnode_for_each_available_child_node(). To enumerate the data nodes in various ways specific to those functions, the node availability test needs to pass. On ACPI, there is no explicit information on this in the first place and the original fwnode_device_is_available() implementation simply returnes false. This leads to the new functions enumerating only available nodes to never return any nodes on ACPI. On DT side most access functions, even those without "_available" part, did only operate on available nodes. Thus from now on, return true from fwnode_device_is_available() on all ACPI data nodes. > > > On DT side most access functions, even those without "_available" part, > > did only operate on available nodes. That wasn't the case on ACPI where > > only device node availability is known explicitly. > > > > Thus from now on, return true from fwnode_device_is_available() on all > > ACPI data nodes. > > > > Fixes: 2294b3af05e9 ("device property: Introduce fwnode_device_is_available()") > > Do you want people to backport this patch? Good question. There are just a couple of drivers using the new fwnode_*_available() functions and I think they're used on DT-based platforms *currently*. So nothing is broken right now as far as I can see (but likely will be in some time without the patch). I guess just dropping Fixes: is an alternative, this wasn't really a bug honestly. Backporting shouldn't hurt either though. > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > 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 80a52a4e66dd..1ee81e771ae6 100644 > > --- a/drivers/acpi/property.c > > +++ b/drivers/acpi/property.c > > @@ -1492,7 +1492,7 @@ acpi_graph_get_remote_endpoint(const struct fwnode_handle *__fwnode) > > static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode) > > { > > if (!is_acpi_device_node(fwnode)) > > - return false; > > + return true; > > > > return acpi_device_is_present(to_acpi_device_node(fwnode)); > > } > > > > base-commit: 7fa366f1b6e376c38966faa42da7f0f2e013fdab
Hi Sakari, On Thu, Dec 19, 2024 at 11:32 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > Thanks for the review. > > On Wed, Dec 18, 2024 at 12:07:52PM +0100, Rafael J. Wysocki wrote: > > On Wed, Dec 18, 2024 at 10:16 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Years after fwnode_device_is_available() was introduced, new functions > > > making use of the function on data nodes have been added, such as > > > fwnode_for_each_available_child_node(), it becomes apparent that returning > > > "false" for all child nodes on ACPI wasn't a workable option. > > > > Can you describe the problem in a bit more detail? > > How about: This is better IMV. At least it actually matches my understanding of the issue. > Years after fwnode_device_is_available() was introduced, new functions > making use of the data node availability information have been added, such > as fwnode_for_each_available_child_node(). I would rephrase the sentence above as follows: "New functions making use of the data node availability information, like fwnode_for_each_available_child_node(), have been added years after fwnode_device_is_available() was introduced." > To enumerate the data nodes in > various ways specific to those functions, the node availability test needs > to pass. > > On ACPI, there is no explicit information on this I guess by "this" you mean the availability of the data (non-device) nodes? > in the first place and > the original fwnode_device_is_available() implementation simply returnes returns > false. This leads to the new functions enumerating only available nodes to > never return any nodes on ACPI. I'd say "This causes new functions that only enumerate available nodes to never return any nodes on ACPI for leaf devices that have child data nodes." > On DT side most access functions, even > those without "_available" part, did only operate on available nodes. On the DT side, :device_is_available points to of_device_is_available() that calls __of_device_is_available() which returns "true" for all nodes without the "status" property, so if I'm not mistaken, on the DT side fwnode_device_is_available() will return "true" for any nodes without the "status" property. I would say something like this: "However, on the DT side, fwnode_device_is_available() returns "true" for all nodes without the "status" property which are analogous to the ACPI data nodes, so there is a difference in behavior between DT and ACPI in that respect." > Thus from now on, return true from fwnode_device_is_available() on all ACPI > data nodes. > > > > > > On DT side most access functions, even those without "_available" part, > > > did only operate on available nodes. That wasn't the case on ACPI where > > > only device node availability is known explicitly. > > > > > > Thus from now on, return true from fwnode_device_is_available() on all > > > ACPI data nodes. > > > > > > Fixes: 2294b3af05e9 ("device property: Introduce fwnode_device_is_available()") > > > > Do you want people to backport this patch? > > Good question. > > There are just a couple of drivers using the new fwnode_*_available() > functions and I think they're used on DT-based platforms *currently*. So > nothing is broken right now as far as I can see (but likely will be in some > time without the patch). > > I guess just dropping Fixes: is an alternative, this wasn't really a bug > honestly. Backporting shouldn't hurt either though. Yes, I would drop the Fixes: tag. If need be, the "stable" people can be asked directly to pick it up, but it's better to avoid automatic backporting of it IMV. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > 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 80a52a4e66dd..1ee81e771ae6 100644 > > > --- a/drivers/acpi/property.c > > > +++ b/drivers/acpi/property.c > > > @@ -1492,7 +1492,7 @@ acpi_graph_get_remote_endpoint(const struct fwnode_handle *__fwnode) > > > static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode) > > > { > > > if (!is_acpi_device_node(fwnode)) > > > - return false; > > > + return true; > > > > > > return acpi_device_is_present(to_acpi_device_node(fwnode)); > > > } > > > > > > base-commit: 7fa366f1b6e376c38966faa42da7f0f2e013fdab > > -- > Kind regards, > > Sakari Ailus >
Hi Rafael, On Thu, Dec 19, 2024 at 12:25:22PM +0100, Rafael J. Wysocki wrote: > Hi Sakari, > > On Thu, Dec 19, 2024 at 11:32 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > Thanks for the review. > > > > On Wed, Dec 18, 2024 at 12:07:52PM +0100, Rafael J. Wysocki wrote: > > > On Wed, Dec 18, 2024 at 10:16 AM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > Years after fwnode_device_is_available() was introduced, new functions > > > > making use of the function on data nodes have been added, such as > > > > fwnode_for_each_available_child_node(), it becomes apparent that returning > > > > "false" for all child nodes on ACPI wasn't a workable option. > > > > > > Can you describe the problem in a bit more detail? > > > > How about: > > This is better IMV. At least it actually matches my understanding of the issue. > > > Years after fwnode_device_is_available() was introduced, new functions > > making use of the data node availability information have been added, such > > as fwnode_for_each_available_child_node(). > > I would rephrase the sentence above as follows: > > "New functions making use of the data node availability information, > like fwnode_for_each_available_child_node(), have been added years > after fwnode_device_is_available() was introduced." Looks good to me. > > > To enumerate the data nodes in > > various ways specific to those functions, the node availability test needs > > to pass. > > > > On ACPI, there is no explicit information on this > > I guess by "this" you mean the availability of the data (non-device) nodes? Yes. I'll use: On ACPI, there is no explicit data node availbility information in the first place and the original fwnode_device_is_available() implementation simply returns false. > > > in the first place and > > the original fwnode_device_is_available() implementation simply returnes > > returns > > > false. This leads to the new functions enumerating only available nodes to > > never return any nodes on ACPI. > > I'd say "This causes new functions that only enumerate available nodes > to never return any nodes on ACPI for leaf devices that have child > data nodes." Ack. > > > On DT side most access functions, even > > those without "_available" part, did only operate on available nodes. > > On the DT side, :device_is_available points to > of_device_is_available() that calls __of_device_is_available() which > returns "true" for all nodes without the "status" property, so if I'm > not mistaken, on the DT side fwnode_device_is_available() will return > "true" for any nodes without the "status" property. > > I would say something like this: > > "However, on the DT side, fwnode_device_is_available() returns "true" > for all nodes without the "status" property which are analogous to the > ACPI data nodes, so there is a difference in behavior between DT and > ACPI in that respect." That's also true. I'll use that, dropping the quotes from "true". The commit message would be, in its entirety: New functions making use of the data node availability information, like fwnode_for_each_available_child_node(), have been added years after fwnode_device_is_available() was introduced. To enumerate the data nodes in various ways specific to those functions, the node availability test needs to pass. On ACPI, there is no explicit data node availbility information in the first place and the original fwnode_device_is_available() implementation simply returns false. This causes new functions that only enumerate available nodes to never return any nodes on ACPI for leaf devices that have child data nodes. However, on the DT side, fwnode_device_is_available() returns true for all nodes without the "status" property which are analogous to the ACPI data nodes, so there is a difference in behavior between DT and ACPI in that respect. Thus from now on, return true from fwnode_device_is_available() on all ACPI data nodes. > > > Thus from now on, return true from fwnode_device_is_available() on all ACPI > > data nodes. > > > > > > > > > On DT side most access functions, even those without "_available" part, > > > > did only operate on available nodes. That wasn't the case on ACPI where > > > > only device node availability is known explicitly. > > > > > > > > Thus from now on, return true from fwnode_device_is_available() on all > > > > ACPI data nodes. > > > > > > > > Fixes: 2294b3af05e9 ("device property: Introduce fwnode_device_is_available()") > > > > > > Do you want people to backport this patch? > > > > Good question. > > > > There are just a couple of drivers using the new fwnode_*_available() > > functions and I think they're used on DT-based platforms *currently*. So > > nothing is broken right now as far as I can see (but likely will be in some > > time without the patch). > > > > I guess just dropping Fixes: is an alternative, this wasn't really a bug > > honestly. Backporting shouldn't hurt either though. > > Yes, I would drop the Fixes: tag. If need be, the "stable" people can > be asked directly to pick it up, but it's better to avoid automatic > backporting of it IMV. I agree. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > 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 80a52a4e66dd..1ee81e771ae6 100644 > > > > --- a/drivers/acpi/property.c > > > > +++ b/drivers/acpi/property.c > > > > @@ -1492,7 +1492,7 @@ acpi_graph_get_remote_endpoint(const struct fwnode_handle *__fwnode) > > > > static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode) > > > > { > > > > if (!is_acpi_device_node(fwnode)) > > > > - return false; > > > > + return true; > > > > > > > > return acpi_device_is_present(to_acpi_device_node(fwnode)); > > > > } > > > > > > > > base-commit: 7fa366f1b6e376c38966faa42da7f0f2e013fdab > >
Hi Sakari, On Thu, Dec 19, 2024 at 2:37 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Thu, Dec 19, 2024 at 12:25:22PM +0100, Rafael J. Wysocki wrote: > > Hi Sakari, > > > > On Thu, Dec 19, 2024 at 11:32 AM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > Hi Rafael, > > > > > > Thanks for the review. > > > > > > On Wed, Dec 18, 2024 at 12:07:52PM +0100, Rafael J. Wysocki wrote: > > > > On Wed, Dec 18, 2024 at 10:16 AM Sakari Ailus > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > > > Years after fwnode_device_is_available() was introduced, new functions > > > > > making use of the function on data nodes have been added, such as > > > > > fwnode_for_each_available_child_node(), it becomes apparent that returning > > > > > "false" for all child nodes on ACPI wasn't a workable option. > > > > > > > > Can you describe the problem in a bit more detail? > > > > > > How about: > > > > This is better IMV. At least it actually matches my understanding of the issue. > > > > > Years after fwnode_device_is_available() was introduced, new functions > > > making use of the data node availability information have been added, such > > > as fwnode_for_each_available_child_node(). > > > > I would rephrase the sentence above as follows: > > > > "New functions making use of the data node availability information, > > like fwnode_for_each_available_child_node(), have been added years > > after fwnode_device_is_available() was introduced." > > Looks good to me. > > > > > > To enumerate the data nodes in > > > various ways specific to those functions, the node availability test needs > > > to pass. > > > > > > On ACPI, there is no explicit information on this > > > > I guess by "this" you mean the availability of the data (non-device) nodes? > > Yes. I'll use: > > On ACPI, there is no explicit data node availbility information in the > first place and the original fwnode_device_is_available() implementation > simply returns false. > > > > > > in the first place and > > > the original fwnode_device_is_available() implementation simply returnes > > > > returns > > > > > false. This leads to the new functions enumerating only available nodes to > > > never return any nodes on ACPI. > > > > I'd say "This causes new functions that only enumerate available nodes > > to never return any nodes on ACPI for leaf devices that have child > > data nodes." > > Ack. > > > > > > On DT side most access functions, even > > > those without "_available" part, did only operate on available nodes. > > > > On the DT side, :device_is_available points to > > of_device_is_available() that calls __of_device_is_available() which > > returns "true" for all nodes without the "status" property, so if I'm > > not mistaken, on the DT side fwnode_device_is_available() will return > > "true" for any nodes without the "status" property. > > > > I would say something like this: > > > > "However, on the DT side, fwnode_device_is_available() returns "true" > > for all nodes without the "status" property which are analogous to the > > ACPI data nodes, so there is a difference in behavior between DT and > > ACPI in that respect." > > That's also true. I'll use that, dropping the quotes from "true". > > The commit message would be, in its entirety: > > New functions making use of the data node availability information, like > fwnode_for_each_available_child_node(), have been added years after > fwnode_device_is_available() was introduced. To enumerate the data nodes > in various ways specific to those functions, the node availability test > needs to pass. > > On ACPI, there is no explicit data node availbility information in the > first place and the original fwnode_device_is_available() implementation > simply returns false. This causes new functions that only enumerate > available nodes to never return any nodes on ACPI for leaf devices that > have child data nodes. > > However, on the DT side, fwnode_device_is_available() returns true > for all nodes without the "status" property which are analogous to the > ACPI data nodes, so there is a difference in behavior between DT and > ACPI in that respect. > > Thus from now on, return true from fwnode_device_is_available() on all ACPI > data nodes. Looks good to me now, thanks!
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c index 80a52a4e66dd..1ee81e771ae6 100644 --- a/drivers/acpi/property.c +++ b/drivers/acpi/property.c @@ -1492,7 +1492,7 @@ acpi_graph_get_remote_endpoint(const struct fwnode_handle *__fwnode) static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode) { if (!is_acpi_device_node(fwnode)) - return false; + return true; return acpi_device_is_present(to_acpi_device_node(fwnode)); }
Years after fwnode_device_is_available() was introduced, new functions making use of the function on data nodes have been added, such as fwnode_for_each_available_child_node(), it becomes apparent that returning "false" for all child nodes on ACPI wasn't a workable option. On DT side most access functions, even those without "_available" part, did only operate on available nodes. That wasn't the case on ACPI where only device node availability is known explicitly. Thus from now on, return true from fwnode_device_is_available() on all ACPI data nodes. Fixes: 2294b3af05e9 ("device property: Introduce fwnode_device_is_available()") Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/acpi/property.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 7fa366f1b6e376c38966faa42da7f0f2e013fdab