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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

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@kernel.org> 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 ?)
till the dependencies are cleared but then, without a _CRS, where will
the consumer driver find the platform device resources ?

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'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 ;-)

Any thoughts appreciated.

Thanks,
Lorenzo