Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 3 Oct 2018 08:46:30 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Reinette Chatre <reinette.chatre@intel.com>
cc:     fenghua.yu@intel.com, tony.luck@intel.com, jithu.joseph@intel.com,
        gavin.hindman@intel.com, dave.hansen@intel.com, mingo@redhat.com,
        hpa@zytor.com, x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] x86/intel_rdt: Introduce utility to obtain CDP
 peer
In-Reply-To: <6e8c2eddf0cb2521fe7018357a0fa6f8dba7a882.1537987801.git.reinette.chatre@intel.com>
Message-ID: <alpine.DEB.2.21.1810030843180.1435@nanos.tec.linutronix.de>
References: <cover.1537987801.git.reinette.chatre@intel.com> <6e8c2eddf0cb2521fe7018357a0fa6f8dba7a882.1537987801.git.reinette.chatre@intel.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 26 Sep 2018, Reinette Chatre wrote:
> + * Return: 0 if a CDP peer was found, <0 on error or if no CDP peer exists.
> + *         If a CDP peer was found, @r_cdp will point to the peer RDT resource
> + *         and @d_cdp will point to the peer RDT domain.
> + */
> +static int __attribute__((unused)) rdt_cdp_peer_get(struct rdt_resource *r,
> +						    struct rdt_domain *d,
> +						    struct rdt_resource **r_cdp,
> +						    struct rdt_domain **d_cdp)
> +{
> +	struct rdt_resource *_r_cdp = NULL;
> +	struct rdt_domain *_d_cdp = NULL;
> +	int ret = 0;
> +
> +	switch (r->rid) {
> +	case RDT_RESOURCE_L3DATA:
> +		_r_cdp = &rdt_resources_all[RDT_RESOURCE_L3CODE];
> +		break;
> +	case RDT_RESOURCE_L3CODE:
> +		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L3DATA];
> +		break;
> +	case RDT_RESOURCE_L2DATA:
> +		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L2CODE];
> +		break;
> +	case RDT_RESOURCE_L2CODE:
> +		_r_cdp =  &rdt_resources_all[RDT_RESOURCE_L2DATA];
> +		break;
> +	default:
> +		ret = -ENOENT;
> +		goto out;
> +	}
> +
> +	/*
> +	 * When a new CPU comes online and CDP is enabled then the new
> +	 * RDT domains (if any) associated with both CDP RDT resources
> +	 * are added in the same CPU online routine while the
> +	 * rdtgroup_mutex is held. It should thus not happen for one
> +	 * RDT domain to exist and be associated with its RDT CDP
> +	 * resource but there is no RDT domain associated with the
> +	 * peer RDT CDP resource. Hence the WARN.
> +	 */
> +	_d_cdp = rdt_find_domain(_r_cdp, d->id, NULL);
> +	if (WARN_ON(!_d_cdp)) {
> +		_r_cdp = NULL;
> +		ret = -ENOENT;

While this should never happen, the return value is ambiguous. I'd rather
use EINVAL or such and propagate it further down at the call site.

Thanks,

	tglx