Re: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain

[Thomas Gleixner]

Dave Jiang <dave.jiang@xxxxxxxxx> writes:
> From: Megha Dey <megha.dey@xxxxxxxxx>
>
> When DEV_MSI is enabled, the dev_msi_default_domain is updated to the
> base DEV-MSI irq  domain. If interrupt remapping is enabled, we create

s/we//

> a new IR-DEV-MSI irq domain and update the dev_msi_default domain to
> the same.
>
> For X86, introduce a new irq_alloc_type which will be used by the
> interrupt remapping driver.
>
> Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> Signed-off-by: Megha Dey <megha.dey@xxxxxxxxx>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> ---
>  arch/x86/include/asm/hw_irq.h       |    1 +
>  arch/x86/kernel/apic/msi.c          |   12 ++++++
>  drivers/base/dev-msi.c              |   66 +++++++++++++++++++++++++++++++----
>  drivers/iommu/intel/irq_remapping.c |   11 +++++-
>  include/linux/intel-iommu.h         |    1 +
>  include/linux/irqdomain.h           |   11 ++++++
>  include/linux/msi.h                 |    3 ++

Why is this mixing generic code, x86 core code and intel specific driver
code? This is new functionality so:

      1) Provide the infrastructure
      2) Add support to architecture specific parts
      3) Enable it

> +
> +#ifdef CONFIG_DEV_MSI
> +int dev_msi_prepare(struct irq_domain *domain, struct device *dev,
> +			   int nvec, msi_alloc_info_t *arg)
> +{
> +	memset(arg, 0, sizeof(*arg));
> +
> +	arg->type = X86_IRQ_ALLOC_TYPE_DEV_MSI;
> +
> +	return 0;
> +}
> +#endif

What is this? Tons of new lines for taking up more space and not a
single comment.

> -static int dev_msi_prepare(struct irq_domain *domain, struct device *dev,
> +int __weak dev_msi_prepare(struct irq_domain *domain, struct device *dev,
>  			   int nvec, msi_alloc_info_t *arg)
>  {
>  	memset(arg, 0, sizeof(*arg));

Oh well. So every architecure which needs to override this and I assume
all which are eventually going to support it need to do the memset() in
their override.

       memset(arg,,,);
       arch_dev_msi_prepare();


> -	dev_msi_default_domain = msi_create_irq_domain(fn, &dev_msi_domain_info, parent);
> +	/*
> +	 * This initcall may come after remap code is initialized. Ensure that
> +	 * dev_msi_default domain is updated correctly.

What? No, this is a disgusting hack. Get your ordering straight, that's
not rocket science.

> +#ifdef CONFIG_IRQ_REMAP

IRQ_REMAP is x86 specific. Is this file x86 only or intended to be for
general use? If it's x86 only, then this should be clearly
documented. If not, then these x86'isms have no place here.

> +struct irq_domain *create_remap_dev_msi_irq_domain(struct irq_domain *parent,
> +						   const char *name)

So we have msi_create_irq_domain() and this is about dev_msi, right? So
can you please stick with a consistent naming scheme?

> +{
> +	struct fwnode_handle *fn;
> +	struct irq_domain *domain;
> +
> +	fn = irq_domain_alloc_named_fwnode(name);
> +	if (!fn)
> +		return NULL;
> +
> +	domain = msi_create_irq_domain(fn, &dev_msi_ir_domain_info, parent);
> +	if (!domain) {
> +		pr_warn("failed to initialize irqdomain for IR-DEV-MSI.\n");
> +		return ERR_PTR(-ENXIO);
> +	}
> +
> +	irq_domain_update_bus_token(domain, DOMAIN_BUS_PLATFORM_MSI);
> +
> +	if (!dev_msi_default_domain)
> +		dev_msi_default_domain = domain;

Can this be called several times? If so, then this lacks a comment. If
not, then this condition is useless.

Thanks,

        tglx