From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 68FC5C636CC for ; Tue, 31 Jan 2023 14:54:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F0DE56B0071; Tue, 31 Jan 2023 09:54:38 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EBE556B0072; Tue, 31 Jan 2023 09:54:38 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D86BF6B0074; Tue, 31 Jan 2023 09:54:38 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id C9C396B0071 for ; Tue, 31 Jan 2023 09:54:38 -0500 (EST) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 97D80C0BE6 for ; Tue, 31 Jan 2023 14:54:38 +0000 (UTC) X-FDA: 80415390636.28.39C1660 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf07.hostedemail.com (Postfix) with ESMTP id DC3224000F for ; Tue, 31 Jan 2023 14:54:36 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=korg header.b=CJz47aPt; dmarc=pass (policy=none) header.from=linuxfoundation.org; spf=pass (imf07.hostedemail.com: domain of gregkh@linuxfoundation.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=gregkh@linuxfoundation.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1675176877; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=dGhVBHkfNDr8NXk5YqpIG62+OGzvy68sVsJbE1/PEdI=; b=YsQDZi/DUc7F+wVkyHAuefEi/vaG5keb7w/pXxdsxkYAbzsbBVkksBiimiSRuh2IkJWeeb +++IOjeqZM32YnVLkYvtxjPmXaPHfbhDJXloXR2RzZLbX30qQG5aDs2u9Umr3mQ5Kzdd1V BLon7+TNxI0YCD8NQPWYFrRWLxgWHzk= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=korg header.b=CJz47aPt; dmarc=pass (policy=none) header.from=linuxfoundation.org; spf=pass (imf07.hostedemail.com: domain of gregkh@linuxfoundation.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=gregkh@linuxfoundation.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1675176877; a=rsa-sha256; cv=none; b=KF/2nBhKy+c4Bh++Vkm18Cg9tOjvcG3yokGc9vs6wMJQVJ66ypHfvSuJ4WSyHHv4o7Hxc6 fgbkXm8AOlPimr8f9tIH8AJE4iTWleVar8mcB4zCN1t/F4luFn0EQzHOmmQiXSwms4R5Ff zoR3gvHh0+j2EHDBBiTXTTEp7bPwOF0= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B64DA6153C; Tue, 31 Jan 2023 14:54:35 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CBD19C433D2; Tue, 31 Jan 2023 14:54:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1675176875; bh=LkTpjPaCa+ALHaNDDa63BHYDcfbV54QQXWyMOfgVUK8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CJz47aPty2hMzTN8JvtOJOpOwVkwUGF4Me2UKY8wa0UIcbrwQIACmBSupaWHC2lAU 2tBqRRcVzblq0Wy4c1TyQ6bA838hHnemR/vdsOJ0legWxJUQawfZiYdA3cDB7v1HWb bOdtK8MMg2diZYZT7PhbOQGqW3RWmwK6KFq53cjo= Date: Tue, 31 Jan 2023 15:54:32 +0100 From: Greg KH To: Gavin Shan Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, david@redhat.com, osalvador@suse.de, rafael@kernel.org, shan.gavin@gmail.com Subject: Re: [PATCH v2] drivers/base/memory: Use array to show memory block state Message-ID: References: <20230120233814.368803-1-gshan@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230120233814.368803-1-gshan@redhat.com> X-Rspamd-Queue-Id: DC3224000F X-Rspamd-Server: rspam09 X-Rspam-User: X-Stat-Signature: k7bcp33bmrbt7rdpme1hdakrcft65e9c X-HE-Tag: 1675176876-766824 X-HE-Meta: U2FsdGVkX1+HzAehIawA+j4/vG69d0NG9j0k4aEodYxmCfZo4NRH6OZjdx5btkaQ3QGp2XpMB1Fs2nQPjbCwqmECvHSuou8Nd7zeLFWTTG/KvSDzW7s0B4oBXixXz5xMZ4xKGZZGbm6nqnNqk/i3fc2AKfT4Le+bmbkbp8JptJ7GIvV3WzY7LJGdffgv/B7/xb9eVUrqlLGURH4ApOzrny8PJ7R7P4fQOX41EsEdeerERI48cJ3H74I0vZd+L2gmjqR8k7TDJevtRuK0NRlYRNq23nXBU8xtdNJxotZd/L0CxmpGt1bYm5I9TdKRuXKGtkMxqwpKltfkoF/0nQVr/tT9wlweLjEU0AJ10ynNhYLRrY+BjQL87PSePzLjphB+nDCg5HTLOIQ01433pv8ihRZ5AxTwtvVkmxBRDhwI/FF4IiW5bw1wPkN9VTYI0cmgFgWkFUqFGk9y2TKIMheuuQX312jKWyEGGOGmeDtFm9RutW4TZiUL1E+bOnVnNBgz7tZO9x0DjL5OTEkBejk/Ytqi5DHjVqJSItOklAdsCOt0Efun2jTA/pU8scPBcDy3p8yFJgrIUm00rIFZdPjrnRLvCRryc1j81LebYStuNcDuSfMiM6yd5x14C1/XLyXelJceE6Lv2ORzWNnFok+07tp+dLBiM4D2LXWNtRIrOpETuJno/v+ECRQeprchaTZShpeLmsXaBTUZmqsabcCyXCsWrpEn2kZJUkLwprUZ2V39dOpCfLBjwJ1+dDPyiFGJQ5KPSXKF1nVT/jKX8/KW2DtsBq0a6AoKXDX78JX/9snz/iXED5DqG5UReewK/qrTkNllb+joEnBgz96zsMhMRPU7UiiMXOtd538Yy6dW27tGbxcsFIDf7nfkgrFo+bO5K3smqKpc8jSjPzZ+GAiJLn3eRCJJ8cAmNFXJasZ9IrFLBnCSKVwa1ZLCbHt96lrDEXo62k/jLa2BULZdoVu JM696P9a UFPGGjaU7O9yWQSBSaW8tWrcJ9b2x80yyxbnYWPmkL7up4lID4Hive3Ai5/PwYYJXVebcNf7ngg+A6q/k7Qjuz01mefFZEFYXExWx+XVHg+dyz2zsvuz7N9VLd2w9k1HvhSm7YINSo8L7RZVWxQHrBsy/qFVaATvvMTn9pyNE5FZvzTnx1/oL9EGr1FrZHjlLZwC2oksWpd66B7zDETY/tsoRaRJGo1XcZflIQkOoOqjLVr/XYn+OtFAZ6/65Mlu6rTq9/gwlp8nsTi7AEoVAWN1t/Qjdr7n9u/wVPMEceGk4CLAB7M4Muf0mDTLLVC2HNP+bMzncYiQu6szROrRoPRBkB7hIaLDq3ouv9PF3yrjOPWtZYuqOdaIqqMA0hqqpEMoQ X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Sat, Jan 21, 2023 at 07:38:14AM +0800, Gavin Shan wrote: > Use an array to show memory block state from '/sys/devices/system/ > memory/memoryX/state', to simplify the code. But does it really? Now you have an implicit binding between the order of this specific string array and an enumerated type that is defined in some other location. This makes any future changes really really hard to determine that you got this correct. Besides, WARN_ON() > is removed since the warning can be caught by the return value, > which is "ERROR-UNKNOWN-%ld\n". A system reboot caused by WARN_ON() > is definitely unexpected as Greg mentioned. > > No functional change intended. > > Signed-off-by: Gavin Shan > --- > v2: Drop WARN_ON() (Greg) > --- > drivers/base/memory.c | 25 ++++++------------------- > 1 file changed, 6 insertions(+), 19 deletions(-) > > diff --git a/drivers/base/memory.c b/drivers/base/memory.c > index b456ac213610..0fdacdc79806 100644 > --- a/drivers/base/memory.c > +++ b/drivers/base/memory.c > @@ -141,28 +141,15 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct memory_block *mem = to_memory_block(dev); > - const char *output; > + static const char *const mem_state_str[] = { > + NULL, "online", "going-offline", NULL, "offline", > + }; > > - /* > - * We can probably put these states in a nice little array > - * so that they're not open-coded > - */ > - switch (mem->state) { > - case MEM_ONLINE: > - output = "online"; > - break; > - case MEM_OFFLINE: > - output = "offline"; > - break; > - case MEM_GOING_OFFLINE: > - output = "going-offline"; > - break; > - default: > - WARN_ON(1); > + if (mem->state >= ARRAY_SIZE(mem_state_str) || > + !mem_state_str[mem->state]) > return sysfs_emit(buf, "ERROR-UNKNOWN-%ld\n", mem->state); > - } > > - return sysfs_emit(buf, "%s\n", output); > + return sysfs_emit(buf, "%s\n", mem_state_str[mem->state]); Overall, the current code is simpler and easier to maintain and understand over time. You don't have to mess with an array length, or anything else like that. I'm all for removing the WARN_ON() if you want to do that, but I think this is a regression in the ability to maintain this code for the next 40+ years, sorry. greg k-h