From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     netdev@vger.kernel.org
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        UNGLinuxDriver@microchip.com,
        Colin Foster <colin.foster@in-advantage.com>,
        linux-kernel@vger.kernel.org
Subject: [PATCH net 2/3] net: mscc: ocelot: fix transfer from region->buf to ocelot->stats
Date:   Tue, 21 Mar 2023 03:03:24 +0200
Message-Id: <20230321010325.897817-3-vladimir.oltean@nxp.com>
In-Reply-To: <20230321010325.897817-1-vladimir.oltean@nxp.com>
References: <20230321010325.897817-1-vladimir.oltean@nxp.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?JilcWyzTd1WwrH/fiqPreDdl+IcpHJRC5mfZy+Ce8XEyvRuaY70hmSDENWV6?=
 =?us-ascii?Q?7ryDIp1K4Jbf2ASaW9S8NlqnlMP18vN2leF5x+NCKeZHD3taxnNaHQ8eOIKv?=
 =?us-ascii?Q?bVi1M0OfEOeU3WXXr0DravvkHpa/nojtAlpfVZ5Zxq1P36IAs4NDEl3R/hnD?=
 =?us-ascii?Q?po9MRrIXx0xcRNIxhYVE+HCD0JwI8Km+4gpubMaoBB1cSGkWW82zEMQw+vug?=
 =?us-ascii?Q?hsEuus/jbsewL2Tp2oE79+yEbIan3k+Yl6MumDop1cGs4cyvHGWAMZ3g7U8M?=
 =?us-ascii?Q?WabFte2NNZUlmKDXhHxaVzATfhaqCz0BrjCsFqpmKuLyCJ6riRgyohnufmXw?=
 =?us-ascii?Q?jefn94NeGthHKChwDdn95K3MfEpPKH0tKLKGdSzsScojoxhd6shcK3ApT8SN?=
 =?us-ascii?Q?QtEwjaRNkYm/QYaE6SlPoz/iviJk4+poGLOvD9CbrY+tw4IuQzT4DX6HfIUZ?=
 =?us-ascii?Q?DRXm87Qa2sJ4H8ORIUki0hWDx13eueS+SIX5Nlt9TmJGROvl0U82s3ql94Ea?=
 =?us-ascii?Q?UnHxZDpkQ9Nj8YHNorvS7+hsvsBKeCFFD1blRi662IiiLUfZjfph+kihfJrY?=
 =?us-ascii?Q?wZWKa7k1HONfme2+KMIQxrfF/bRHh9H3BLNgffSNho1R2dpNJqhW8Yzir5Fh?=
 =?us-ascii?Q?5dddczuqhAmbAs2ZwFx+beTPkETLLxQiV9LbibNmQzdspA4ZRIWYJ0RXMhVk?=
 =?us-ascii?Q?pKbBiiVddhR4Q53AbVfD1X94w3sK/4hJ9qT0F4R+Y25acC/OVN9GjHZiuzjZ?=
 =?us-ascii?Q?xqmoYaDRqMtmgQe/juX7jyC5Z6B6KQ72g/WfLC4hdTX/RlrPGc+UzHYCj1B1?=
 =?us-ascii?Q?PykYLp7ej4mIimM42ITSTu7WQcd2L3kr7NipWnyAIW0E+oR8El9OWCdjVaRJ?=
 =?us-ascii?Q?J0d+hSDkaDjdvZkl7Ln7CsE3ZQoAWFeh2N/KfX5P8U/clmkUTWKrHJg63J0L?=
 =?us-ascii?Q?RZoT2kteghISWiqm2zwVsQJ5k3dYPn1y9Q6SqjiZwMxCGOSb1y5dhXqQcAPG?=
 =?us-ascii?Q?p+1+yqZQgEimgBinxILBc0W16PQZZIl0RWyv3tf86kX0wPvfO3+LtrIyo9qD?=
 =?us-ascii?Q?H8Mfp0atxzBUVOMdit3C9OiiWToT9jdcXFAA6Fbw8RIF3+YlxC8RLxSIMSCU?=
 =?us-ascii?Q?ADp+WvT6kk3V6yY6zi19H1mJKEiUWHyujWx64MEjwhjd8PYZVmje5VxEY1yY?=
 =?us-ascii?Q?8hmn4xkA38iyL2Bi1P9jbPbPSvYK7LVkAbPwkzWoiAznh2s7ARjIfQ+IoHO5?=
 =?us-ascii?Q?NwQ3OvWPfAnDqGloXEq97DtUxfW4pqspYXf8Bn1HB15hinkB3lyGw7lwUskS?=
 =?us-ascii?Q?HdQ5/DdUDW2WWRnp1MpLXobMBSqUf1Z8xSRzp5HMda8h9mukPQnMc2RJrTkE?=
 =?us-ascii?Q?LDJe7pKTT9/gVVhZ33y83w8WHYs5dSvUidhjwThhjeD78zcBr8aXTxO8n8aW?=
 =?us-ascii?Q?kALUOlmTd33ChYiR6xbbGMonJQrM5QzkbeTQBuaJB/somzWnEs8U8EBsabTo?=
 =?us-ascii?Q?e7ljYQ8j4AmBkZoNsJ6gOlr8U5SdbDXFm1Hz6Q8ID5DU+dATFqNaH66bsuf8?=
 =?us-ascii?Q?Sjfu688Fu+16J3zwI9lGY64KZxPyM9W2vVnxTVPlMuPj0iI040+p4R65W52d?=
 =?us-ascii?Q?XQ=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c19406ab-ead8-4a8c-d4c6-08db29a81c07
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6452.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2023 01:03:40.6139
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gMSk9jqA+9dxQ2lrvNUfbLCrJfymqVezeUh8ybG0d1FdUbKQ9CpMBqR0UkTDLA5vRzwEF6L/hch4nfjrUwuWIw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB7911
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

To understand the problem, we need some definitions.

The driver is aware of multiple counters (enum ocelot_stat), yet not all
switches supported by the driver implement all counters. There are 2
statistics layouts: ocelot_stats_layout and ocelot_mm_stats_layout, the
latter having 36 counters more than the former.

ocelot->stats[] is not a compact array, i.e. there are elements within
it which are not going to be populated for ocelot_stats_layout. On the
other hand, ocelot->stats[] is easily indexable, for example "tx_octets"
for port 3 can be found at ocelot->stats[3 * OCELOT_NUM_STATS +
OCELOT_STAT_TX_OCTETS], and that is why we keep it sparse.

Regions, as created by ocelot_prepare_stats_regions(), are compact
(every element from region->buf will correspond to a counter that is
present in this switch's layout) but are not easily indexable.

Let's define holes as the ranges of values of enum ocelot_stat for which
ocelot_stats_layout doesn't have a "reg" defined. For example, there is
a hole between OCELOT_STAT_RX_GREEN_PRIO_7 and OCELOT_STAT_TX_OCTETS
which is of 23 elements that are only present on ocelot_mm_stats_layout,
and as such, they are also present in enum ocelot_stat. Let's define the
left extremity of the hole - the last enum ocelot_stat still defined -
as A (in this case OCELOT_STAT_RX_GREEN_PRIO_7) and the right extremity -
the first enum ocelot_stat that is defined after a series of undefined
ones - as B (in this case OCELOT_STAT_TX_OCTETS).

There is a bug in the procedure which transfers stats from region->buf[]
to ocelot->stats[].

For each hole in the ocelot_stats_layout, the logic transfers the stats
starting with enum ocelot_stat B to ocelot->stats[] index A + 1. So all
stats after a hole are saved to a position which is off by B - A + 1
elements.

This causes 2 kinds of issues:
(a) counters which shouldn't increment increment
(b) counters which should increment don't

Holes in the ocelot_stat_layout automatically imply the end of a region
and the beginning of a new one; however the reverse is not necessarily
true. For example, for ocelot_mm_stat_layout, there could be multiple
regions (which indicate discontinuities in register addresses) while
there is no hole (which indicates discontinuities in enum ocelot_stat
values).

In the example above, the stats from the second region->buf[] are not
transferred to ocelot->stats starting with index
"port * OCELOT_NUM_STATS + OCELOT_STAT_TX_OCTETS" as they should, but
rather, starting with element
"port * OCELOT_NUM_STATS + OCELOT_STAT_RX_GREEN_PRIO_7 + 1".

That stats[] array element is not reported to user space for switches
that use ocelot_stat_layout, and that is how issue (b) occurs.

However, if the length of the second region is larger than the hole,
then some stats will start to be transferred to the ocelot->stats[]
indices which *are* reported to user space, but those indices contain
wrong values (corresponding to unexpected counters). This is how issue
(a) occurs.

The procedure, as it was introduced in commit d87b1c08f38a ("net: mscc:
ocelot: use bulk reads for stats"), was not buggy, because there were no
holes in the struct ocelot_stat_layout instances at that time. The
problem is that when those holes were introduced, the function was not
updated to take them into consideration.

To update the procedure, we need to know, for each region, which enum
ocelot_stat corresponds to its region->base. We have no way of deducing
that based on the contents of struct ocelot_stats_region, so we need to
add this information.

Fixes: ab3f97a9610a ("net: mscc: ocelot: export ethtool MAC Merge stats for Felix VSC9959")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_stats.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
index 096c81ec9dd6..f18371154475 100644
--- a/drivers/net/ethernet/mscc/ocelot_stats.c
+++ b/drivers/net/ethernet/mscc/ocelot_stats.c
@@ -258,6 +258,7 @@ struct ocelot_stat_layout {
 struct ocelot_stats_region {
 	struct list_head node;
 	u32 base;
+	enum ocelot_stat first_stat;
 	int count;
 	u32 *buf;
 };
@@ -341,11 +342,12 @@ static int ocelot_port_update_stats(struct ocelot *ocelot, int port)
  */
 static void ocelot_port_transfer_stats(struct ocelot *ocelot, int port)
 {
-	unsigned int idx = port * OCELOT_NUM_STATS;
 	struct ocelot_stats_region *region;
 	int j;
 
 	list_for_each_entry(region, &ocelot->stats_regions, node) {
+		unsigned int idx = port * OCELOT_NUM_STATS + region->first_stat;
+
 		for (j = 0; j < region->count; j++) {
 			u64 *stat = &ocelot->stats[idx + j];
 			u64 val = region->buf[j];
@@ -355,8 +357,6 @@ static void ocelot_port_transfer_stats(struct ocelot *ocelot, int port)
 
 			*stat = (*stat & ~(u64)U32_MAX) + val;
 		}
-
-		idx += region->count;
 	}
 }
 
@@ -915,6 +915,7 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
 			WARN_ON(last >= layout[i].reg);
 
 			region->base = layout[i].reg;
+			region->first_stat = i;
 			region->count = 1;
 			list_add_tail(&region->node, &ocelot->stats_regions);
 		}
-- 
2.34.1