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

Commit Message

Suravee Suthikulpanit Sept. 20, 2014, 4:31 p.m. UTC

From: Suravee Suthikulpanit <[e-mail]>

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.

Signed-off-by: Suravee Suthikulpanit <[e-mail]>
Acked-by: Marc Zyngier <[e-mail]>
Cc: Mark Rutland <[e-mail]>
Cc: Jason Cooper <[e-mail]>
Cc: Catalin Marinas <[e-mail]>
Cc: Will Deacon <[e-mail]>
---
 arch/arm64/kernel/Makefile |  1 +
 arch/arm64/kernel/msi.c    | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)
 create mode 100644 arch/arm64/kernel/msi.c

Comments

Will Deacon Sept. 22, 2014, 9:15 a.m. UTC | #1

Hi Suravee,

Just a few minor comments.

On Sat, Sep 20, 2014 at 05:31:37PM +0100, [e-mail] wrote:
> From: Suravee Suthikulpanit <[e-mail]>
> 
> 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.

s/implelments/implements/

> Signed-off-by: Suravee Suthikulpanit <[e-mail]>
> Acked-by: Marc Zyngier <[e-mail]>
> Cc: Mark Rutland <[e-mail]>
> Cc: Jason Cooper <[e-mail]>
> Cc: Catalin Marinas <[e-mail]>
> Cc: Will Deacon <[e-mail]>
> ---
>  arch/arm64/kernel/Makefile |  1 +
>  arch/arm64/kernel/msi.c    | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+)
>  create mode 100644 arch/arm64/kernel/msi.c
> 
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index df7ef87..a921c42 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -29,6 +29,7 @@ arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
>  arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
>  arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
>  arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
> +arm64-obj-$(CONFIG_PCI_MSI)		+= msi.o
>  
>  obj-y					+= $(arm64-obj-y) vdso/
>  obj-m					+= $(arm64-obj-m)
> 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.

You can drop the GICv2M reference here, as there's not really anything
GIC-specific in this file.

> + * Copyright (C) 2014 Advanced Micro Devices, Inc.
> + * Authors: Suravee Suthikulpanit <[e-mail]>
> + *
> + * 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().
> + *
> + * Note:
> + * Current implementation assumes that all interrupt controller used in
> + * ARM64 architecture _MUST_ supports multi-MSI.

I think you can remove this comment, to be honest.

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

Shouldn't this be called arch_setup_msi_irqs, since the intention is that
you override the weak symbol?

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

When does this function ever return a value > 0?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thomas Gleixner Sept. 22, 2014, 11:08 p.m. UTC | #2

On Sat, 20 Sep 2014, [e-mail] wrote:

> From: Suravee Suthikulpanit <[e-mail]>
> 
> 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 <[e-mail]>
> Acked-by: Marc Zyngier <[e-mail]>

....

>  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 <[e-mail]>

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-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Suravee Suthikulpanit Sept. 23, 2014, 5:04 p.m. UTC | #3

Thomas,

Sorry again for the mistake on my part. Let me try to address some other 
concerns you have below.


On 09/22/2014 04:08 PM, Thomas Gleixner wrote:
> On Sat, 20 Sep 2014, [e-mail] wrote:
>
>> From: Suravee Suthikulpanit <[e-mail]>
>>
>> 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.

[Suravee] This is mainly be cause the weak version of 
arch_setup_msi_irqs() in the drivers/pci/msi.c doesn't support 
multi-MSI. Sorry for not being clear in the commit message.

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

[Suravee] This was supposed to be arch_setup_msi_irqs(). My bad. I'm 
fixing this in the next version.

......

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

[Suravee] So, Marc and I have discussed in the past that at this point, 
we are not seeing the case that there will be interrupt or 
MSI-controller that will not support multi-MSI.  If you think this 
should not be the case, would you please share your thought.

......

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

[Suravee] I noticed that in the x86 version, there is a callback that 
each MSI controller need to register for handling the multi-MSI stuff.

In gicv2m_setup_msi_irq(), there is logic which handles the setup for 
multi-MSI and MSIx separately. In case of multi-MSI, the vectors are 
allocated on the first call to arch_setup_msi_irq(). Here, Marc and I 
are trying to simplify the arch-specific code so that each GIC 
controller (V2m and V3) would not need to implement and register the 
callbacks separately for handling multi-MSI.

The thing that is broken here is the error handling where the 
arch_setup_msi_irqs() is supposed to return the number of available MSI 
vectors. It would fail to do so because the arch_setup_msi_irq() would 
not return positive value. We should be able to fix this by 
re-implementing the arch_setup_msi_irq() and arch_setup_msi_irqs() to 
allow returning of positive values.

Please let me know what you think. I am open for suggestions :)

Thanks,

Suravee
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thomas Gleixner Sept. 23, 2014, 9:33 p.m. UTC | #4

On Tue, 23 Sep 2014, Suravee Suthikulpanit wrote:
> > > 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.
> 
> [Suravee] This is mainly be cause the weak version of arch_setup_msi_irqs() in
> the drivers/pci/msi.c doesn't support multi-MSI. Sorry for not being clear in
> the commit message.

Groan. I asked you: 

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

And your answer is that the function in drivers/pci/msi.c does not
support Multi-MSI. Hell I know that myself. And there is a fricking
good reason why allocating multi-MSI via

    for_each_msi()
	alloc_msi_irq();

is wrong. And while it might work by chance, there is no guarantee
that it will work. It works for Multi-MSIX, but that has an additional
X at the end and is a different beast when it comes to interrupts.

I have no idea how crooked you are trying to work around that on the
GIC side, but its going to be wrong and convoluted.

Read and understand the MSI and MSI-X spec and the subtle differences
of interrupt delivery. And if you groked that come back with a proper
explanation why that patch makes sense or just go back to the drawing
board and do it proper.

Thanks,

	tglx




--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index df7ef87..a921c42 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -29,6 +29,7 @@  arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
 arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
 arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
+arm64-obj-$(CONFIG_PCI_MSI)		+= msi.o
 
 obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
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 <[e-mail]>
+ *
+ * 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().
+ *
+ * Note:
+ * Current implementation assumes that all interrupt controller used in
+ * ARM64 architecture _MUST_ supports multi-MSI.
+ */
+int arm64_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
+{
+	struct msi_desc *entry;
+	int ret;
+
+	list_for_each_entry(entry, &dev->msi_list, list) {
+		ret = arch_setup_msi_irq(dev, entry);
+		if (ret < 0)
+			return ret;
+		if (ret > 0)
+			return -ENOSPC;
+	}
+
+	return 0;
+}