lkml.org 
[lkml]   [2020]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 3/3] edac: Add support for Amazon's Annapurna Labs L2 EDAC
From
Date


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
>

\
 
 \ /
  Last update: 2020-01-20 15:53    [W:0.075 / U:0.220 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site