From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f175.google.com (mail-qk1-f175.google.com [209.85.222.175]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 99E9628C866 for ; Mon, 12 Jan 2026 22:56:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.222.175 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768258584; cv=none; b=Z/2NPyYOxL6OQFNGheS/TYXhRfp41wT7djWaf2iLqClW+KxAhwrIf1pvNVuBdTGkxP5fv/6EkxS9aNSArgNJhidL7OAcjYoBrtA0qObS0CBn9snD7Uwrezz0g+M78vQDILSM4zQZ3x13279JdshnQseerRrys/M3lBfoONbyS7w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768258584; c=relaxed/simple; bh=ejCplHzC+Tu2w3PsevSnIjD8tqUKuLqW+qh97NVsHB4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=q2GUZEGa+TPfDCzUDFszj7A+uJG7eOfz/Y9WvD8HMYm19TkZpCJK7nFeOt6nFUR7q4IW0SwyqmqAtlzFS8k4+QdVt+YPM7HIdzlb3zL021tVHj1oNLKcUYk0HCIPWaq/8YkocDRMSrBUfgAgCBLTAYmb3luHQkA6JaLnGdd1g0Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=gourry.net; spf=pass smtp.mailfrom=gourry.net; dkim=pass (2048-bit key) header.d=gourry.net header.i=@gourry.net header.b=a1X6wWsX; arc=none smtp.client-ip=209.85.222.175 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=gourry.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gourry.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gourry.net header.i=@gourry.net header.b="a1X6wWsX" Received: by mail-qk1-f175.google.com with SMTP id af79cd13be357-8c2c36c10dbso641184285a.2 for ; Mon, 12 Jan 2026 14:56:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gourry.net; s=google; t=1768258581; x=1768863381; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=EI2wzbmRYxHUYAt7IwCanPsfLjl0x0cz9M/LLZ/wyRc=; b=a1X6wWsXfc1QoDeL3uyt5y8pCEz6wC2VEsT/mPPqlRMG8Um9ZtQpU1WUJr1FWJpcWN aSruaby7NTKFO9kWrFHdRGMTNE6AS0C1aKn/KQgeOuB/MIew6/bdpkM0fJg3CdDI1n7g AtMCoqOXyRUp4cWCAZV3b1/Pxka84Wc6hlz1EyP0P/dhEv/2HasuUCtcUjjefOSvs0hf 3UTDAfm/OCaaAF4x3HX/eOdA2/Ck7JRYC/ncwEdTFvaX9D1mjdeTzT3XfS8yKG9ogGmQ 6DwQfapSjNgxh5s3yaAboclIrLgcBzhpfahV0qF8eeFuWjT6ROEaOEqQc/x7+Cx2msJn zfsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768258581; x=1768863381; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=EI2wzbmRYxHUYAt7IwCanPsfLjl0x0cz9M/LLZ/wyRc=; b=CMaKJbqWVhgsnnRHGRvM5e2JmVTIBR/usfGD9fNyHGvej+5mSCGrgdkrFa71IxC30k gHIpF9cyayqFNEy8NEQ9Iq2eDr7dsTciifUXPIZ9W6VY9E3+F9iVb1uQsDKS2M9YA2qb 3lCIy3kDV/5pgAahNDCWsblxmC1BPEL0iXEA7Y0DNLES1WLDvPvn4rxHyor9dE66rfUR WdzJVwcaNNnAA7+hwodi54OS1+08m4G6voHGEzDZyX5p9gh2PUrvIM5AWo4+9JFXcXmX 6gXSvZS+oKrQl//94SwCMRPc+4/brR3gnhMcA+fTFnHHak8iyDz1rWpoQz7To0V1merc LV5Q== X-Forwarded-Encrypted: i=1; AJvYcCV9kDH3Y+5/u/8uwnXASUnfaKX0A51a/pEMiluepzXdemaQrOnE4AShJFNOsF2lbs7+J5wuJLyG3oD5DOI=@vger.kernel.org X-Gm-Message-State: AOJu0YyK5Y3cyWRDlIta57nwpmWJx+NWm4G75GoPjs5CpueumNoZn+d0 dL5Wi3PhqK05H5SIceI+SOTd6rHXn/a/xO2AlQexV4owamNuEPS/8IkxHIvhrCBODog= X-Gm-Gg: AY/fxX7Ns0S0CfSpNt1CuUIVEfJNhZcA0P3ZEzn1P13AZtWgETKJqcwDxV1Dq+8ZkDH 4wZ7JA4AJUXscoGJdxcXf4qUwDWW0LPWEeNgIcRnKfSisDzXvFbsrmTPTvN4q3RexUWDongzES9 2IPKgP5YapXGE2ZUUVb2ZMTj0xd+P39/46xWbO2NSVh2zilcUUm+FSMuts/BAGLQrd/yjpOaA0J PFB6d/6u2vLXFXa5rpgKr9IH5psikhFuuNArS6g1B6HRdDqBnIn26sJ0YmutUFCyD62+AfOkA9F tX5A/5dlQ1Az7kDsIwyv/HIPao0DlbMmI6pQzqE3Btm8UYqFPpdj6JXX710h+HDmaokyaOyNbN/ Nl0m+KQCX02cygLPxVXfZLtBoNmhqmghojmbTGxRHLsHxDqpUCxklLFSytDZfQOPqXf+8qxfSnu SydJlyElvKyZGIwjC45pqfpjzpboyGoMLP6DT+Ulw2cZvgE6mtHYrWbyHT7fD7Jk13DrGCAw== X-Google-Smtp-Source: AGHT+IFv3Sn0EsUlnOalQIMkDKqklRX1J7jw1KxbNLDLTvSkqm6TMh8FGq2VJ+f3/jQ5s5cnkQpChQ== X-Received: by 2002:a05:620a:4626:b0:8bb:7dd8:1922 with SMTP id af79cd13be357-8c38939d234mr2764850085a.40.1768258581518; Mon, 12 Jan 2026 14:56:21 -0800 (PST) Received: from gourry-fedora-PF4VCD3F (pool-96-255-20-138.washdc.ftas.verizon.net. [96.255.20.138]) by smtp.gmail.com with ESMTPSA id af79cd13be357-8c37f532bc3sm1599675485a.45.2026.01.12.14.56.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Jan 2026 14:56:20 -0800 (PST) Date: Mon, 12 Jan 2026 17:55:47 -0500 From: Gregory Price To: "Cheatham, Benjamin" Cc: linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@meta.com, dave@stgolabs.net, jonathan.cameron@huawei.com, dave.jiang@intel.com, alison.schofield@intel.com, vishal.l.verma@intel.com, ira.weiny@intel.com, dan.j.williams@intel.com, David Hildenbrand Subject: Re: [PATCH 2/6] cxl: add sysram_region memory controller Message-ID: References: <20260112163514.2551809-1-gourry@gourry.net> <20260112163514.2551809-3-gourry@gourry.net> <0233bdab-9b59-4394-9ce4-c3a5df2be06d@amd.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0233bdab-9b59-4394-9ce4-c3a5df2be06d@amd.com> On Mon, Jan 12, 2026 at 03:10:41PM -0600, Cheatham, Benjamin wrote: > On 1/12/2026 10:35 AM, Gregory Price wrote: > > Add a sysram memctrl that directly hotplugs memory without needing to > > route through DAX. This simplifies the sysram usecase considerably. > > > > The sysram memctl adds new sysfs controls when registered: > > region/memctrl/[hotplug, hotunplug, state] > > > > hotplug: controller attempts to hotplug the memory region > > hotunplug: controller attempts to offline and hotunplug the memory region > > Nit: Would it be better to use hotadd/hotremove here instead of hotplug/hotunplug? The terms > are basically synonymous, but I think hotadd and hotremove are more descriptive. I will defer to David on this. I think keeping the terminology consistent is better, but also hotplug is overloaded between physical and logical. It ultimately means the same thing to be honest. > > state: [online,online_normal,offline] > > online : controller onlines blocks in ZONE_MOVABLE > > online_normal: controller onlines blocks in ZONE_NORMAL > > The naming for online states could be improved imo. I understand and agree with the motivation > behind the names, but I could see the use of the word "normal" being confusing to less savvy users. > You could change it to include the zone for both (online_movable/online_normal), but I think it may > be easier to mark which one has drawbacks, i.e. change "online_normal" to something like "online_nonremovable". > That way, anyone who doesn't want to go find the documentation for these can understand the user-visible > impact. > > In any case, all of these attributes need ABI documentation as well. > This is what i was getting at originally, I will consider the other feedback and spin a v2 with this simplified a bit. I'm leaning towards agreeing with Dan and David that probably we just keep online/online_movable since it's consistent with base/memory.c, but we can continue to have this argument. I don't think we can reasonable get away from users of this interface understanding the implications of ZONEs, since whatever they choose to do dictates what zone the memory gets added to. > > +static DEFINE_MUTEX(cxl_memory_type_lock); > > +static LIST_HEAD(cxl_memory_types); > > + > > +static struct cxl_region *to_cxl_region(struct device *dev) > > +{ > > + if (dev->type != &cxl_region_type) > > + return NULL; > > + return container_of(dev, struct cxl_region, dev); > > +} > > What's the reasoning behind redefining this in this file? It's still defined in cxl/core/region.c, > so I would probably just drop the static there and include it through core.h. > Just cruft from rapidly moving stuff around. Will fixup. > > + rc = cxl_sysram_range(cxlr, &range); > > + if (rc) { > > + dev_info(dev, "range %#llx-%#llx too small after alignment\n", > > + range.start, range.end); > > This should probably be a warning instead. You do it for the next check which is essentially the same > case, so may as well do it here. ack. > > + if (!total_len) { > > + dev_warn(dev, "rejecting CXL region without any memory after alignment\n"); > > + return -EINVAL; > > + } > > I don't think this check is needed. cxl_sysram_range() checks if the range->start == range->end (i.e. size == 0) > and errors out. That should cause the above check to error out before this. ack > > + /* > > + * Setup flags for System RAM. Leave _BUSY clear so add_memory() can add > > + * a child resource. Do not inherit flags from parent since it may set > > + * flags unknown to us that will the break add_memory() below. > > + */ > > + res->flags = IORESOURCE_SYSTEM_RAM; > > + mhp_flags = MHP_NID_IS_MGID; > > + rc = add_memory_driver_managed(data->mgid, range.start, > > + range_len(&range), sysram_name, mhp_flags); > > Look like mhp_flags is only used once, I'd get rid of it and just use MHP_NID_IS_MGID instead. > ack - yeah this was cribbed from dax.c Thank you! ~Gregory