From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753172AbdL1DFs (ORCPT ); Wed, 27 Dec 2017 22:05:48 -0500 Received: from mail.cn.fujitsu.com ([183.91.158.132]:13269 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752840AbdL1DFr (ORCPT ); Wed, 27 Dec 2017 22:05:47 -0500 X-IronPort-AV: E=Sophos;i="5.43,368,1503331200"; d="scan'208";a="33252155" Date: Thu, 28 Dec 2017 11:04:57 +0800 From: Chao Fan To: Greg KH CC: , , , Subject: Re: [RFC PATCH] memory-hotplug: add sysfs immovable_mem attribute Message-ID: <20171228030457.GC15078@localhost.localdomain> References: <20171227123012.22159-1-fanc.fnst@cn.fujitsu.com> <20171227124726.GB3373@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20171227124726.GB3373@kroah.com> User-Agent: Mutt/1.9.1 (2017-09-22) X-Originating-IP: [10.167.225.56] X-yoursite-MailScanner-ID: A473348AE9FB.A94F0 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: fanc.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 27, 2017 at 01:47:26PM +0100, Greg KH wrote: >On Wed, Dec 27, 2017 at 08:30:12PM +0800, Chao Fan wrote: >> In sometimes users specify the memory region in immovable node in >> some kernel commandline, such as "kernel_core" or the "immovable_mem=" >> in the patchset that I have send. But users don't know the memory >> region. So add this interface to print it. >> >> It will show like this: "nn@ss,nn@ss,...". "nn" means the size of memory >> region, "ss" means the start position of this region. >> >> Signed-off-by: Chao Fan >> --- >> drivers/base/memory.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) Hi Greg, Thanks for your review. > >Why did you not also create the needed Documentation/ABI/ file update? Sorry, I didn't notice the document issues. > >That's required for sysfs attributes. > >> >> diff --git a/drivers/base/memory.c b/drivers/base/memory.c >> index 1d60b58a8c19..9cadf1a9dccb 100644 >> --- a/drivers/base/memory.c >> +++ b/drivers/base/memory.c >> @@ -25,6 +25,7 @@ >> >> #include >> #include >> +#include >> >> static DEFINE_MUTEX(mem_sysfs_mutex); >> >> @@ -389,6 +390,52 @@ static ssize_t show_phys_device(struct device *dev, >> } >> >> #ifdef CONFIG_MEMORY_HOTREMOVE >> +/* >> + * Immovable memory region >> + */ >> + >> +static ssize_t >> +show_immovable_mem(struct device *dev, struct device_attribute *attr, >> + char *buf) >> +{ >> + struct acpi_table_header *table_header = NULL; >> + struct acpi_srat_mem_affinity *ma; >> + struct acpi_subtable_header *th; >> + unsigned long long table_size; >> + unsigned long long table_end; >> + char pbuf[35], *p = buf; >> + int len; >> + >> + acpi_get_table(ACPI_SIG_SRAT, 0, &table_header); >> + >> + table_size = sizeof(struct acpi_table_srat); >> + table_end = (unsigned long)table_header + table_header->length; >> + th = (struct acpi_subtable_header *)((unsigned long) >> + table_header + table_size); >> + >> + while (((unsigned long)th) + >> + sizeof(struct acpi_subtable_header) < table_end) { >> + if (th->type == 1) { >> + ma = (struct acpi_srat_mem_affinity *)th; >> + if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) >> + continue; >> + len = sprintf(pbuf, "%llx@%llx", >> + ma->length, ma->base_address); > >sysfs is "one value per file", and if you ever have to care if you are >overrunning the length of the buffer, that's a huge hint you are doing Yes, you are right, the length of "buf" should be limited here. I saw in this file, one sysfs only has one value here. But the memory has more than one region and may be discontinuous, so I stitch the string. So should I send the new version, or you think this feature should not be placed here? Thanks, Chao Fan >something wrong here. > >sorry, > >greg k-h > >