Re: [PATCH 19/24] irqchip/gic-v5: Add GICv5 IRS/SPI support

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Apr 08 2025 at 12:50, Lorenzo Pieralisi wrote:
> +struct iaffid_entry {
> +	u16 iaffid;
> +	bool valid;
> +};

See the previous documentation link and search for struct definitions on
that page.

> +static int gicv5_irs_wait_for_spi_op(struct gicv5_irs_chip_data *irs_data)
> +{
> +	int ret;
> +	u32 statusr;

See documentaion...

> +	ret = readl_relaxed_poll_timeout_atomic(
> +			irs_data->irs_base + GICV5_IRS_SPI_STATUSR, statusr,
> +			FIELD_GET(GICV5_IRS_SPI_STATUSR_IDLE, statusr), 1,
> +			USEC_PER_SEC);

See previous mail about how to make stuff like this readable. My eyes
bleed already.

> +	if (ret == -ETIMEDOUT) {

unlikely(ret == ...) perhaps?

> +		pr_err_ratelimited("Time out waiting for IRS SPI to be configured\n");

> +static int __init gicv5_irs_init_bases(struct gicv5_irs_chip_data *irs_data,
> +				       void __iomem *irs_base,
> +				       struct fwnode_handle *handle)
> +{
> +	u32 cr0, cr1;
> +	struct device_node *np = to_of_node(handle);

Sigh

> +static int __init gicv5_irs_of_init_affinity(struct device_node *node,
> +				      struct gicv5_irs_chip_data *irs_data,
> +				      u8 iaffid_bits)

Moar random coding style choices.

> +{
> +	/*
> +	 * Detect IAFFID<->CPU mappings from the device tree and
> +	 * record IRS<->CPU topology information.
> +	 */
> +	int ret, i, ncpus, niaffids;
> +	u16 *iaffids;
> +	u16 iaffid_mask = GENMASK(iaffid_bits - 1, 0);
> +
> +	ncpus = of_property_count_elems_of_size(node, "cpus", sizeof(u32));
> +	if (WARN_ON(ncpus < 0))
> +		return -EINVAL;

Do you really need all these warnings?

> +
> +	niaffids = of_property_count_elems_of_size(node, "arm,iaffids",
> +						   sizeof(u16));
> +	if (WARN_ON(niaffids != ncpus))
> +		return -EINVAL;
> +
> +	iaffids = kcalloc(niaffids, sizeof(*iaffids), GFP_KERNEL);
> +	if (!iaffids)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u16_array(node, "arm,iaffids", iaffids, niaffids);
> +	if (ret)
> +		return ret;

Leaks iaffids. Please use

      u16 *iaffids __free(kfree) = kcalloc(...);

and the compiler will take care of that.

> +static int __init gicv5_irs_init(struct device_node *node)
> +{
> +	void __iomem *irs_base;
> +	struct gicv5_irs_chip_data *irs_data;
> +	int ret;
> +	u32 idr;
> +	u8 iaffid_bits;
> +
> +	irs_data = kzalloc(sizeof(*irs_data), GFP_KERNEL);

__free(kfree)

> +	if (!irs_data)
> +		return -ENOMEM;

> +	if (irs_data->spi_range)
> +		pr_info("%s detected SPI range [%u-%u]\n",
> +						of_node_full_name(node),
> +						irs_data->spi_min,
> +						irs_data->spi_min +
> +						irs_data->spi_range - 1);

Please put those _five_ lines into brackets. It's not required by the
compiler, but for reading. Brackets should be omitted only if the
statement which follows ‘if’, ‘for’, ‘while’ etc. is truly a single
line.

> +static int gicv5_iri_irq_get_irqchip_state(struct irq_data *d,
> +					   enum irqchip_irq_state which,
> +					   bool *val, u8 hwirq_type)
> +{
> +	u64 icsr, cdrcfg = d->hwirq | FIELD_PREP(GICV5_GIC_CDRCFG_TYPE_MASK,
> +						 hwirq_type);
> +
> +	preempt_disable();

That's required because the calling contexts protection (raw spinlock
held and interrupts disabled) is not enough, right?

> +	gic_insn(cdrcfg, GICV5_OP_GIC_CDRCFG);
> +	isb();
> +	icsr = read_sysreg_s(SYS_ICC_ICSR_EL1);
> +	preempt_enable();

> +static int gicv5_irq_spi_domain_translate(struct irq_domain *d,
> +					  struct irq_fwspec *fwspec,
> +					  irq_hw_number_t *hwirq,
> +					  unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count < 3)
> +			return -EINVAL;
> +
> +		if (fwspec->param[0] != GICV5_HWIRQ_TYPE_SPI)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +
> +		return 0;
> +	}

The only difference between this and the ppi variant is the type check
of param[0]. Common helper perhaps?

Thanks,

        tglx