Re: [PATCH v3 07/19] iommu/riscv: Add IRQ domain for interrupt remapping

On Fri, Aug 07, 2026 at 10:32:15PM +0200, Thomas Gleixner wrote:
> On Fri, Aug 07 2026 at 20:17, Andrew Jones wrote:
> > +static int riscv_iommu_ir_irq_domain_alloc_irqs(struct irq_domain *irqdomain,
> > +						unsigned int irq_base, unsigned int nr_irqs,
> > +						void *arg)
> > +{
> > +	struct irq_data *data;
> > +	int i, ret;
> > +
> > +	ret = irq_domain_alloc_irqs_parent(irqdomain, irq_base, nr_irqs, arg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < nr_irqs; i++) {
> 
> for (unsigned int i = 0; .....
> 
> nr_irqs is unsigned after all
> 
> The struct irq_data declaration want's to be inside the loop as that's
> the scope where it is used.

ack

> 
> > +		data = irq_domain_get_irq_data(irqdomain, irq_base + i);
> 
> 
> > +		data->chip = &riscv_iommu_ir_irq_chip;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops riscv_iommu_ir_irq_domain_ops = {
> > +	.alloc = riscv_iommu_ir_irq_domain_alloc_irqs,
> > +	.free = irq_domain_free_irqs_parent,
> 
> https://docs.kernel.org/process/maintainer-tip.html#struct-declarations-and-initializers

I'll align the member names.

> 
> > +};
> > +
> > +static const struct msi_parent_ops riscv_iommu_ir_msi_parent_ops = {
> > +	.prefix			= "IR-",
> > +	.supported_flags	= MSI_GENERIC_FLAGS_MASK |
> > +				  MSI_FLAG_PCI_MSIX,
> > +	.required_flags		= MSI_FLAG_USE_DEF_DOM_OPS |
> > +				  MSI_FLAG_USE_DEF_CHIP_OPS |
> > +				  MSI_FLAG_PCI_MSI_MASK_PARENT,
> > +	.chip_flags		= MSI_CHIP_FLAG_SET_ACK,
> > +	.init_dev_msi_info	= msi_parent_init_dev_msi_info,
> > +};
> > +
> > +struct irq_domain *riscv_iommu_ir_irq_domain_create(struct device *dev,
> > +						    struct riscv_iommu_info *info)
> 
> You have 100 characters, please use them.

I also like to use the full 100 and will even accept a checkpatch warning
for a few extra chars when splitting (IHMO) would be worse. In this case
if I left info on the same line we'd go to 102 but splitting on parameter
lists isn't too evil, so I'd probably leave this one split.

> 
> > +{
> > +	struct irq_domain *irqparent = dev_get_msi_domain(dev);
> > +	struct irq_domain *irqdomain;
> > +	struct fwnode_handle *fn;
> > +	char *fwname __free(kfree) = NULL;
> 
> https://docs.kernel.org/process/maintainer-tip.html#variable-declarations

I'll move fwname up so I don't spoil Christmas.

> 
> > +	if (!irqparent)
> > +		return NULL;
> > +
> > +	fwname = kasprintf(GFP_KERNEL, "IOMMU-IR-%s", dev_name(dev));
> > +	if (!fwname)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	fn = irq_domain_alloc_named_fwnode(fwname);
> > +	if (!fn)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	irqdomain = irq_domain_create_hierarchy(irqparent, 0, 0, fn,
> > +						&riscv_iommu_ir_irq_domain_ops,
> > +						info);
> 
> 100 chars.

I'll change it to

	irqdomain = irq_domain_create_hierarchy(irqparent, 0, 0, fn,
                                                &riscv_iommu_ir_irq_domain_ops, info);

> 
> > +	if (!irqdomain) {
> > +		irq_domain_free_fwnode(fn);
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	/*
> > +	 * The RISC-V IOMMU doesn't validate MSI data, so we can't set
> > +	 * IRQ_DOMAIN_FLAG_ISOLATED_MSI. This means VFIO requires
> > +	 * allow_unsafe_interrupts.
> 
> what is allow_unsafe_interrupts? A variable, a function a parameter or
> what?

I'll change this to:

The means VFIO requires its allow_unsafe_interrupts module parameter.

Thanks,
drew