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 B099BC28B25 for ; Fri, 7 Mar 2025 23:32:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C711B280004; Fri, 7 Mar 2025 18:32:27 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C20A0280001; Fri, 7 Mar 2025 18:32:27 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AC210280004; Fri, 7 Mar 2025 18:32:27 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 8C64A280001 for ; Fri, 7 Mar 2025 18:32:27 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id A859D5664D for ; Fri, 7 Mar 2025 23:32:29 +0000 (UTC) X-FDA: 83196356418.08.32CC8F4 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.14]) by imf11.hostedemail.com (Postfix) with ESMTP id C487340002 for ; Fri, 7 Mar 2025 23:32:26 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=I7UXXK2j; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf11.hostedemail.com: domain of alison.schofield@intel.com designates 192.198.163.14 as permitted sender) smtp.mailfrom=alison.schofield@intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741390347; 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=/pCwvdDaXy8JvrG11zGmnHOEGK6uJk1zP/ayIk25FqI=; b=yDAaBUK24z37XqRzEt1ggOcNV/GMXejhFP0GkGBB15WHztTEWDrTrmAI2Noevz56P+2QyF HCd2G3D+5ev0ht3ac54pKehlM4hJKcilswaclbKW/bC6P0nq19vVkMG4CGMn2ptbJEZlL2 KYdeku40GDEsxLC90J/0vrLGQ39Y4tk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741390347; a=rsa-sha256; cv=none; b=Y0/ij8WVTblE6ZMtbgkD6/Nbip6OoAKFvN6GYgiYwrO/sQdjhPSShVgE1KMlJOVl+ro1tJ eee+kUTxUKWhRqJfIrgSbRxSOlJhLahvOz5VfzDe/JTX1JHtE+GVgYpHh3xLD7TCQ47H7n 0LeRUy82/sAhZjL0Q9HGRCI5QzwmAmM= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=I7UXXK2j; dmarc=pass (policy=none) header.from=intel.com; spf=pass (imf11.hostedemail.com: domain of alison.schofield@intel.com designates 192.198.163.14 as permitted sender) smtp.mailfrom=alison.schofield@intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1741390347; x=1772926347; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=9uegT5rClggSiNvb9L16+SIxJzylbDheZMGszd0JPAg=; b=I7UXXK2jretQtmdecoWgfEK73F1GsgrRVIazQg+Z/WpX8w9IzRR88cuo HuV6kWLWtza03NWlsuVoBeQbcaW2opL9XC0W+gh0oMz+ArruV4h5KTN81 tEVawKaYnu+mbIaANma/6Jzl9mePa0dDNEaQz10l5cv/+qpEcgbUdXmjT e95aFndiGND0cheQNs/EulUSDI8bPyvlKby6/yLJ3cdkzoRzu8AuMXJeM 9VSlhfm0MEEngDulDf4sCV9xWQdhs8ANzjzUpQ4QaiTN0pFwkOXx6zi8e L6NyXWe1/qv56Cp30Au37OUYbHv+q8XEEVr9MGW5eGUZ59OwpcMNqCRVg g==; X-CSE-ConnectionGUID: merlT5O7SnyChAKIYHU0ww== X-CSE-MsgGUID: IPWVp4aGRoWqKyUW1KzBGg== X-IronPort-AV: E=McAfee;i="6700,10204,11366"; a="42678652" X-IronPort-AV: E=Sophos;i="6.14,230,1736841600"; d="scan'208";a="42678652" Received: from fmviesa006.fm.intel.com ([10.60.135.146]) by fmvoesa108.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Mar 2025 15:32:25 -0800 X-CSE-ConnectionGUID: JIGCjZvrQSanygRLNBfyHg== X-CSE-MsgGUID: gIjzKmhgTLK+QXB0T03IxA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.14,230,1736841600"; d="scan'208";a="119278955" Received: from aschofie-mobl2.amr.corp.intel.com (HELO aschofie-mobl2.lan) ([10.125.110.159]) by fmviesa006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Mar 2025 15:32:22 -0800 Date: Fri, 7 Mar 2025 15:32:20 -0800 From: Alison Schofield To: Jonathan Cameron Cc: shiju.jose@huawei.com, linux-cxl@vger.kernel.org, dan.j.williams@intel.com, dave@stgolabs.net, dave.jiang@intel.com, vishal.l.verma@intel.com, ira.weiny@intel.com, david@redhat.com, Vilas.Sridharan@amd.com, linux-edac@vger.kernel.org, linux-acpi@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, bp@alien8.de, tony.luck@intel.com, rafael@kernel.org, lenb@kernel.org, mchehab@kernel.org, leo.duran@amd.com, Yazen.Ghannam@amd.com, rientjes@google.com, jiaqiyan@google.com, Jon.Grimm@amd.com, dave.hansen@linux.intel.com, naoya.horiguchi@nec.com, james.morse@arm.com, jthoughton@google.com, somasundaram.a@hpe.com, erdemaktas@google.com, pgonda@google.com, duenwen@google.com, gthelen@google.com, wschwartz@amperecomputing.com, dferguson@amperecomputing.com, wbs@os.amperecomputing.com, nifan.cxl@gmail.com, tanxiaofei@huawei.com, prime.zeng@hisilicon.com, roberto.sassu@huawei.com, kangkang.shen@futurewei.com, wanghuiqiang@huawei.com, linuxarm@huawei.com Subject: Re: [PATCH 8/8] cxl/memfeature: Add CXL memory device memory sparing control feature Message-ID: References: <20250227223816.2036-1-shiju.jose@huawei.com> <20250227223816.2036-9-shiju.jose@huawei.com> <20250307091137.00006a0a@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250307091137.00006a0a@huawei.com> X-Stat-Signature: 36upeatd4ja6xnjtb4rwj9ya9ss8w69k X-Rspam-User: X-Rspamd-Queue-Id: C487340002 X-Rspamd-Server: rspam04 X-HE-Tag: 1741390346-881387 X-HE-Meta: U2FsdGVkX19cH6Bpp1xaBr7mLaCX2Cd3e57BZZRdMwj/dyCp7FgHK0v4g/TD2vN9MjqxaBzokRnespStJCrKbmRZ5x8o+8zqzUlLver15sonJaWmEKyY528fPIOdfjR7TIAp2+RCVumA69ghhzNzrGi9qUZwvjFwp08N6zSFO6Ol0tfK3OusfNK4j3lCb7mDqDBAzB4ZzUhbelNv3/ut3oVP+/70DvAE8HB3bnLQ8H/CNVNtbLQgwj/OS2VKIKdTxjgOTQsphH8QRhs7xVDgWAEVclovTLa+o7836geqZCpOx7gOEoK5L7rUgklDNkG0BA7CkyntKPpIpm5radsvTU4yyaPay9DoVnoPg/4LZohFFjnvuGBEqU6dXe9X81ZxiPU8zGwX0P0poNp0WLLDxXoZpAcwp37T336NMAVDAc5fOYhezWjZuKhv8KsApMU46k4gnmGZXqFveZOGf/gjbZHB8PB7MvHiSS0osea6iIo4NzTDx+OAfpAxsvuir32lgrvJah3fB0QasLQkOfcmy/CqfbZJ4dQyRcMvmsD8JrDlUIXQMxXXlYsL2KG3efHolLO7m6Wu6K5wDCHFW3bEW92PMQeu4j28m824Iu6DS1sn+T+Ij9KvDz1dBy3FKTfA1Zvcsqj12LAGmpp2y+AxQy2Uxr/KJ7uu8Q/crugjy/2PqtkavvccE78vqbM3gZVeATwAZTL2iUug9JmMtjqdob+wC4gs+A3Kf29F4aLpj+biK9TmD8Ir8gt6yL0nmufW/fJmGZo4tNNMi6n2Iw1NqHhqiVg0W5Ekkxo+lm0N42XVu23eqED/1nHzhqaNxgYA2G98YsJWsV8c9xr2UwyZ/Fwn4i9HPFyC8/I94eixdgiEQI/mmC3GvflNNc5tY2sEjnqn6iaoHhWhbUdTD7N/nwvSJBYJHEjo5zKANWHTmngvYndWD+PHCcYGrcPNMO1VLu52DDw7EI4g/H/bCRj uj72diR+ +gmHPzlZNS/97ZwS/1567MPlZZBTS4ROTfA+bhOXkjimWqo2o01wsIuIBiifPQ2vcSQkFGlio+Qj0nUP40oKw54dfIJdVkCjz0rXPykdl6DjOw0Pjb9k/pApyvzevlO0PxRDA9WK7UT5yRK5ihZXkl+mK3Ol4sxkl47EjVB5qCZexBlsMSTGZj35MV1z0xz38IhFFdnlzuOlc7N3Hn35iynaVgz29AcvI26xkJbGTZHyIdSGRqUT+KtFSNwHC/jt0KZDdaWx1I4nEJGkdMtpCbW+Z39h1EDVAzqYo 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: List-Subscribe: List-Unsubscribe: On Fri, Mar 07, 2025 at 09:11:37AM +0800, Jonathan Cameron wrote: > On Thu, 27 Feb 2025 22:38:15 +0000 > wrote: > > > From: Shiju Jose > > snip > Similar comment to earlier on maybe using single line comments > in more places rather than multiline. Perhaps worth doing > that if you are respinning for other reasons. Following on Jonathan's feedback, a couple of things- - Within the CXL subsystem (maybe it's kernel wide) there is a style or custom, that comments that only need to occupy a single line only use a single line. This set should follow that. When code is styled uniformally it is easier to read. - This next thing I recognize because I have a bad habit of doing it myself. Narrating! Some of these (should be single line) comments are needlessly narrating the code. A comment is useful if it explains something not obvious, but when we have descriptive function names and variables, less commentary is needed. see below... > > Reviewed-by: Jonathan Cameron > > > +static int cxl_mem_do_sparing_op(struct device *dev, > > + struct cxl_mem_sparing_context *cxl_sparing_ctx, > > + struct cxl_memdev_sparing_params *rd_params) > > +{ > > + struct cxl_memdev *cxlmd = cxl_sparing_ctx->cxlmd; > > + struct cxl_memdev_sparing_in_payload sparing_pi; > > + struct cxl_event_dram *rec = NULL; > > + u16 validity_flags = 0; > > + > > + if (!rd_params->cap_safe_when_in_use) { > > + /* > > + * Memory to repair must be offline > > + */ > > + if (cxl_are_decoders_committed(cxlmd)) > > + return -EBUSY; > > + /* > > + * offline, so good for repair > > + */ > More places as below where a single line comment would be fine > and make a reader scroll a bit less. This got cut off, but I think the code can tell the story without narration. (per my other patch feedback maybe you will rename this something like is_memdev_memory_offline())? snip > > + /* > > + * Read CXL device's sparing capabilities. > a below. > > + */ > > + ret = cxl_mem_sparing_get_attrs(cxl_sparing_ctx, &rd_params); Great name, no clarifying comment needed. I'm not looking through them all. I'll leave that to you. But I appreciate you looking and updating to single line and removing the comment entirely where needless. It helps keep the code base uniform in style which makes reading it easier. Thanks Shiju :) > > + if (ret) > > + return ret; > > + > > + /* > > + * Set default value for persist_mode. > > + */ > > If respining some of these comments don't need to be multiline. > >