| 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: <6f60bb97-86fc-4cda-a373-a991547bbd77@gmail.com>
Date: Fri, 12 Sep 2025 09:01:49 +0200
From: Matthias Brugger <matthias.bgg@...il.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
linux-remoteproc@...r.kernel.org
Cc: arnd@...db.de, andersson@...nel.org, mathieu.poirier@...aro.org,
wenst@...omium.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
kernel@...labora.com
Subject: Re: [PATCH] remoteproc: mtk_scp: Construct FW path if firmware-name
not present
On 11/09/2025 16:00, AngeloGioacchino Del Regno wrote:
> After a reply on the mailing lists [1] it emerged that the DT
> property "firmware-name" should not be relied on because of
> possible issues with firmware versions.
> For MediaTek SCP, there has never been any firmware version vs
> driver version desync issue but, regardless, the firmwares are
> always using the same name and they're always located in a path
> with a specific pattern.
>
> Instead of unconditionally always relying on the firmware-name
> devicetree property to get a path to the SCP FW file, drivers
> should construct a name based on what firmware it knows and
> what hardware it is running on.
>
> In order to do that, add a `scp_get_default_fw_path()` function
> that constructs the path and filename based on two of the infos
> that the driver can get:
> 1. The compatible string with the highest priority (so, the
> first one at index 0); and
> 2. The type of SCP HW - single-core or multi-core.
>
> This means that the default firmware path is generated as:
> - Single core SCP: mediatek/(soc_model)/scp.img
> for example: mediatek/mt8183/scp.img;
>
> - Multi core SCP: mediatek/(soc_model)/scp_c(core_number).img
> for example: mediatek/mt8188/scp_c0.img for Core 0, and
> mediatek/mt8188/scp_c1.img for Core 1.
>
As we inventing a naming scheme here: if we decide that signle core FW is calle
scp_c0.img we can get rid of some code.
> Note that the generated firmware path is being used only if the
> "firmware-name" devicetree property is not present in the SCP
> node or in the SCP Core node(s).
>
> [1 - Reply regarding firmware-name property]
> Link: https://lore.kernel.org/all/7e8718b0-df78-44a6-a102-89529d6abcce@app.fastmail.com/
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> ---
> drivers/remoteproc/mtk_scp.c | 64 ++++++++++++++++++++++++++++++++----
> 1 file changed, 58 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index 8206a1766481..80fcb4b053b3 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -16,6 +16,7 @@
> #include <linux/remoteproc.h>
> #include <linux/remoteproc/mtk_scp.h>
> #include <linux/rpmsg/mtk_rpmsg.h>
> +#include <linux/string.h>
>
> #include "mtk_common.h"
> #include "remoteproc_internal.h"
> @@ -1093,22 +1094,73 @@ static void scp_remove_rpmsg_subdev(struct mtk_scp *scp)
> }
> }
>
> +/**
> + * scp_get_default_fw_path() - Get default SCP firmware path
> + * @dev: SCP Device
> + * @core_id: SCP Core number
> + *
> + * This function generates a path based on the following format:
> + * mediatek/(soc_model)/scp(_cX).img; for multi-core or
> + * mediatek/(soc_model)/scp.img for single core SCP HW
> + *
> + * Return: A devm allocated string containing the full path to
> + * a SCP firmware or an error pointer
> + */
> +static const char *scp_get_default_fw_path(struct device *dev, int core_id)
> +{
> + struct device_node *np = core_id < 0 ? dev->of_node : dev->parent->of_node;
> + char scp_fw_file[7] = "scp_cX";
We provide a string that we later overwrite. I'd prefer to have just the
reservation without any 'artificial' string in it.
> + const char *compatible, *soc;
> + int ret;
> +
> + /* Use only the first compatible string */
> + ret = of_property_read_string_index(np, "compatible", 0, &compatible);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + /* If the compatible string's length is implausible bail out early */
> + if (strlen(compatible) < strlen("mediatek,mtXXXX-scp"))
Seems like a double check of compatible. Why is dt-bindings for that not enough?
> + return ERR_PTR(-EINVAL);
> +
> + /* If the compatible string starts with "mediatek,mt" assume that it's ok */
> + if (!str_has_prefix(compatible, "mediatek,mt"))
Same here.
> + return ERR_PTR(-EINVAL);
> +
> + if (core_id >= 0)
> + ret = snprintf(scp_fw_file, ARRAY_SIZE(scp_fw_file), "scp_c%1d", core_id);
> + else
> + ret = snprintf(scp_fw_file, ARRAY_SIZE(scp_fw_file), "scp");
> + if (ret <= 0)
> + return ERR_PTR(ret);
> +
> + soc = &compatible[strlen("mediatek,")];
> +
> + return devm_kasprintf(dev, GFP_KERNEL, "mediatek/%.*s/%s.img",
> + (int)strlen("mtXXXX"), soc, scp_fw_file);
> +}
> +
> static struct mtk_scp *scp_rproc_init(struct platform_device *pdev,
> struct mtk_scp_of_cluster *scp_cluster,
> - const struct mtk_scp_of_data *of_data)
> + const struct mtk_scp_of_data *of_data,
> + int core_id)
> {
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> struct mtk_scp *scp;
> struct rproc *rproc;
> struct resource *res;
> - const char *fw_name = "scp.img";
> + const char *fw_name;
> int ret, i;
> const struct mtk_scp_sizes_data *scp_sizes;
>
> ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> - if (ret < 0 && ret != -EINVAL)
> - return ERR_PTR(ret);
> + if (ret) {
> + fw_name = scp_get_default_fw_path(dev, core_id);
Wouldn't it make more sense to encapsulate the whole fw_name retrival in one
function, e.g. scp_get_fw_path.
> + if (IS_ERR(fw_name)) {
> + dev_err(dev, "Cannot get firmware path: %ld\n", PTR_ERR(fw_name));
> + return ERR_CAST(fw_name);
> + }
> + }
>
> rproc = devm_rproc_alloc(dev, np->name, &scp_ops, fw_name, sizeof(*scp));
> if (!rproc) {
> @@ -1212,7 +1264,7 @@ static int scp_add_single_core(struct platform_device *pdev,
> struct mtk_scp *scp;
> int ret;
>
> - scp = scp_rproc_init(pdev, scp_cluster, of_device_get_match_data(dev));
> + scp = scp_rproc_init(pdev, scp_cluster, of_device_get_match_data(dev), -1);
> if (IS_ERR(scp))
> return PTR_ERR(scp);
>
> @@ -1259,7 +1311,7 @@ static int scp_add_multi_core(struct platform_device *pdev,
> goto init_fail;
> }
>
> - scp = scp_rproc_init(cpdev, scp_cluster, cluster_of_data[core_id]);
> + scp = scp_rproc_init(cpdev, scp_cluster, cluster_of_data[core_id], core_id);
> put_device(&cpdev->dev);
> if (IS_ERR(scp)) {
> ret = PTR_ERR(scp);
Powered by blists - more mailing lists