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

[Rafael J. Wysocki (Intel)]

On Mon, Oct 19, 2026 at 12:34 PM Lorenzo Pieralisi
<lpieralisi@xxxxxxxxxx> wrote:
>
> On Thu, Oct 15, 2026 at 10:46:26PM +0200, Rafael J. Wysocki (Intel) wrote:
> > Sorry for the delay.
>
> Thanks for chiming in.
>
> > 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.
>
> All we need to find dependencies is a GSI number, the question though
> is what are we really solving by creating a dependency (and where
> that GSI number will be stored eventually so that the platform driver
> at probe - when dependencies are cleared, can retrieve it).
>
> I think that what we want is preventing a platform device to be created
> (ie we don't create platform devices at static table init time anymore
> but we let ACPI core do it at acpi_bus_attach(adev)/enumeration time ?)

Basically, yes, the idea is to defer the creation of the platform
device until the dependency is met.

> till the dependencies are cleared but then, without a _CRS, where will
> the consumer driver find the platform device resources ?

Well, good question.

> There are multiple facets in this issue and possibly some of them have
> been already solved but I need to pull them together.
>
> > 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.
>
> Yes it is getting messy. This RFC, with all its flaws, does not transparently
> prevents enumeration but rather relies on -EPROBE_DEFER to sort out
> dependencies - though it does so by adding more stuff into ACPI static fwnode
> that to be honest I'd rather see gone.

I think what you need is a notification mechanism that will trigger
when a specific device (enumerated via the ACPI Namespace) becomes
operational.  It doesn't look like any of the existing ones (platform
device probe deferral or ACPI device dependencies tracking) is really
suitable for your use case.

> > 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?
>
> That's a fair point - though then code will have to work with firmware
> that does NOT explicitly require those devices in the namespace (ie: firmware
> as it is specified _now_); it can turn out to be a mess even less
> manageable than this discussion ;-)

If it has to work under these conditions anyway, then adding stuff to
the namespace is rather pointless.