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.pieral...@arm.com>
>> Suggested-by: Tomasz Nowicki <t...@semihalf.com>
>> Signed-off-by: Hanjun Guo <hanjun....@linaro.org>
>> Cc: Marc Zyngier <marc.zyng...@arm.com>
>> Cc: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
>> Cc: Sinan Kaya <ok...@codeaurora.org>
>> Cc: Tomasz Nowicki <t...@semihalf.com>
>> Cc: Thomas Gleixner <t...@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
