Re: [PATCH 18/24] irqchip/gic-v5: Add GICv5 PPI support

[Lorenzo Pieralisi]

On Tue, Apr 08, 2025 at 11:42:29PM +0200, Thomas Gleixner wrote:
> On Tue, Apr 08 2025 at 12:50, Lorenzo Pieralisi wrote:
> > +
> > +static void gicv5_ppi_priority_init(void)
> > +{
> > +	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRIORITY_MI),
> > +				 SYS_ICC_PPI_PRIORITYR0_EL1);
> 
> Just let stick it out. You have 100 characters. All over the place...

I will do.

> > +static int gicv5_ppi_irq_set_irqchip_state(struct irq_data *d,
> > +					   enum irqchip_irq_state which,
> > +					   bool val)
> > +{
> > +	u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> > +
> > +	switch (which) {
> > +	case IRQCHIP_STATE_PENDING:
> > +		if (val) {
> > +			if (d->hwirq < 64)
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_SPENDR0_EL1);
> > +			else
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_SPENDR1_EL1);
> > +
> > +		} else {
> > +			if (d->hwirq < 64)
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_CPENDR0_EL1);
> > +			else
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_CPENDR1_EL1);
> > +		}
> > +
> > +		return 0;
> > +	case IRQCHIP_STATE_ACTIVE:
> > +		if (val) {
> > +			if (d->hwirq < 64)
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_SACTIVER0_EL1);
> > +			else
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_SACTIVER1_EL1);
> > +		} else {
> > +			if (d->hwirq < 64)
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_CACTIVER0_EL1);
> > +			else
> > +				write_sysreg_s(hwirq_id_bit,
> > +					       SYS_ICC_PPI_CACTIVER1_EL1);
> > +		}
> 
> You already precalculate hwirq_id_bit. Can't you do something similar
> for the registers?
> 
> 	case IRQCHIP_STATE_PENDING:
>         	u32 reg = val ? SYS_ICC_PPI_SPENDR1_EL1 : SYS_ICC_PPI_SPENDR0_EL1;
> 
>                 write_sysreg_s(hwirq_id_bit, reg);
>                 return 0;
> 	case IRQCHIP_STATE_ACTIVE:
>                 ....
> 
> Ditto in the get_state() function.
> 
> No?

Yes, more readable.

> > +static int gicv5_irq_ppi_domain_translate(struct irq_domain *d,
> > +					  struct irq_fwspec *fwspec,
> > +					  irq_hw_number_t *hwirq,
> > +					  unsigned int *type)
> > +{
> > +	if (is_of_node(fwspec->fwnode)) {
> 
> It'd be way more readable to invert this check
> 
>      if (!is_of_node(...))
>      	return -EINVAL;
> 
> so that the subsequent checks are just a read through.

Will do.

> > +		if (fwspec->param_count < 3)
> > +			return -EINVAL;
> > +
> > +		if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI)
> > +			return -EINVAL;
> > +
> > +		*hwirq = fwspec->param[1];
> > +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> > +
> > +		return 0;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> 
> > +static void gicv5_irq_ppi_domain_free(struct irq_domain *domain,
> > +				      unsigned int virq, unsigned int nr_irqs)
> > +{
> > +	struct irq_data *d;
> > +
> > +	if (WARN_ON(nr_irqs != 1))
> 
> WARN_ON_ONCE ?

Yes.

> > +		return;
> > +
> > +	d = irq_domain_get_irq_data(domain, virq);
> > +
> > +	irq_set_handler(virq, NULL);
> > +	irq_domain_reset_irq_data(d);
> > +}
> > +
> > +static int gicv5_irq_ppi_domain_select(struct irq_domain *d,
> > +				       struct irq_fwspec *fwspec,
> > +				       enum irq_domain_bus_token bus_token)
> > +{
> > +	/* Not for us */
> > +	if (fwspec->fwnode != d->fwnode)
> > +		return 0;
> > +
> > +	if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI) {
> > +		// only handle PPIs
> 
> Commenting the obvious?
> 

Will remove it.

> > +		return 0;
> > +	}
> > +
> > +	return (d == gicv5_global_data.ppi_domain);
> > +}
> > +
> > +static const struct irq_domain_ops gicv5_irq_ppi_domain_ops = {
> > +	.translate	= gicv5_irq_ppi_domain_translate,
> > +	.alloc		= gicv5_irq_ppi_domain_alloc,
> > +	.free		= gicv5_irq_ppi_domain_free,
> > +	.select		= gicv5_irq_ppi_domain_select
> > +};
> > +
> > +static inline void handle_irq_per_domain(u32 hwirq)
> > +{
> > +	u32 hwirq_id;
> > +	struct irq_domain *domain = NULL;
> > +	u8 hwirq_type = FIELD_GET(GICV5_HWIRQ_TYPE, hwirq);
> 
> So far you managed to comply with the documented reverse fir tree
> ordering.
> 
>   https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
> 
> Why are you changing coding style in the middle of the code?

Mea culpa, don't bother commenting on this further, point taken.

> > +	hwirq_id = FIELD_GET(GICV5_HWIRQ_ID, hwirq);
> > +
> > +	if (hwirq_type == GICV5_HWIRQ_TYPE_PPI)
> > +		domain = gicv5_global_data.ppi_domain;
> > +
> > +	if (generic_handle_domain_irq(domain, hwirq_id)) {
> > +		pr_err("Could not handle, hwirq = 0x%x", hwirq_id);
> 
> pr_err_once() perhaps?
> 
> > +		gicv5_hwirq_eoi(hwirq_id, hwirq_type);
> > +	}
> > +}
> > +
> > +static asmlinkage void __exception_irq_entry
> > +gicv5_handle_irq(struct pt_regs *regs)
> > +{
> > +	u64 ia;
> > +	bool valid;
> > +	u32 hwirq;
> 
> See above
> 
> > +	ia = gicr_insn(GICV5_OP_GICR_CDIA);
> > +	valid = GICV5_GIC_CDIA_VALID(ia);
> 
> And please move that to the declaration lines
> 
> > +static int __init gicv5_init_domains(struct fwnode_handle *handle)
> > +{
> > +	gicv5_global_data.fwnode = handle;
> > +	gicv5_global_data.ppi_domain = irq_domain_create_linear(
> > +		handle, 128, &gicv5_irq_ppi_domain_ops, NULL);
> 
> The ever changing choice of coding styles across functions is really
> interesting. Obviously the length of 'gicv5_global_data.ppi_domain'
> forces ugly, but that does not mean it needs to be that way:
> 
>        struct irqdomain *d;
> 
>        d = irq_domain_create_linear(handle, 128, &gicv5_irq_ppi_domain_ops, NULL);
>        if (!d)
>        		return - ENOMEM;
> 
>        irq_domain_update_bus_token(d, DOMAIN_BUS_WIRED);
>        gicv5_global_data.fwnode = handle;
>        gicv5_global_data.ppi_domain = d;
>        return 0;
> 
> No?

Yes it is better.

> > +static int __init gicv5_of_init(struct device_node *node,
> > +				struct device_node *parent)
> > +{
> > +	int ret;
> > +
> > +	ret = gicv5_init_domains(&node->fwnode);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gicv5_set_cpuif_pribits();
> > +
> > +	ret = gicv5_starting_cpu(smp_processor_id());
> 
> You invoke the CPU hotplug callback for the boot CPU explicitly, but
> what the heck installs the actual hotplug callback for the secondary
> CPUs?

That comes with a subsequent patch[21]. I mentioned in the cover letter
that I tried to split the functionality into interrupt types to ease
review (well, it does not look like I succeeded, sorry) and then in
patch [21] (when LPIs backing IPIs are implemented), enable SMP.

The point is, we need patches [18-21] to enable SMP booting.

I can squash [18-21] all together or I can enable the hotplug callback
here but this patch stand alone is not functional for the reasons
above, let me know please what's best in your opinion and I will do.

Above all, thank you very much for reviewing the series.

Lorenzo