On Thu, Jun 15, 2017 at 4:13 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 15/06/17 11:35, Lorenzo Pieralisi wrote:
>> On Wed, Jun 14, 2017 at 03:23:17PM +0530, Ganapatrao Kulkarni wrote:
>>
>> [...]
>>
>>>>> +static int __init
>>>>> +acpi_parse_its_affinity(struct acpi_subtable_header *header,
>>>>> + const unsigned long end)
>>>>> +{
>>>>> + struct acpi_srat_its_affinity *its_affinity;
>>>>> +
>>>>> + its_affinity = (struct acpi_srat_its_affinity *)header;
>>>>> + if (!its_affinity)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + acpi_table_print_srat_entry(header);
>>
>> You can leave this info printing but see below.
ok.
>>
>>>>> +
>>>>> + /* let architecture-dependent part to do it */
>>>>> + acpi_numa_its_affinity_init(its_affinity);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static int __initdata parsed_numa_memblks;
>>>>>
>>>>> static int __init
>>>>> @@ -445,7 +473,7 @@ int __init acpi_numa_init(void)
>>>>>
>>>>> /* SRAT: Static Resource Affinity Table */
>>>>> if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) {
>>>>> - struct acpi_subtable_proc srat_proc[3];
>>>>> + struct acpi_subtable_proc srat_proc[4];
>>>>>
>>>>> memset(srat_proc, 0, sizeof(srat_proc));
>>>>> srat_proc[0].id = ACPI_SRAT_TYPE_CPU_AFFINITY;
>>>>> @@ -454,6 +482,8 @@ int __init acpi_numa_init(void)
>>>>> srat_proc[1].handler = acpi_parse_x2apic_affinity;
>>>>> srat_proc[2].id = ACPI_SRAT_TYPE_GICC_AFFINITY;
>>>>> srat_proc[2].handler = acpi_parse_gicc_affinity;
>>>>> + srat_proc[3].id = ACPI_SRAT_TYPE_GIC_ITS_AFFINITY;
>>>>> + srat_proc[3].handler = acpi_parse_its_affinity;
>>>>>
>>>>> acpi_table_parse_entries_array(ACPI_SIG_SRAT,
>>>>> sizeof(struct acpi_table_srat),
>>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>>>> index 45ea1933..84936da 100644
>>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>>> @@ -1861,7 +1861,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>>>>> goto dom_err;
>>>>> }
>>>>>
>>>>> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
>>>>> + err = its_probe_one(&res, dom_handle,
>>>>> + acpi_numa_get_its_nid(its_entry->translation_id));
>>>>
>>>> If that's the only usage I wonder whether we really need all arm64
>>>> arch code/data, instead of parsing the SRAT in ITS code driver straight
>>>> away at probe, retrieve its node and be done with this.
>>>>
>>>> I understand you replicated what x86/GICC does with APIC code, I would
>>>> like to understand though if we see a reason why (or better, why we keep
>>>> the relevant stashed data in arch/arm64 instead of the ITS driver).
>>>
>>> it is been thought to do ITS sub table parse along with other SRAT
>>> tables. and use the mapping later when ITS devices are
>>> initialised/probed. IMO, it is more appropriate to keep all SRAT sub
>>> table parsing to same function/place rather than moving to driver.
>>
>> I do not follow. If it is just used in ITS driver code to set the ITS
>> affinity node what's the point of stashing data and adding callbacks
>> when you can simply parse the SRAT and be done with it ?
>>
>> Or you have something on top of these patches that require ITS node
>> information and the calls you added ? If so post the code please.
>>
>> Regardless, it's ITS specific information, ITS is managed through an
>> irqchip driver on ARM64 so even if you decided to stash the SRAT ITS
>> information it does not belong in arch/arm64 IMO, you can implement
>> acpi_numa_its_affinity_init() in the ITS driver but AFAICS for the time
>> being it would be just useless that's the point I am making.
>
> Agreed. As long as there is only the ITS as a consumer of that
> information, there is no need to pollute the rest of the kernel with it.
> Once we have another consumer, we can look at making that code common.
> In the meantime, keeping it in the ITS code is the right thing to do.
agreed, i will change it in next version.
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
thanks
Ganapat
--
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
