| 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: <db81aa4e-dc1f-4c6d-b58f-82883622b0dc@gmail.com>
Date: Tue, 4 Feb 2025 18:59:53 +0200
From: Laurentiu Mihalcea <laurentiumihalcea111@...il.com>
To: Frank Li <Frank.li@....com>
Cc: Bard Liao <yung-chuan.liao@...ux.intel.com>,
Daniel Baluta <daniel.baluta@....com>,
Iuliana Prodan <iuliana.prodan@....com>, Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>, Mark Brown <broonie@...nel.org>,
linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org,
imx@...ts.linux.dev
Subject: Re: [PATCH 1/9] ASoC: SOF: imx: introduce more common structures and
functions
On 2/3/2025 9:52 PM, Frank Li wrote:
> On Mon, Feb 03, 2025 at 12:18:00PM -0500, Laurentiu Mihalcea wrote:
>> From: Laurentiu Mihalcea <laurentiu.mihalcea@....com>
>>
>> The SOF drivers for imx chips have a lot of duplicate code
>> and routines/code snippets that could certainly be reused
>> among drivers.
>>
>> As such, introduce a new set of structures and functions
>> that will help eliminate the redundancy and code size of
>> the drivers.
> please wrap at 75 chars
sure, fix in v2
>
>> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@....com>
>> Reviewed-by: Daniel Baluta <daniel.baluta@....com>
>> Reviewed-by: Iuliana Prodan <iuliana.prodan@....com>
>> ---
>> sound/soc/sof/imx/imx-common.c | 419 ++++++++++++++++++++++++++++++++-
>> sound/soc/sof/imx/imx-common.h | 149 ++++++++++++
>> 2 files changed, 567 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/sof/imx/imx-common.c b/sound/soc/sof/imx/imx-common.c
>> index fce6d9cf6a6b..5921900335c8 100644
>> --- a/sound/soc/sof/imx/imx-common.c
>> +++ b/sound/soc/sof/imx/imx-common.c
>> @@ -1,11 +1,16 @@
>> // SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
>> //
>> -// Copyright 2020 NXP
>> +// Copyright 2020-2025 NXP
>> //
>> // Common helpers for the audio DSP on i.MX8
>>
>> +#include <linux/firmware/imx/dsp.h>
>> #include <linux/module.h>
>> +#include <linux/of_reserved_mem.h>
>> +#include <linux/of_address.h>
> keep alphabet order
ok
>
>> +#include <linux/pm_domain.h>
>> #include <sound/sof/xtensa.h>
>> +
>> #include "../ops.h"
>>
>> #include "imx-common.h"
>> @@ -74,5 +79,417 @@ void imx8_dump(struct snd_sof_dev *sdev, u32 flags)
>> }
>> EXPORT_SYMBOL(imx8_dump);
>>
>> +static void imx_handle_reply(struct imx_dsp_ipc *ipc)
>> +{
>> + unsigned long flags;
>> + struct snd_sof_dev *sdev = imx_dsp_get_data(ipc);
>> +
>> + spin_lock_irqsave(&sdev->ipc_lock, flags);
>> + snd_sof_ipc_process_reply(sdev, 0);
>> + spin_unlock_irqrestore(&sdev->ipc_lock, flags);
> Are you sure have to use spin_lock?
not sure, this definition was taken from previous drivers. I'd say keep it for now
as removing it would require some more testing which will take some time.
(snip)
>> +static int imx_suspend(struct snd_sof_dev *sdev, unsigned int target_state)
>> +{
>> + int ret;
>> + const struct sof_dsp_power_state target_power_state = {
>> + .state = target_state,
>> + };
>> +
>> + if (!pm_runtime_suspended(sdev->dev)) {
>> + ret = imx_common_suspend(sdev);
>> + if (ret < 0) {
>> + dev_err(sdev->dev, "failed to common suspend: %d\n", ret);
>> + return ret;
>> + }
>> + }
>> +
>> + return snd_sof_dsp_set_power_state(sdev, &target_power_state);
> does pm_runtime_force_suspend()/pm_runtime_force_resume() work?
no, these functions are not called directly by the PM core, but rather by the SOF core.
Using the proposed functions would result in the SOF core PM functions (i.e: sof_resume/sof_suspend)
being called twice in the case of system suspend/resume, which is wrong.
(snip)
>> +
>> +static int imx_probe(struct snd_sof_dev *sdev)
>> +{
>> + int ret;
>> + struct platform_device *pdev;
>> + struct imx_common_data *common;
>> + struct dev_pm_domain_attach_data domain_data = {
>> + .pd_names = NULL, /* no filtering */
>> + .pd_flags = PD_FLAG_DEV_LINK_ON,
>> + };
> try keep reverse Christmas tree.
sure, fix in V2
>
>> +
>> + pdev = to_platform_device(sdev->dev);
>> +
>> + common = devm_kzalloc(sdev->dev, sizeof(*common), GFP_KERNEL);
>> + if (!common)
>> + return dev_err_probe(sdev->dev, -ENOMEM,
>> + "failed to allocate common data\n");
>> +
>> + common->ipc_dev = platform_device_register_data(sdev->dev, "imx-dsp",
>> + PLATFORM_DEVID_NONE,
>> + pdev, sizeof(*pdev));
>> + if (IS_ERR(common->ipc_dev))
>> + return dev_err_probe(sdev->dev, PTR_ERR(common->ipc_dev),
>> + "failed to create IPC device\n");
>> +
>> + /* let the devres API take care of unregistering this platform
>> + * driver when no longer required.
>> + */
> I remember only network subsystem use this type comments style.
will fix in V2
>
>> + ret = devm_add_action_or_reset(sdev->dev,
>> + imx_unregister_action,
>> + common->ipc_dev);
>> + if (ret)
>> + return dev_err_probe(sdev->dev, ret, "failed to add devm action\n");
>> +
>> + common->ipc_handle = dev_get_drvdata(&common->ipc_dev->dev);
>> + if (!common->ipc_handle)
>> + return dev_err_probe(sdev->dev, -EPROBE_DEFER,
>> + "failed to fetch IPC handle\n");
>> +
>> + ret = imx_parse_ioremap_memory(sdev);
>> + if (ret < 0)
>> + return dev_err_probe(sdev->dev, ret,
>> + "failed to parse/ioremap memory regions\n");
>> +
>> + if (get_chip_info(sdev)->has_dma_reserved) {
>> + ret = of_reserved_mem_device_init_by_name(sdev->dev,
>> + pdev->dev.of_node,
>> + "dma");
> do you need put "of_reserved_mem_device_release()" at imx_unregister_action?
>
> The below error path will miss call of_reserved_mem_device_release().
good catch, thx. Fix in V2.
Powered by blists - more mailing lists