On Sat, May 07, 2022 at 11:20:54AM +0200, Pali Rohár wrote:
> On Saturday 07 May 2022 10:01:52 Marc Zyngier wrote:
> > On Fri, 06 May 2022 19:55:46 +0100,
> > Pali Rohár <pali@xxxxxxxxxx> wrote:
> > >
> > > On Friday 06 May 2022 19:47:25 Marc Zyngier wrote:
> > > > On Fri, 06 May 2022 19:30:51 +0100,
> > > > Pali Rohár <pali@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Friday 06 May 2022 19:19:46 Marc Zyngier wrote:
> > > > > > On Fri, 06 May 2022 14:40:25 +0100,
> > > > > > Pali Rohár <pali@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > +static void armada_370_xp_soc_err_irq_unmask(struct irq_data *d);
> > > > > > > +
> > > > > > > static inline bool is_percpu_irq(irq_hw_number_t irq)
> > > > > > > {
> > > > > > > if (irq <= ARMADA_370_XP_MAX_PER_CPU_IRQS)
> > > > > > > @@ -509,6 +517,27 @@ static void armada_xp_mpic_reenable_percpu(void)
> > > > > > > armada_370_xp_irq_unmask(data);
> > > > > > > }
> > > > > > >
> > > > > > > + /* Re-enable per-CPU SoC Error interrupts that were enabled before suspend */
> > > > > > > + for (irq = 0; irq < soc_err_irq_num_regs * 32; irq++) {
> > > > > > > + struct irq_data *data;
> > > > > > > + int virq;
> > > > > > > +
> > > > > > > + virq = irq_linear_revmap(armada_370_xp_soc_err_domain, irq);
> > > > > > > + if (virq == 0)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + data = irq_get_irq_data(virq);
> > > > > > > +
> > > > > > > + if (!irq_percpu_is_enabled(virq))
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + armada_370_xp_soc_err_irq_unmask(data);
> > > > > > > + }
> > > > > >
> > > > > > So you do this loop and all these lookups, both here and in the resume
> > > > > > function (duplicated code!) just to be able to call the unmask
> > > > > > function? This would be better served by two straight writes of the
> > > > > > mask register, which you'd conveniently save on suspend.
> > > > > >
> > > > > > Yes, you have only duplicated the existing logic. But surely there is
> > > > > > something better to do.
> > > > >
> > > > > Yes, I just used existing logic.
> > > > >
> > > > > I'm not rewriting driver or doing big refactor of it, as this is not in
> > > > > the scope of the PCIe AER interrupt support.
> > > >
> > > > Fair enough. By the same logic, I'm not taking any change to the
> > > > driver until it is put in a better shape. Your call.
> > >
> > > If you are maintainer of this code then it is expected from _you_ to
> > > move the current code into _better shape_ as you wrote and expect. And
> > > then show us exactly, how new changes in this driver should look like,
> > > in examples.
> >
> > Sorry, but that's not how this works. You are the one willing to
> > change a sub-par piece of code, you get to make it better. You
> > obviously have the means (the HW) and the incentive (these patches).
> > But you don't get to make something even more unmaintainable because
> > you're unwilling to do some extra work.
> >
> > If you're unhappy with my position, that's fine. I suggest you take it
> > with Thomas, and maybe even Linus. As I suggested before, you can also
> > post a patch removing me as the irqchip maintainer. I'm sure that will
> > spark an interesting discussion.
>
> You have already suggested it in email [1] but apparently you are _not_
> maintainer of mvebu pci controller. get_maintainer.pl for part about
> which you have talked in [1] says:
>
> $ ./scripts/get_maintainer.pl -f drivers/pci/controller/pci-aardvark.c
> Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx> (maintainer:PCI DRIVER FOR AARDVARK (Marvell Armada 3700))
> "Pali Rohár" <pali@xxxxxxxxxx> (maintainer:PCI DRIVER FOR AARDVARK (Marvell Armada 3700))
> Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> (supporter:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS)
> Rob Herring <robh@xxxxxxxxxx> (reviewer:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS)
Please just refactor the code in question. You've wasted more time
arguing about it than it would take to do. Having done a bit of PCI
refactoring, I can tell you hardly anyone else does. I can barely even
get comments/acks on refactoring until I break platforms (which happens
a lot). Maintainers have no other leverage other than what Marc pointed
out.
In any case, I think there's no way the PCI maintainers will take this
as-is at this point.
Rob
