[PATCH v1 1/1] drm/xe: fix the ERR_PTR() returned on failure to allocate tiny pt

[Mirsad Todorovac]

Hi,

On 12/9/24 17:49, Rodrigo Vivi wrote:
> On Thu, Nov 21, 2024 at 10:20:58PM +0100, Mirsad Todorovac wrote:
>> Running coccinelle spatch gave the following warning:
>>
>> ./drivers/gpu/drm/xe/tests/xe_migrate.c:226:5-11: inconsistent IS_ERR and PTR_ERR on line 228.
>>
>> The code reports PTR_ERR(pt) when IS_ERR(tiny) is checked:
>>
>> → 211         pt = xe_bo_create_pin_map(xe, tile, m->q->vm, XE_PAGE_SIZE,
>>   212                                   ttm_bo_type_kernel,
>>   213                                   XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>>   214                                   XE_BO_FLAG_PINNED);
>>   215         if (IS_ERR(pt)) {
>>   216                 KUNIT_FAIL(test, "Failed to allocate fake pt: %li\n",
>>   217                            PTR_ERR(pt));
>>   218                 goto free_big;
>>   219         }
>>   220
>>   221         tiny = xe_bo_create_pin_map(xe, tile, m->q->vm,
>> → 222                                     2 * SZ_4K,
>>   223                                     ttm_bo_type_kernel,
>>   224                                     XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>>   225                                     XE_BO_FLAG_PINNED);
>> → 226         if (IS_ERR(tiny)) {
>> → 227                 KUNIT_FAIL(test, "Failed to allocate fake pt: %li\n",
>> → 228                            PTR_ERR(pt));
>>   229                 goto free_pt;
>>   230         }
>>
>> Now, the IS_ERR(tiny) and the corresponding PTR_ERR(pt) do not match.
>>
>> Returning PTR_ERR(tiny), as the last failed function call, seems logical.
>>
>> Fixes: dd08ebf6c3525 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Matthew Auld <matthew.auld at intel.com>
>> Cc: Matt Roper <matthew.d.roper at intel.com>
>> Cc: "Thomas Hellström" <thomas.hellstrom at linux.intel.com>
>> Cc: Francois Dugast <francois.dugast at intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Philippe Lecluse <philippe.lecluse at intel.com>
>> Cc: Nirmoy Das <nirmoy.das at intel.com>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: José Roberto de Souza <jose.souza at intel.com>
>> Cc: David Airlie <airlied at gmail.com>
>> Cc: Maxime Ripard <mripard at kernel.org>
>> Cc: Faith Ekstrand <faith.ekstrand at collabora.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Simona Vetter <simona at ffwll.ch>
>> Cc: Mauro Carvalho Chehab <mchehab at kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>> Cc: Akshata Jahagirdar <akshata.jahagirdar at intel.com>
>> Cc: David Kershner <david.kershner at intel.com>
>> Cc: intel-xe at lists.freedesktop.org
>> Cc: dri-devel at lists.freedesktop.org
>> Cc: linux-kernel at vger.kernel.org
> 
> I clean up this cc list. no reason for this simple patch to have this huge list.

Indeed, Sir.

Please proceed as it seems fit to you. I think I used the get_maintainer.pl script,
blame and the Cc: list from the fixed commit.

I thought that "more eyes see more", but this is indeed a very small patch (+2/-2 lines).

However, the bug finding tools make me find bugs in widely spread range of drivers,
and I do not expect to understand +30 million lines of code any time soon :-)

> I also fixed the checkpatch complains in the commit message.

I am sorry for the trouble caused.

> And pushed to drm-xe-next
> 
> Thanks for the patch.

Not at all.

I understood that patching drivers must go through rigorous scrutiny.

God bless and I hope you'll have great advent holidays.

Best regards,
Mirsad Todorovac

>> Signed-off-by: Mirsad Todorovac <mtodorovac69 at gmail.com>
>> ---
>>  v1:
>> 	There is also the logical problem which this patch developer is not qualified to fix:
>> 	first the fake pt tries to allocate size of (1 << (XE_PT_SHIFT=12)) = 4096 bytes,
>> 	then "tiny pt" tries to allocate 2 * SZ_4K, twice as much. Is this what was meant
>> 	to accomplish?
>>
>> 	drivers/gpu/drm/xe/xe_bo.h#L46:
>> 	#define XE_PTE_SHIFT			12
>> 	#define XE_PAGE_SIZE			(1 << XE_PTE_SHIFT)
>>
>> 	include/linux/sizes.h#L23:
>> 	#define SZ_4K				0x00001000
>>
>>  drivers/gpu/drm/xe/tests/xe_migrate.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/tests/xe_migrate.c b/drivers/gpu/drm/xe/tests/xe_migrate.c
>> index 1a192a2a941b..3bbdb362d6f0 100644
>> --- a/drivers/gpu/drm/xe/tests/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/tests/xe_migrate.c
>> @@ -224,8 +224,8 @@ static void xe_migrate_sanity_test(struct xe_migrate *m, struct kunit *test)
>>  				    XE_BO_FLAG_VRAM_IF_DGFX(tile) |
>>  				    XE_BO_FLAG_PINNED);
>>  	if (IS_ERR(tiny)) {
>> -		KUNIT_FAIL(test, "Failed to allocate fake pt: %li\n",
>> -			   PTR_ERR(pt));
>> +		KUNIT_FAIL(test, "Failed to allocate tiny fake pt: %li\n",
>> +			   PTR_ERR(tiny));
>>  		goto free_pt;
>>  	}
>>  
>> -- 
>> 2.43.0
>>