Received-SPF: pass (google.com: domain of linux-kernel+bounces-128104-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <b35d0de5-be46-42a1-b7e7-b24a72d945c7@oracle.com>
Date: Tue, 2 Apr 2024 19:35:41 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] drm/panthor: Fix couple of NULL vs IS_ERR() bugs
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>,
        Grant Likely <grant.likely@linaro.org>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        dan.carpenter@linaro.org, kernel-janitors@vger.kernel.org,
        error27@gmail.com
References: <20240402134709.1706323-1-harshit.m.mogalapalli@oracle.com>
 <20240402160226.4a1ac2d1@collabora.com>
Content-Language: en-US
From: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
In-Reply-To: <20240402160226.4a1ac2d1@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?QmlLL0Vka053SUtNMHNldEowQ2hQM3JNa0xZTTg2TitQVkxxeGdReXpQMTRu?=
 =?utf-8?B?UlVVZ3poeGM1Y1U4cnhaZlRKOWt3MEM1SUtVcm5WdFJtZWVxYk5Na3lNVDln?=
 =?utf-8?B?MDlTWFJGRnAwcy9IY1dsRkZzQmI4NlUyNUdHcnorUVdQTTVzQUZvQ1FOWHJj?=
 =?utf-8?B?NkJWUjdJU0ZNdks3ekZwNTVhTFNNQVhzV0Fwc0lBMVZKMmV3YTJzM2dHWENG?=
 =?utf-8?B?UEFYZjltZEFQL0RzTWg3V0tBd3lrNXh0V21EQ0tMb1l1a0xHcFlKcDBkWGYx?=
 =?utf-8?B?bDRsTFJZUUVpN2I0QW5kWkdUT2haNm5FNzlXc0FPSUlCNzFFbXNLWEpzQ1hB?=
 =?utf-8?B?UWJyWklrT0UrUXBRNHRuTjRrSHYza1R1czJNSnVjOUcxa1QvQVF2UVlQdDh0?=
 =?utf-8?B?YW82Z2NxZGhqcndUVVF4eXdKTEYxNHJaZUFuTnlmUFFncXh3Nm9LQjRrVkhM?=
 =?utf-8?B?MzZLN1U0R25JWFVRWWN3eURoeGV1Q1NJRElYL29JVlRjRjFLc3hhdjM0b1Bn?=
 =?utf-8?B?U2VTUVpaQXdjUnFXNFlwWEtUeXYzcmhsOENiMTVyQTF3cThCeTk4RU5mZ1JO?=
 =?utf-8?B?VzNqQ0ZXMlhUcVlyNlhVekZ1czREUFZVMkpLWU9lTTdCQ0ZVVnJia1FWT25j?=
 =?utf-8?B?YXRvR3FMTU1sNXc4TWhoblhIYzZrMzdIN1dIdnN4b0hDM0IvSElCUjJWT3k1?=
 =?utf-8?B?ZTdJZFNOOTNsV1JsejI5aEFnaXRXWWRrSksxd1h6eTZOVk56TkZESEdYWUsv?=
 =?utf-8?B?cHVGdEtJYVkzcjNMRVdzM0FNM0FxZ0FZeGgwVE93YUJyQzgxcHc3RGxMYlN4?=
 =?utf-8?B?VkMvMEtNRzRZQXZjZVlQVkZzNlVGQUJhSmNPSWtCT0IycVFVTEVFWkFKTlBY?=
 =?utf-8?B?VEVJWHhRdEV3ektNdCtyK0s0MlcrWit3bk03QUVGZHVhSXphTFRpdnpndjJp?=
 =?utf-8?B?RzZ6NzJONlZCTmVBNHkvZG9waXpub25qU0hvMVlCSGtLT2ZFZWFlY2t0dm9M?=
 =?utf-8?B?REwrSDFsZUdjb3l3aXFIY1YzVmo5Z2xIVHJLSEFkeHMxYTJnaWVsZkZHbWlT?=
 =?utf-8?B?eERrL0prdWsxWTM3SllQaHZKUi9YU3lQaDVKR2NtcFJpTFdMRytjTHArcjEr?=
 =?utf-8?B?RXFPUlVONGVadno1eWNOMHVER3RGRzYzZ1U4eUNKaWY1OWhoWDA5NjRnT1h1?=
 =?utf-8?B?NDJuRGMzYXRsSmJ0V0x6SEZkZFl2NEQzL2lYMTYxOUZhV3F2YnhZOE1pYURa?=
 =?utf-8?B?KzcyVzc4cGtxcnVuckRTSEJ3QnZpTnVYbjRrZWJrT1p5RmRXQVJBWHpWbktE?=
 =?utf-8?B?U2RtakoyUFdBVUxwWlM5dmlxdUJReFpUN3Y0ZHpVaFFtVi8rM2hZS2FvUGRE?=
 =?utf-8?B?ZVpXVmFjWmtVSFJYNSttUlVRZS9CNWI5dTVOTDJ0T1NNV0d6ampuekFQOU44?=
 =?utf-8?B?Vzh0SEJKalpwWVRJOWFGWjRXbFhDaklPZkhVUUJ0TU13cmh5VDNvL3Q2cmZv?=
 =?utf-8?B?cFJBcHlNVHk1ZzVsaUUwQkw4WGZRZXRBUFRXYlZmeVNNd0NFMHdkaVRlTG4v?=
 =?utf-8?B?TStxc2E0cVhlMmliU0p6ZDJPQzdkTVY2LzRGUkVGb0x1a0pGUkF0THhyQjVD?=
 =?utf-8?B?Y2hyb0VKS1ZGZWtwYkVOdVpFQU95NXN5WDNmeThSS0FZU3Y5blJlM1FtS2Zo?=
 =?utf-8?B?Y2J4N2l3TEpCeVR6ajNlMUlRUkhwNkVOODllcE44N01kRW1UalJhSnNNcTJz?=
 =?utf-8?B?Vit3THFYVTRqUHNQNlVJTEZCYlpCZkJ3YXhUQ1pxVkpxK0Q4OWVma0hCdzdK?=
 =?utf-8?B?aDZmMXR5K2ZPY0NxdGRpUjBDQ2tlT2lFbnRaVjRiallQWUdlRStwYzBQUDh0?=
 =?utf-8?B?UmNwVzZmL04rNGdNMDI1dUdRNkNQcXk5bHZEeFQyYWgvQlpiMDB5SGhvZFpG?=
 =?utf-8?B?eDh6dEJmbndTaFpXbHdjakFMZWEzclhqeHA2UzlMNWR3d2gzWjI1dHlCOHVh?=
 =?utf-8?B?VmNUVEVFUGVKWVRkOHBQck0xWkFWWGdDdUlURXJtNTZ6VzNhc0VXOGdBTTRj?=
 =?utf-8?B?U0pVY0d1NysxM0h0UzJwc2JlajIybFhPYksxSW9LazV2NXI4Mzg0ODM1Sy9P?=
 =?utf-8?B?SG5Qc2piT0RVWXNrUlhvejNaUWg1K1VyNS9WRitHSU5PdGc4Y3JFUlI5S2FD?=
 =?utf-8?Q?ECJ+ElfDVN6Ioh9RgqmGhpE=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	OVQ0cOkElWhsb2TV1PF/+thxi/MTgjRE+ozWilNX17ujTJLthr1F7mT21x8kSfaVOK6B9E8xnvGgLUbKFiwdpURtO9KAzXdA90icHCL9hfB3EJ0L40+XbVRWmcRU8fQubXNMOUmI2YWMklpMmBppTDd7dHFSYgrkjVcyiA+0PmLjI1aE36ptkfKEz5VnGfS/pW7fuK4oKKyyE1YafT9/dSwP+o2JHx9lurQBEXhwi35SaP9Wps8HKu/PtB9lVySbSSRNeNGZH2enBoAEe20UKFuG++yFJkLRWzonPGRUegchLRSbVbPBkHvgloKr6/dwZ7XQTX+VPOsPbYwpXPCXCcmFDFHp1ZGz+wpkufkx8a7xvoeroIZz5DQBilO8dX7HhsK0+7EWCNGXSzHGS8XNytKCocvkm46+LwwMmg4EkAf/KpMRIwbvutxRtiYkeWzi1wm9SPleJDozA+jHGYqLlFDakGo/YaMlHPfcrep2aADmY7n3XGZpZmyWTc/fi0IOBXsmX7vBq8J57eTdbB5ohUqEZF5+eEAMvB1IEPy5kkcL37BTyb+qOA0bBjDbCXS+WCZxk5+NkoY7m0lo5ig2YfgrD3adRB+kh2yhgU3nI9M=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 22b84a58-0efe-4f2c-31c9-08dc531e0234
X-MS-Exchange-CrossTenant-AuthSource: PH8PR10MB6290.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Apr 2024 14:05:53.3270
 (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: UCuTCaKS/XliQp+GZSdw0LEN9Tma68TuQnMJNfgPrwWxpAqXozHKmVWfk7yQQiU2NhJ/IvgCCGKgITLBkAULK8XhFLM03roFtmjTui4pFxj9YOa9ri6Jr0kTUkTSvJxY
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB5709
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_07,2024-04-01_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 adultscore=0
 spamscore=0 phishscore=0 bulkscore=0 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2403210000
 definitions=main-2404020103
X-Proofpoint-GUID: Dy65GmtL6TwWSCUkfN8MP8xNUND7_Nz3
X-Proofpoint-ORIG-GUID: Dy65GmtL6TwWSCUkfN8MP8xNUND7_Nz3

Hi Boris,
On 02/04/24 19:32, Boris Brezillon wrote:
> On Tue,  2 Apr 2024 06:47:08 -0700
> Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote:
> 
>> 1. The devm_drm_dev_alloc() function returns error pointers.
>>     Update the error handling to check for error pointers instead of NULL.
>> 2. Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and
>>     NULL(when create is false and if there is no poool attached to the
>>     VM)
>> 	- Change the function to return error pointers, when pool is
>> 	  NULL return -ENOENT
>> 	- Also handle the callers to check for IS_ERR() 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
>>
>> v1->v2: Fix the function panthor_vm_get_heap_pool() to only return error
>> pointers and handle the caller sites [Suggested by Boris Brezillon]
>> 	- Also merge these IS_ERR() vs NULL bugs into same patch
>> ---
>>   drivers/gpu/drm/panthor/panthor_drv.c   | 6 +++---
>>   drivers/gpu/drm/panthor/panthor_mmu.c   | 2 ++
>>   drivers/gpu/drm/panthor/panthor_sched.c | 2 +-
>>   3 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
>> index 11b3ccd58f85..c8374cd4a30d 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);
>>   		goto out_put_vm;
>>   	}
>>   
>> @@ -1385,7 +1385,7 @@ static int panthor_probe(struct platform_device *pdev)
>>   
>>   	ptdev = devm_drm_dev_alloc(&pdev->dev, &panthor_drm_driver,
>>   				   struct panthor_device, base);
>> -	if (!ptdev)
>> +	if (IS_ERR(ptdev))
>>   		return -ENOMEM;
>>   
> 
> Sorry, that one deserves a separate patch.
> 

Ah okay, I was confused about the same.
So I will send a V3 removing that part and could you please use the 
independent patch that I sent before ?

https://lore.kernel.org/all/20240402104041.1689951-1-harshit.m.mogalapalli@oracle.com/

Thanks,
Harshit

>>   	platform_set_drvdata(pdev, ptdev);
>> 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);
>>   
>> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
>> index 5f7803b6fc48..617df2b980d0 100644
>> --- a/drivers/gpu/drm/panthor/panthor_sched.c
>> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
>> @@ -1343,7 +1343,7 @@ static int group_process_tiler_oom(struct panthor_group *group, u32 cs_id)
>>   	if (unlikely(csg_id < 0))
>>   		return 0;
>>   
>> -	if (!heaps || frag_end > vt_end || vt_end >= vt_start) {
>> +	if (IS_ERR(heaps) || frag_end > vt_end || vt_end >= vt_start) {
>>   		ret = -EINVAL;
>>   	} else {
>>   		/* We do the allocation without holding the scheduler lock to avoid
> 
>