linux-kernel - Re: [PATCH 1/9] ASoC: SOF: imx: introduce more common structures and functions

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: <Z6ONWfIycA0oHEB6@lizhi-Precision-Tower-5810>
Date: Wed, 5 Feb 2025 11:10:01 -0500
From: Frank Li <Frank.li@....com>
To: Laurentiu Mihalcea <laurentiumihalcea111@...il.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 Tue, Feb 04, 2025 at 06:59:53PM +0200, Laurentiu Mihalcea wrote:
>
>
>
> 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.
>

Change lock type need seperate patch. analyze is more important then test.
It is really hard to hit race condition at normal user case.

You can check if any irq handle use ipc_lock? If irq handle already defer
to thread irq, mutex most likely is enough.

Frank

> (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.

Does SOC core should check it? I don't reject this change, but check
run time pm status in suspend often cause some bugs.

Frank

>
> (snip)
>
...