Re: [V8 1/2] irqchip: gic: Add support for multiple MSI for ARM64

[Thomas Gleixner]

On Sat, 20 Sep 2014, suravee.suthikulpanit@xxxxxxx wrote:

> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> 
> This patch implelments the ARM64 version of arch_setup_msi_irqs(),
> which does not return 1 for when PCI_CAP_ID_MSI and nvec > 1.

I can see that myself. What your changelog is missing is the reason
WHY you think that copying that code from drivers/pci/msi.c and
removing the "PCI_CAP_ID_MSI and nvec > 1" has any value.
 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> Acked-by: Marc Zyngier <Marc.Zyngier@xxxxxxx>

....

>  arch/arm64/kernel/Makefile |  1 +
>  arch/arm64/kernel/msi.c    | 41 +++++++++++++++++++++++++++++++++++++++++

And that new function "arm64_setup_msi_irqs" is declared in which
header file exactly?

> diff --git a/arch/arm64/kernel/msi.c b/arch/arm64/kernel/msi.c
> new file mode 100644
> index 0000000..a295862
> --- /dev/null
> +++ b/arch/arm64/kernel/msi.c
> @@ -0,0 +1,41 @@
> +/*
> + * ARM64 architectural MSI implemention
> + *
> + * Support for Message Signalelled Interrupts for systems that
> + * implement ARM Generic Interrupt Controller: GICv2m.
> + *
> + * Copyright (C) 2014 Advanced Micro Devices, Inc.
> + * Authors: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>

Copying stuff from A to B makes a real case for copyright, i.e. I'm
impressed by your ability to copy right.

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/msi.h>
> +#include <linux/pci.h>
> +
> +/*
> + * ARM64 function for seting up MSI irqs.
> + * Based on driver/pci/msi.c: arch_setup_msi_irqs().

This is not based on. This is a verbatim copy with the omission of two
lines. Very creative that ...

> + *
> + * Note:
> + * Current implementation assumes that all interrupt controller used in
> + * ARM64 architecture _MUST_ supports multi-MSI.

Great assumption....

> + */
> +int arm64_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)

What the heck is calling that code? The follow up patch does not and
due to lack of a declaration this patch is completely pointless.

So you add a new file with a pointless changelog and a boasting
copyright notice which adds completely useless functionality?

Well done.

At least you are consistent on the useless side of affairs:

> +{
> +	struct msi_desc *entry;
> +	int ret;
> +
> +	list_for_each_entry(entry, &dev->msi_list, list) {
> +		ret = arch_setup_msi_irq(dev, entry);

Anyone who has the slightest idea how multi-MSI works will know that
this CANNOT work at all, but that's none of my business.

What's part of my business is to state that in my role as the
maintainer of all things related to interrupts this is the worst patch
I've seen in the last 10 years. Along with the saddening fact that it
carries the Acked-by of someone who should have known better.

At least if confirms my suspicion that ARM64 is a dignified successor
of the already infamous ARM32 universe.

I really can't bear the suspension to see the first 1500+ vendor patch
series of dubious quality supporting a real ARM64.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/