Re: [PATCH] PCI: mvebu: Use devm_request_irq() for registering interrupt handler

[Lorenzo Pieralisi]

On Sun, Sep 11, 2022 at 05:45:16PM +0200, Pali Rohár wrote:
> On Monday 29 August 2022 18:51:09 Pali Rohár wrote:
> > On Sunday 10 July 2022 02:06:59 Pali Rohár wrote:
> > > On Saturday 09 July 2022 18:44:30 Bjorn Helgaas wrote:
> > > > [+cc Marc, since he commented on this]
> > > > 
> > > > On Sat, Jul 09, 2022 at 04:31:51PM +0200, Pali Rohár wrote:
> > > > > On Friday 01 July 2022 16:29:41 Pali Rohár wrote:
> > > > > > On Thursday 23 June 2022 11:27:47 Bjorn Helgaas wrote:
> > > > > > > On Tue, May 24, 2022 at 02:28:17PM +0200, Pali Rohár wrote:
> > > > > > > > Same as in commit a3b69dd0ad62 ("Revert "PCI: aardvark: Rewrite IRQ code to
> > > > > > > > chained IRQ handler"") for pci-aardvark driver, use devm_request_irq()
> > > > > > > > instead of chained IRQ handler in pci-mvebu.c driver.
> > > > > > > >
> > > > > > > > This change fixes affinity support and allows to pin interrupts from
> > > > > > > > different PCIe controllers to different CPU cores.
> > > > > > > 
> > > > > > > Several other drivers use irq_set_chained_handler_and_data().  Do any
> > > > > > > of them need similar changes?  The commit log suggests that using
> > > > > > > chained IRQ handlers breaks affinity support.  But perhaps that's not
> > > > > > > the case and the real culprit is some other difference between mvebu
> > > > > > > and the other drivers.
> > > > > > 
> > > > > > And there is another reason to not use irq_set_chained_handler_and_data
> > > > > > and instead use devm_request_irq(). Armada XP has some interrupts
> > > > > > shared and it looks like that irq_set_chained_handler_and_data() API
> > > > > > does not handle shared interrupt sources too.
> > > > > > 
> > > > > > I can update commit message to mention also this fact.
> > > > > 
> > > > > Anything needed from me to improve this fix?
> > > > 
> > > > My impression from Marc's response [1] was that this patch would
> > > > "break the contract the kernel has with userspace" and he didn't think
> > > > this was acceptable.  But maybe I'm not understanding it correctly.
> > > 
> > > This is argument which Marc use when he does not have any argument.
> > > 
> > > Support for dedicated INTx into pci-mvebu.c was introduced just recently
> > > and I used irq_set_chained_handler_and_data() just because I thought it
> > > is a good idea and did not know about all those issues with it. So there
> > > cannot be any breakage by this patch.
> > > 
> > > I already converted other pci-aardvark.c driver to use
> > > irq_set_chained_handler_and_data() API because wanted it... But at the
> > > end _that conversion_ caused breakage of afinity support and so this
> > > conversion had to be reverted:
> > > https://lore.kernel.org/linux-pci/20220515125815.30157-1-pali@xxxxxxxxxx/#t
> > > 
> > > Based on his past decisions, above suggestions which cause _real_
> > > breakage and his expressions like mvebu should be put into the trash,
> > > I'm not going to listen him anymore. The only breaking is done by him.
> > > 
> > > 
> > > There are two arguments why to not use irq_set_chained_handler_and_data:
> > > 
> > > 1) It does not support afinity and therefore has negative performance
> > >    impact on Armada platforms with more CPUs and more PCIe ports.
> > > 
> > > 2) It does not support shared interrupts and therefore it will break
> > >    hardware on which interrupt lines are shares (mostly Armada XP).
> > > 
> > > So these issues have to be fixed and currently I see only option to
> > > switch irq_set_chained_handler_and_data() to devm_request_irq() which I
> > > did in this fixup patch.
> > 
> > Any progress here? This patch is waiting here since end of May and if
> > something is going to be broken then it is this fact of ignoring reported
> > issues and proposed patch. Do you better solution how to fix commit
> > ec075262648f?
> 
> After two weeks I'm reminding this fix patch again...

There is no point complaining about something you were asked
to change, really - there is not.

You were given feedback, feel free to ignore it, it won't help
getting this patch upstream - it is as simple as that, sorry.

Thanks,
Lorenzo