Re: [PATCH 1/1] ACPI: property: Consider data nodes as being available

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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
> >

-- 
Kind regards,

Sakari Ailus