Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <529b10f7-6f74-9fdb-db45-0ba589ba0cb3@intel.com>
Date:   Wed, 12 Apr 2023 14:23:06 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH net-next 8/8] net: mscc: ocelot: fix ineffective WARN_ON()
 in ocelot_stats.c
Content-Language: en-US
To:     Vladimir Oltean <vladimir.oltean@nxp.com>, <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>
References: <20230412124737.2243527-1-vladimir.oltean@nxp.com>
 <20230412124737.2243527-9-vladimir.oltean@nxp.com>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20230412124737.2243527-9-vladimir.oltean@nxp.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dDZLZkxhQ0Z5b21BK1dHWk0zNkRMSHZKWDRjN3BjOVBXdlRMQlI1bWJwejlX?=
 =?utf-8?B?QU81WExpTHBIckk1UkczL0w0N0hsR2dEVUU3dUt2a292WjJ3RGgrUFRLRTRU?=
 =?utf-8?B?d3E5Szhsa092R3FoL21JNmVmM1MzUkJnTlFEcVJYNGc3NG1vOFRORlhJUmdv?=
 =?utf-8?B?T2ZGUWhOaE1XV0VpZDgzVkJZMkxYaWkzbWpYbDdXcWlkNjlMS01JMnVZL0Yv?=
 =?utf-8?B?QWZRZDBjV29XdGlLRkpkaDZFeFB4c2hialBGK0IvVjRVaTdKQ2sxcmQyWFl0?=
 =?utf-8?B?OElISUFyR0ZTdElXTHhxZnptQlJWRzVyTlRTdXVZYzM0L3o0WjZIUnM4ZDNi?=
 =?utf-8?B?Wk1lRkNaNkJnRGcrRGhKWU9ieTZ2eFZST1BuRjRQdFRwNGVpMzZ1anRRc29m?=
 =?utf-8?B?c291THFtQ2pRbEZFOVNNR3c4cW5PY1UzVkg0bVhCejVnYVB0WFJVSmlFZGNq?=
 =?utf-8?B?Vm1kbTBIMWNaMzFyTEVYdHNIdmh1cjVubll0YUJHSkl6SFhsQjZnV0w2YWhh?=
 =?utf-8?B?STlaWVVxK2tsV093OWVKYS9VVExTMDFpWndSOGluaThtaUxBMGFGY0VxZEl2?=
 =?utf-8?B?MW5UZkNLTEljY3l1S0hWSWI1UEo5ZUdqVVJTTTVvV3FqNHVjY3hKQWFPcEVM?=
 =?utf-8?B?RWVqcCsrVEpzcFZUWlp4b3A3dmFLMXc5SlMrVEtwaHdjNGxTL21FU0NqUWk3?=
 =?utf-8?B?S2hTOFo3cTJBV21NSzJZOHBDN0tSMUtxTkVkL3hQOGxaRDF1MmJUTE1vWHR0?=
 =?utf-8?B?RVBDdmdDKzd4L2FsQ0JoRGVVSmZIQU5abm1tc3VTdGg3c0lRTUJNRGErY0lk?=
 =?utf-8?B?QkkxWmdiQTBJWTRMSC91SitOTENOZVBWTHcvVStJMzEzV2NIVy8xZ3R2c01H?=
 =?utf-8?B?bWhlSUpDVVRHK2NHQlBUVXc3WFpaRUwzN3dVclBUSTFQYm5UbElIdlo3WHd4?=
 =?utf-8?B?MUwyZjExeFgxeU5pY2FvZnFiRmNtSzdjbkk5YnEyTFBWVXBBLzNpd3JnMXg4?=
 =?utf-8?B?OHlRQkkrTzl5MG9DUXc4L2xPdG5vazVKUk1QU0p2bDdUL0dJRUhaSUh2Ujht?=
 =?utf-8?B?dTdPVEljallpTm1xQUI0cng4SVRScVNLdVZhNkdwY1FMRnB0cHBMdTVLNWpl?=
 =?utf-8?B?dXpJZTI4N3Z1RXFZR0lLd0VPRjNoZUlLMzZNMDVmR0Vnd0ZZeDkyZjkra05D?=
 =?utf-8?B?MXpDRTJldm9aUmRlTjNXdXVPVU1pajk3V1gybU1HZ3Q3MWx5aGpCaXJobFNR?=
 =?utf-8?B?ejJpdTdTRDZpS010OGhCYU9BakZUdFhmMzcxVDk2SjhmK0pPb1FrM2FxV0JC?=
 =?utf-8?B?ZU9BRk9FN0RuNk9JaXNtNElaVFdOTmlrNU9LWUtXeUY4dXFMNHFvQzdaZnVR?=
 =?utf-8?B?QzJZWmVhK0pOV3Q1TG9uZEIrV2hRUjNmWEw0Tk1WM3I1OHk5Um5RVmIzUWx4?=
 =?utf-8?B?b3BYall4TWJMK1BYSGF3S0U0OGxpM3FGZHJRQUNTOXhwQXd5TDN2b1lLVWN4?=
 =?utf-8?B?aWZRVUdpV3ppSGVvenNrb3U2TkMxSC9VaU5UVWhLZkpGRDdIa2dVTCtFblR2?=
 =?utf-8?B?ZVdpaDg5a1hHZEdWajlNV3gvc2E0ZlRCZmQwcHNGMGgrY3JtVkZUTHpvNVov?=
 =?utf-8?B?TFZ0OWF5Z1lKclhPMVdTK3V5bTJKaDlVR1I1YTlLOHh1RkJTMDREdzdmTUlR?=
 =?utf-8?B?SEdpM2dqZ1VHREQxUWFHOWNIOFZuNGRKa1JtY005RXYxenVEQ2dlejFJY2pw?=
 =?utf-8?B?VFQxTFIwYzJCK1V6ZlZ5T3k5SVAvc1FWc1ZpTktEOGdPVE1DL3BsUWIxOHZS?=
 =?utf-8?B?SkF1OWdIRlRvbWJlRFpTNVdPOG5mZkZnKzlPTmZhcGxaUzkwZDJtUDNFMGVJ?=
 =?utf-8?B?NkpvSnFBc3o0V0FRWEZYQ1dDeEh2SUVNd1pNZ2J2eVdGS3JHQUJyVzJZcDNY?=
 =?utf-8?B?R2Rrc0N4SHZ0QkJsVlZGL2dWaUVEQXR2SGJBeUNlczE2R1RKenRsYUY0ekw2?=
 =?utf-8?B?SDVmWHU1d3kyb2lFRDVpQm5PUE4yd3FrYzV0UjRic0RyNkc4TzZoVm5IQVE0?=
 =?utf-8?B?TXJQclowMEJ5eWhyVE9ORGRHb2NXK01LaVFRUm1NYUdCK2JJNFN6bjAzNFM4?=
 =?utf-8?B?ZVAxcnhVMnIveEhKd3ZFNTZTRDNTWmhnbkN5c0d5MlNOZnpvTHUxQVdZMGR4?=
 =?utf-8?B?TVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b146a138-8aaa-4997-51e9-08db3b9c17cb
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2023 21:23:00.4877
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: balkpMDK6rBlCLVuHHUKPu73dJ3/JjAp/0yOIzFrD3HG3gDAPt8Uds7TklAue6SfMj56rj7L+60K1Ngcqh+zxpvPm2Osu6WyA20LiIbEfIk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR11MB5860
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-3.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> Since it is hopefully now clear that, since "last" and "layout[i].reg"
> are enum types and not addresses, the existing WARN_ON() is ineffective
> in checking that the _addresses_ are sorted in the proper order.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

This is definitely more clear after reviewing the other code dealing
with these encoded register addresses.

Nice fix.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks,
Jake

>  drivers/net/ethernet/mscc/ocelot_stats.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
> index e82c9d9d0ad3..5c55197c7327 100644
> --- a/drivers/net/ethernet/mscc/ocelot_stats.c
> +++ b/drivers/net/ethernet/mscc/ocelot_stats.c
> @@ -901,6 +901,17 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
>  		if (!layout[i].reg)
>  			continue;
>  
> +		/* enum ocelot_stat must be kept sorted in the same order
> +		 * as the addresses behind layout[i].reg in order to have
> +		 * efficient bulking
> +		 */
> +		if (last) {
> +			WARN(ocelot->map[SYS][last & REG_MASK] >= ocelot->map[SYS][layout[i].reg & REG_MASK],
> +			     "reg 0x%x had address 0x%x but reg 0x%x has address 0x%x, bulking broken!",
> +			     last, ocelot->map[SYS][last & REG_MASK],
> +			     layout[i].reg, ocelot->map[SYS][layout[i].reg & REG_MASK]);
> +		}
> +
>  		if (region && ocelot->map[SYS][layout[i].reg & REG_MASK] ==
>  		    ocelot->map[SYS][last & REG_MASK] + 4) {
>  			region->count++;
> @@ -910,12 +921,6 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
>  			if (!region)
>  				return -ENOMEM;
>  
> -			/* enum ocelot_stat must be kept sorted in the same
> -			 * order as layout[i].reg in order to have efficient
> -			 * bulking
> -			 */
> -			WARN_ON(last >= layout[i].reg);
> -
>  			region->base = layout[i].reg;
>  			region->first_stat = i;
>  			region->count = 1;