| 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
| ||
|
Message-ID: <2fd3aabd.785b.190914ec1a6.Coremail.andyshrk@163.com>
Date: Mon, 8 Jul 2024 15:46:16 +0800 (CST)
From: "Andy Yan" <andyshrk@....com>
To: "Dragan Simic" <dsimic@...jaro.org>
Cc: linux-rockchip@...ts.infradead.org, dri-devel@...ts.freedesktop.org,
heiko@...ech.de, hjc@...k-chips.com, andy.yan@...k-chips.com,
maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
javierm@...hat.com
Subject: Re:Re: [PATCH] drm/rockchip: cdn-dp: Remove redundant workarounds
for firmware loading
Hi Dragan，
At 2024-07-04 18:35:42, "Dragan Simic" <dsimic@...jaro.org> wrote:
>Hello Andy,
>
>On 2024-07-04 04:10, Andy Yan wrote:
>> At 2024-07-04 07:32:02, "Dragan Simic" <dsimic@...jaro.org> wrote:
>>> After the additional firmware-related module information was
>>> introduced by
>>> the commit c0677e41a47f ("drm/rockchip: cdn-dp-core: add
>>> MODULE_FIRMWARE
>>> macro"), there's no longer need for the firmware-loading workarounds
>>> whose
>>> sole purpose was to prevent the missing firmware blob in an initial
>>> ramdisk
>>> from causing driver initialization to fail. Thus, delete the
>>> workarounds,
>>> which removes a sizable chunk of redundant code.
>>
>> What would happen if there was no ramdisk? And the firmware is in
>> rootfs ？
>>
>> For example： A buildroot based tiny embedded system。
>
>Good point, let me explain, please.
>
>In general, if a driver is built into the kernel, there should also be
>an initial ramdisk that contains the related firmware blobs, because
>it's
>unknown is the root filesystem available when the driver is probed. If
>a driver is built as a module and there's no initial ramdisk, having
>the related firmware blobs on the root filesystem should be fine,
>because
>the firmware blobs and the kernel module become available at the same
>time, through the root filesystem. [1]
>
>Another option for a driver built statically into the kernel, when
>there's
>no initial ramdisk, is to build the required firmware blobs into the
>kernel
>image. [2] Of course, that's feasible only when a kernel image is built
>specificially for some device, because otherwise it would become too
>large
>because of too many drivers and their firmware blobs becoming included,
>but that seems to fit the Buildroot-based example.
>
>To sum it up, mechanisms already exist in the kernel for various
>scenarios
>when it comes to loading firmware blobs. Even if the deleted workaround
>attempts to solve some issue specific to some environment, that isn't
>the
>right place or the right way for solving any issues of that kind.
>
>While preparing this patch, I even tried to find another kernel driver
>that
>also implements some similar workarounds for firmware loading, to
>justify
>the existence of such workarounds and to possibly move them into the
>kernel's
>firmware-loading interface. Alas, I was unable to find such workarounds
>in
>other drivers, which solidified my reasoning behind classifying the
>removed
>code as out-of-place and redundant.
For some tiny embedded system，there is no such ramdisk，for example：
a buildroot based rootfs，the buildroot only generate rootfs。
And FYI， there are mainline drivers try to fix such issue by defer_probe，for example：
smc_abc[0]
There are also some other similar scenario in gpu driver{1}[2]
[0]https://elixir.bootlin.com/linux/latest/source/drivers/tee/optee/smc_abi.c#L1518
[1]https://patchwork.kernel.org/project/dri-devel/patch/20240109120604.603700-1-javierm@redhat.com/
[2]https://lore.kernel.org/dri-devel/87y1918psd.fsf@minerva.mail-host-address-is-not-set/T/
>
>[1]
>https://www.kernel.org/doc/Documentation/driver-api/firmware/direct-fs-lookup.rst
>[2]
>https://www.kernel.org/doc/Documentation/driver-api/firmware/built-in-fw.rst
>
>>> Various utilities used by Linux distributions to generate initial
>>> ramdisks
>>> need to obey the firmware-related module information, so we can rely
>>> on the
>>> firmware blob being present in the generated initial ramdisks.
>>>
>>> Signed-off-by: Dragan Simic <dsimic@...jaro.org>
>>> ---
>>> drivers/gpu/drm/rockchip/cdn-dp-core.c | 53 +++++---------------------
>>> 1 file changed, 10 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>> b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>> index bd7aa891b839..e1a7c6a1172b 100644
>>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>>> @@ -44,9 +44,9 @@ static inline struct cdn_dp_device
>>> *encoder_to_dp(struct drm_encoder *encoder)
>>> #define DPTX_HPD_DEL (2 << 12)
>>> #define DPTX_HPD_SEL_MASK (3 << 28)
>>>
>>> -#define CDN_FW_TIMEOUT_MS (64 * 1000)
>>> #define CDN_DPCD_TIMEOUT_MS 5000
>>> #define CDN_DP_FIRMWARE "rockchip/dptx.bin"
>>> +
>>> MODULE_FIRMWARE(CDN_DP_FIRMWARE);
>>>
>>> struct cdn_dp_data {
>>> @@ -909,61 +909,28 @@ static int cdn_dp_audio_codec_init(struct
>>> cdn_dp_device *dp,
>>> return PTR_ERR_OR_ZERO(dp->audio_pdev);
>>> }
>>>
>>> -static int cdn_dp_request_firmware(struct cdn_dp_device *dp)
>>> -{
>>> - int ret;
>>> - unsigned long timeout = jiffies +
>>> msecs_to_jiffies(CDN_FW_TIMEOUT_MS);
>>> - unsigned long sleep = 1000;
>>> -
>>> - WARN_ON(!mutex_is_locked(&dp->lock));
>>> -
>>> - if (dp->fw_loaded)
>>> - return 0;
>>> -
>>> - /* Drop the lock before getting the firmware to avoid blocking boot
>>> */
>>> - mutex_unlock(&dp->lock);
>>> -
>>> - while (time_before(jiffies, timeout)) {
>>> - ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev);
>>> - if (ret == -ENOENT) {
>>> - msleep(sleep);
>>> - sleep *= 2;
>>> - continue;
>>> - } else if (ret) {
>>> - DRM_DEV_ERROR(dp->dev,
>>> - "failed to request firmware: %d\n", ret);
>>> - goto out;
>>> - }
>>> -
>>> - dp->fw_loaded = true;
>>> - ret = 0;
>>> - goto out;
>>> - }
>>> -
>>> - DRM_DEV_ERROR(dp->dev, "Timed out trying to load firmware\n");
>>> - ret = -ETIMEDOUT;
>>> -out:
>>> - mutex_lock(&dp->lock);
>>> - return ret;
>>> -}
>>> -
>>> static void cdn_dp_pd_event_work(struct work_struct *work)
>>> {
>>> struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device,
>>> event_work);
>>> struct drm_connector *connector = &dp->connector;
>>> enum drm_connector_status old_status;
>>> -
>>> int ret;
>>>
>>> mutex_lock(&dp->lock);
>>>
>>> if (dp->suspended)
>>> goto out;
>>>
>>> - ret = cdn_dp_request_firmware(dp);
>>> - if (ret)
>>> - goto out;
>>> + if (!dp->fw_loaded) {
>>> + ret = request_firmware(&dp->fw, CDN_DP_FIRMWARE, dp->dev);
>>> + if (ret) {
>>> + DRM_DEV_ERROR(dp->dev, "Loading firmware failed: %d\n", ret);
>>> + goto out;
>>> + }
>>> +
>>> + dp->fw_loaded = true;
>>> + }
>>>
>>> dp->connected = true;
>>>
Powered by blists - more mailing lists