[PATCH 1/2] ACPI/AEST: Initial AEST driver

[Tyler Baicar]

Hi Shuuichirou,

I should be able to get a v2 patch series out by the end of the month.

Thanks,
Tyler

On 4/20/2022 3:54 AM, ishii.shuuichir at fujitsu.com wrote:
> Hi, Tyler.
> 
> When do you plan to post the v2 patch series?
> Please let me know if you don't mind.
> 
> Best regards.
> 
>> -----Original Message-----
>> From: Tyler Baicar <baicar at amperemail.onmicrosoft.com>
>> Sent: Friday, December 17, 2021 8:33 AM
>> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir at fujitsu.com>; 'Tyler Baicar'
>> <baicar at os.amperecomputing.com>; patches at amperecomputing.com;
>> abdulhamid at os.amperecomputing.com; darren at os.amperecomputing.com;
>> catalin.marinas at arm.com; will at kernel.org; maz at kernel.org;
>> james.morse at arm.com; alexandru.elisei at arm.com; suzuki.poulose at arm.com;
>> lorenzo.pieralisi at arm.com; guohanjun at huawei.com; sudeep.holla at arm.com;
>> rafael at kernel.org; lenb at kernel.org; tony.luck at intel.com; bp at alien8.de;
>> mark.rutland at arm.com; anshuman.khandual at arm.com;
>> vincenzo.frascino at arm.com; tabba at google.com; marcan at marcan.st;
>> keescook at chromium.org; jthierry at redhat.com; masahiroy at kernel.org;
>> samitolvanen at google.com; john.garry at huawei.com; daniel.lezcano at linaro.org;
>> gor at linux.ibm.com; zhangshaokun at hisilicon.com; tmricht at linux.ibm.com;
>> dchinner at redhat.com; tglx at linutronix.de; linux-kernel at vger.kernel.org;
>> linux-arm-kernel at lists.infradead.org; kvmarm at lists.cs.columbia.edu;
>> linux-acpi at vger.kernel.org; linux-edac at vger.kernel.org;
>> Vineeth.Pillai at microsoft.com
>> Subject: Re: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>
>> Hi Shuuichirou,
>>
>> Thank you for your feedback!
>>
>> On 12/9/2021 3:10 AM, ishii.shuuichir at fujitsu.com wrote:
>>> Hi, Tyler.
>>>
>>> We would like to make a few comments.
>>>
>>>> -----Original Message-----
>>>> From: Tyler Baicar <baicar at os.amperecomputing.com>
>>>> Sent: Thursday, November 25, 2021 2:07 AM
>>>> To: patches at amperecomputing.com; abdulhamid at os.amperecomputing.com;
>>>> darren at os.amperecomputing.com; catalin.marinas at arm.com;
>>>> will at kernel.org; maz at kernel.org; james.morse at arm.com;
>>>> alexandru.elisei at arm.com; suzuki.poulose at arm.com;
>>>> lorenzo.pieralisi at arm.com; guohanjun at huawei.com;
>>>> sudeep.holla at arm.com; rafael at kernel.org; lenb at kernel.org;
>>>> tony.luck at intel.com; bp at alien8.de; mark.rutland at arm.com;
>>>> anshuman.khandual at arm.com; vincenzo.frascino at arm.com;
>>>> tabba at google.com; marcan at marcan.st; keescook at chromium.org;
>>>> jthierry at redhat.com; masahiroy at kernel.org; samitolvanen at google.com;
>>>> john.garry at huawei.com; daniel.lezcano at linaro.org; gor at linux.ibm.com;
>>>> zhangshaokun at hisilicon.com; tmricht at linux.ibm.com;
>>>> dchinner at redhat.com; tglx at linutronix.de;
>>>> linux-kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>>>> kvmarm at lists.cs.columbia.edu; linux-acpi at vger.kernel.org;
>>>> linux-edac at vger.kernel.org; Ishii, Shuuichirou/石井
>>>> 周一郎 <ishii.shuuichir at fujitsu.com>; Vineeth.Pillai at microsoft.com
>>>> Cc: Tyler Baicar <baicar at os.amperecomputing.com>
>>>> Subject: [PATCH 1/2] ACPI/AEST: Initial AEST driver
>>>>
>>>> Add support for parsing the ARM Error Source Table and basic handling
>>>> of errors reported through both memory mapped and system register
>> interfaces.
>>>>
>>>> Assume system register interfaces are only registered with private
>>>> peripheral interrupts (PPIs); otherwise there is no guarantee the
>>>> core handling the error is the core which took the error and has the
>>>> syndrome info in its system registers.
>>>>
>>>> Add logging for all detected errors and trigger a kernel panic if
>>>> there is any uncorrected error present.
>>>>
>>>> Signed-off-by: Tyler Baicar <baicar at os.amperecomputing.com>
>>>> ---
>>> [...]
>>>
>>>> +static int __init aest_init_node(struct acpi_aest_hdr *node) {
>>>> +	union acpi_aest_processor_data *proc_data;
>>>> +	union aest_node_spec *node_spec;
>>>> +	struct aest_node_data *data;
>>>> +	int ret;
>>>> +
>>>> +	data = kzalloc(sizeof(struct aest_node_data), GFP_KERNEL);
>>>> +	if (!data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	data->node_type = node->type;
>>>> +
>>>> +	node_spec = ACPI_ADD_PTR(union aest_node_spec, node,
>>>> node->node_specific_offset);
>>>> +
>>>> +	switch (node->type) {
>>>> +	case ACPI_AEST_PROCESSOR_ERROR_NODE:
>>>> +		memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_processor));
>>>> +		break;
>>>> +	case ACPI_AEST_MEMORY_ERROR_NODE:
>>>> +		memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_memory));
>>>> +		break;
>>>> +	case ACPI_AEST_SMMU_ERROR_NODE:
>>>> +		memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_smmu));
>>>> +		break;
>>>> +	case ACPI_AEST_VENDOR_ERROR_NODE:
>>>> +		memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_vendor));
>>>> +		break;
>>>> +	case ACPI_AEST_GIC_ERROR_NODE:
>>>> +		memcpy(&data->data, node_spec, sizeof(struct
>>>> acpi_aest_gic));
>>>> +		break;
>>>> +	default:
>>>> +		kfree(data);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if (node->type == ACPI_AEST_PROCESSOR_ERROR_NODE) {
>>>> +		proc_data = ACPI_ADD_PTR(union acpi_aest_processor_data,
>>>> node_spec,
>>>> +					 sizeof(acpi_aest_processor));
>>>> +
>>>> +		switch (data->data.processor.resource_type) {
>>>> +		case ACPI_AEST_CACHE_RESOURCE:
>>>> +			memcpy(&data->proc_data, proc_data,
>>>> +			       sizeof(struct acpi_aest_processor_cache));
>>>> +			break;
>>>> +		case ACPI_AEST_TLB_RESOURCE:
>>>> +			memcpy(&data->proc_data, proc_data,
>>>> +			       sizeof(struct acpi_aest_processor_tlb));
>>>> +			break;
>>>> +		case ACPI_AEST_GENERIC_RESOURCE:
>>>> +			memcpy(&data->proc_data, proc_data,
>>>> +			       sizeof(struct acpi_aest_processor_generic));
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	ret = aest_init_interface(node, data);
>>>> +	if (ret) {
>>>> +		kfree(data);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return aest_init_interrupts(node, data);
>>> If aest_init_interrupts() failed, is it necessary to release the data
>>> pointer acquired by kzalloc?
>> aest_init_interrupts() returns an error if any of the interrupts in the interrupt list
>> fails, but it's possible that some interrupts in the list registered successfully. So
>> we attempt to keep chugging along in that scenario because some interrupts may
>> be enabled and registered with the interface successfully. If any interrupt
>> registration fails, there will be a print notifying that there was a failure when
>> initializing that node.
>>>> +}
>>>> +
>>>> +static void aest_count_ppi(struct acpi_aest_hdr *node)
>>>> +{
>>>> +	struct acpi_aest_node_interrupt *interrupt;
>>>> +	int i;
>>>> +
>>>> +	interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, node,
>>>> +				 node->node_interrupt_offset);
>>>> +
>>>> +	for (i = 0; i < node->node_interrupt_count; i++, interrupt++) {
>>>> +		if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
>>>> +			num_ppi++;
>>>> +	}
>>>> +}
>>>> +
>>>> +static int aest_starting_cpu(unsigned int cpu)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < num_ppi; i++)
>>>> +		enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int aest_dying_cpu(unsigned int cpu)
>>>> +{
>>> Wouldn't it be better to execute disable_percpu_irq(), which is paired
>>> with enable_percpu_irq(), in aest_dying_cpu()?
>>
>> Good point. I will add that in the next version.
>>
>> Thanks,
>>
>> Tyler
>