Re: [PATCH v6 05/14] ACPI: platform-msi: retrieve dev id from IORT

[Lorenzo Pieralisi]

On Thu, Jan 05, 2017 at 08:45:37PM +0800, Hanjun Guo wrote:
> Hi Lorenzo,
> 
> On 2017/1/5 3:18, Lorenzo Pieralisi wrote:
> >On Mon, Jan 02, 2017 at 09:31:36PM +0800, Hanjun Guo wrote:
> >>For devices connecting to ITS, it needs dev id to identify
> >>itself, and this dev id is represented in the IORT table in
> >>named componant node [1] for platform devices, so in this
> >>patch we will scan the IORT to retrieve device's dev id.
> >>
> >>Introduce iort_pmsi_get_dev_id() with pointer dev passed
> >>in for that purpose.
> >>
> >>[1]: https://static.docs.arm.com/den0049/b/DEN0049B_IO_Remapping_Table.pdf
> >>
> >>Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> >>Tested-by: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
> >>Tested-by: Majun <majun258@xxxxxxxxxx>
> >>Tested-by: Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx>
> >>Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> >>Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> >>Cc: Tomasz Nowicki <tn@xxxxxxxxxxxx>
> >>Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> >>---
> >> drivers/acpi/arm64/iort.c                     | 26 ++++++++++++++++++++++++++
> >> drivers/irqchip/irq-gic-v3-its-platform-msi.c |  4 +++-
> >> include/linux/acpi_iort.h                     |  8 ++++++++
> >> 3 files changed, 37 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >>index 174e983..ab7bae7 100644
> >>--- a/drivers/acpi/arm64/iort.c
> >>+++ b/drivers/acpi/arm64/iort.c
> >>@@ -444,6 +444,32 @@ u32 iort_msi_map_rid(struct device *dev, u32 req_id)
> >> }
> >>
> >> /**
> >>+ * iort_pmsi_get_dev_id() - Get the device id for a device
> >>+ * @dev: The device for which the mapping is to be done.
> >>+ * @dev_id: The device ID found.
> >>+ *
> >>+ * Returns: 0 for successful find a dev id, errors otherwise
> >>+ */
> >>+int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
> >>+{
> >>+	struct acpi_iort_node *node;
> >>+
> >>+	if (!iort_table)
> >>+		return -ENODEV;
> >>+
> >>+	node = iort_find_dev_node(dev);
> >>+	if (!node) {
> >>+		dev_err(dev, "can't find related IORT node\n");
> >>+		return -ENODEV;
> >>+	}
> >>+
> >>+	if(!iort_node_get_id(node, dev_id, IORT_MSI_TYPE, 0))
> >
> >I disagree with this approach. For named components we know that
> >there are always two steps involved (second optional):
> >
> >(1) Retrieve the initial id (this may well provide the final mapping)
> >(2) Map the id (optional if (1) represents the map type we need)
> >
> >That's the reason why I kept iort_node_get_id() and iort_node_map_rid()
> >separated.
> >
> >Now, what we can do is to create an iort_node_map_id() function that is
> >PCI agnostic (ie rename rid to id :)), whose rid_in is either a PCI RID
> >or the outcome of a previous call to iort_node_get_id() for named
> >components, that's in my opinion cleaner.
> 
> iort_node_map_rid() was designed for that purpose, and we can use it
> for platform device, the issue that we need to pass a req id
> unconditionally which is not needed for platform device, Tomasz
> proposed a similar solution to rework iort_node_map_rid(), and
> I think it makes sense.
> 
> >
> >It would be even cleaner if you passed a type_mask (or write a
> >wrapper function for that) that is:
> >
> >(IORT_MSI_TYPE | IORT_IOMMU_TYPE)
> 
> Sorry, I got little lost here, could you explain it in detail?

Yes sorry I was not clear. What I wanted to say is, for named
components, that do not have an intrinsic id, we have to call
iort_node_get_id() regardless of the type mask, we have to have
a way to get the "source/initial id", so basically the type_mask
is not important at all, it becomes important when it comes to
understanding what type of id the value returned from
iort_node_get_id() is.

So basically, passing:

#define IORT_TYPE_ANY (IORT_MSI_TYPE | IORT_IOMMU_TYPE)

as type_mask to iort_node_get_id() means "retrieve any kind of
initial id", that's what I wanted to say.

In iort_iommu_configure() iort_node_get_id() is a bit different because
we want only a type of id, ie a streamid, therefore the mask that we
pass in is IORT_IOMMU_TYPE.

> >and we just use the returned parent pointer to check if the mapping
> >providing the initial id correspond to the type we are looking for (eg
> >ITS) or we need to map the retrieved initial id any further, with
> >iort_node_map_id(), to get to the final identifier.
> >
> >Thoughts ?
> 
> I think rework iort_node_map_rid() and not extend iort_node_get_id()
> is the right direction, could you explain a bit more then I can demo
> the code?

What you can do is create a wrapper, say iort_node_map_platform_id()
(whose signature is equivalent to iort_node_map_rid() minus rid_in)
that carries out the two steps outlined above.

To do that I suggest the following:

(1) I send a patch to "fix" iort_node_get_id() (ie index issue you
    reported)
(2) We remove type_mask handling from iort_node_get_id()
(3) We create iort_node_map_platform_id() that (pseudo-code, I can
    write the patch if it is clearer):

struct acpi_iort_node *iort_node_map_platform_id(u8 type_mask, int index,
						 ...)
{
	u32 id, id_out;
	struct acpi_iort_node *parent = iort_node_get_id(&id, index);

	if (!parent)
		return NULL;

	/* we should probably rename iort_node_map_rid() too */
	if (!(IORT_TYPE_MASK(parent->type) & type_mask)
		parent = iort_node_map_rid(parent, id, &id_out, type_mask);

	return parent;
}

(4) we update current iort_node_get_id() users and move them over
    to iort_node_map_platform_id()

Let me know if that's clear so that we can agree on a way forward.

Thanks,
Lorenzo