[PATCH v7 09/15] ACPI: platform-msi: retrieve dev id from IORT

[guohanjun@huawei.com (Hanjun Guo)]

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@arm.com>
>> Suggested-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Sinan Kaya <okaya@codeaurora.org>
>> Cc: Tomasz Nowicki <tn@semihalf.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  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