Received-SPF: pass (google.com: domain of linux-kernel+bounces-162679-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 29 Apr 2024 09:23:01 -0700
From: Ira Weiny <ira.weiny@intel.com>
To: Li Zhijian <lizhijian@fujitsu.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>,
	<linux-cxl@vger.kernel.org>
CC: <linux-kernel@vger.kernel.org>, Li Zhijian <lizhijian@fujitsu.com>
Subject: Re: [PATCH] cxl/region: Fix memregion leaks in devm_cxl_add_region()
Message-ID: <662fc9659f6ab_19ca3929435@iweiny-mobl.notmuch>
References: <20240428053715.327444-1-lizhijian@fujitsu.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240428053715.327444-1-lizhijian@fujitsu.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?NuSYdMJQSRl75JIUHe5TzSBRBgDUFXBsorCButRU5d87IXnS8gyguBOqkY92?=
 =?us-ascii?Q?+m1XTO/qpk9BEkvq1s/+sdIu3ZCcwEf6m3znxohToLMce/QMVFNOh0w8ckqw?=
 =?us-ascii?Q?zHsEuiu36pRbJifFkvcyMhQU3kmoYwlxK6r/akeElFllUKEwjsvTQS1UDIur?=
 =?us-ascii?Q?uA35L8F7ZO+OWDBbTihNsb46LQozcNeadP8IUcEpI9yjAzRRA/WadBF/OTBv?=
 =?us-ascii?Q?KsY/YyE7n32kLr+EIVGhH63iQQHuVSvNl9MB9+wi+A617EisTPXzjuk19F1c?=
 =?us-ascii?Q?YlT1ro7boBlao7qQK0Npg+Aol8/9evuair75Ntze4SrrS+sgVN0EEwpgWdqu?=
 =?us-ascii?Q?34VaIniVxjgMxa6bXwROqTl6kFDeE+oCh/0KIMjKvKyHj2rgkrTMrvkOaNZh?=
 =?us-ascii?Q?TJWLvSbUzGMVoPIdPRp9YOAdVZg64hb9LWDJSv9Tz3lpkwtQRyX7hyp+1rOQ?=
 =?us-ascii?Q?8+voL1xIk3CS5DLCcQTbqQ7cmpKeg6eSVnMrt6NG3CbkQW1TkRGxmKODgWS1?=
 =?us-ascii?Q?REIFCiXY0j5B+NUl1qnty/5BqnAPnrOS0mFbaOqT2YzwzFeuvHA/SHmY/+Ws?=
 =?us-ascii?Q?HcuGCFdPZEjfyUlPQ+p7chTKg1dCTo28uV+4HV9o5VBF0mw0G7DYnsI8rB6d?=
 =?us-ascii?Q?7pmg3RLC48Z0/k76OBfS76wfQO8zkx4qOiAo0WTxaBp7yBgSpee0oax9T9Fe?=
 =?us-ascii?Q?R8X6KpIKuPB0/dq0g3kg8ggdMCRYy6XiDJ1IueC0nCN7P9JTWNNy672AC3Ft?=
 =?us-ascii?Q?nAbR4XlUAgmMwWKiKiH+svo7v0xR4Ad7DBxjjbiS+SGusp3W86SZL2oxa04C?=
 =?us-ascii?Q?HlXk6fkKwS5rCDc3Uo3jthEXRt/5TV08oKOjy+SbzM5EAm0jUV1g8nUjX8EB?=
 =?us-ascii?Q?s2C68eSnwQIGnt0I18ysLaFnd0FOr+Ire/dyrt6x4Ylq3fyZfdCpWQ54KyH6?=
 =?us-ascii?Q?YEiG+zTJS5V3l8mxErsaSWL0CE5RTzbFYQK+HkXga4/Ciy6jQ3UHPcGua4Fe?=
 =?us-ascii?Q?CkTVRu1VSePtsmN0+IsBfmM1rtzmsTorT4TdT5Htav0m67gIFQufR6v2jks4?=
 =?us-ascii?Q?MZ4AeSEJ4hMKy4cAEPitlqjHZtjL1PrpGnQX/BG1VOMzrqLjsIUB66q/qW6j?=
 =?us-ascii?Q?NOnbZOAfjdTfmNHai5m+KEUdS1c0ZlhfYKaVI3/CP0b+Zf5p2MPy8cWbeP62?=
 =?us-ascii?Q?IMvEXFdNlyc6GFIRTRASF+qpqynVw2XBlvyUMVg+tJcWiwbBuMkuZhAcCYI0?=
 =?us-ascii?Q?VTPgIaVmcYBWmQ7Y3cNz9P5KmHoV8QMi6aFBlxDYyi0cdUnyZGnYLyJBsU2E?=
 =?us-ascii?Q?zkLGNyHPjMdn1zEAcz1KQjVr2DTmMmF6lYooYLKKelYHSPgDtrlE+okz9Ico?=
 =?us-ascii?Q?hoeSz8GFUHM/O8jvLYLLqJedakz+AGftHMpm0fcRJKCc/WHjr2CJDVAHuNFS?=
 =?us-ascii?Q?agwh8pb5zAJcNt2HRGOjb0cjOgE/3HYYrNuof+TdxTucqAy+SY98KHXqqYMP?=
 =?us-ascii?Q?VD6RfAJkYwI2UuOS3V14ZZ3qn0bBGAvQkXwAfyxrUzAFE3Mui+NEKgO/BLgD?=
 =?us-ascii?Q?ZvaYx7PNrK4dOQOe3tn14FTuB9GiDFYcdKZ6n4jg?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7821af0d-d909-4972-74c5-08dc6868a5e6
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 16:23:05.1193
 (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: MiAjIKBnLlPvEvo9MgfBcPB2gG2Uzotd+dgIG50ciRJBkWRJbz9C2r2Q5WBELx5qZ1QtOnQxnnEeMRKqBSKmyw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB8176
X-OriginatorOrg: intel.com

Li Zhijian wrote:
> Move memregion_free(id) out of cxl_region_alloc() and
> explicately free memregion in devm_cxl_add_region() error path.
  ^^^^
  explicitly

BTW you should run checkpatch.pl which will check spelling.

I'm not following what the problem is you are trying to fix.  This seems
like it just moves the memregion_free() around a bit but the logic is the
same.

Ira

> 
> After cxl_region_alloc() succeed, memregion will be freed by
> cxl_region_type.release()
> 
> Fixes: 6e099264185d ("cxl/region: Add volatile region creation support")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  drivers/cxl/core/region.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 812b2948b6c6..8535718a20f0 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2250,10 +2250,8 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
>  	struct device *dev;
>  
>  	cxlr = kzalloc(sizeof(*cxlr), GFP_KERNEL);
> -	if (!cxlr) {
> -		memregion_free(id);
> +	if (!cxlr)
>  		return ERR_PTR(-ENOMEM);
> -	}
>  
>  	dev = &cxlr->dev;
>  	device_initialize(dev);
> @@ -2358,12 +2356,15 @@ static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
>  		break;
>  	default:
>  		dev_err(&cxlrd->cxlsd.cxld.dev, "unsupported mode %d\n", mode);
> +		memregion_free(id);
>  		return ERR_PTR(-EINVAL);
>  	}
>  
>  	cxlr = cxl_region_alloc(cxlrd, id);
> -	if (IS_ERR(cxlr))
> +	if (IS_ERR(cxlr)) {
> +		memregion_free(id);
>  		return cxlr;
> +	}
>  	cxlr->mode = mode;
>  	cxlr->type = type;
>  
> -- 
> 2.29.2
>