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

Commit Message

Hanjun Guo Jan. 11, 2017, 3:06 p.m. UTC

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

Suggested-by: Lorenzo Pieralisi <[e-mail]>
Suggested-by: Tomasz Nowicki <[e-mail]>
Signed-off-by: Hanjun Guo <[e-mail]>
Cc: Marc Zyngier <[e-mail]>
Cc: Lorenzo Pieralisi <[e-mail]>
Cc: Sinan Kaya <[e-mail]>
Cc: Tomasz Nowicki <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
---
 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(-)

Comments

Lorenzo Pieralisi Jan. 13, 2017, 12:11 p.m. UTC | #1

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.

> Suggested-by: Lorenzo Pieralisi <[e-mail]>
> Suggested-by: Tomasz Nowicki <[e-mail]>
> Signed-off-by: Hanjun Guo <[e-mail]>
> Cc: Marc Zyngier <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>
> Cc: Sinan Kaya <[e-mail]>
> Cc: Tomasz Nowicki <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> ---
>  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.

> +			*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.

> +	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.

Lorenzo

> +			return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +/**
>   * iort_dev_find_its_id() - Find the ITS identifier for a device
>   * @dev: The device.
>   * @req_id: Device's requester ID
> diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
> index ebe933e..e801fc0 100644
> --- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
> @@ -15,6 +15,7 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/acpi_iort.h>
>  #include <linux/device.h>
>  #include <linux/msi.h>
>  #include <linux/of.h>
> @@ -56,7 +57,8 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
>  
>  	msi_info = msi_get_domain_info(domain->parent);
>  
> -	ret = of_pmsi_get_dev_id(domain, dev, &dev_id);
> +	ret = dev->of_node ? of_pmsi_get_dev_id(domain, dev, &dev_id) :
> +		iort_pmsi_get_dev_id(dev, &dev_id);
>  	if (ret)
>  		return ret;
>  
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 77e0809..ef99fd52 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -33,6 +33,7 @@
>  void acpi_iort_init(void);
>  bool iort_node_match(u8 type);
>  u32 iort_msi_map_rid(struct device *dev, u32 req_id);
> +int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
>  struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
>  /* IOMMU interface */
>  void iort_set_dma_mask(struct device *dev);
> @@ -42,9 +43,16 @@ static inline void acpi_iort_init(void) { }
>  static inline bool iort_node_match(u8 type) { return false; }
>  static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
>  { return req_id; }
> +
>  static inline struct irq_domain *iort_get_device_domain(struct device *dev,
>  							u32 req_id)
>  { return NULL; }
> +
> +static inline int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
> +{
> +	return -ENODEV;
> +}
> +
>  /* IOMMU interface */
>  static inline void iort_set_dma_mask(struct device *dev) { }
>  static inline
> -- 
> 1.9.1
>

Hanjun Guo Jan. 14, 2017, 4:28 a.m. UTC | #2

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 <[e-mail]>
>> Suggested-by: Tomasz Nowicki <[e-mail]>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> Cc: Marc Zyngier <[e-mail]>
>> Cc: Lorenzo Pieralisi <[e-mail]>
>> Cc: Sinan Kaya <[e-mail]>
>> Cc: Tomasz Nowicki <[e-mail]>
>> Cc: Thomas Gleixner <[e-mail]>
>> ---
>>  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

Lorenzo Pieralisi Jan. 16, 2017, 11:25 a.m. UTC | #3

On Sat, Jan 14, 2017 at 12:28:35PM +0800, Hanjun Guo wrote:
> 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?

Yes, I would introduce iort_node_map_platform_id() and in the same
patch update current iort_node_get_id() users (ie iort_iommu_configure())
to it. No functional change intended.

Then in subsequent patches you can retrieve the ITS device id for
platform devices through it.

Code is in your series, you just have to reshuffle it slightly.

> >> Suggested-by: Lorenzo Pieralisi <[e-mail]>
> >> Suggested-by: Tomasz Nowicki <[e-mail]>
> >> Signed-off-by: Hanjun Guo <[e-mail]>
> >> Cc: Marc Zyngier <[e-mail]>
> >> Cc: Lorenzo Pieralisi <[e-mail]>
> >> Cc: Sinan Kaya <[e-mail]>
> >> Cc: Tomasz Nowicki <[e-mail]>
> >> Cc: Thomas Gleixner <[e-mail]>
> >> ---
> >>  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 :)

I see. I would like to change the IORT mapping API functions to always pass
in an argument:

struct iort_idmap {
	struct acpi_iort_node *parent;
	u32 id;
};

and return an int, because current functions (eg iort_node_map_rid())
return a parent IORT node but also the mapped id as a value-result
and that's not easy to follow (also Sinan raised this point which I
think it is fair).

I think we'd better postpone this change to next cycle, so you can
leave the pointer check:

if (id_out)

I will clean this up later, basically what we would end up doing to just
retrieve the parent pointer would be the IORT equivalent of what we have
in DT:

of_parse_phandle()
  -> __of_parse_phandle_with_args() #we call it with cell_count == 0


at the end of the day it is just to make code easier to follow, since it
is functions internal to IORT compilation unit it is ok for now to leave
it as-is.

Thanks,
Lorenzo

Hanjun Guo Jan. 16, 2017, 1:21 p.m. UTC | #4

On 2017/1/16 19:25, Lorenzo Pieralisi wrote:
> On Sat, Jan 14, 2017 at 12:28:35PM +0800, Hanjun Guo wrote:
>> 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?
> Yes, I would introduce iort_node_map_platform_id() and in the same
> patch update current iort_node_get_id() users (ie iort_iommu_configure())
> to it. No functional change intended.
>
> Then in subsequent patches you can retrieve the ITS device id for
> platform devices through it.

Good point, I will update the patch set.

>
> Code is in your series, you just have to reshuffle it slightly.
>
>>>> Suggested-by: Lorenzo Pieralisi <[e-mail]>
>>>> Suggested-by: Tomasz Nowicki <[e-mail]>
>>>> Signed-off-by: Hanjun Guo <[e-mail]>
>>>> Cc: Marc Zyngier <[e-mail]>
>>>> Cc: Lorenzo Pieralisi <[e-mail]>
>>>> Cc: Sinan Kaya <[e-mail]>
>>>> Cc: Tomasz Nowicki <[e-mail]>
>>>> Cc: Thomas Gleixner <[e-mail]>
>>>> ---
>>>>  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 :)
> I see. I would like to change the IORT mapping API functions to always pass
> in an argument:
>
> struct iort_idmap {
> 	struct acpi_iort_node *parent;
> 	u32 id;
> };
>
> and return an int, because current functions (eg iort_node_map_rid())
> return a parent IORT node but also the mapped id as a value-result
> and that's not easy to follow (also Sinan raised this point which I
> think it is fair).
>
> I think we'd better postpone this change to next cycle, so you can
> leave the pointer check:
>
> if (id_out)
>
> I will clean this up later, basically what we would end up doing to just
> retrieve the parent pointer would be the IORT equivalent of what we have
> in DT:
>
> of_parse_phandle()
>   -> __of_parse_phandle_with_args() #we call it with cell_count == 0
>
>
> at the end of the day it is just to make code easier to follow, since it
> is functions internal to IORT compilation unit it is ok for now to leave
> it as-is.
>

OK, thank you very for the review.

Hanjun

Patch

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)
+			*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
+ */
+int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
+{
+	int i;
+	struct acpi_iort_node *node;
+
+	if (!iort_table)
+		return -ENODEV;
+
+	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))
+			return 0;
+	}
+
+	return -ENODEV;
+}
+
+/**
  * iort_dev_find_its_id() - Find the ITS identifier for a device
  * @dev: The device.
  * @req_id: Device's requester ID
diff --git a/drivers/irqchip/irq-gic-v3-its-platform-msi.c b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
index ebe933e..e801fc0 100644
--- a/drivers/irqchip/irq-gic-v3-its-platform-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-platform-msi.c
@@ -15,6 +15,7 @@ 
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/acpi_iort.h>
 #include <linux/device.h>
 #include <linux/msi.h>
 #include <linux/of.h>
@@ -56,7 +57,8 @@  static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
 
 	msi_info = msi_get_domain_info(domain->parent);
 
-	ret = of_pmsi_get_dev_id(domain, dev, &dev_id);
+	ret = dev->of_node ? of_pmsi_get_dev_id(domain, dev, &dev_id) :
+		iort_pmsi_get_dev_id(dev, &dev_id);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 77e0809..ef99fd52 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -33,6 +33,7 @@ 
 void acpi_iort_init(void);
 bool iort_node_match(u8 type);
 u32 iort_msi_map_rid(struct device *dev, u32 req_id);
+int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id);
 struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
 /* IOMMU interface */
 void iort_set_dma_mask(struct device *dev);
@@ -42,9 +43,16 @@  static inline void acpi_iort_init(void) { }
 static inline bool iort_node_match(u8 type) { return false; }
 static inline u32 iort_msi_map_rid(struct device *dev, u32 req_id)
 { return req_id; }
+
 static inline struct irq_domain *iort_get_device_domain(struct device *dev,
 							u32 req_id)
 { return NULL; }
+
+static inline int iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
+{
+	return -ENODEV;
+}
+
 /* IOMMU interface */
 static inline void iort_set_dma_mask(struct device *dev) { }
 static inline