All of lore.kernel.org
 help / color / mirror / Atom feed
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>,
	Marc Zyngier <marc.zyngier@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment
Date: Thu, 13 Jun 2019 12:03:04 +1000	[thread overview]
Message-ID: <3093d174ddd183fe5b6e949a62a15e72aa373e26.camel@kernel.crashing.org> (raw)
In-Reply-To: <e4c7b434452775d00b6621012ad5e263076b3fcf.camel@kernel.crashing.org>

On Wed, 2019-06-12 at 15:16 +1000, Benjamin Herrenschmidt wrote:
> pci_msi_create_irq_domain -> pci_msi_domain_update_chip_ops will
> set those two already since the driver sets MSI_FLAG_USE_DEF_CHIP_OPS
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> [UNTESTED]
> 
> Just something I noticed while browsing through those drivers in
> search of ways to factor some of the code.
> 
> That leads to a question here:
> 
> Some MSI drivers such as this one (or any using the defaults
> mask/unmask
> provided by drivers/pci/msi.c) only call the PCI MSI mask/unmask
> functions.
> 
> Some other drivers call those PCI function but *also* call the parent
> mask/unmask (giv-v2m for example) which generally is the inner domain
> which just itself forwards to its own parent.

  .../...

So I looked at x86 and it also only uses pci_msi_unmask_irq, it doesn't
mask at the parent level. And it also specifies those explicitly which
isn't necessary so the same trivial cleanup patch could be done (happy
to do it unless I missed something here).

Question: If that's indeed the rule we want to establish, should we
consider making all MSI controllers just use the PCI masking and remove
the forwarding to the parent ?

The ones that do the parent, at least in drivers/irqchip/* and
drivers/pci/controller/* (ther are more in arch code) are all the GIC
ones (v2m, v3-its, v3-mbi), alpine which was copied on GIC I think,
tango and dwc.

The other approach would be to make the generic ops setup by
pci_msi_domain_update_chip_ops call the parent as well .. if there is
one and it has corresponding mask/unmask callbacks. That means things
like armada_370 would be unaffected since their "middle" irqdomain chip
doesn't have them, at least until somebody decides that masking at the
parent level as well is a good thing. I *think* it would also work for
x86 since the parent in that case is x86_vector_domain which also
doesn't have mask and unmask callbacks, so it would be a nop change.

Let me know what you think.

Cheers,
Ben.




WARNING: multiple messages have this Message-ID (diff)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>,
	Marc Zyngier <marc.zyngier@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment
Date: Thu, 13 Jun 2019 12:03:04 +1000	[thread overview]
Message-ID: <3093d174ddd183fe5b6e949a62a15e72aa373e26.camel@kernel.crashing.org> (raw)
In-Reply-To: <e4c7b434452775d00b6621012ad5e263076b3fcf.camel@kernel.crashing.org>

On Wed, 2019-06-12 at 15:16 +1000, Benjamin Herrenschmidt wrote:
> pci_msi_create_irq_domain -> pci_msi_domain_update_chip_ops will
> set those two already since the driver sets MSI_FLAG_USE_DEF_CHIP_OPS
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> [UNTESTED]
> 
> Just something I noticed while browsing through those drivers in
> search of ways to factor some of the code.
> 
> That leads to a question here:
> 
> Some MSI drivers such as this one (or any using the defaults
> mask/unmask
> provided by drivers/pci/msi.c) only call the PCI MSI mask/unmask
> functions.
> 
> Some other drivers call those PCI function but *also* call the parent
> mask/unmask (giv-v2m for example) which generally is the inner domain
> which just itself forwards to its own parent.

  .../...

So I looked at x86 and it also only uses pci_msi_unmask_irq, it doesn't
mask at the parent level. And it also specifies those explicitly which
isn't necessary so the same trivial cleanup patch could be done (happy
to do it unless I missed something here).

Question: If that's indeed the rule we want to establish, should we
consider making all MSI controllers just use the PCI masking and remove
the forwarding to the parent ?

The ones that do the parent, at least in drivers/irqchip/* and
drivers/pci/controller/* (ther are more in arch code) are all the GIC
ones (v2m, v3-its, v3-mbi), alpine which was copied on GIC I think,
tango and dwc.

The other approach would be to make the generic ops setup by
pci_msi_domain_update_chip_ops call the parent as well .. if there is
one and it has corresponding mask/unmask callbacks. That means things
like armada_370 would be unaffected since their "middle" irqdomain chip
doesn't have them, at least until somebody decides that masking at the
parent level as well is a good thing. I *think* it would also work for
x86 since the parent in that case is x86_vector_domain which also
doesn't have mask and unmask callbacks, so it would be a nop change.

Let me know what you think.

Cheers,
Ben.




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2019-06-13 16:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-12  5:16 [PATCH+DISCUSSION] irqchip: armada-370-xp: Remove redundant ops assignment Benjamin Herrenschmidt
2019-06-12  5:16 ` Benjamin Herrenschmidt
2019-06-13  2:03 ` Benjamin Herrenschmidt [this message]
2019-06-13  2:03   ` Benjamin Herrenschmidt
2019-06-13  9:22 ` Marc Zyngier
2019-06-13  9:22   ` Marc Zyngier
2019-06-13 10:56   ` Benjamin Herrenschmidt
2019-06-13 10:56     ` Benjamin Herrenschmidt

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=3093d174ddd183fe5b6e949a62a15e72aa373e26.camel@kernel.crashing.org \
    --to=benh@kernel.crashing.org \
    --cc=gregory.clement@free-electrons.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=thomas.petazzoni@free-electrons.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.