Received-SPF: pass (google.com: domain of linux-kernel+bounces-128034-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <99b0bc65-a3ce-4522-a1dd-a304498fc453@oracle.com>
Date: Tue, 2 Apr 2024 18:52:12 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drm/panthor: Fix NULL vs IS_ERR() bug in
 panthor_ioctl_tiler_heap_destroy()
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Steven Price <steven.price@arm.com>, Liviu Dudau <liviu.dudau@arm.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
        Heiko Stuebner <heiko@sntech.de>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, dan.carpenter@linaro.org,
        kernel-janitors@vger.kernel.org, error27@gmail.com
References: <20240402103358.1689379-1-harshit.m.mogalapalli@oracle.com>
 <20240402143337.38e864e9@collabora.com>
Content-Language: en-US
From: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
In-Reply-To: <20240402143337.38e864e9@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?ekQ5bE5JNDZXcDdUYXlxdzlveWpmRUNJdlNoa0RFVXp4Y3M3WG0wcnZMZXRB?=
 =?utf-8?B?djhBcWJXRzl6c2NpTk5kcTZtM202VnpZdWVRUThKWm1DYzE0ZEJmb001c0RO?=
 =?utf-8?B?QjdwMmVxNUhVdmVlMUtwcmpLRzB1dURmVDRxMlV2VWZ4MHdMZEk2OWFGWjRt?=
 =?utf-8?B?VnJUL0d2RkZ3QjV6b3c2azczMkRvV3ArS01PeWNSN3ZTQ1lxa2NZY0JZbHla?=
 =?utf-8?B?bnc0bDVxZm8zb1o5VGU0UkszR21DcVZUUmYvZ1BDbEN4U0hwUUVUdFlNb01M?=
 =?utf-8?B?SzFtWVdrUnJQYTdMK29taHNZSVdKc2xXSk9vbVFDQ25sVm10NWk2elFRSGdr?=
 =?utf-8?B?RTNCRnB3NUMxNGprcEcxZ3NXa1pNeG9LeVpGK1N1REZTL0I0OTZFVnhSY3Nk?=
 =?utf-8?B?RytDV3o4NTNzYUI2WFhWTk40Q3grRzRnQUtqTmphMmZxSTAvK1hGZ3VvVUR5?=
 =?utf-8?B?OWU0N1l3ZFdzbmVoc3pZeE1GT3gxMldYU2tWVEx0TVNPekF6akkyMkZHQ3FN?=
 =?utf-8?B?YldLTUpIZ014dW5MQ3FWQThLK0xwa2pyeTdSTTM5bUswY2xDak56SFVVcnFq?=
 =?utf-8?B?cVNXTFI1NXppZEllNkszUURlNFBrTFZiT0ptaDlsR1pHMURTWlhKbmZTQkEz?=
 =?utf-8?B?c2cyS3ZiUXhrK05valpQMzE5SEZEL3Q2WFc0NzZZZGtxZ05GTi9VaklBVE1S?=
 =?utf-8?B?ek9wY2tESmt0L3B6WHJMQnQvc3VETi81SmNTc0RKOE5pMFA1Ni9qdDQ2RXBZ?=
 =?utf-8?B?R3JIT0NGUytwU1dBYWpZTU5sbm9SOXFlSzBzUmNWbUEvam1aV3ZnSlpGV2l2?=
 =?utf-8?B?dkRXSXlkeXNXeFVtZENBWHlNejN6M21qUFZZOEkyZ0NpS2ZpQWROZDBQSU5Y?=
 =?utf-8?B?YVFOak54YVFCOGdoMnBSYkJOS2x0U0RmWC8xQUtyKzg1WkV5YTV3MTUwNFhJ?=
 =?utf-8?B?N1A4UmpLUlE1WCtlaFRwS2s0Qm9MOTlycDh6Y0k0MGFmSTF1dUlkaGJOTEc4?=
 =?utf-8?B?UTdNNW16OEI1dCtxaE9DeHg0STZJdTI2TWFBQktDdWNLUU1ZUStpUmhwMko0?=
 =?utf-8?B?OU4vMWdBWlBWNHhROS9pZTJnenNRdk84MkYzY1QyV0R5NytWSEcxUXBNemJH?=
 =?utf-8?B?NEVvbWRQRFhxSTJOU3J0L2p3MTJPUDZkUVFoZEF5eHBab2RkeTIwUEFQM21o?=
 =?utf-8?B?dlg4M25MS21CejlRSFBaSUZmeStaT3IrdXlCSE5aM21IMGJuUmZ4bW1iVFhI?=
 =?utf-8?B?WnpGSG5lSHJhem9YZTF2OVFRVTFTSmQybXA1bjhibURKcjVjTm9WR2x5cDh5?=
 =?utf-8?B?SWJkOXZ4d0xpT0l3ekJJZkZ1UklOUURBVCtEZlRkMUQzd3dKTWVsZFdnR21C?=
 =?utf-8?B?blpMZ1d0LzEzdG9wMWNCN2VzZFpiMHUyTjdnOUdoVDNaWW95cWRIdm9BK3NB?=
 =?utf-8?B?TXFBMXBheldiMHJhaEUyWFJJdG1aV3dmRGJLeTRBc2k2ZjVTb1dJMkNQU0ZL?=
 =?utf-8?B?eEszQ3ZTMnNXcFdXK1QwRDAyWHZObVVIRkpjbmhENy95Zk5kMGk1K2pEcllB?=
 =?utf-8?B?UG1Dd09TYldnOEpoZGUzR3JOUXV4MDVFUkdFSmRkL3JTR3pRbzVJd1MxYVl2?=
 =?utf-8?B?QzZXWEhrVzZWY3hDODdQemIxNE5iakJvSmlQelF5YTRsWkRVSEpFTFFSdkFZ?=
 =?utf-8?B?MisrZUVmN1RybDZDWFZ6UlNKejF2ZStUWVVnRkJUZktmNjVaMUpVVDFwRGVt?=
 =?utf-8?B?aHNSVmZPZEZtaDRNQXJQWjFxaGpnRFhGNmpBenZXYjZCR1MwaDdnNGRPTlA3?=
 =?utf-8?B?aW9RQ3RwNDlab2pCQXhMSnY2UVBjenFjcjJwR0NzZXAzK0pTc3N3Nyt3R0cx?=
 =?utf-8?B?WjBLK3lWTEh3TXFxSk9FcHd0d1Z0S3NuWUZXNUxiMHMrdmdzNWEwNy9zQTYz?=
 =?utf-8?B?SFhoN0k5U0dMWWhJVXhCU2JkTlZKOXAyYkxoZk8xMHhzbXVBSHlTU2ZsaDlz?=
 =?utf-8?B?Um93U1JEN0g5RmpWZVozWEZ6VkZIWkFXdUdRaGI5U0lVMDg5SlhXekxqMWY3?=
 =?utf-8?B?ZEhLeEpRbVU3aEs0cThxUG1TVGxEV0Y4VTExWDBFa0VubzNkODlwTXhVc2ZT?=
 =?utf-8?B?TDgxcGc4OVhBUDFPTHJmWW5JT0J2ZXNLWXkvTUcwOTgrTjZsbitwNGxnNXVR?=
 =?utf-8?Q?XTmc0nhaItd7qjBkaFv8fYc=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	1SM4d6S2jp/ce+YeYRkXhJTK/wBijr58hElmD+7VNSuwqrGMMqr6rXM8gZv6TZE0oy0p1siQAlAB3H5niDSMUKGgFkyBjoH9QCwYYNLsBbqY2KB4T+O2IwbDgA3/ipsscXxaza77qrYrUKv7spHY3KOz7KLj/QV/1wf4EW/NBAAnylZpF00akSIgla1Euqg5g0xCxmdVG3QXnhbK1AWV0MPGdlBgwhMo7SLl/6OwxgFdSi0SVoSCpW9SkTuYwJ3Ip36rk1tQcBJ7k2ri+Zz5U8/K3IInOGOPTpzLuMfjdAU7ZTkUFrpmd70+5SU2B5tPVfSOyc0TIgY7NB7KYsKSp6iBszs04TVPFTmI+CfuxWt5IS8YAx/mb9qtKsC+VlUwnjIcPEhvVhneRSPHSv9t7Kc3+kJTNlx+tXugFg8MjnHCDHNmtJojVPeDSl/XA1kaOlHpqWrHNjEMqdKc0VN7nRBzuew3ohbCUkiAhzNuLznwgmUo+uyrTQBfi37S1TYihLl8khDVNJ7fboWQ5/kKsH6hcOjtynMmDXtS+sZMdGrksL8ZFelMKKvPqTgXOQ1oliA6ZO+beXxDsmEBqsPX4OaDiiJpA+BatuBImGh1HrQ=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 98572261-dce6-4780-8b58-08dc5317eebf
X-MS-Exchange-CrossTenant-AuthSource: PH8PR10MB6290.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 13:22:23.6638
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ZktIsPwHwx8zeIUidAPGUah7As3wKbZhlCHT3SuuTW64XynW1iWnId9rCx48Fcwh5M1T1OT8Hli3d8wSpSBnTkg/TTOEU/Z2GfTKZZfFBuqjiJp7vT/RRphdypYvFMQ7
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5199
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-02_06,2024-04-01_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0
 mlxlogscore=999 adultscore=0 bulkscore=0 phishscore=0 mlxscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2403210000 definitions=main-2404020098
X-Proofpoint-ORIG-GUID: NJiZtu9h05yiXPrZJOxD25OeMsDn8SQC
X-Proofpoint-GUID: NJiZtu9h05yiXPrZJOxD25OeMsDn8SQC

Hello Boris,

On 02/04/24 18:03, Boris Brezillon wrote:
> Hello Harshit,
> 
> On Tue,  2 Apr 2024 03:33:58 -0700
> Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote:
> 
>> panthor_vm_get_heap_pool() returns ERR_PTR on failure.
>>
>> Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block")
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>> ---
>> This is spotted by smatch and the patch is only compile tested
>> ---
>>   drivers/gpu/drm/panthor/panthor_drv.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
>> index 11b3ccd58f85..050b905b0453 100644
>> --- a/drivers/gpu/drm/panthor/panthor_drv.c
>> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
>> @@ -1090,8 +1090,8 @@ static int panthor_ioctl_tiler_heap_destroy(struct drm_device *ddev, void *data,
>>   		return -EINVAL;
>>   
>>   	pool = panthor_vm_get_heap_pool(vm, false);
>> -	if (!pool) {
>> -		ret = -EINVAL;
>> +	if (IS_ERR(pool)) {
>> +		ret = PTR_ERR(pool);
> 
> Actually, panthor_vm_get_heap_pool() will return NULL if there's no
> heap pool attached to this VM and create=false, so this was correct.
> This being said, I'm fine making that consistent by returning
> ERR_PTR(-ENOENT) instead of NULL in that case. This way we don't have
> two different semantics based on the 'create' value.
> 

Thanks for explaining. I missed the case where create is false and there 
is no heap pool attached, so panthor_vm_get_heap_pool() can return NULL.

1878  *
1879  * Return: A valid pointer on success, an ERR_PTR() otherwise.
1880  */
1881 struct panthor_heap_pool *panthor_vm_get_heap_pool(struct 
panthor_vm *vm, bool create)

The documentation says it returns ERR_PTR() on failure, so is it worth 
doing something like:

diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c 
b/drivers/gpu/drm/panthor/panthor_mmu.c
index fdd35249169f..e1285cdb09ff 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -1893,6 +1893,8 @@ struct panthor_heap_pool 
*panthor_vm_get_heap_pool(struct panthor_vm *vm, bool c
                         vm->heaps.pool = panthor_heap_pool_get(pool);
         } else {
                 pool = panthor_heap_pool_get(vm->heaps.pool);
+               if (!pool)
+                       pool = ERR_PTR(-ENOENT);
         }
         mutex_unlock(&vm->heaps.lock);


and change all callers of panthor_vm_get_heap_pool() to only check for 
IS_ERR() ?


> Oh, and please merge everything into a single patch instead of one patch
> per call-site.
> 

Sure, I noticed one after the other. I will fix them together in v2.

Thanks,
Harshit
> Regards,
> 
> Boris
> 
>>   		goto out_put_vm;
>>   	}
>>   
>