Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: "Hawa, Hanna" <hhhawa@amazon.com>
To: James Morse <james.morse@arm.com>
Cc: <bp@alien8.de>, <mchehab@kernel.org>, <mark.rutland@arm.com>,
	<robh+dt@kernel.org>, <frowand.list@gmail.com>,
	<davem@davemloft.net>, <gregkh@linuxfoundation.org>,
	<linus.walleij@linaro.org>, <daniel@iogearbox.net>,
	<paulmck@linux.ibm.com>, <Sudeep.Holla@arm.com>,
	<linux-kernel@vger.kernel.org>, <linux-edac@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <dwmw@amazon.co.uk>,
	<benh@amazon.com>, <ronenk@amazon.com>, <talel@amazon.com>,
	<jonnyc@amazon.com>, <hanochu@amazon.com>
Subject: Re: [PATCH v7 3/3] edac: Add support for Amazon's Annapurna Labs L2 EDAC
Date: Mon, 20 Jan 2020 16:52:54 +0200
Message-ID: <ce51b629-a9b9-9848-8cbb-620d8a6549c3@amazon.com> (raw)
In-Reply-To: <a6c7d925-22fb-4324-ce12-662b97976e61@arm.com>



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
> 

      reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-15 12:09 [PATCH v7 0/3] Add support for Amazon's Annapurna Labs EDAC for L1/L2 Hanna Hawa
2019-10-15 12:09 ` [PATCH v7 1/3] edac: Add support for Amazon's Annapurna Labs L1 EDAC Hanna Hawa
2020-01-15 18:49   ` James Morse
2020-01-20 14:41     ` Hawa, Hanna
2019-10-15 12:09 ` [PATCH v7 2/3] of: EXPORT_SYMBOL_GPL of_find_next_cache_node Hanna Hawa
2019-10-17 14:09   ` Rob Herring
2019-10-15 12:09 ` [PATCH v7 3/3] edac: Add support for Amazon's Annapurna Labs L2 EDAC Hanna Hawa
2020-01-15 18:50   ` James Morse
2020-01-20 14:52     ` Hawa, Hanna [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ce51b629-a9b9-9848-8cbb-620d8a6549c3@amazon.com \
    --to=hhhawa@amazon.com \
    --cc=Sudeep.Holla@arm.com \
    --cc=benh@amazon.com \
    --cc=bp@alien8.de \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=dwmw@amazon.co.uk \
    --cc=frowand.list@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hanochu@amazon.com \
    --cc=james.morse@arm.com \
    --cc=jonnyc@amazon.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mchehab@kernel.org \
    --cc=paulmck@linux.ibm.com \
    --cc=robh+dt@kernel.org \
    --cc=ronenk@amazon.com \
    --cc=talel@amazon.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git