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

[Robin Murphy <robin.murphy@arm.com>]

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.

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