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

From: Thomas Gleixner <tglx@kernel.org>
To: Andrew Jones <andrew.jones@oss.qualcomm.com>,
	linux-riscv@lists.infradead.org, iommu@lists.linux.dev
Cc: linux-kernel@vger.kernel.org, tomasz.jeznach@linux.dev,
	tjeznach@rivosinc.com, jgg@ziepe.ca, jgg@nvidia.com,
	joro@8bytes.org, will@kernel.org, robin.murphy@arm.com,
	pjw@kernel.org, palmer@dabbelt.com, anup@brainfault.org,
	kevin.tian@intel.com, fangyu.yu@linux.alibaba.com
Subject: Re: [PATCH v3 07/19] iommu/riscv: Add IRQ domain for interrupt remapping
Date: Fri, 07 Aug 2026 22:32:15 +0200	[thread overview]
Message-ID: <875x1lekog.ffs@fw13> (raw)
In-Reply-To: <20260807181713.228535-8-andrew.jones@oss.qualcomm.com>

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.

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

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

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

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

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

Please write comments which do not require to grep the tree.

Thanks,

        tglx

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv