Received-SPF: pass (google.com: domain of linux-kernel+bounces-168106-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 3 May 2024 12:09:27 -0700
From: Ira Weiny <ira.weiny@intel.com>
To: Alison Schofield <alison.schofield@intel.com>, Ira Weiny
	<ira.weiny@intel.com>
CC: Dave Jiang <dave.jiang@intel.com>, Fan Ni <fan.ni@samsung.com>, "Jonathan
 Cameron" <Jonathan.Cameron@huawei.com>, Navneet Singh
	<navneet.singh@intel.com>, Dan Williams <dan.j.williams@intel.com>, Davidlohr
 Bueso <dave@stgolabs.net>, Vishal Verma <vishal.l.verma@intel.com>,
	<linux-btrfs@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 05/26] cxl/core: Simplify cxl_dpa_set_mode()
Message-ID: <6635366717079_e1f582941d@iweiny-mobl.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-5-b7b00d623625@intel.com>
 <ZhSPHvoTHl28GXt1@aschofie-mobl2>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZhSPHvoTHl28GXt1@aschofie-mobl2>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?o2uVBqF3RfDDLq0w5e+lGl6LXLbWSF3raVX1zXRhh9KHRp1BMRMUrLU2P91r?=
 =?us-ascii?Q?Y/ks/9PEFcncC1/5sIycbCv7syxQpLmWCEPm8Ze9nj3sFxYbcRb5O6H4yPvV?=
 =?us-ascii?Q?Ecl47hHioPj5lC/52b7VwzHBPq9jEJ626evNH8n2r3qVRTsCOeKQuMmb4UAs?=
 =?us-ascii?Q?YGHv21+FPIflKs6uOQNpblJdevYFB/ORJ2j6V6+1OZXf7iU6gYnAUAOy+Qaj?=
 =?us-ascii?Q?Bwhxqar8KuaAtrQtg7zjYsolwF5bS0emu9RZVRhmaXbetJxOpu+8FmWNhZZ2?=
 =?us-ascii?Q?Vm5ZRlsmvxg3jWDYrh9XCMv2utzqAU4ypDvAVfFISqwhmcF4tEq+WA0sEdQ3?=
 =?us-ascii?Q?0N9m3BwonHGR24gJ24hwmwu6enT8apBmVm8aAbFd35Df3w87iYUiGnPtNvRy?=
 =?us-ascii?Q?ldPFtB9B2ET9dWDWPBa/P1txrCOJdbRrEoUU1Gq0QKuW9h31PD7dJDMntiu8?=
 =?us-ascii?Q?at8PEpD4OVDRWi5viSF9pFEiehtfaUOJH5EfoNT0VxAxsVteSqtMx+2tv3Rw?=
 =?us-ascii?Q?sy4NQUyby5aSsjUsXPv5mrDuhsmUI1f/HOSp4Qkku4j00VlXP9sL1ldkTWQ4?=
 =?us-ascii?Q?urVYwH82S8gtEuTmAiRXHg358i1TLGPMhvIX2F8nbNbOeMl3DyP9vtiwZ1dn?=
 =?us-ascii?Q?WhXem37HooSwmIP9bQMKtQ1DTwU+S3idud3mQAnHZh9IPuqdPpxkLzYeb3+E?=
 =?us-ascii?Q?EdIng9ofqOIM2cMyiQYIMKBZU1J8ub+D0QoyQTES0zjgc33WjAT05q8vJRyj?=
 =?us-ascii?Q?65zFssoHpuG4fcyAHMZvRsfEQye8cqtnm2DQXDbiMlJiAjEIAbC3suq9GVeZ?=
 =?us-ascii?Q?uTm/vDXraHMek3eG0DKcciHiC+a5cZ3r2uYNtOqRZLjqXEJkzIqGV6IET0zf?=
 =?us-ascii?Q?NMBnTT73YF0N6ubDgMGF3aKEfmDnSDPRa7UxNXQcDeLlvWdsDIepLKOCYI87?=
 =?us-ascii?Q?RX8wGFzd/38+oQs2FecKJ2ZB4j7pmw4CSK7k00c1ShijgEpk++3gpK4VjJzT?=
 =?us-ascii?Q?0DH04IchvYpF7SHx6rseEcVcziVVGRD5u/KJfmTv+jifFimA8JYmxRD2of4R?=
 =?us-ascii?Q?4Yv0BSACstljakw5/xXipS/Y3Sx++Nt40Ng9qcD8+zx223SEGYyLVdg1b7NL?=
 =?us-ascii?Q?zYh03oQVqQTEx8HdqdK3Jb39LMzDa/iDT2e0KmkobEcNt2IqTE+86lEsLHvz?=
 =?us-ascii?Q?J+PJEBCLO+XiCA4CONDBrrhoi6Ug1OjdNyxw0p69gGDUmgfMCu5lOBH/3uIA?=
 =?us-ascii?Q?iMa915mhOgQge9QuYlM9TjP2Fa4zILsZt2ibY8jabfH2euU7U3VVIpyfbz5s?=
 =?us-ascii?Q?VJsD/UXNN8/OnFOkGGPo+rX/r3Jbw2jv99v0sUGToF9W+43Gg95GdHQY5X7C?=
 =?us-ascii?Q?6L+o7WkyoIEsooJOT96mQhuXqmFUZ9wx1mtROUnX6ZgicdDTv3VFtOa97ZQB?=
 =?us-ascii?Q?oPFvZLmuEMJK0JjdyrIsz0i54oXqNzJS6mIszxa6zDESK9gXO1FDz9PJcnB1?=
 =?us-ascii?Q?/soUQzguzJSISl38LwQmCYSFINNR+31Goezrw8n1prf4Og1bbucQ6ubtjTPb?=
 =?us-ascii?Q?MCU7jOKMjdgDajV2MAqBMFOMFZtdWHgC8viN8XU7?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0d3dae98-a683-4208-68f7-08dc6ba48ff7
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 19:09:31.5703
 (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: grlN2k42p29FUbLqe1FevHBOMZYKsINWFSqBz6GJtP8ovdkaeeoR7RoQAZVgqk5bhMU1QH93jESVzu8yXBPJrA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5072
X-OriginatorOrg: intel.com

Alison Schofield wrote:
> On Sun, Mar 24, 2024 at 04:18:08PM -0700, Ira Weiny wrote:
> > cxl_dpa_set_mode() checks the mode for validity two times, once outside
> > of the DPA RW semaphore and again within.
> 
> Not true.

Sorry for not being clear.  It does check the mode 2x but not for
validity.  I'll clarify.

> It only checks mode once before the lock. It checks for
> capacity after the lock. If it didn't check mode before the lock,
> then unsupported modes would fall through.

But we can check the mode 1 time and either check the size or fail.

> 
> > The function is not in a critical path.
> 
> Implying what here?  OK to check twice (even though it wasn't)
> or OK to expand scope of locking.

Implying that checking the mode outside the lock is not required.

> 
> > Prior to Dynamic Capacity the extra check was not much
> > of an issue.  The addition of DC modes increases the complexity of
> > the check.
> > 
> > Simplify the mode check before adding the more complex DC modes.
> > 
> 
> The addition of the DC mode check doesn't seem complex.

It is if you have to check it 2 times.

> 
> Pardon my picking at the words, but if you'd like to refactor the
> function, just say so. The final result is a bit more readable, but
> also adding the DC mode checks without refactoring would read fine
> also.

When I added the DC mode to this function without this refactoring it was
quite a bit more code and ugly IMO.  So this cleanup helped.  If I were
not adding the DC code there would be much less reason to change this
function.


[snip]

> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 7d97790b893d..66b8419fd0c3 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -411,44 +411,35 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> >  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> >  	struct device *dev = &cxled->cxld.dev;
> > -	int rc;
> >  
> > +	guard(rwsem_write)(&cxl_dpa_rwsem);
> > +	if (cxled->cxld.flags & CXL_DECODER_F_ENABLE)
> > +		return -EBUSY;
> > +
> > +	/*
> > +	 * Check that the mode is supported by the current partition
> > +	 * configuration
> > +	 */
> >  	switch (mode) {
> >  	case CXL_DECODER_RAM:
> > +		if (!resource_size(&cxlds->ram_res)) {
> > +			dev_dbg(dev, "no available ram capacity\n");
> > +			return -ENXIO;
> > +		}
> > +		break;
> >  	case CXL_DECODER_PMEM:
> > +		if (!resource_size(&cxlds->pmem_res)) {
> > +			dev_dbg(dev, "no available pmem capacity\n");
> > +			return -ENXIO;
> > +		}
> >  		break;
> >  	default:
> >  		dev_dbg(dev, "unsupported mode: %d\n", mode);
> >  		return -EINVAL;
> >  	}
> >  
> 
> delete extra line

You don't like the space following the switch?

Ira