Received-SPF: pass (google.com: domain of linux-kernel+bounces-54635-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <dd862d7a-11bd-482a-b248-60d4616fb6ef@arm.com>
Date: Tue, 6 Feb 2024 10:00:19 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] perf/arm-cmn: Workaround AmpereOneX errata AC04_MESH_1
 (incorrect child count)
Content-Language: en-GB
To: Ilkka Koskinen <ilkka@os.amperecomputing.com>,
 Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20240205194655.1567434-1-ilkka@os.amperecomputing.com>
From: Robin Murphy <robin.murphy@arm.com>
In-Reply-To: <20240205194655.1567434-1-ilkka@os.amperecomputing.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2024-02-05 7:46 pm, Ilkka Koskinen wrote:
> AmpereOneX mesh implementation has a bug in HN-P nodes that makes them
> report incorrect child count. The failing crosspoints report 8 children
> while they only have two.

Ooh, fun :)

> When the driver tries to access the inexistent child nodes, it believes it
> has reached an invalid node type and probing fails. The workaround is to
> ignore those incorrect child nodes and continue normally.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
>   drivers/perf/arm-cmn.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index c584165b13ba..97fed8ec3693 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -2168,6 +2168,23 @@ static enum cmn_node_type arm_cmn_subtype(enum cmn_node_type type)
>   	}
>   }
>   
> +static inline bool arm_cmn_is_ampereonex_bug(const struct arm_cmn *cmn,
> +					     struct arm_cmn_node *dn,
> +					     u16 child_count, int child)
> +{
> +	/*
> +	 * The bug occurs only when a crosspoint reports 8 children
> +	 * while it only has two HN-P child nodes.
> +	 */
> +	dn -= 2;
> +
> +	if (arm_cmn_model(cmn) == CMN650 && child_count == 8 &&
> +	    child == 2 && dn->type == CMN_TYPE_HNP)
> +		return true;
> +
> +	return false;
> +}
> +
>   static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
>   {
>   	void __iomem *cfg_region;
> @@ -2292,6 +2309,14 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
>   
>   		for (j = 0; j < child_count; j++) {
>   			reg = readq_relaxed(xp_region + child_poff + j * 8);
> +			if (reg == 0)
> +				if (arm_cmn_is_ampereonex_bug(cmn, dn, child_count, j))
> +					/*
> +					 * We know there are only two real children and the rest 6
> +					 * are inexistent. Thus, we can skip the rest of the loop
> +					 */
> +					break;
> +

TBH I don't see much harm in taking an even simpler approach, so I'd be
inclined to not bother being all that specific beyond documenting it,
something like the below:

Cheers,
Robin.

----->8-----

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index c584165b13ba..7e3aa7e2345f 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -2305,6 +2305,17 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
  				dev_dbg(cmn->dev, "ignoring external node %llx\n", reg);
  				continue;
  			}
+			/*
+			 * AmpereOneX erratum AC04_MESH_1 makes some XPs report a bogus
+			 * child count larger than the number of valid child pointers.
+			 * A child offset of 0 can only occur on CMN-600; otherwise it
+			 * would imply the root node being its own grandchild, which
+			 * we can safely dismiss in general.
+			 */
+			if (reg == 0 && cmn->part != PART_CMN600) {
+				dev_dbg(cmn->dev, "bogus child pointer?\n");
+				continue;
+			}
  
  			arm_cmn_init_node_info(cmn, reg & CMN_CHILD_NODE_ADDR, dn);