Subject: Re: [PATCH v4] dma-buf: system_heap: Use 'for_each_sgtable_sg' in
 pages free flow
To:     guangming.cao@mediatek.com, greg@kroah.com
Cc:     Brian.Starkey@arm.com, benjamin.gaignard@linaro.org,
        dri-devel@lists.freedesktop.org, john.stultz@linaro.org,
        labbott@redhat.com, linaro-mm-sig@lists.linaro.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, linux-mediatek@lists.infradead.org,
        lmark@codeaurora.org, matthias.bgg@gmail.com, robin.murphy@arm.com,
        stable@vger.kernel.org, sumit.semwal@linaro.org,
        wsd_upstream@mediatek.com, kuan-ying.lee@mediatek.com
References: <YaB/JHP/pMbgRJ1O@kroah.com>
 <20211126074904.88388-1-guangming.cao@mediatek.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <260c250e-e867-a797-d899-f0b285315c56@amd.com>
Date:   Fri, 26 Nov 2021 11:05:31 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <20211126074904.88388-1-guangming.cao@mediatek.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZkRTMkdwSXJHNUlmL2F3emh1OVJ3Qjcrb3hsMmxhWXdMRzBsL3A2eFdIajlM?=
 =?utf-8?B?OVMxcjZhZFZOd3lOQmYwZU82RUhMYzhUdm5YUk40R2w4N25SeThEaTJuUmQy?=
 =?utf-8?B?VW9wR0ZZSHBiMDEvcmpVWVg3UXgzbjRvSlM3ZzJsTVkybk9XbjEyWnF0N0Ju?=
 =?utf-8?B?Q1NiLzdkSllJYmE5a0ZkRkgyRWsvc2RTaitGYmJBTFJ6WCtrdVJmSkVoM2s5?=
 =?utf-8?B?T3kxeS9wbnI5ZlVDVG8wVEpXWnlGa2w3cnZiQUJIZGlCM1NSU3c2T0Vzck1G?=
 =?utf-8?B?anBMUm9tTlR5ZGg1RklMRDZLNERCOUFvOVdhUm9FNHIwRjdmdFBlL2hhd2tS?=
 =?utf-8?B?cU9LcUlveGNKbTgyNUpBMWFDaHU5ZFhmL2ZtR3FMaDI3NmlpVVZWbUtCdUxP?=
 =?utf-8?B?dlVjNEFsd2lKdStPL2JpbjlRcEdEaGJZcFA2Z1RRVldrTkNCZktsNGhXN0F6?=
 =?utf-8?B?SDJXbndqWldGTCtIRzk2dTJpN1pMZUNyRkJvRmFDNjRVN2ZnbUVpbm10MkNi?=
 =?utf-8?B?cjlaQ1R3NFdTMUpnV1RMRW1MSkhKQ3NhUkpid0hFM0Z4ZVpPdCtHdjRkaGdL?=
 =?utf-8?B?UnJqWCsxZGhPM3BuMTdRSExZc1F0em15d1Z2azdMUkY3RnVEcU1RVGZPY3dm?=
 =?utf-8?B?cURxYk5MTTZvQ3Q0UU8yTm5SWEpvVE1IdytYUU9zV3JuY0ppRFd5MnNTNnBX?=
 =?utf-8?B?d2F6ZWRPejNQcldFMjh4emNrS2hjZnpwS0YvbkpNeVA3MGY1SStYbGhCekNJ?=
 =?utf-8?B?U1orUjlXeEFSZ3ZtbHkxRHZNcHFLM0hMeGZmK0F2dlZ1citwMDl6U0dPeDBi?=
 =?utf-8?B?ZUY3eUFLMXdqMmpSVUZxWmZHT21CMFo5UXZ3ZWZLcDhYakpjbEpWa2YxL2lx?=
 =?utf-8?B?T25IWFdINjNpWnZZb1F4N3o4WjBBWGVodVFjWEdsdXM1NGJLN3N6Y3IxK0J4?=
 =?utf-8?B?ckxVUmZLdnoveXNTZ3c3WWxOVUZ4d3F0Sm5tU3M0aVJuSWVYZGl4TVJpQ1dX?=
 =?utf-8?B?bTNaaWVXRUFGSTU3ZDAwMTRRWFEwZnU0ek9VdE00S0JmejVXMW5JeXF4cVZp?=
 =?utf-8?B?c3U1OGRieXpCZk43UDhtOEE1K2JQYjhZbWdhQ0hUOGFFKzEwazJPbkVtLzFH?=
 =?utf-8?B?b05ycjlWN0ZPNCs0MWRxSzRVTXVNV0hLdFBCbkxvcFplbTFnWkpCY0cxSG9W?=
 =?utf-8?B?SHN0MmxBM3VNVnNTbTNlOFhsNTIxTFZpQWlDRk1lTDduR3c0QWZWcE80aU9t?=
 =?utf-8?B?aUdhU1FHdDBUemhuNFp3YkYwOFE0TXM2cldHTGVKZmJjRE5sTTJhWXNuSUpQ?=
 =?utf-8?B?Ukt1MXN6d0Jjck8xemRxTXR4b1J1ZlpxTlZmZTdOVUhMaEk3b29NTDNhOXE2?=
 =?utf-8?B?Wk1Db25lQVNBV0pESTI0NkFRWitiWG5qTmM1K0ZaZi82S0ZaVER1R2hrcTlR?=
 =?utf-8?B?UHZJN2k4YWhhWjRleGE5S2xDZnlIckI5NlYzakVsUFk5RDM5RHJub0NDY2NP?=
 =?utf-8?B?emkzUExVMThHUnJLRko1M094UDhXQ3FRQUROTW8yUVdRSlVPRnUxUXNKbzJq?=
 =?utf-8?B?Q2l0ODNlSjFraFB3RUt3N05pMEJ5bHhwTmRYNmZZMHFrT1JPaFd6Z0pQUTln?=
 =?utf-8?B?bkc5NGF1UTY3cjczQjh0TFdNT05idXJkQUM4YWF2ZnpmVXRQSmdDS2txMWxD?=
 =?utf-8?B?MmlYc2VhRURKVXpYbFFqQ3FFbDR5c01rb3lhYkduY0FlOWxvYTBxN3Q3M05F?=
 =?utf-8?B?eGJmVlcvQ3IrR2IrbDhlVlZ3WE9yQ2VNV05Rbm9zc0ZrY2FPYmp2dUxNZTIw?=
 =?utf-8?B?MmgxL1FFemZWSElEcklFd0hPYWEra1FPdjMzRGZheVhBYktOc3QzZzRzKzdr?=
 =?utf-8?B?QlhPYTFRbGRLVE84WjJwT3ZWcnd5eFY2NnVnREFXNkJUR2ZiZ0FwR1VtVFJZ?=
 =?utf-8?B?eWNMaWdScHJ6S2xlNDYvaTVMMko5eHM3TGRrWURFRURmYmpqbTlTVUM3MlNC?=
 =?utf-8?B?MCsyQkZMb3d4cjFnVzlvelR1UjNoQzhwVmZoMDgyQnNpaGdVcll4U0gwNnVW?=
 =?utf-8?B?NWUxTXFtUTNRRnAvWGdaZGtLNTBGdHJ6QzBPWWRSRWJoKzc4S0FobktGSUZG?=
 =?utf-8?B?VHZCNkNrVGFqcmRIc2xDWVc0OURHbHdVN2thdmhMa1V0VTZLK0w3ckJYcXdQ?=
 =?utf-8?Q?e/2zoPdPB2elJOSL6dTArC4=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2dae7444-ad3f-4794-b66f-08d9b0c44da2
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1201MB0192.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Nov 2021 10:05:41.7681
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Ocvy1BRVmc40ju7yAV1YYMrUaEG0hQTFZM1E7hxBcvG6g4JH84GA9BOZ79TLRIXX
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1487
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 26.11.21 um 08:49 schrieb guangming.cao@mediatek.com:
> From: Guangming <Guangming.Cao@mediatek.com>
>
> For previous version, it uses 'sg_table.nent's to traverse sg_table in pages
> free flow.
> However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of
> created entries in the DMA adderess space.
> So, use 'sg_table.nents' in pages free flow will case some pages can't be freed.
>
> Here we should use sg_table.orig_nents to free pages memory, but use the
> sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common
> helper 'for_each_sg' which maybe cause memory leak) is much better.
>
> Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available")
> Signed-off-by: Guangming <Guangming.Cao@mediatek.com>
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> Cc: <stable@vger.kernel.org> # 5.11.*
> ---
> v4: Correct commit message
>      1. Cc stable@vger.kernel.org in commit message and add required kernel version.
>      2. Add reviewed-by since patch V2 and V4 are same and V2 is reviewed by Robin.
>      3. There is no new code change in V4.
> V3: Cc stable@vger.kernel.org
>      1. This patch needs to be merged stable branch, add stable@vger.kernel.org
>         in mail list.
>      2. Correct some spelling mistake.
>      3. There is No new code change in V3.
> V2: use 'for_each_sgtable_sg' to 'replece for_each_sg' as suggested by Robin.
>
> ---
>   drivers/dma-buf/heaps/system_heap.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 23a7e74ef966..8660508f3684 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf)
>   	int i;
>   
>   	table = &buffer->sg_table;
> -	for_each_sg(table->sgl, sg, table->nents, i) {
> +	for_each_sgtable_sg(table, sg, i) {
>   		struct page *page = sg_page(sg);
>   
>   		__free_pages(page, compound_order(page));