From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C149BC433F5 for ; Wed, 16 Feb 2022 01:15:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237124AbiBPBPe (ORCPT ); Tue, 15 Feb 2022 20:15:34 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:39784 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234408AbiBPBPd (ORCPT ); Tue, 15 Feb 2022 20:15:33 -0500 Received: from smtp1.axis.com (smtp1.axis.com [195.60.68.17]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ABA6EF5411 for ; Tue, 15 Feb 2022 17:15:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axis.com; q=dns/txt; s=axis-central1; t=1644974122; x=1676510122; h=date:to:cc:subject:message-id:references:mime-version: content-transfer-encoding:in-reply-to:from; bh=g35iL+XdAZtrpkfdYXumLXKfbz2JqMVxn8HY3vybn0c=; b=cHtDzfaaJ3Nq1ldb6nDD+zE8+CJGKLHO8eDXw8K0Y3ljpLqw4ri1t1zT IyUEEo5g9aCOhWmWqPIGn9nT5+6myzeRL/jtRlc2sVEmTNYPk3PDHFwG7 o9zX219qU9fCfYyjJsIRhkCXJWVIP5Ub5htzYJkK1g6kUc0MlQV0vddrp zu5vQFn+LMzyTR2+CvF7yiyKostfJ8b2VRPi/sEP0AgcM0luEWYsHxQVZ 0tyksrsxA2jxdypbqCzeL4JHVvhhmAE7qgOLFXa81H7Cw6tFj8OLvVlEg Fed8cBjv3+9yGPaoSva1U42AeZdXtDp0LdynoTPnsebgvTELCDB5va8X/ A==; Date: Wed, 16 Feb 2022 02:15:19 +0100 To: Rob Herring CC: =?iso-8859-1?Q?M=E5rten?= Lindahl , kernel , "devicetree@vger.kernel.org" , Frank Rowand Subject: Re: [PATCH v2] of: dma_configure: Reuse existing DMA map range Message-ID: References: <20220214153208.2984026-1-marten.lindahl@axis.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: From: Marten Lindahl Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Wed, Feb 16, 2022 at 12:59:31AM +0100, Rob Herring wrote: > On Tue, Feb 15, 2022 at 4:58 PM Rob Herring wrote: > > > > On Mon, 14 Feb 2022 16:32:08 +0100, Mårten Lindahl wrote: > > > When unbinding/binding a driver with DMA mapped memory, the DMA map is > > > not freed when the driver is reloaded. This leads to a memory leak when > > > the DMA map is overwritten when reprobing the driver. > > > > > > This can be reproduced with a platform driver having a dma-range: > > > > > > dummy { > > > ... > > > #address-cells = <0x2>; > > > #size-cells = <0x2>; > > > ranges; > > > dma-ranges = <...>; > > > ... > > > }; > > > > > > and then unbinding/binding it: > > > > > > ~# echo soc:dummy >/sys/bus/platform/drivers//unbind > > > > > > DMA map object 0xffffff800b0ae540 still being held by &pdev->dev > > > > > > ~# echo soc:dummy >/sys/bus/platform/drivers//bind > > > ~# echo scan > /sys/kernel/debug/kmemleak > > > ~# cat /sys/kernel/debug/kmemleak > > > unreferenced object 0xffffff800b0ae540 (size 64): > > > comm "sh", pid 833, jiffies 4295174550 (age 2535.352s) > > > hex dump (first 32 bytes): > > > 00 00 00 80 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > 00 00 00 80 00 00 00 00 00 00 00 80 00 00 00 00 ................ > > > backtrace: > > > [] create_object.isra.0+0x108/0x344 > > > [] kmemleak_alloc+0x8c/0xd0 > > > [] __kmalloc+0x440/0x6f0 > > > [] of_dma_get_range+0x124/0x220 > > > [] of_dma_configure_id+0x40/0x2d0 > > > [] platform_dma_configure+0x5c/0xa4 > > > [] really_probe+0x8c/0x514 > > > [] __driver_probe_device+0x9c/0x19c > > > [] device_driver_attach+0x54/0xbc > > > [] bind_store+0xc4/0x120 > > > [] drv_attr_store+0x30/0x44 > > > [] sysfs_kf_write+0x50/0x60 > > > [] kernfs_fop_write_iter+0x124/0x1b4 > > > [] new_sync_write+0xdc/0x160 > > > [] vfs_write+0x23c/0x2a0 > > > [] ksys_write+0x64/0xec > > > > > > Don't get a new dma_range_map if there already is one. Check for an > > > existing map and reuse it, or else get the map as before. This will > > > prevent overwriting the old map which is still valid. > > > > > > Signed-off-by: Mårten Lindahl > > > --- > > > > > > v2: > > > - Reuse range map instead of getting a new and freeing the old. > > > > > > drivers/of/device.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > Applied, thanks! Hi Rob! > > Ummm, a bit too quick here. I was looking at the history to add a > Fixes tag. Looking at commit 89c7cb1608ac ("of/device: Update > dma_range_map only when dev has valid dma-ranges"), I think this > change may cause a change in behavior as 'ret' is used as a 'we have a > valid dma-ranges' flag. So if dma_range_map was set, but not from > dma-ranges, the behavior will be different. I really appreciate your second review on this. It is not very obvious how the ret flag is used in that case, but I agree it may change expected behavior. > > Instead, I think this needs to be something like the following patch. > Really we have 3 occurrences of the same clean-up in dd.c and that > should be refactored. But the below is probably better for a backport > and refactoring should come after. Actually, when I found the leak I saw two places to free the dma_range_map, the first where I put it in patch1, and the second in __device_release_driver. Now, as you also suggest, it seems the better place to do it is in __device_release_driver. But I must admit I missed the place in really_probe, so again, your eyes on this are very helpful. I will make a new patch on dd instead. Thanks! Kind regards Mårten > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 9eaaff2f556c..f0a508fd7913 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -629,6 +632,9 @@ static int really_probe(struct device *dev, struct > device_driver *drv) > drv->remove(dev); > > devres_release_all(dev); > + arch_teardown_dma_ops(dev); > + kfree(dev->dma_range_map); > + dev->dma_range_map = NULL; > driver_sysfs_remove(dev); > dev->driver = NULL; > dev_set_drvdata(dev, NULL); > @@ -1209,6 +1215,8 @@ static void __device_release_driver(struct > device *dev, struct device *parent) > > devres_release_all(dev); > arch_teardown_dma_ops(dev); > + kfree(dev->dma_range_map); > + dev->dma_range_map = NULL; > dev->driver = NULL; > dev_set_drvdata(dev, NULL); > if (dev->pm_domain && dev->pm_domain->dismiss)