linux-kernel - Re: [PATCH] drm/panthor: Add defer probe for firmware load

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87frv9zthu.fsf@minerva.mail-host-address-is-not-set>
Date: Thu, 25 Apr 2024 11:22:53 +0200
From: Javier Martinez Canillas <javierm@...hat.com>
To: Steven Price <steven.price@....com>, Andy Yan <andyshrk@....com>,
 boris.brezillon@...labora.com
Cc: daniel@...ll.ch, airlied@...il.com, liviu.dudau@....com,
 maarten.lankhorst@...ux.intel.com, tzimmermann@...e.de,
 dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, Andy Yan
 <andy.yan@...k-chips.com>
Subject: Re: [PATCH] drm/panthor: Add defer probe for firmware load

Steven Price <steven.price@....com> writes:

Hello Steven,

> On 13/04/2024 12:49, Andy Yan wrote:
>> From: Andy Yan <andy.yan@...k-chips.com>
>> 
>> The firmware in the rootfs will not be accessible until we
>> are in the SYSTEM_RUNNING state, so return EPROBE_DEFER until
>> that point.
>> This let the driver can load firmware when it is builtin.
>
> The usual solution is that the firmware should be placed in the
> initrd/initramfs if the module is included there (or built-in). The same
> issue was brought up regarding the powervr driver:
>
> https://lore.kernel.org/dri-devel/20240109120604.603700-1-javierm@redhat.com/T/
>
> I'm not sure if that ever actually reached a conclusion though. The
> question was deferred to Greg KH but I didn't see Greg actually getting
> involved in the thread.
>

Correct, there was not conclusion reached in that thread.

>> Signed-off-by: Andy Yan <andy.yan@...k-chips.com>
>> ---
>> 
>>  drivers/gpu/drm/panthor/panthor_fw.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
>> index 33c87a59834e..25e375f8333c 100644
>> --- a/drivers/gpu/drm/panthor/panthor_fw.c
>> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
>> @@ -1336,8 +1336,17 @@ int panthor_fw_init(struct panthor_device *ptdev)
>>  	}
>>  
>>  	ret = panthor_fw_load(ptdev);
>> -	if (ret)
>> +	if (ret) {
>> +		/*
>> +		 * The firmware in the rootfs will not be accessible until we
>> +		 * are in the SYSTEM_RUNNING state, so return EPROBE_DEFER until
>> +		 * that point.
>> +		 */
>> +		if (system_state < SYSTEM_RUNNING)
>
> This should really only be in the case where ret == -ENOENT - any other
> error and the firmware is apparently present but broken in some way, so
> there's no point deferring.
>
> I also suspect we'd need some change in panthor_fw_load() to quieten
> error messages if we're going to defer on this, in which case it might
> make more sense to move this logic into that function.
>

In the thread you referenced I suggested to add that logic in request_firmware()
(or add a new request_firmware_defer() helper function) that changes the request
firmare behaviour to return -EPROBE_DEFER instead of -ENOENT.

Since as you mentioned, this isn't specific to panthor and an issue that I also
faced with the powervr driver.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat