| Message ID | 1411230698-8081-2-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Suravee, Just a few minor comments. On Sat, Sep 20, 2014 at 05:31:37PM +0100, suravee.suthikulpanit@amd.com wrote: > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > > 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 <Suravee.Suthikulpanit@amd.com> > Acked-by: Marc Zyngier <Marc.Zyngier@arm.com> > Cc: Mark Rutland <Mark.Rutland@arm.com> > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Catalin Marinas <Catalin.Marinas@arm.com> > Cc: Will Deacon <Will.Deacon@arm.com> > --- > 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 <suravee.suthikulpanit@amd.com> > + * > + * 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
On Sat, 20 Sep 2014, suravee.suthikulpanit@amd.com wrote: > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > > 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@amd.com> > Acked-by: Marc Zyngier <Marc.Zyngier@arm.com> .... > 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@amd.com> 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
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, suravee.suthikulpanit@amd.com wrote: > >> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> >> 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
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
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 <suravee.suthikulpanit@amd.com> + * + * 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; +}