Re: [PATCH RFC 02/11] ACPI: Introduce irq_get() for static fwnodes

[Rafael J. Wysocki (Intel)]

Sorry for the delay.

On Fri, Oct 9, 2026 at 9:57 AM Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx> wrote:
>
> On Tue, Oct 06, 2026 at 05:19:23PM +0100, Robin Murphy wrote:
> > On 28/09/2026 5:15 pm, Lorenzo Pieralisi wrote:
> > > On Fri, Sep 25, 2026 at 01:54:26PM +0300, Andy Shevchenko wrote:
> > > > On Fri, Sep 25, 2026 at 12:30:07PM +0200, Lorenzo Pieralisi wrote:
> > > > > On Fri, Sep 25, 2026 at 12:49:54PM +0300, Andy Shevchenko wrote:
> > > > > > On Fri, Sep 25, 2026 at 09:48:01AM +0200, Lorenzo Pieralisi wrote:
> > > >
> > > > ...
> > > >
> > > > > > > +static int acpi_static_fwnode_read_u32_prop_index(const struct fwnode_handle *fwnode,
> > > > > > > +                                             const char *propname,
> > > > > > > +                                             unsigned int index, u32 *value)
> > > > > > > +{
> > > > > > > +   u32 *values;
> > > > > > > +   int ret, count;
> > > > > > > +
> > > > > > > +   count = fwnode_property_count_u32(fwnode, propname);
> > > > > > > +   if (count < 0)
> > > > > > > +           return count;
> > > > > > > +
> > > > > > > +   if (index >= count)
> > > > > > > +           return -ENOENT;
> > > > > > > +
> > > > > > > +   values = kcalloc(count, sizeof(*values), GFP_KERNEL);
> > > > > > > +   if (!values)
> > > > > > > +           return -ENOMEM;
> > > > > > > +
> > > > > > > +   ret = fwnode_property_read_u32_array(fwnode, propname, values, count);
> > > > > > > +   if (!ret)
> > > > > > > +           *value = values[index];
> > > > > >
> > > > > > Use standard pattern, id est
> > > > > >
> > > > > >       if (ret)
> > > > > >               ...
> > > > > >
> > > > > > > +   kfree(values);
> > > > > >
> > > > > > You want to use __free()
> > > > > >
> > > > > > > +   return ret;
> > > > > > > +}
> > > > > >
> > > > > > I believe the whole approach is suboptimal, if you wish get indexed value (but why?)
> > > > >
> > > > > Why what (that's what the irq_get() interface requires ?) I agree it is
> > > > > suboptimal - the whole point of the series is an RFC on using properties
> > > > > to store GSI number/flags, then how to read them we will optimize it
> > I>>> when/if we agree that's the approach to be taken.
> > > >
> > > > Why to have indexed APIs. The callers usually do not want a single item from an
> > > > array, they want all of them or a big pile (exception is the array of strings,
> > > > but we have matching functions for that).
> > > >
> > > > As per approach, this patch is against device property and I don't think we are
> > > > going to agree on the approach taken in *this* patch.
> > >
> > > Coming back to this, what is needed here is a way to stash the GSI number,
> > > flags and name in stardard storage (like OF nodes IRQ properties, ACPI objects
> > > _CRS, etc).
> > >
> > > To give you some background, I thought about adding an array of:
> > >
> > > GSI [number, flags, name]
> > >
> > > to the ACPI static fwnode itself. Then basically the irq_get() API has got
> > > all the information it needs from the primary fwnode; while doing that I
> > > thought that this might have been a bit like reinventing software nodes
> > > (granted, software node properties are ways more general that what I suggest
> > > above and I don't expect any other property to be ever added to static fwnodes)
> > > that's why I put together the series as it is, precisely to understand what's
> > > best.
> > >
> > > When you say "against device property", what do you mean precisely ?
> > >
> > > I suppose you mean that this approach goes against software node properties
> > > usage guidelines ?
> > >
> > > Anyway, again, it is just to provide some context, we are talking about
> > > a bunch of platform devices, it is nice to have a generic solution but
> > > it has to be simple enough too given its scope, I understand very well
> > > your concern with this approach as-is - it is there to find a way
> > > forward together and show what the problem is.
> >
> > I guess software node properties look inviting in theory since they're a
> > ready-made mechanism for effectively attaching arbitrary data to a fwnode,
> > however looking at patches 6-7 and the amount of boilerplate involved in the
> > reality, I'm inclined to think that a dedicated structure for static nodes
> > is probably the best and most efficient compromise here. The redundancy
> > argument rather falls apart once we end up creating copies of those
> > contentiously-made-up property name strings for every node, especially given
> > that any one of those alone is probably already more than the amount of
> > actual data we need per node...
> >
> > Perhaps the "purest" option might be to just store a function pointer in the
> > fwnode wrapper for each static table type to provide its own internal
> > irq_get() callback, so not stashing intermediate GSI data at all, but I
> > suspect that might end up being more work overall.
>
> Hi Rafael, Robin,
>
> I have not started prototyping it yet but I have got a question as a
> follow-up to our chat at LPC about crafting an ACPI device for the static
> table entries and sort out the probe dependencies using autodep.
>
> After spur of the moment reaction during the session, I now remember I
> thought about crafting an ACPI device for the static table entries and
> started digging then bumped into acpi_add_single_object() as you
> suggested.
>
> The point though is: what about the acpi_handle and the AML required to
> sort out the dependencies using the _CRS ? We have got the info to fill
> the _CRS coming from static table entries (MMIO, GSI) but I am not sure how I
> can form a full-fledged ACPI namespace object in the kernel, without
> that in place I am not sure how the autodep mechanism can possibly
> work.

The missing ACPI handle is not a hard part IMV, it is a matter of
modifying struct acpi_dep_data to be able to hold struct acpi_device
pointers instead of ACPI handles in case the devices in question have
no ACPI handles and adjusting the code using it.

Finding the dependencies is harder.  I guess it may involve walking
the namespace and looking up a device object with a specific device ID
or similar.

Note that the dependencies can be added to acpi_dep_list after
creating a struct acpi_device with acpi_add_single_object() and
acpi_scan_add_dep() is not really suitable for that, but it should be
doable if the dependencies can be found.

> I need to have a deeper look into this next week but I wanted to ask first
> whether there was something I missed - most likely.

Well, it is not really straightforward.

I'm still not sure why you can't represent all devices in the ACPI
namespace in addition to describing some of them in static tables.  I
guess that would work for everybody, wouldn't it?