linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Rob Herring <robh@kernel.org>
To: "Pali Rohár" <pali@kernel.org>
Cc: "Marc Zyngier" <maz@kernel.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Andrew Lunn" <andrew@lunn.ch>,
	"Gregory Clement" <gregory.clement@bootlin.com>,
	"Sebastian Hesselbarth" <sebastian.hesselbarth@gmail.com>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Marek Behún" <kabel@kernel.org>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/6] irqchip/armada-370-xp: Implement SoC Error interrupts
Date: Mon, 9 May 2022 18:12:25 -0500	[thread overview]
Message-ID: <Ynmf2SHN90yvsOmP@robh.at.kernel.org> (raw)
In-Reply-To: <20220507092054.b7yu23nj667l6xhy@pali>

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@kernel.org> 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@kernel.org> 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@kernel.org> 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@bootlin.com> (maintainer:PCI DRIVER FOR AARDVARK (Marvell Armada 3700))
> "Pali Rohár" <pali@kernel.org> (maintainer:PCI DRIVER FOR AARDVARK (Marvell Armada 3700))
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> (supporter:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS)
> Rob Herring <robh@kernel.org> (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

  parent reply	other threads:[~2022-05-09 23:12 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-06 13:40 [PATCH 0/6] PCI: mvebu: Add support for PME and AER interrupts Pali Rohár
2022-05-06 13:40 ` [PATCH 1/6] dt-bindings: irqchip: armada-370-xp: Update information about MPIC SoC Error Pali Rohár
2022-05-17  0:18   ` Rob Herring
2022-05-06 13:40 ` [PATCH 2/6] irqchip/armada-370-xp: Implement SoC Error interrupts Pali Rohár
2022-05-06 18:19   ` Marc Zyngier
2022-05-06 18:30     ` Pali Rohár
2022-05-06 18:47       ` Marc Zyngier
2022-05-06 18:55         ` Pali Rohár
2022-05-07  9:01           ` Marc Zyngier
2022-05-07  9:20             ` Pali Rohár
2022-05-07  9:42               ` Marc Zyngier
2022-05-07 11:15                 ` Pali Rohár
2022-05-09 23:12               ` Rob Herring [this message]
2022-05-09  8:51           ` Thomas Gleixner
2022-05-06 13:40 ` [PATCH 3/6] ARM: dts: armada-38x.dtsi: Add node for MPIC SoC Error IRQ controller Pali Rohár
2022-05-06 13:40 ` [PATCH 4/6] dt-bindings: PCI: mvebu: Update information about summary interrupt Pali Rohár
2022-05-06 13:40 ` [PATCH 5/6] PCI: mvebu: Implement support for interrupts on emulated bridge Pali Rohár
2022-05-06 13:40 ` [PATCH 6/6] ARM: dts: armada-385.dtsi: Add definitions for PCIe summary interrupts Pali Rohár
2022-05-06 14:22 ` [PATCH 0/6] PCI: mvebu: Add support for PME and AER interrupts Pali Rohár

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Ynmf2SHN90yvsOmP@robh.at.kernel.org \
    --to=robh@kernel.org \
    --cc=andrew@lunn.ch \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=gregory.clement@bootlin.com \
    --cc=kabel@kernel.org \
    --cc=kw@linux.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=maz@kernel.org \
    --cc=pali@kernel.org \
    --cc=sebastian.hesselbarth@gmail.com \
    --cc=tglx@linutronix.de \
    --cc=thomas.petazzoni@bootlin.com \
    --subject='Re: [PATCH 2/6] irqchip/armada-370-xp: Implement SoC Error interrupts' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).