| Message ID | 20210506153153.30454-16-pali@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: aardvark: Various driver fixes | expand |
On Thu, 06 May 2021 16:31:26 +0100, Pali Rohár <pali@kernel.org> wrote: > > This name is visible in /proc/interrupts file and for better reading it > should have at most 8 characters. Also there is no need to allocate this > name dynamically, since there is only one PCIe controller on Armada 37xx. > This aligns with how the MSI irq_chip in this driver names it's interrupt > ("advk-MSI"). And *because* the name is visible in /proc/interrupts, it has become an ABI, and cannot be changed anymore. We had the exact same issue with Tegra this merge window as I accidentally changed "Tegra" to "tegra", resulting in userspace programs failing find stuff in /proc/interrupts. Please keep the name as is, no matter how ugly it is. Thanks, M.
On Fri, 07 May 2021 10:08:18 +0100 Marc Zyngier <maz@kernel.org> wrote: > On Thu, 06 May 2021 16:31:26 +0100, > Pali Rohár <pali@kernel.org> wrote: > > > > This name is visible in /proc/interrupts file and for better reading it > > should have at most 8 characters. Also there is no need to allocate this > > name dynamically, since there is only one PCIe controller on Armada 37xx. > > This aligns with how the MSI irq_chip in this driver names it's interrupt > > ("advk-MSI"). > > And *because* the name is visible in /proc/interrupts, it has become > an ABI, and cannot be changed anymore. > > We had the exact same issue with Tegra this merge window as I > accidentally changed "Tegra" to "tegra", resulting in userspace > programs failing find stuff in /proc/interrupts. > > Please keep the name as is, no matter how ugly it is. Hmm, I am 99% sure that for the A3720 platform this ABI change would not affect anybody. And it does make the driver's irq names confusing. Can't we really do anything here? Note that there were suggestions from some people to completely remove this driver due to the many problems it has which Pali is trying to solve. But if the driver was removed and then later introduced again without these problems, the new version would use the "advk-INT" IRQ name... Marek
On Mon, 24 May 2021 15:36:51 +0100, Marek Behún <kabel@kernel.org> wrote: > > On Fri, 07 May 2021 10:08:18 +0100 > Marc Zyngier <maz@kernel.org> wrote: > > > On Thu, 06 May 2021 16:31:26 +0100, > > Pali Rohár <pali@kernel.org> wrote: > > > > > > This name is visible in /proc/interrupts file and for better reading it > > > should have at most 8 characters. Also there is no need to allocate this > > > name dynamically, since there is only one PCIe controller on Armada 37xx. > > > This aligns with how the MSI irq_chip in this driver names it's interrupt > > > ("advk-MSI"). > > > > And *because* the name is visible in /proc/interrupts, it has become > > an ABI, and cannot be changed anymore. > > > > We had the exact same issue with Tegra this merge window as I > > accidentally changed "Tegra" to "tegra", resulting in userspace > > programs failing find stuff in /proc/interrupts. > > > > Please keep the name as is, no matter how ugly it is. > > Hmm, I am 99% sure that for the A3720 platform this ABI change would not > affect anybody. And it does make the driver's irq names confusing. > Can't we really do anything here? No, this is final. Show anything in /proc/*, maintain it forever. We already went there with the bogomips crap showing up in /proc/cpuinfo. There is no way you can know what userspace does, and the best course of action is not to change things for some dubious value of "nicer" or "less confusing". > Note that there were suggestions from some people to completely remove > this driver due to the many problems it has which Pali is trying to > solve. But if the driver was removed and then later introduced again > without these problems, the new version would use the "advk-INT" IRQ > name... No, you would have to keep the *exact same output*. Userspace doesn't know about drivers, and expect things in /proc to be stable. Frankly, there are more important things to do than to worry about the shape of /proc/interrupts. And if we could change it, I'd simply get rid of it (you really should look at it on a system that has ~200 CPUs...). Thanks, M.
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c index 08f1157e1c5e..c50421af9d06 100644 --- a/drivers/pci/controller/pci-aardvark.c +++ b/drivers/pci/controller/pci-aardvark.c @@ -1022,14 +1022,7 @@ static int advk_pcie_init_irq_domain(struct advk_pcie *pcie) } 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->name = "advk-INT"; irq_chip->irq_mask = advk_pcie_irq_mask; irq_chip->irq_unmask = advk_pcie_irq_unmask;
This name is visible in /proc/interrupts file and for better reading it should have at most 8 characters. Also there is no need to allocate this name dynamically, since there is only one PCIe controller on Armada 37xx. This aligns with how the MSI irq_chip in this driver names it's interrupt ("advk-MSI"). Signed-off-by: Pali Rohár <pali@kernel.org> Reviewed-by: Marek Behún <kabel@kernel.org> --- drivers/pci/controller/pci-aardvark.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)