Re: [PATCH v7 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC

Re: [PATCH v7 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC

[James Morse]

Hi Hanna,

(This was still on my list. I've not seen a newer version, its not in next, and it still
applies, so:)

On 15/10/2019 13:09, Hanna Hawa wrote:
> Adds support for Amazon's Annapurna Labs L1 EDAC driver to detect and
> report L1 errors.

> diff --git a/drivers/edac/al_l1_edac.c b/drivers/edac/al_l1_edac.c
> new file mode 100644
> index 000000000000..e363a80b4d13
> --- /dev/null
> +++ b/drivers/edac/al_l1_edac.c
> @@ -0,0 +1,190 @@

> +#include <asm/sysreg.h>
> +#include <linux/bitfield.h>
> +#include <linux/of.h>
> +#include <linux/smp.h>

You need <linux/platform_device.h> for platform_device_register_simple().

[...]

> +static void al_l1_edac_cpumerrsr_read_status(void *arg)
> +{

> +	for (i = 0; i < repeat; i++) {
> +		if (fatal)
> +			edac_device_handle_ue(edac_dev, 0, 0, msg);
> +		else
> +			edac_device_handle_ce(edac_dev, 0, 0, msg);
> +	}

What serialises these? You kick this off from on_each_cpu(), what stops two CPUs calling
this at the same time? 'edac_dev->counters.ce_count += count;' will go wrong in this case.

I think you need a spinlock around the edac_device_* calls that take edac_dev so that only
one occurs at a time.


> +}
> +
> +static void al_l1_edac_check(struct edac_device_ctl_info *edac_dev)
> +{
> +	on_each_cpu(al_l1_edac_cpumerrsr_read_status, edac_dev, 1);
> +}
> +
> +static int al_l1_edac_probe(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *edac_dev;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	edac_dev = edac_device_alloc_ctl_info(0, DRV_NAME, 1, "L", 1, 1, NULL,
> +					      0, edac_device_alloc_index());
> +	if (!edac_dev)
> +		return -ENOMEM;
> +
> +	edac_dev->edac_check = al_l1_edac_check;
> +	edac_dev->dev = dev;
> +	edac_dev->mod_name = DRV_NAME;
> +	edac_dev->dev_name = dev_name(dev);
> +	edac_dev->ctl_name = "L1_cache";
> +	platform_set_drvdata(pdev, edac_dev);
> +
> +	ret = edac_device_add_device(edac_dev);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +err:

(this goto has one user, meaning you can remove it by restructuring the code)


> +	dev_err(dev, "Failed to add L1 edac device (%d)\n", ret);
> +	edac_device_free_ctl_info(edac_dev);
> +
> +	return ret;
> +}
> +

> +static const struct of_device_id al_l1_edac_of_match[] = {
> +	{ .compatible = "al,alpine-v2" },
> +	{ .compatible = "amazon,alpine-v3" },
> +	{}
> +};

Unusually these are machine compatibles. It may be worth a comment that these are the
platforms which are known to have Cortex-A57/A72 configured with this support, and access
to the registers enabled by firmware.


> +MODULE_DEVICE_TABLE(of, al_l1_edac_of_match);

[..]

> +static int __init al_l1_init(void)
> +{
> +	struct device_node *root = of_find_node_by_path("/");
> +	int ret;

root could be NULL here.


> +	if (!of_match_node(al_l1_edac_of_match, root))
> +		return 0;
> +
> +	ret = platform_driver_register(&al_l1_edac_driver);
> +	if (ret) {
> +		pr_err("Failed to register %s (%d)\n", DRV_NAME, ret);
> +		return ret;
> +	}
> +
> +	edac_l1_device = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
> +	if (IS_ERR(edac_l1_device)) {
> +		pr_err("Failed to register EDAC AL L1 platform device\n");
> +		return PTR_ERR(edac_l1_device);
> +	}
> +
> +	return 0;
> +}

With the edac_device_handle_ce() race fixed:
Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

Re: [PATCH v7 3/3] edac: Add support for Amazon's Annapurna Labs L2 EDAC

[James Morse]

Hi Hanna,

On 15/10/2019 13:09, Hanna Hawa wrote:
> Adds support for Amazon's Annapurna Labs L2 EDAC driver to detect and
> report L2 errors.


> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7887a62dc843..0eabcfcf91a9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -748,6 +748,11 @@ M:	Hanna Hawa <hhhawa@amazon.com>
>  S:	Maintained
>  F:	drivers/edac/al_l1_edac.c
>  
> +AMAZON ANNAPURNA LABS L2 EDAC
> +M:	Hanna Hawa <hhhawa@amazon.com>
> +S:	Maintained
> +F:	drivers/edac/al_l2_edac.c

(Why not add the file to the previous section? All this does is come up with an email
address based on the file)


> diff --git a/drivers/edac/al_l2_edac.c b/drivers/edac/al_l2_edac.c
> new file mode 100644
> index 000000000000..156610c85591
> --- /dev/null
> +++ b/drivers/edac/al_l2_edac.c
> @@ -0,0 +1,251 @@
> +static int al_l2_edac_probe(struct platform_device *pdev)
> +{

> +	for_each_possible_cpu(i) {
> +		struct device_node *cpu;
> +		struct device_node *cpu_cache;
> +		struct al_l2_cache *l2_cache;
> +		bool found = false;
> +
> +		cpu = of_get_cpu_node(i, NULL);
> +		if (!cpu)
> +			continue;
> +
> +		cpu_cache = of_find_next_cache_node(cpu);
> +		list_for_each_entry(l2_cache, &al_l2->l2_caches, list_node) {
> +			if (l2_cache->of_node == cpu_cache) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (found) {
> +			cpumask_set_cpu(i, &l2_cache->cluster_cpus);

			of_node_put(cpu_cache); ?

(I can see why you might keep the reference in the else block)


> +		} else {
> +			l2_cache = devm_kzalloc(dev, sizeof(*l2_cache),
> +						GFP_KERNEL);
> +			l2_cache->of_node = cpu_cache;
> +			list_add(&l2_cache->list_node, &al_l2->l2_caches);
> +			cpumask_set_cpu(i, &l2_cache->cluster_cpus);
> +		}
> +
> +		of_node_put(cpu);
> +	}
> +
> +	if (list_empty(&al_l2->l2_caches)) {
> +		dev_err(dev, "L2 Cache list is empty for EDAC device\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}

You are doing this at probe time to create a static list of which CPUs map onto the L2
caches. cacheinfo does something very similar, but it looks like you can't use it as its
only populated for online CPUs, and you'd need to walk multiple CPUs cacheinfo leaves to
find the same information. The alternative is more complicated.


> +	ret = edac_device_add_device(edac_dev);
> +	if (ret)

Any references held in the al_l2->l2_caches list leak here.


> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	dev_err(dev, "Failed to add L2 edac device (%d)\n", ret);
> +	edac_device_free_ctl_info(edac_dev);
> +
> +	return ret;
> +}


> +static int al_l2_edac_remove(struct platform_device *pdev)
> +{
> +	struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev);

Do you need to roll over the al_l2->l2_caches list to of_node_put() the l2_cache's ?


> +	edac_device_del_device(edac_dev->dev);
> +	edac_device_free_ctl_info(edac_dev);
> +
> +	return 0;
> +}

[..]

> +static const struct of_device_id al_l2_edac_of_match[] = {
> +	{ .compatible = "al,alpine-v2" },
> +	{ .compatible = "amazon,alpine-v3" },
> +	{}
> +};

Same comment on these being machine compatibles and what property that applies to.

The code to match the platform and create the platform_device is identical, is there any
way it can be shared?

I'm guessing the two-files is because these can be built as independent modules. Would
anyone ever have one, but not the other? The L1 support is optional, but you've listed the
same set of platforms in both cases here, so do we need to support one but not the other
today?


Thanks,

James

Re: [PATCH v7 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC

[Hawa, Hanna]

On 1/15/2020 8:49 PM, James Morse wrote:
> Hi Hanna,
> 
> (This was still on my list. I've not seen a newer version, its not in next, and it still
> applies, so:)

Thank you.

> 
> On 15/10/2019 13:09, Hanna Hawa wrote:
>> Adds support for Amazon's Annapurna Labs L1 EDAC driver to detect and
>> report L1 errors.
> 
>> diff --git a/drivers/edac/al_l1_edac.c b/drivers/edac/al_l1_edac.c
>> new file mode 100644
>> index 000000000000..e363a80b4d13
>> --- /dev/null
>> +++ b/drivers/edac/al_l1_edac.c
>> @@ -0,0 +1,190 @@
> 
>> +#include <asm/sysreg.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/of.h>
>> +#include <linux/smp.h>
> 
> You need <linux/platform_device.h> for platform_device_register_simple().

Will be added in next PS.

> 
> [...]
> 
>> +static void al_l1_edac_cpumerrsr_read_status(void *arg)
>> +{
> 
>> +	for (i = 0; i < repeat; i++) {
>> +		if (fatal)
>> +			edac_device_handle_ue(edac_dev, 0, 0, msg);
>> +		else
>> +			edac_device_handle_ce(edac_dev, 0, 0, msg);
>> +	}
> 
> What serialises these? You kick this off from on_each_cpu(), what stops two CPUs calling
> this at the same time? 'edac_dev->counters.ce_count += count;' will go wrong in this case.
> 
> I think you need a spinlock around the edac_device_* calls that take edac_dev so that only
> one occurs at a time.

Agree with you, will add spinlock in next PS.

> 
> 
>> +}
>> +
>> +static void al_l1_edac_check(struct edac_device_ctl_info *edac_dev)
>> +{
>> +	on_each_cpu(al_l1_edac_cpumerrsr_read_status, edac_dev, 1);
>> +}
>> +
>> +static int al_l1_edac_probe(struct platform_device *pdev)
>> +{
>> +	struct edac_device_ctl_info *edac_dev;
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	edac_dev = edac_device_alloc_ctl_info(0, DRV_NAME, 1, "L", 1, 1, NULL,
>> +					      0, edac_device_alloc_index());
>> +	if (!edac_dev)
>> +		return -ENOMEM;
>> +
>> +	edac_dev->edac_check = al_l1_edac_check;
>> +	edac_dev->dev = dev;
>> +	edac_dev->mod_name = DRV_NAME;
>> +	edac_dev->dev_name = dev_name(dev);
>> +	edac_dev->ctl_name = "L1_cache";
>> +	platform_set_drvdata(pdev, edac_dev);
>> +
>> +	ret = edac_device_add_device(edac_dev);
>> +	if (ret)
>> +		goto err;
>> +
>> +	return 0;
>> +err:
> 
> (this goto has one user, meaning you can remove it by restructuring the code)

Will be fixed in next PS.

> 
> 
>> +	dev_err(dev, "Failed to add L1 edac device (%d)\n", ret);
>> +	edac_device_free_ctl_info(edac_dev);
>> +
>> +	return ret;
>> +}
>> +
> 
>> +static const struct of_device_id al_l1_edac_of_match[] = {
>> +	{ .compatible = "al,alpine-v2" },
>> +	{ .compatible = "amazon,alpine-v3" },
>> +	{}
>> +};
> 
> Unusually these are machine compatibles. It may be worth a comment that these are the
> platforms which are known to have Cortex-A57/A72 configured with this support, and access
> to the registers enabled by firmware.

Will be added.

> 
> 
>> +MODULE_DEVICE_TABLE(of, al_l1_edac_of_match);
> 
> [..]
> 
>> +static int __init al_l1_init(void)
>> +{
>> +	struct device_node *root = of_find_node_by_path("/");
>> +	int ret;
> 
> root could be NULL here.

Will be fixed.

> 
> 
>> +	if (!of_match_node(al_l1_edac_of_match, root))
>> +		return 0;
>> +
>> +	ret = platform_driver_register(&al_l1_edac_driver);
>> +	if (ret) {
>> +		pr_err("Failed to register %s (%d)\n", DRV_NAME, ret);
>> +		return ret;
>> +	}
>> +
>> +	edac_l1_device = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
>> +	if (IS_ERR(edac_l1_device)) {
>> +		pr_err("Failed to register EDAC AL L1 platform device\n");
>> +		return PTR_ERR(edac_l1_device);
>> +	}
>> +
>> +	return 0;
>> +}
> 
> With the edac_device_handle_ce() race fixed:
> Reviewed-by: James Morse <james.morse@arm.com>

Thanks,
Hanna

> 
> 
> Thanks,
> 
> James
> 

Re: [PATCH v7 3/3] edac: Add support for Amazon's Annapurna Labs L2 EDAC

[Hawa, Hanna]

On 1/15/2020 8:50 PM, James Morse wrote:
> Hi Hanna,
> 
> On 15/10/2019 13:09, Hanna Hawa wrote:
>> Adds support for Amazon's Annapurna Labs L2 EDAC driver to detect and
>> report L2 errors.
> 
> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7887a62dc843..0eabcfcf91a9 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -748,6 +748,11 @@ M:	Hanna Hawa <hhhawa@amazon.com>
>>   S:	Maintained
>>   F:	drivers/edac/al_l1_edac.c
>>   
>> +AMAZON ANNAPURNA LABS L2 EDAC
>> +M:	Hanna Hawa <hhhawa@amazon.com>
>> +S:	Maintained
>> +F:	drivers/edac/al_l2_edac.c
> 
> (Why not add the file to the previous section? All this does is come up with an email
> address based on the file)

Added new section as this separated driver.

> 
> 
>> diff --git a/drivers/edac/al_l2_edac.c b/drivers/edac/al_l2_edac.c
>> new file mode 100644
>> index 000000000000..156610c85591
>> --- /dev/null
>> +++ b/drivers/edac/al_l2_edac.c
>> @@ -0,0 +1,251 @@
>> +static int al_l2_edac_probe(struct platform_device *pdev)
>> +{
> 
>> +	for_each_possible_cpu(i) {
>> +		struct device_node *cpu;
>> +		struct device_node *cpu_cache;
>> +		struct al_l2_cache *l2_cache;
>> +		bool found = false;
>> +
>> +		cpu = of_get_cpu_node(i, NULL);
>> +		if (!cpu)
>> +			continue;
>> +
>> +		cpu_cache = of_find_next_cache_node(cpu);
>> +		list_for_each_entry(l2_cache, &al_l2->l2_caches, list_node) {
>> +			if (l2_cache->of_node == cpu_cache) {
>> +				found = true;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (found) {
>> +			cpumask_set_cpu(i, &l2_cache->cluster_cpus);
> 
> 			of_node_put(cpu_cache); ?
> 
> (I can see why you might keep the reference in the else block)

Will be added in next PS.

> 
> 
>> +		} else {
>> +			l2_cache = devm_kzalloc(dev, sizeof(*l2_cache),
>> +						GFP_KERNEL);
>> +			l2_cache->of_node = cpu_cache;
>> +			list_add(&l2_cache->list_node, &al_l2->l2_caches);
>> +			cpumask_set_cpu(i, &l2_cache->cluster_cpus);
>> +		}
>> +
>> +		of_node_put(cpu);
>> +	}
>> +
>> +	if (list_empty(&al_l2->l2_caches)) {
>> +		dev_err(dev, "L2 Cache list is empty for EDAC device\n");
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
> 
> You are doing this at probe time to create a static list of which CPUs map onto the L2
> caches. cacheinfo does something very similar, but it looks like you can't use it as its
> only populated for online CPUs, and you'd need to walk multiple CPUs cacheinfo leaves to
> find the same information. The alternative is more complicated.

> 
> 
>> +	ret = edac_device_add_device(edac_dev);
>> +	if (ret)
> 
> Any references held in the al_l2->l2_caches list leak here.
> 
> 
>> +		goto err;
>> +
>> +	return 0;
>> +
>> +err:
>> +	dev_err(dev, "Failed to add L2 edac device (%d)\n", ret);
>> +	edac_device_free_ctl_info(edac_dev);
>> +
>> +	return ret;
>> +}
> 
> 
>> +static int al_l2_edac_remove(struct platform_device *pdev)
>> +{
>> +	struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev);
> 
> Do you need to roll over the al_l2->l2_caches list to of_node_put() the l2_cache's ?

will add loop after for_each_possible_cpu() to call of_node_put() on 
each l2_cache.

> 
> 
>> +	edac_device_del_device(edac_dev->dev);
>> +	edac_device_free_ctl_info(edac_dev);
>> +
>> +	return 0;
>> +}
> 
> [..]
> 
>> +static const struct of_device_id al_l2_edac_of_match[] = {
>> +	{ .compatible = "al,alpine-v2" },
>> +	{ .compatible = "amazon,alpine-v3" },
>> +	{}
>> +};
> 
> Same comment on these being machine compatibles and what property that applies to.

Fix comments from your review from L1 driver.

> 
> The code to match the platform and create the platform_device is identical, is there any
> way it can be shared?
> 
> I'm guessing the two-files is because these can be built as independent modules. Would
> anyone ever have one, but not the other? The L1 support is optional, but you've listed the
> same set of platforms in both cases here, so do we need to support one but not the other
> today?

It's not related to that platform will have one, but not the other. The 
two drivers are not related to each other, I agree with you that there 
is identical code in matching platform. But this is not good reason to 
combine the two drivers.

Thanks,
Hanna

> 
> 
> Thanks,
> 
> James
>