Hi Lorenzo, On 2017/1/13 20:11, Lorenzo Pieralisi wrote: > On Wed, Jan 11, 2017 at 11:06:33PM +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 component node >> [1] for platform devices, so in this patch we will scan the IORT to >> retrieve device's dev id. >> >> 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), this >> is needed for use cases such as NC (named component) -> SMMU -> ITS >> mappings. >> >> we have API iort_node_get_id() for step (1) above and >> iort_node_map_rid() for step (2), so create a wrapper >> iort_node_map_platform_id() to retrieve the dev id. >> >> [1]: https://static.docs.arm.com/den0049/b/DEN0049B_IO_Remapping_Table.pdf > This patch should be split and IORT changes should be squashed with > patch 10. If split the changes for IORT and its platform msi, API introduced in IORT will not be used in a single patch, seems violate the suggestion of "new introduced API needs to be used in the same patch", did I miss something? > >> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> >> Suggested-by: Tomasz Nowicki <tn@xxxxxxxxxxxx> >> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx> >> Cc: Marc Zyngier <marc.zyngier@xxxxxxx> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> >> Cc: Sinan Kaya <okaya@xxxxxxxxxxxxxx> >> Cc: Tomasz Nowicki <tn@xxxxxxxxxxxx> >> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx> >> --- >> drivers/acpi/arm64/iort.c | 56 +++++++++++++++++++++++++++ >> drivers/irqchip/irq-gic-v3-its-platform-msi.c | 4 +- >> include/linux/acpi_iort.h | 8 ++++ >> 3 files changed, 67 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >> index 069a690..95fd20b 100644 >> --- a/drivers/acpi/arm64/iort.c >> +++ b/drivers/acpi/arm64/iort.c >> @@ -30,6 +30,7 @@ >> #define IORT_MSI_TYPE (1 << ACPI_IORT_NODE_ITS_GROUP) >> #define IORT_IOMMU_TYPE ((1 << ACPI_IORT_NODE_SMMU) | \ >> (1 << ACPI_IORT_NODE_SMMU_V3)) >> +#define IORT_TYPE_ANY (IORT_MSI_TYPE | IORT_IOMMU_TYPE) >> >> struct iort_its_msi_chip { >> struct list_head list; >> @@ -406,6 +407,34 @@ static struct acpi_iort_node *iort_node_map_id(struct acpi_iort_node *node, >> return NULL; >> } >> >> +static >> +struct acpi_iort_node *iort_node_map_platform_id(struct acpi_iort_node *node, >> + u32 *id_out, u8 type_mask, >> + int index) >> +{ >> + struct acpi_iort_node *parent; >> + u32 id; >> + >> + /* step 1: retrieve the initial dev id */ >> + parent = iort_node_get_id(node, &id, IORT_TYPE_ANY, index); >> + if (!parent) >> + return NULL; >> + >> + /* >> + * optional step 2: map the initial dev id if its parent is not >> + * the target type we wanted, map it again for the use cases such >> + * as NC (named component) -> SMMU -> ITS. If the type is matched, >> + * return the parent pointer directly. >> + */ >> + if (!(IORT_TYPE_MASK(parent->type) & type_mask)) >> + parent = iort_node_map_id(parent, id, id_out, type_mask); >> + else >> + if (id_out) > Remove this pointer check. This was added because of NULL pointer reference, I passed NULL for id_out because I only want to get its parent node, I think we have four options: - Introduce a new API to get the parent only from the scratch, but it will duplicate the code a lot; - Don't check the id_out in iort_node_map_platform_id(), and introduce a wrapper and pass the dummy id for iort_node_map_platform_id() : static struct acpi_iort_node *iort_node_get_platform_parent{struct device *dev, u8 type_mask} { struct acpi_iort_node *node, *parent = NULL; int i; u32 dummy_id; node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT, iort_match_node_callback, dev); if (!node) return NULL; for (i = 0; i < node->mapping_count; i++) { /* we just want to get the parent node */ parent = iort_node_map_platform_id(node, &dummy_id, IORT_MSI_TYPE, i); if (parent) break; } return parent; } - Similar solution as above but don't introduce wrapper, just use dummy_id if iort_node_map_platform_id() is called; - Use the solution I proposed in this patch. Please share you suggestion on this :) > >> + *id_out = id; >> + >> + return parent; >> +} >> + >> static struct acpi_iort_node *iort_find_dev_node(struct device *dev) >> { >> struct pci_bus *pbus; >> @@ -444,6 +473,33 @@ 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 > Nit: -ENODEV on error > >> + */ >> +int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id) >> +{ >> + int i; >> + struct acpi_iort_node *node; >> + >> + if (!iort_table) >> + return -ENODEV; > I do not think this iort_table check is needed. Agreed, it will be checked in iort_scan_node() and it's called in iort_find_dev_node(). > >> + node = iort_find_dev_node(dev); >> + if (!node) >> + return -ENODEV; >> + >> + for (i = 0; i < node->mapping_count; i++) { >> + if(iort_node_map_platform_id(node, dev_id, IORT_MSI_TYPE, i)) > ^ > > Nit: Missing a space. I was on a flight when updating the patches, seems it's not a good place for coding :) I will update the patch set when you are ok with the solutions I proposed, thank you very much for the review. Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
