Subject: Re: [PATCH v2] dma-buf: dma-heap: Add a size check for allocation
To:     John Stultz <john.stultz@linaro.org>, guangming.cao@mediatek.com
Cc:     Sumit Semwal <sumit.semwal@linaro.org>,
        Benjamin Gaignard <benjamin.gaignard@linaro.org>,
        Liam Mark <lmark@codeaurora.org>,
        Laura Abbott <labbott@redhat.com>,
        Brian Starkey <Brian.Starkey@arm.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        "open list:DMA-BUF HEAPS FRAMEWORK" <linux-media@vger.kernel.org>,
        "open list:DMA-BUF HEAPS FRAMEWORK" <dri-devel@lists.freedesktop.org>,
        "moderated list:DMA-BUF HEAPS FRAMEWORK" 
        <linaro-mm-sig@lists.linaro.org>,
        open list <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>,
        Bo Song <bo.song@mediatek.com>,
        Libo Kang <libo.kang@mediatek.com>,
        jianjiao zeng <jianjiao.zeng@mediatek.com>,
        mingyuan ma <mingyuan.ma@mediatek.com>,
        Yunfei Wang <yf.wang@mediatek.com>, wsd_upstream@mediatek.com
References: <20211217094104.24977-1-guangming.cao@mediatek.com>
 <20211227095102.6054-1-guangming.cao@mediatek.com>
 <CALAqxLVA4jUk-2o28RZobrPDUnuXfV1xN77Pt8Pu1o27V3aUQQ@mail.gmail.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <c05749d0-4c24-8763-a459-27257b2524ed@amd.com>
Date:   Tue, 4 Jan 2022 08:47:54 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <CALAqxLVA4jUk-2o28RZobrPDUnuXfV1xN77Pt8Pu1o27V3aUQQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Q2FrcUx6aXlybTBqQkFzVTJZSCtOY01XS20xNmlaZ0FsQ09zS1ZiMkFLZ0Fk?=
 =?utf-8?B?WmVvMEMwY0QvVDlDQmZMZng1T29IUktFYmJKWVBZZEZST0xwOXBCYTM3c0Vn?=
 =?utf-8?B?TWxBMVJINDArL25oTFRzdEhkamxnQjV3NHZDWDlkWjE4QnRNOVJCSmdBRnhW?=
 =?utf-8?B?OS9FNWV3R0s3Q1FTS1g2UEY4VW8xaE1QVElFM0tObWNHMUkwWkM4b0hIQjMr?=
 =?utf-8?B?WUhsTjl1bE9tMU5IV2ZzOUVhQ0NJMXJYeTVoTkw4b0hXaTAzL0Z3SDR1YkVK?=
 =?utf-8?B?TTMxVFNwNXZYbVJRMmVpRDlVRUZxeWdMUXVyMWRnU3c1SHlmcTdlM1p0OXBK?=
 =?utf-8?B?QUMrbGw1L1hJd3FvN1VMaUFYWldjc2l1bWk4Mk1SL21HZURia2JlRXduT01u?=
 =?utf-8?B?U3VkUnI4QXJ5Wm45M2JlSFdXNmtOZXN6MHNYb2Z4TVhBWEQ5RU5MK0lWS0NM?=
 =?utf-8?B?eWVwSUVCcFBzdGsyaTZ1Vjc2VEJPRS9HNkx5TjhaY0tBeWhuWjdJQkt4Umpq?=
 =?utf-8?B?NW5QOTlZMWc4ZzZQTXJWVEpXN1RudUlIS3krVm5LeGxBVGpDYjVSK016Q2dl?=
 =?utf-8?B?U0JiUHU4NlAra2NGOHhFYmJ5YW0veEZJczZPR1RHb3RjZ0tZTlZnOXFtRk9t?=
 =?utf-8?B?QjBUZ29seHh0VUFHYnNoWXhXRnJmb0kxaXJRK1BsSXNNN29laU4ydnJDRFpQ?=
 =?utf-8?B?SW1WOGlqM05qTzhNRDhaUENiSjRLaG9HMVp6MzNPZFpzdEdxZ29Jc3ppeG96?=
 =?utf-8?B?aksvMW9iWmlRSi9XalIzTWtHTmNUV1FIcjJRbmNxK0hteEpYamV4NVd5V3JK?=
 =?utf-8?B?QTFwVjlRenVSTi8reWxUQklSYnZJOC9RQlBpd093aVVUZGFoU0hrazNXb05s?=
 =?utf-8?B?ZTA0ZStZalhQbEUzdUMyY1hmWEcyNWRwSXdOeFpXbDA1cjRmcFc2bEVkV0tq?=
 =?utf-8?B?R25nYldUZlptVkVxTXhhQkZRV2xSZS9hK1I0amxZcEF4M0JnRGZxVHVjYWIw?=
 =?utf-8?B?K0FMWENDNjExeWtxcE1wZ3EvR3FtVXZUZ1dkQ1lRU2xSZWxTejcwRzlPSVF6?=
 =?utf-8?B?OFo3VTB5QVB4b0VPdVhxNkNtMldiQXJ6ZFFtOUszSkg5UWZyb2hFd2FCemc2?=
 =?utf-8?B?eEMxNnlLNFdYRTZjRjYrakNKY2VFc013YUR4WU5POWJiMDlTb2k1Zlgzek03?=
 =?utf-8?B?MDFmdWl3SzVQQ3JNL3BDRVhvL0Q4d2VONEE0S3BJcXdHSjRHZUViYkFLNDVN?=
 =?utf-8?B?N0swYTdmWU42ajFhcHhKaVFUbWdIUTNmbmNCWjkxZVVSanZUdW11R3N2OU1i?=
 =?utf-8?B?RXNuTlJ5d2RPM3lBR0VFOWExcHVVWkxBeDg2UFF4R0VWUVB6MUxQaEloVjlk?=
 =?utf-8?B?dWIrQXpPbGx2b0VjRXNYclcxVEl1WWplS1BCY3plWWlXVWRUcUZNajM5Y1po?=
 =?utf-8?B?aGRZSXhBR1ZJVjNxZVg4T1dsZHlnalo0MzB1SlltS2YycmFicnR4VmpyN0o2?=
 =?utf-8?B?bGF3MTIwN1BXRlM3aW91ZmxlZEdFbHlWdHZGSHVSL0tVckRpRm5wNWVscUh4?=
 =?utf-8?B?UDIvSVFmRlF2T0o4bkhGNXZkd0JNSlQ2R1l1UTRnaVRQY3lDNmtrZ3hTUWpv?=
 =?utf-8?B?UFR5MUE2cXExV05zRmZmUEFQTm1UTFFYRnFORkltRzExMEVGWDBpNWE3NFBW?=
 =?utf-8?B?VHpiU0djSFJIaThqZlNkd0RGY1Z2b2RCVmh0bU1NZzNnQWV4SEdLZ1Z5NlBI?=
 =?utf-8?B?c3g4a3pXTldTaGFnZEs2ZzNCc3ZUVXBCSHZNeC9idWRFUmVEbEFwS3hFZkw3?=
 =?utf-8?B?OU1wUkV6VVpmeFZXYU5vWEJ5VHlySC9zODdCY2dCYmxLQ0p2YjdudWZpcEVt?=
 =?utf-8?B?VnRtaE9BNllEMHl1Zm1QLzlKU1ZLM0NrYWo1bjRsMXBXT2ltOSs0TitpZEFZ?=
 =?utf-8?B?YkNGR0lYRHdseVJjTUt1citJbXpvZGNSUytZWGphdERDMWxIWFQ3SDF6OXpz?=
 =?utf-8?B?TmRDaW14dzdPY0tqTUdQKzhPTXExa3Fwck5pcHpjcEZJMW90alBsaEhWeGZV?=
 =?utf-8?B?ZkNpaFRtNWlmK3Q4R3VjUm5sVUVESjA0SThIL3JGMSsyclBmeFlaSktVcXNM?=
 =?utf-8?B?NG9OYzFkRDNqeWd0S05ZallMYWVYZlkwWXJRaDZjV2hiUWIvUlNXT1d5NDFt?=
 =?utf-8?Q?bmp1cgeHXVVD/tJUxXaG5BE=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dbe28ac9-d954-4061-97a1-08d9cf568a6e
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1201MB0192.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2022 07:48:05.0963
 (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: lzUH+RJ/48KKBhYBKCp7lHA0rvnl4SwHJEG3fOY4WRoQOZab2+ucW81PJqxqu93x
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1201MB0238
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 03.01.22 um 19:57 schrieb John Stultz:
> On Mon, Dec 27, 2021 at 1:52 AM <guangming.cao@mediatek.com> wrote:
>> From: Guangming <Guangming.Cao@mediatek.com>
>>
> Thanks for submitting this!
>
>> Add a size check for allcation since the allocation size is
> nit: "allocation" above.
>
>> always less than the total DRAM size.
> In general, it might be good to add more context to the commit message
> to better answer *why* this change is needed rather than what the
> change is doing.  ie: What negative thing happens without this change?
> And so how does this change avoid or improve things?

Completely agree, just one little addition: Could you also add this why 
as comment to the code?

When we stumble over this five years from now it is absolutely not 
obvious why we do this.

Thanks,
Christian.

>
>
>> Signed-off-by: Guangming <Guangming.Cao@mediatek.com>
>> Signed-off-by: jianjiao zeng <jianjiao.zeng@mediatek.com>
>> ---
>> v2: 1. update size limitation as total_dram page size.
>>      2. update commit message
>> ---
>>   drivers/dma-buf/dma-heap.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
>> index 56bf5ad01ad5..e39d2be98d69 100644
>> --- a/drivers/dma-buf/dma-heap.c
>> +++ b/drivers/dma-buf/dma-heap.c
>> @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
>>          struct dma_buf *dmabuf;
>>          int fd;
>>
>> +       if (len / PAGE_SIZE > totalram_pages())
>> +               return -EINVAL;
> This seems sane. I know ION used to have some 1/2 of memory cap to
> avoid unnecessary memory pressure on crazy allocations.
>
> Could you send again with an improved commit message?
>
> thanks
> -john