Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: James Morse <james.morse@arm.com>
To: Hanna Hawa <hhhawa@amazon.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: Wed, 15 Jan 2020 18:50:10 +0000
Message-ID: <a6c7d925-22fb-4324-ce12-662b97976e61@arm.com> (raw)
In-Reply-To: <20191015120927.10470-4-hhhawa@amazon.com>

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

  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 [this message]
2020-01-20 14:52     ` Hawa, Hanna

Reply instructions:

You may reply publically 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=a6c7d925-22fb-4324-ce12-662b97976e61@arm.com \
    --to=james.morse@arm.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=hhhawa@amazon.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