Re: [PATCH 2/2] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Marc Zyngier <marc.zyngier@arm.com>]

On Fri, 18 Sep 2015 10:54:02 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 09/18/2015 01:51 AM, Marc Zyngier wrote:
> > On Thu, 17 Sep 2015 11:00:59 -0700
> > David Daney <ddaney.cavm@gmail.com> wrote:
> >
> > Hi David,
> >
> >> From: David Daney <david.daney@cavium.com>
> >>
> >> Search up the device hierarchy to find devices with a "msi-map"
> >> property, if found apply the mapping to the GIC device id.
> >>
> >> Signed-off-by: David Daney <david.daney@cavium.com>
> >> ---
> >>   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 73 ++++++++++++++++++++++++++++++++
> >>   1 file changed, 73 insertions(+)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> index cf351c6..aa61cef 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> >> @@ -73,6 +73,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	struct pci_dev *pdev;
> >>   	struct its_pci_alias dev_alias;
> >>   	struct msi_domain_info *msi_info;
> >> +	struct device *parent_dev;
> >> +	struct device_node *msi_controller_node = NULL;
> >>
> >>   	if (!dev_is_pci(dev))
> >>   		return -EINVAL;
> >> @@ -84,6 +86,77 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> >>   	dev_alias.count = nvec;
> >>
> >>   	pci_for_each_dma_alias(pdev, its_get_pci_alias, &dev_alias);
> >> +	/*
> >> +	 * Walk up the device parent links looking for one with a
> >> +	 * "msi-map" property.
> >> +	 */
> >
> > My first objection is the location of this parsing. It shouldn't be
> > driver specific, but instead be part of the generic OF handling
> > (nothing in these properties is GICv3 specific, even if the ITS is the
> > only user so far).
> 
> OK, I agree that this should eventually end up in generic OF handling 
> code.  I just wanted to get something out to initiate discussion.
> 
> The next patch revision will move this to a more generic home.
> 
> >
> >> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
> >
> > Is there a limit how far we should go up the parent chain to find a
> > msi-map? My hunch is that you should stop at the first device that does
> > have an of_node, as it is the one that should contain the msi-map
> > property.
> 
> I think there is the possibility of finding something like a bridge that 
> has an of_node, but does not have the "msi-map" property.  I currently 
> have exactly this configuration, as some of the on-SoC devices sit 
> behind a bridge, but need an of_node to obtain unprobable properties and 
> children (the MDIO bus devices are like this).
> 
> So if we want to abort the walk early, we should at least go up until we 
> find "msi-map" in the of_node.

I don't really see a case where we would traverse a series of nodes
where the msi-map property wouldn't be in the first node. Could you
please give me an example?

[...]

> >> +		msi_controller_node = of_find_node_by_phandle(phandle);
> >> +		if (domain->of_node != msi_controller_node) {
> >> +			dev_err(dev,
> >> +				"ERROR: msi-map mismatch \"%s\" vs. \"%s\"\n",
> >> +				domain->of_node->full_name,
> >> +				msi_controller_node ? NULL : msi_controller_node->full_name);
> >
> > Why is that an error? a RC can be configured to master multiple
> > MSI-controllers,
> 
> Something has already associated the PCI device with this 
> MSI-controller.  Therefore I think the reference in the map must refer 
> to this ITS MSI-controller instance.
> 
> 
> > and the kernel picks one of them for a given device.
> > This is illustrated by "Example (5)" in the binding, where a device can
> > master two MSI controllers.
> 
> The PCI host may have many MSI controllers, but I think a given PCI 
> device will have only one (based on bus:devfn) that is looked up in the map.

A PCI device will only be configured to talk to a single MSI
controller, but here you stop parsing the msi-map on the first match,
and assume that you must have found the right MSI controller:

I think this should read:

+			if (masked_devid < rid_base ||
+			    masked_devid >= rid_base + rid_len ||
			    domain->of_node != of_find_node_by_phandle(phandle)) {
+				msi_map_len -= 4 * sizeof(__be32);
+				msi_map += 4;
+				continue;
+			}
+			matched = true;
+			break;

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.