Received-SPF: pass (google.com: domain of linux-kernel+bounces-164675-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: Tue, 30 Apr 2024 12:58:59 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>, Li Zhijian
	<lizhijian@fujitsu.com>
CC: <dave@stgolabs.net>, <dave.jiang@intel.com>, <alison.schofield@intel.com>,
	<vishal.l.verma@intel.com>, <ira.weiny@intel.com>,
	<dan.j.williams@intel.com>, <linux-cxl@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cxl/region: Fix cxlr_pmem leaks
Message-ID: <66314d8343103_10c2129413@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240428030748.318985-1-lizhijian@fujitsu.com>
 <20240430174540.000039ce@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240430174540.000039ce@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?J2GlMW5jrKnWyKurtyTDHq5/M79Wc+kaToIjhOQ6YC84yMtNzm3PuljBGPUx?=
 =?us-ascii?Q?FyVLvTqvurtCrn1bEmo/Qpoc1kc8V484RgZTHvghmQD9ZaqYYMgZkvDzEs84?=
 =?us-ascii?Q?es5kgZMNiExrdQyjS8HefgK3JQ9E3gQj1W77gw1kE+Addh6f8JxoHcJzD3g4?=
 =?us-ascii?Q?Nc3c9RLxRURVO/1EP5JwHSDhYWefp6sss7O0RYlsUOOxeJliQaU1pja6G2bE?=
 =?us-ascii?Q?1D3Tvi6w7TO2yurUjNPLSdONE6zp+JFZ63vQClqEM7nQlJzrHwDiH2bz+muE?=
 =?us-ascii?Q?ovZfxZ88ZO0fOuh1PvGcxtVEDm3VvM7gdafxE8ljLO8Azp1+5MeFHjpVUX6/?=
 =?us-ascii?Q?h8/SdfgIzIDkKzO/4fj2g9uM7RtG7L7jCE0Zajo3BLnUXfCfpv6rZUWR64Bs?=
 =?us-ascii?Q?6vtMtekHe+Df7swTOQJq0ZmMSSWVdsDuhaDO4HOMPqGREuw8vdBBcX+gvcDz?=
 =?us-ascii?Q?UyJmXMXeUuqBng8wLB73KFbLK5w9xj10Uy2MwASsXMqUqRr5RCzB+Bb2xdth?=
 =?us-ascii?Q?Za4/Am1/6cMOJsUeE9C13QGEEjI7n7bWPvLPlGqkMiB5Cf9n+CIF81qZF+XD?=
 =?us-ascii?Q?SabqqqPzAg/a+zzDvnC0ph9ORWk/iFC+fYBYXsIohRcWfNiGcGFRucf9kJsF?=
 =?us-ascii?Q?uAeqcnn/mS7bFdSVoHdIqIIKEqSL2xEWirt8wwZ/HLDZBVyUj4XZbFm1aOXe?=
 =?us-ascii?Q?Z6h5eni/aJORslBeNGipTiE4zGVkT8VJQSzLuhrMg1cN76XDm88vLmNJhXWT?=
 =?us-ascii?Q?Z3Vzesy4Rmqrmetjphjz8P3P+Fl2bkuhU+lEKAOw2m88uh2s0pcTJAigMvmq?=
 =?us-ascii?Q?/XOya+B5Bf1O8BZ5L3hCgDr8tsWFjR2xo1EymtpM+Z44BbSbm/M05EuikvU5?=
 =?us-ascii?Q?ENzGDJjw6hiuM8qsVBXT+K9nUkXg94bx9PN5ZgMfUZ5iSz6NT6YN9+SrzT4z?=
 =?us-ascii?Q?4a2uNbH+nRAXVrR41nqHNxlxqs3aBGZbhRVWnOyALEtlWyavdxQFF9ei88oW?=
 =?us-ascii?Q?8RFcjhL/9Xmk7sEx3cMjc3yZb6GlJEoYIqa+yr6BMp8s2R7LP0rynGRMCE+d?=
 =?us-ascii?Q?OVITqngWMCfZGHi8QIptrCYhnHerEbzKndfuZHSmpuIEs16oH7bLzXl8rDjk?=
 =?us-ascii?Q?YMa3tYCoHreCad5TN+fxw0J0RQPmtLvbrNUrG4Fbx9DSggoZ+ttvwXVQ+abh?=
 =?us-ascii?Q?6uPzU7YM6SlD5NoE2sL4MJeiFxWGetENI9BLPy5X0WOzNYuqmtEDugFJV6ZF?=
 =?us-ascii?Q?jytU0IHco0WS7HkIs4Q18fWe7jTO4WCaSdw4a8onK4dpldzvIyKx/h7kl4Wv?=
 =?us-ascii?Q?ykkXjObj8+RhfYkGzdcJFkdnNWqVQatxOsFJqTy4P0x4k8YFkWzIGWz6hFda?=
 =?us-ascii?Q?PQdjogYeg9QwYkFssA8pdsmKUdaE3L3gKufnoG0+T9hyBIJrnTboItIVmSi5?=
 =?us-ascii?Q?c2cI1pemlGAzBrv8t1oHSMefkHJopH8Lp7APUv+ezXv6nqiDMa2e8AvPQZDW?=
 =?us-ascii?Q?s5fKa4LKL7E+bNl4JzVi1fPI2R09DqQQ4D7CoS3+VI+VOTO0f2em6Eeq0O0b?=
 =?us-ascii?Q?C9NlZ/5rCUbQmKI6yNcbvyo77XdhGUKN9tQ1Zcl6rBIkUut9+HocgYoJht1s?=
 =?us-ascii?Q?7g=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 77226b73-dd55-48f5-da6e-08dc694ffb2b
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2024 19:59:01.8167
 (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: gUQBz+rMOSN+ARcJwMdzTBpbAN7/akkR/2JKgE7vcF71Bfjh0oMDUH0aL5g0WZERfSWh3BHDfgyMtJqWUAadysEkWT1bYgXjObOccftIIqI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6334
X-OriginatorOrg: intel.com

Jonathan Cameron wrote:
> On Sun, 28 Apr 2024 11:07:48 +0800
> Li Zhijian <lizhijian@fujitsu.com> wrote:
> 
> > Before this error path, cxlr_pmem pointed to a kzalloc() memory, free
> > it to avoid this memory leaking.
> > 
> > Fixes: f17b558d6663 ("cxl/pmem: Refactor nvdimm device registration, delete the workqueue")
> > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> 
> Fix is good, but this looks like nice case for conversion to cleanup.h stuff
> perhaps better to just do that?  Would need a small amount of dancing
> on the final return to return cxlr->cxlr_pmem + pointer steal
> when setting cxlr->cxlr_pmem a few lines up.
> 
> Also guard for the rwsem.
> 
> Dave, Dan, worth doing or take this minimal fix and spin around later?
> 
> If you think this is the way to go.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I had spun up the conversion patch as a replacement in response to this.
However, after looking at that I think merging this fix with a follow-on
cleanup is they way to go because the cleanup touches locking, memory
allocation, and changes the calling convention.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

I'll send the follow-on cleanup shortly.