Received-SPF: pass (google.com: domain of linux-kernel+bounces-162664-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 29 Apr 2024 09:14:21 -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 2/2] cxl/region: Fix missing put_device(region_dev)
Message-ID: <662fc75dc1106_19ca3929456@iweiny-mobl.notmuch>
References: <20240429013154.368118-1-lizhijian@fujitsu.com>
 <20240429013154.368118-2-lizhijian@fujitsu.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240429013154.368118-2-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?2JyFkt18Hx+cyrchHfZ1V1Xa89PlHC1B7UIqTtr5PDHaPij1ahd9kBTP0EyF?=
 =?us-ascii?Q?aLb+XSdVVw6zUntdA+Yx8Gl8HLPciZbHDVssN5e04NDpYXI7gtKnALw/t6nN?=
 =?us-ascii?Q?O1KE9l8tj0sa/WryYhxDmw9kdBF+jSb3Y9tEYxSYvcQaqQVaz5KDaeoGJAx2?=
 =?us-ascii?Q?aDRgHoFrEdfHEqwqOkIL2EDhjWMSL6hHJMf7rmMYKN3QYhOinuHgQ/EN+w/k?=
 =?us-ascii?Q?VkMzMLBXBdWKnhP1uWdnsoXNzQrJ9vH5ZhoBcd805Hs+l7g49mlJo9EwDVG1?=
 =?us-ascii?Q?fIXvW8kpnkvKo1hrmkZaB80BLJGhX33OANu1IRCqUZ3E/S75Oaz9SiGH43SJ?=
 =?us-ascii?Q?qXacHvcwrSofrWbdnI/zQWfiCS6KoR/rDMqW8L2fOL47GZ3mioGibok3eJNR?=
 =?us-ascii?Q?1is90PpI1uU/NDCFzT6ueM2EOSvdRwMjMovdDd1fssSDDsJ3+K/dl3TDRXdr?=
 =?us-ascii?Q?hBEISymOtpQxLrtke8IT32iPC8IL2RBbgG4UmDP6uxX7gp9j+97PRH6Zjq3k?=
 =?us-ascii?Q?EtsX3x69qmqByFMsM2ozqXBjJOPkbTNFQPIIHLl7k98c35Uixiq30MyIS1bn?=
 =?us-ascii?Q?1tFbbsoEcknmZdc65u2i6fC5GbkQouE0Rdbz4Eez7gcH/hdZFH3iQYycYWtK?=
 =?us-ascii?Q?qEHNY1HJt/H2umP4Nffkma5zTQVkMn1rfz9pVDJmAjnQF53UNcXwI0ErFmGO?=
 =?us-ascii?Q?8eOYSTEFqfgp+5BcaAigq8jamnDObSYOY0rd4JMLvH803lH348HQZzN7tQ03?=
 =?us-ascii?Q?FQPI+3uUDZyH49vAVJdAinwPt6oXzoOLT1ZFPywE5S/w/jQM/hmBRXeTeeLF?=
 =?us-ascii?Q?z4Z+ZrfQWo0wlmXIjdUFATs4oQaJuu92dUiFm6HqUoySCCJZ9V/M9IhJCPXu?=
 =?us-ascii?Q?yNaf5Qp72MgDhqQuzXOS+SKsqesFY+knzrwlMofde0i0KrFkgFR+0s4SnRgf?=
 =?us-ascii?Q?CuVDVv0RKw0FRf/UxeZRgF8IM2IJoERnqrdZ4pBzaw5yd/RTUUrZ4QeMThvx?=
 =?us-ascii?Q?OHf1GfFURve7rpnR3+yh3Mt9zrdNFDC6IghTAozPDrak3JhsGsO9Q4w1ue7T?=
 =?us-ascii?Q?0gV6Ozh4XSG9QOU97Afghouh4h6BfP6oP6fwSXg4qm/UnCQ8RQRgyeS0pcz2?=
 =?us-ascii?Q?zI2xN+F26J+ThhLpoVz9/UMdYfKnNYst+oRhox5Efseo6iMNmrmj+fKBpX04?=
 =?us-ascii?Q?CIBCiExAqzL1KJflVdvlqeltA/kMdAK8z0S20mn7ayIN55yq2LnJ82wKYfyj?=
 =?us-ascii?Q?b4n+fR0ETpTh5VWhoa3VXj9zQZN3zv2bkXEkgPrcPb4yzaGW3tDP1MMyUkOD?=
 =?us-ascii?Q?wZjeT0U3cxfkDjz6mMLP3FKyNvwbFFgQ7HsNrtBPxZ2thT/GCdx6DQaUOWLp?=
 =?us-ascii?Q?OXigwnZGDn6PktJ1AIkvDlvbCeS8PTdc3wpK8cpyl6cl3oFjeAUBy/c1afaD?=
 =?us-ascii?Q?Dl/6xP/6J2L1lcE5caEc4/1ufoxAKq5ePkMBrGGWsyb5zQLO7Ve4MFNk2IIR?=
 =?us-ascii?Q?aMguFTB6l1FPry45trGR7Q10lNR0IuUuCWericSeHNPtXAdxxyaOlWvEx9r4?=
 =?us-ascii?Q?Lghk9kQ3s2W9hGkcXJxcitS8DsO63259HkIx+tmu?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1d557f22-2591-4f32-226c-08dc68676fb2
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:14:24.6753
 (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: RZmsz8NFtIL2xh4JKqgGOMKpxU5TS8G2YVrwFis2SXqAxlZ29ln1Pm2+BT3Wcg1z1pQt6/DPjmLY9K38gT0U6g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4757
X-OriginatorOrg: intel.com

Li Zhijian wrote:
> >         mutex_lock(&cxlrd->range_lock);
> >         region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
> >                                        match_region_by_range);
> >         if (!region_dev)
> >                 cxlr = construct_region(cxlrd, cxled);
> >         else
> >                 cxlr = to_cxl_region(region_dev);
> >         mutex_unlock(&cxlrd->range_lock);
> >
> >         rc = PTR_ERR_OR_ZERO(cxlr);
> >         if (rc)
> >                 goto out;
> >
> >         if (!region_dev)
> >                 region_dev = &cxlr->dev;

This diff hunk in the commit message is very odd.  I would drop it.  We
know this builds on patch 1 where you made the above change.

> 
> When to_cxl_region(region_dev) fails, put_device(region_dev) should be
> called to decrease the reference count added by device_find_child().

I __think__ this is what Dan was pointing out but I'm not sure.

I wanted to point out that to_cxl_region() can't fail because the
device_find_child() is checking that the device is a region device.

Dan, is that what you were saying when you mentioned there were more
serious issues if to_cxl_region() were to fail?

Ira

> 
> Simply put_device(region_dev) if region_dev is valid in the error path.
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
>  drivers/cxl/core/region.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3c80aa263a65..75390865382f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3117,8 +3117,9 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  				p->res);
>  	}
>  
> -	put_device(region_dev);
>  out:
> +	if (region_dev)
> +		put_device(region_dev);
>  	put_device(cxlrd_dev);
>  	return rc;
>  }
> -- 
> 2.29.2
>