| Message ID | 20220110015018.26359-24-kabel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: aardvark controller fixes BATCH 4 | expand |
On 2022-01-10 01:50, Marek Behún wrote: > Marc Zyngier says [1] that we should use struct irq_chip as a global > static struct in the driver. Even though the structure currently > contains a dynamic member (parent_device), Marc says [2] that he plans > to kill it and make the structure completely static. > > We have already converted others irq_chip structures in this driver in > this way, but we omitted this one because the .name member is > dynamically created from device's name, and the name is displayed in > sysfs, so changing it would break sysfs ABI. > > The rationale for changing the name (to "advk-INT") in spite of sysfs > ABI, and thus allowing to convert to a static structure, is that after > the other changes we made in this series, the IRQ chip is basically > something different: it no logner generates ERR and PME interrupts > (they > are generated by emulated bridge's rp_irq_chip). There is no 'is spite of the ABI'. If you don't understand why we don't break the ABI, you have an even bigger problem. So NAK to this patch, now and forever. Any change to the structure to make it read-only must allow the preservation of the existing names when they are generated by the driver. M.
On Mon, 10 Jan 2022 09:28:39 +0000 Marc Zyngier <maz@kernel.org> wrote: > On 2022-01-10 01:50, Marek Behún wrote: > > Marc Zyngier says [1] that we should use struct irq_chip as a global > > static struct in the driver. Even though the structure currently > > contains a dynamic member (parent_device), Marc says [2] that he plans > > to kill it and make the structure completely static. > > > > We have already converted others irq_chip structures in this driver in > > this way, but we omitted this one because the .name member is > > dynamically created from device's name, and the name is displayed in > > sysfs, so changing it would break sysfs ABI. > > > > The rationale for changing the name (to "advk-INT") in spite of sysfs > > ABI, and thus allowing to convert to a static structure, is that after > > the other changes we made in this series, the IRQ chip is basically > > something different: it no logner generates ERR and PME interrupts > > (they > > are generated by emulated bridge's rp_irq_chip). > > There is no 'is spite of the ABI'. If you don't understand why > we don't break the ABI, you have an even bigger problem. > > So NAK to this patch, now and forever. Any change to the structure to > make it read-only must allow the preservation of the existing names > when they are generated by the driver. Dear Marc, That's why I put it as a last patch here :) I have the questions 1) the first is that this driver has only ever been used on Armada 37xx, where there has always been only one PCIe controller, and it's name always was d0070000.pcie, so the irq_chip was always called d0070000.pcie-irq. So we could theoretically infer from config options if we are building for Armada 37xx: if ARM64 and ARMADA_37XX_CLK config options are enabled, make the name d0070000.pcie-irq, otherwise advk-INT ? 2) I tried to look for the name d0070000.pcie-irq in /proc and /sysfs, but couldn't find it there (not in /proc/interrupts nor anywhere else). It is possible that I omitted something, or that with other PCIe card it will show up when corresponding driver is loaded. But theoretically, if I could prove that until now this never appeared anywhere in sysfs for some reason, then we could change it, right? Becuase that way it isn't sysfs ABI change. Thanks. Marek
On Monday 10 January 2022 09:28:39 Marc Zyngier wrote: > On 2022-01-10 01:50, Marek Behún wrote: > > Marc Zyngier says [1] that we should use struct irq_chip as a global > > static struct in the driver. Even though the structure currently > > contains a dynamic member (parent_device), Marc says [2] that he plans > > to kill it and make the structure completely static. > > > > We have already converted others irq_chip structures in this driver in > > this way, but we omitted this one because the .name member is > > dynamically created from device's name, and the name is displayed in > > sysfs, so changing it would break sysfs ABI. > > > > The rationale for changing the name (to "advk-INT") in spite of sysfs > > ABI, and thus allowing to convert to a static structure, is that after > > the other changes we made in this series, the IRQ chip is basically > > something different: it no logner generates ERR and PME interrupts (they > > are generated by emulated bridge's rp_irq_chip). > > There is no 'is spite of the ABI'. If you don't understand why > we don't break the ABI, you have an even bigger problem. > > So NAK to this patch, now and forever. Any change to the structure to > make it read-only must allow the preservation of the existing names > when they are generated by the driver. Marc, you already presented that you do not like Armada 3720 platform and that you do not care about it. But please do not slowdown development for this platform. Arguments about ABIs, breaking it and similar are not relevant here as this current kernel implementation is broken. And has to be replaced by a working one. We are doing on it for more than year. It really does not make sense to try doing some backward compatibility with something which is broken by design and does not work. It just take lot of time without any value. We really need to more forward and fix driver as in current state is PCIe on Armada 3720 unusable.
On Mon, 10 Jan 2022 10:53:24 +0000, Pali Rohár <pali@kernel.org> wrote: > > On Monday 10 January 2022 09:28:39 Marc Zyngier wrote: > > On 2022-01-10 01:50, Marek Behún wrote: > > > Marc Zyngier says [1] that we should use struct irq_chip as a global > > > static struct in the driver. Even though the structure currently > > > contains a dynamic member (parent_device), Marc says [2] that he plans > > > to kill it and make the structure completely static. > > > > > > We have already converted others irq_chip structures in this driver in > > > this way, but we omitted this one because the .name member is > > > dynamically created from device's name, and the name is displayed in > > > sysfs, so changing it would break sysfs ABI. > > > > > > The rationale for changing the name (to "advk-INT") in spite of sysfs > > > ABI, and thus allowing to convert to a static structure, is that after > > > the other changes we made in this series, the IRQ chip is basically > > > something different: it no logner generates ERR and PME interrupts (they > > > are generated by emulated bridge's rp_irq_chip). > > > > There is no 'is spite of the ABI'. If you don't understand why > > we don't break the ABI, you have an even bigger problem. > > > > So NAK to this patch, now and forever. Any change to the structure to > > make it read-only must allow the preservation of the existing names > > when they are generated by the driver. > > Marc, you already presented that you do not like Armada 3720 platform > and that you do not care about it. What I like or not is irrelevant here. What I ask for is that userspace ABIs are not broken. > But please do not slowdown development for this platform. That's quite an accusation. > Arguments about ABIs, breaking it and similar are not relevant here as > this current kernel implementation is broken. And has to be replaced by > a working one. We are doing on it for more than year. > > It really does not make sense to try doing some backward compatibility > with something which is broken by design and does not work. It just take > lot of time without any value. > > We really need to more forward and fix driver as in current state is > PCIe on Armada 3720 unusable. This patch doesn't fix anything. It has the potential to break userspace, and I'm not having any of it. You may not care about backward compatibility, but this is thankfully *not* your pet playground. You can claim that I am doing a bad job. In which case, feel free to submit a patch removing me from the MAINTAINER file, and we can have that discussion. In the meantime, I will continue to oppose these kind of patches that pretend to 'fix' things without adding any value. M.
On Mon, 10 Jan 2022 14:44:17 +0000 Marc Zyngier <maz@kernel.org> wrote: > On Mon, 10 Jan 2022 10:53:24 +0000, > Pali Rohár <pali@kernel.org> wrote: > > > > On Monday 10 January 2022 09:28:39 Marc Zyngier wrote: > > > On 2022-01-10 01:50, Marek Behún wrote: > > > > Marc Zyngier says [1] that we should use struct irq_chip as a global > > > > static struct in the driver. Even though the structure currently > > > > contains a dynamic member (parent_device), Marc says [2] that he plans > > > > to kill it and make the structure completely static. > > > > > > > > We have already converted others irq_chip structures in this driver in > > > > this way, but we omitted this one because the .name member is > > > > dynamically created from device's name, and the name is displayed in > > > > sysfs, so changing it would break sysfs ABI. > > > > > > > > The rationale for changing the name (to "advk-INT") in spite of sysfs > > > > ABI, and thus allowing to convert to a static structure, is that after > > > > the other changes we made in this series, the IRQ chip is basically > > > > something different: it no logner generates ERR and PME interrupts (they > > > > are generated by emulated bridge's rp_irq_chip). > > > > > > There is no 'is spite of the ABI'. If you don't understand why > > > we don't break the ABI, you have an even bigger problem. > > > > > > So NAK to this patch, now and forever. Any change to the structure to > > > make it read-only must allow the preservation of the existing names > > > when they are generated by the driver. > > > > Marc, you already presented that you do not like Armada 3720 platform > > and that you do not care about it. > > What I like or not is irrelevant here. What I ask for is that > userspace ABIs are not broken. > > > But please do not slowdown development for this platform. > > That's quite an accusation. > > > Arguments about ABIs, breaking it and similar are not relevant here as > > this current kernel implementation is broken. And has to be replaced by > > a working one. We are doing on it for more than year. > > > > It really does not make sense to try doing some backward compatibility > > with something which is broken by design and does not work. It just take > > lot of time without any value. > > > > We really need to more forward and fix driver as in current state is > > PCIe on Armada 3720 unusable. > > This patch doesn't fix anything. It has the potential to break > userspace, and I'm not having any of it. You may not care about > backward compatibility, but this is thankfully *not* your pet > playground. > > You can claim that I am doing a bad job. In which case, feel free to > submit a patch removing me from the MAINTAINER file, and we can have > that discussion. > > In the meantime, I will continue to oppose these kind of patches that > pretend to 'fix' things without adding any value. > > M. Dear Marc, that is why I put this patch as last patch of this series, so that it could be potentially dropped. I mostly agree with your points, Pali does not. Pali, let's not sabotage ourselves with needless arguments. Marc, Pali means well, but sometimes when he has different opinion, he can get quite argumentative. Let's ignore this patch for now. Marc, what do you think about the other patches? Did you have time to look at them? Thanks. Marek
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c index 2c5cc929b94f..087a0b22d573 100644 --- a/drivers/pci/controller/pci-aardvark.c +++ b/drivers/pci/controller/pci-aardvark.c @@ -276,7 +276,6 @@ struct advk_pcie { int irq; struct irq_domain *rp_irq_domain; struct irq_domain *irq_domain; - struct irq_chip irq_chip; raw_spinlock_t irq_lock; struct irq_domain *msi_domain; struct irq_domain *msi_inner_domain; @@ -1338,14 +1337,19 @@ static void advk_pcie_irq_unmask(struct irq_data *d) raw_spin_unlock_irqrestore(&pcie->irq_lock, flags); } +static struct irq_chip advk_irq_chip = { + .name = "advk-INT", + .irq_mask = advk_pcie_irq_mask, + .irq_unmask = advk_pcie_irq_unmask, +}; + static int advk_pcie_irq_map(struct irq_domain *h, unsigned int virq, irq_hw_number_t hwirq) { struct advk_pcie *pcie = h->host_data; irq_set_status_flags(virq, IRQ_LEVEL); - irq_set_chip_and_handler(virq, &pcie->irq_chip, - handle_level_irq); + irq_set_chip_and_handler(virq, &advk_irq_chip, handle_level_irq); irq_set_chip_data(virq, pcie); return 0; @@ -1404,7 +1408,6 @@ static int advk_pcie_init_irq_domain(struct advk_pcie *pcie) struct device *dev = &pcie->pdev->dev; struct device_node *node = dev->of_node; struct device_node *pcie_intc_node; - struct irq_chip *irq_chip; int ret = 0; raw_spin_lock_init(&pcie->irq_lock); @@ -1415,28 +1418,14 @@ static int advk_pcie_init_irq_domain(struct advk_pcie *pcie) return -ENODEV; } - irq_chip = &pcie->irq_chip; - - irq_chip->name = devm_kasprintf(dev, GFP_KERNEL, "%s-irq", - dev_name(dev)); - if (!irq_chip->name) { - ret = -ENOMEM; - goto out_put_node; - } - - irq_chip->irq_mask = advk_pcie_irq_mask; - irq_chip->irq_unmask = advk_pcie_irq_unmask; - pcie->irq_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX, &advk_pcie_irq_domain_ops, pcie); if (!pcie->irq_domain) { dev_err(dev, "Failed to get a INTx IRQ domain\n"); ret = -ENOMEM; - goto out_put_node; } -out_put_node: of_node_put(pcie_intc_node); return ret; }
Marc Zyngier says [1] that we should use struct irq_chip as a global static struct in the driver. Even though the structure currently contains a dynamic member (parent_device), Marc says [2] that he plans to kill it and make the structure completely static. We have already converted others irq_chip structures in this driver in this way, but we omitted this one because the .name member is dynamically created from device's name, and the name is displayed in sysfs, so changing it would break sysfs ABI. The rationale for changing the name (to "advk-INT") in spite of sysfs ABI, and thus allowing to convert to a static structure, is that after the other changes we made in this series, the IRQ chip is basically something different: it no logner generates ERR and PME interrupts (they are generated by emulated bridge's rp_irq_chip). [1] https://lore.kernel.org/linux-pci/877dbcvngf.wl-maz@kernel.org/ [2] https://lore.kernel.org/linux-pci/874k6gvkhz.wl-maz@kernel.org/ Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/pci/controller/pci-aardvark.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-)