Received-SPF: pass (google.com: domain of linux-kernel+bounces-158292-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Javier Martinez Canillas <javierm@redhat.com>
To: Steven Price <steven.price@arm.com>, Andy Yan <andyshrk@163.com>,
 boris.brezillon@collabora.com
Cc: daniel@ffwll.ch, airlied@gmail.com, liviu.dudau@arm.com,
 maarten.lankhorst@linux.intel.com, tzimmermann@suse.de,
 dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Andy Yan
 <andy.yan@rock-chips.com>
Subject: Re: [PATCH] drm/panthor: Add defer probe for firmware load
In-Reply-To: <54e4a174-dea7-4588-b8a6-0310c210ffce@arm.com>
References: <20240413114938.740631-1-andyshrk@163.com>
 <54e4a174-dea7-4588-b8a6-0310c210ffce@arm.com>
Date: Thu, 25 Apr 2024 11:22:53 +0200
Message-ID: <87frv9zthu.fsf@minerva.mail-host-address-is-not-set>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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

Hello Steven,

> On 13/04/2024 12:49, Andy Yan wrote:
>> From: Andy Yan <andy.yan@rock-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@rock-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