lkml.org 
[lkml]   [2020]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH RFC v2 03/18] irq/dev-msi: Create IR-DEV-MSI irq domain
    Date
    Dave Jiang <dave.jiang@intel.com> writes:
    > From: Megha Dey <megha.dey@intel.com>
    >
    > 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@intel.com>
    > Signed-off-by: Megha Dey <megha.dey@intel.com>
    > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
    > ---
    > 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

    \
     
     \ /
      Last update: 2020-07-22 22:45    [W:4.190 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site