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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Wed, Apr 09, 2025 at 09:02:58AM +0200, Thomas Gleixner wrote:
> 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.

Right, will fix.

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

Will change it.

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

I will review them.

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

Yes, that's silly.

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

Will do.

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

Ok.

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

Yes it is useless, I will remove it.

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

Definitely.

Thanks a lot,
Lorenzo