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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

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.

Thanks Robin.

Yes, those are the possible options, last one requiring a look-up
from fwnode to the data providing GSI numbers/flags for that specific
fwnode - we do have that for IORT but I need to check for other
devices.

To be honest, I implemented the three of them while prototyping, it is just
a matter of choosing which one we want to support to sort this out.

Thanks !
Lorenzo

> Cheers,
> Robin.
> 
> > 
> > Thanks,
> > Lorenzo
> > 
> > > > > it needs to be retrieved as that in the guts of ACPI. Allocating memory for the whole
> > > > > array to retrieve a single element is simply wrong.
> > > 
> > > ...
> > > 
> > > > > > +#define ACPI_IRQ_PROP_GSI		"linux,acpi-gsi"
> > > > > > +#define ACPI_IRQ_PROP_GSI_TRIGGER	"linux,acpi-gsi-trigger"
> > > > > > +#define ACPI_IRQ_PROP_GSI_POLARITY	"linux,acpi-gsi-polarity"
> > > > > 
> > > > > Oh... This sounds like a big ugly hack.
> > > > 
> > > > That does not help much I am afraid.
> > > > 
> > > > What's a ugly hack ? Property names ? Using properties for this purpose ?
> > > 
> > > Yes, properties started with "linux," for some core functionality.
> > > Yes, using properties for this also doesn't sound right. I think the problem
> > > here is in the table specifications or somewhere that deep. That's why we
> > > ended up in this series.
> > > 
> > > > Again, it is an RFC for this specific reason and I mentioned that in the
> > > > cover letter, thank you for your inputs.
> > > 
> > > And here we have a discussion started :-)
> > > 
> > > P.S. Looks like I was too quick to jump into this thread. I will wait for
> > > others to share their view on all this.
> > > 
> > > -- 
> > > With Best Regards,
> > > Andy Shevchenko
> > > 
> > > 
>