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

[Lorenzo Pieralisi]

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:
> > To describe and map GSIs for firmware nodes created out of ACPI static
> > table entries in a uniform way it is required to define some standard
> > properties and attach them to ACPI static fwnode as secondary nodes.
> > 
> > Define properties names to describe GSIs and their trigger-mode/polarity,
> > and implement an irq_get() callback for static fwnodes so that core code
> > can retrieve and map IRQs for ACPI static fwnodes in standard manner.
> > 
> > An empty stub for property_read_string_array() is also added, so that
> > the fwnode_irq_get_byname() interface falls back (through
> > fwnode_property_read_string_array()) to the secondary
> > fwnode to grab the "interrupt-names" property.
> 
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>
> > Cc: Bartosz Golaszewski <brgl@xxxxxxxxxx>
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > ---
> 
> Same here, please avoid polluting commit message with the Cc list.
> 
> ...
> 
> > +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
when/if we agree that's the approach to be taken.

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

Again, it is an RFC for this specific reason and I mentioned that in the
cover letter, thank you for your inputs.

Lorenzo