[PATCH 0/6] ASoC: Intel: catpt: Round of fixes and PM changes

[PATCH 0/6] ASoC: Intel: catpt: Round of fixes and PM changes

[Cezary Rojewski]

Set of changes addressing gaps in DRAM offset checks, error paths and
PM.

The first two patches are straight-forward, the last four relate to the
power management. The standing out PM change is removal of the
catpt-driver as a system-suspend (S3) blocker. This is a suggestion from
Andy as indeed, audio is not a critical component that should prevent
the system from going into S3. Whatever happens, the driver can recover
on a follow up resume (S3 -> S0).


Cezary Rojewski (6):
  ASoC: Intel: catpt: Fix offset checks
  ASoC: Intel: catpt: Fix error path in hw_params()
  ASoC: Intel: catpt: Fix probing order of driver components
  ASoC: Intel: catpt: Do not ignore errors on runtime resume
  ASoC: Intel: catpt: Do not block the system from suspending
  ASoC: Intel: catpt: Drop catpt_runtime_resume()

 sound/soc/intel/catpt/device.c | 33 +++++++++++++++++++--------------
 sound/soc/intel/catpt/loader.c |  8 ++++----
 sound/soc/intel/catpt/pcm.c    | 16 +++++++++-------
 sound/soc/intel/catpt/sysfs.c  |  2 +-
 4 files changed, 33 insertions(+), 26 deletions(-)

-- 
2.25.1

[PATCH 1/6] ASoC: Intel: catpt: Fix offset checks

[Cezary Rojewski]

Verify if the entire block is found within DRAM, not just the start of
it.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/catpt/loader.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/catpt/loader.c b/sound/soc/intel/catpt/loader.c
index 696d84314eeb..5804de1d89e3 100644
--- a/sound/soc/intel/catpt/loader.c
+++ b/sound/soc/intel/catpt/loader.c
@@ -216,7 +216,7 @@ static int catpt_restore_memdumps(struct catpt_dev *cdev, struct dma_chan *chan)
 			continue;
 
 		off = catpt_to_host_offset(info->offset);
-		if (off < cdev->dram.start || off > cdev->dram.end)
+		if (off < cdev->dram.start || off + info->size >= cdev->dram.end)
 			continue;
 
 		dev_dbg(cdev->dev, "restoring memdump: off 0x%08x size %d\n",
@@ -261,12 +261,12 @@ static int catpt_restore_fwimage(struct catpt_dev *cdev,
 			continue;
 
 		off = catpt_to_host_offset(info->offset);
-		if (off < cdev->dram.start || off > cdev->dram.end)
-			continue;
-
 		r2.start = off;
 		r2.end = r2.start + info->size - 1;
 
+		if (r2.start < cdev->dram.start || r2.end > cdev->dram.end)
+			continue;
+
 		if (!resource_intersection(&r2, &r1, &common))
 			continue;
 		/* calculate start offset of common data area */
-- 
2.25.1

[PATCH 2/6] ASoC: Intel: catpt: Fix error path in hw_params()

[Cezary Rojewski]

Do not leave any resources hanging on the DSP side if applying user
settings fails.

Fixes: 768a3a3b327d ("ASoC: Intel: catpt: Optimize applying user settings")
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/catpt/pcm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c
index f15385683d9c..9baaad13a273 100644
--- a/sound/soc/intel/catpt/pcm.c
+++ b/sound/soc/intel/catpt/pcm.c
@@ -417,8 +417,10 @@ static int catpt_dai_hw_params(struct snd_pcm_substream *substream,
 		return CATPT_IPC_ERROR(ret);
 
 	ret = catpt_dai_apply_usettings(dai, stream);
-	if (ret)
+	if (ret) {
+		catpt_ipc_free_stream(cdev, stream->info.stream_hw_id);
 		return ret;
+	}
 
 	stream->allocated = true;
 	return 0;
-- 
2.25.1

[PATCH 3/6] ASoC: Intel: catpt: Fix probing order of driver components

[Cezary Rojewski]

catpt_dai_pcm_new() is called during the bring up sequence of the
machine board device. That device is a child of the ACPI device the
catpt-driver attaches to. The function manipulates pm_runtime_xxx() to
ensure its parent is woken up before sending IPCs. If the parent's
pm_runtime is not configured before that happens, errors will occur.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/catpt/device.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
index faa916f40069..eed330bc82b6 100644
--- a/sound/soc/intel/catpt/device.c
+++ b/sound/soc/intel/catpt/device.c
@@ -184,22 +184,25 @@ static int catpt_probe_components(struct catpt_dev *cdev)
 		goto err_boot_fw;
 	}
 
-	ret = catpt_register_board(cdev);
-	if (ret) {
-		dev_err(cdev->dev, "register board failed: %d\n", ret);
-		goto err_reg_board;
-	}
-
 	/* reflect actual ADSP state in pm_runtime */
 	pm_runtime_set_active(cdev->dev);
 
 	pm_runtime_set_autosuspend_delay(cdev->dev, 2000);
 	pm_runtime_use_autosuspend(cdev->dev);
 	pm_runtime_mark_last_busy(cdev->dev);
+	/* Enable PM before spawning child device. See catpt_dai_pcm_new(). */
 	pm_runtime_enable(cdev->dev);
+
+	ret = catpt_register_board(cdev);
+	if (ret) {
+		dev_err(cdev->dev, "register board failed: %d\n", ret);
+		goto err_reg_board;
+	}
+
 	return 0;
 
 err_reg_board:
+	pm_runtime_disable(cdev->dev);
 	snd_soc_unregister_component(cdev->dev);
 err_boot_fw:
 	catpt_dmac_remove(cdev);
-- 
2.25.1

[PATCH 4/6] ASoC: Intel: catpt: Do not ignore errors on runtime resume

[Cezary Rojewski]

If pm_runtime_resume_and_get() fails, follow up pm_runtime_xxx()
operate on device in erroneous state.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/catpt/pcm.c   | 12 ++++++------
 sound/soc/intel/catpt/sysfs.c |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c
index 9baaad13a273..abd1cb07c60c 100644
--- a/sound/soc/intel/catpt/pcm.c
+++ b/sound/soc/intel/catpt/pcm.c
@@ -671,7 +671,7 @@ static int catpt_dai_pcm_new(struct snd_soc_pcm_runtime *rtm,
 		return 0;
 
 	ret = pm_runtime_resume_and_get(cdev->dev);
-	if (ret < 0 && ret != -EACCES)
+	if (ret)
 		return ret;
 
 	ret = catpt_ipc_set_device_format(cdev, &devfmt);
@@ -874,7 +874,7 @@ static int catpt_mixer_volume_get(struct snd_kcontrol *kcontrol,
 	int i;
 
 	ret = pm_runtime_resume_and_get(cdev->dev);
-	if (ret < 0 && ret != -EACCES)
+	if (ret)
 		return ret;
 
 	for (i = 0; i < CATPT_CHANNELS_MAX; i++) {
@@ -895,7 +895,7 @@ static int catpt_mixer_volume_put(struct snd_kcontrol *kcontrol,
 	int ret;
 
 	ret = pm_runtime_resume_and_get(cdev->dev);
-	if (ret < 0 && ret != -EACCES)
+	if (ret)
 		return ret;
 
 	ret = catpt_set_dspvol(cdev, cdev->mixer.mixer_hw_id,
@@ -926,7 +926,7 @@ static int catpt_stream_volume_get(struct snd_kcontrol *kcontrol,
 	}
 
 	ret = pm_runtime_resume_and_get(cdev->dev);
-	if (ret < 0 && ret != -EACCES)
+	if (ret)
 		return ret;
 
 	for (i = 0; i < CATPT_CHANNELS_MAX; i++) {
@@ -957,7 +957,7 @@ static int catpt_stream_volume_put(struct snd_kcontrol *kcontrol,
 	}
 
 	ret = pm_runtime_resume_and_get(cdev->dev);
-	if (ret < 0 && ret != -EACCES)
+	if (ret)
 		return ret;
 
 	ret = catpt_set_dspvol(cdev, stream->info.stream_hw_id,
@@ -1033,7 +1033,7 @@ static int catpt_loopback_switch_put(struct snd_kcontrol *kcontrol,
 	}
 
 	ret = pm_runtime_resume_and_get(cdev->dev);
-	if (ret < 0 && ret != -EACCES)
+	if (ret)
 		return ret;
 
 	ret = catpt_ipc_mute_loopback(cdev, stream->info.stream_hw_id, mute);
diff --git a/sound/soc/intel/catpt/sysfs.c b/sound/soc/intel/catpt/sysfs.c
index 048253002ec8..e961e172f9b7 100644
--- a/sound/soc/intel/catpt/sysfs.c
+++ b/sound/soc/intel/catpt/sysfs.c
@@ -16,7 +16,7 @@ static ssize_t fw_version_show(struct device *dev,
 	int ret;
 
 	ret = pm_runtime_resume_and_get(cdev->dev);
-	if (ret < 0 && ret != -EACCES)
+	if (ret)
 		return ret;
 
 	ret = catpt_ipc_get_fw_version(cdev, &version);
-- 
2.25.1

[PATCH 5/6] ASoC: Intel: catpt: Do not block the system from suspending

[Cezary Rojewski]

Even if something goes wrong when performing suspend on DSP, from the
system perspective the component is not critical enough to block the
suspend operation entirely. Leaving recovery to next resume() suffices.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/catpt/device.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
index eed330bc82b6..08788b390874 100644
--- a/sound/soc/intel/catpt/device.c
+++ b/sound/soc/intel/catpt/device.c
@@ -28,7 +28,7 @@
 #define CREATE_TRACE_POINTS
 #include "trace.h"
 
-static int catpt_suspend(struct device *dev)
+static int __catpt_suspend(struct device *dev)
 {
 	struct catpt_dev *cdev = dev_get_drvdata(dev);
 	struct dma_chan *chan;
@@ -106,6 +106,13 @@ static int catpt_resume(struct device *dev)
 	return 0;
 }
 
+/* Do not block the system from suspending, recover on resume() if needed. */
+static int catpt_suspend(struct device *dev)
+{
+	__catpt_suspend(dev);
+	return 0;
+}
+
 static int catpt_runtime_suspend(struct device *dev)
 {
 	if (!try_module_get(dev->driver->owner)) {
@@ -114,7 +121,7 @@ static int catpt_runtime_suspend(struct device *dev)
 	}
 	module_put(dev->driver->owner);
 
-	return catpt_suspend(dev);
+	return __catpt_suspend(dev);
 }
 
 static int catpt_runtime_resume(struct device *dev)
-- 
2.25.1

[PATCH 6/6] ASoC: Intel: catpt: Drop catpt_runtime_resume()

[Cezary Rojewski]

The function provides no value over catpt_resume(), use the latter
directly.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/catpt/device.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
index 08788b390874..61eaeca2cacc 100644
--- a/sound/soc/intel/catpt/device.c
+++ b/sound/soc/intel/catpt/device.c
@@ -124,14 +124,9 @@ static int catpt_runtime_suspend(struct device *dev)
 	return __catpt_suspend(dev);
 }
 
-static int catpt_runtime_resume(struct device *dev)
-{
-	return catpt_resume(dev);
-}
-
 static const struct dev_pm_ops catpt_dev_pm = {
 	SYSTEM_SLEEP_PM_OPS(catpt_suspend, catpt_resume)
-	RUNTIME_PM_OPS(catpt_runtime_suspend, catpt_runtime_resume, NULL)
+	RUNTIME_PM_OPS(catpt_runtime_suspend, catpt_resume, NULL)
 };
 
 /* machine board owned by CATPT is removed with this hook */
-- 
2.25.1

Re: [PATCH 1/6] ASoC: Intel: catpt: Fix offset checks

[Andy Shevchenko]

On Fri, Nov 21, 2025 at 12:43:26PM +0100, Cezary Rojewski wrote:
> Verify if the entire block is found within DRAM, not just the start of
> it.

I would rewrap this to have less distant lengths.

E.g.,

Verify if the entire block is found within DRAM, not just
the start of it.

> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

...

> static int catpt_restore_memdumps(struct catpt_dev *cdev, struct dma_chan *chan)

>  		off = catpt_to_host_offset(info->offset);
> -		if (off < cdev->dram.start || off > cdev->dram.end)
> +		if (off < cdev->dram.start || off + info->size >= cdev->dram.end)
>  			continue;

Hmm... I would rather do something like

		struct resource r;
		...
		resource_set_range(catpt_to_host_offset(info->offset), info->size);
		if (!resource_contains())
			continue;

>  		dev_dbg(cdev->dev, "restoring memdump: off 0x%08x size %d\n",

OTOH it seems more invasive change for kinda a fix.

...

> static int catpt_restore_fwimage(struct catpt_dev *cdev,

>  		off = catpt_to_host_offset(info->offset);
> -		if (off < cdev->dram.start || off > cdev->dram.end)
> -			continue;
> -
>  		r2.start = off;
>  		r2.end = r2.start + info->size - 1;
>  
> +		if (r2.start < cdev->dram.start || r2.end > cdev->dram.end)
> +			continue;

See above, but here we already good for use the respective resource_*() API.

>  		if (!resource_intersection(&r2, &r1, &common))
>  			continue;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] ASoC: Intel: catpt: Drop catpt_runtime_resume()

[Andy Shevchenko]

On Fri, Nov 21, 2025 at 12:43:31PM +0100, Cezary Rojewski wrote:
> The function provides no value over catpt_resume(), use the latter
> directly.

This makes code understanding a bit harder in my opinion. I would not apply
this patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] ASoC: Intel: catpt: Do not block the system from suspending

[Andy Shevchenko]

On Fri, Nov 21, 2025 at 12:43:30PM +0100, Cezary Rojewski wrote:
> Even if something goes wrong when performing suspend on DSP, from the
> system perspective the component is not critical enough to block the
> suspend operation entirely. Leaving recovery to next resume() suffices.

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> +/* Do not block the system from suspending, recover on resume() if needed. */
> +static int catpt_suspend(struct device *dev)
> +{
> +	__catpt_suspend(dev);

Can we use catpt_do_suspend() name? Or something with a better suffix?

Wouldn't be useful to have some debug message to be printed in case of failure?

	ret = catpt_do_suspend(dev);
	if (ret)
		dev_dbg(dev, ..., ret);

?

> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] ASoC: Intel: catpt: Do not block the system from suspending

[Cezary Rojewski]

On 2025-11-24 3:42 PM, Andy Shevchenko wrote:
> On Fri, Nov 21, 2025 at 12:43:30PM +0100, Cezary Rojewski wrote:
>> Even if something goes wrong when performing suspend on DSP, from the
>> system perspective the component is not critical enough to block the
>> suspend operation entirely. Leaving recovery to next resume() suffices.
> 
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> ...
> 
>> +/* Do not block the system from suspending, recover on resume() if needed. */
>> +static int catpt_suspend(struct device *dev)
>> +{
>> +	__catpt_suspend(dev);
> 
> Can we use catpt_do_suspend() name? Or something with a better suffix?
> 
> Wouldn't be useful to have some debug message to be printed in case of failure?
> 
> 	ret = catpt_do_suspend(dev);
> 	if (ret)
> 		dev_dbg(dev, ..., ret);
> 
> ?
> 

No problem renaming the function in v2. As for the debug print, 
__catpt_suspend/catpt_do_suspend() would dump enough information 
already. If I get failure from powering up DRAM/IRAM banks or an IPC 
failure, additional "failed" message on top won't add anything useful.

>> +	return 0;
>> +}
> 

Re: [PATCH 6/6] ASoC: Intel: catpt: Drop catpt_runtime_resume()

[Cezary Rojewski]

On 2025-11-24 3:37 PM, Andy Shevchenko wrote:
> On Fri, Nov 21, 2025 at 12:43:31PM +0100, Cezary Rojewski wrote:
>> The function provides no value over catpt_resume(), use the latter
>> directly.
> 
> This makes code understanding a bit harder in my opinion. I would not apply
> this patch.
> 

Ack, will remove in v2.

Re: [PATCH 1/6] ASoC: Intel: catpt: Fix offset checks

[Cezary Rojewski]

On 2025-11-24 3:31 PM, Andy Shevchenko wrote:
> On Fri, Nov 21, 2025 at 12:43:26PM +0100, Cezary Rojewski wrote:
>> Verify if the entire block is found within DRAM, not just the start of
>> it.
> 
> I would rewrap this to have less distant lengths.
> 
> E.g.,
> 
> Verify if the entire block is found within DRAM, not just
> the start of it.
> 

Sure, will do. Typically I just do 72-chars per line and whatever 
happens, happens.

>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> 
> ...
> 
>> static int catpt_restore_memdumps(struct catpt_dev *cdev, struct dma_chan *chan)
> 
>>   		off = catpt_to_host_offset(info->offset);
>> -		if (off < cdev->dram.start || off > cdev->dram.end)
>> +		if (off < cdev->dram.start || off + info->size >= cdev->dram.end)
>>   			continue;
> 
> Hmm... I would rather do something like
> 
> 		struct resource r;
> 		...
> 		resource_set_range(catpt_to_host_offset(info->offset), info->size);
> 		if (!resource_contains())
> 			continue;
> 
>>   		dev_dbg(cdev->dev, "restoring memdump: off 0x%08x size %d\n",
> 
> OTOH it seems more invasive change for kinda a fix.

Looks elegant though. My idea was to avoid additional operations (which 
resource_contains() does), perhaps unnecessarily as readability suffered 
because of that. Ack.

> 
> ...
> 
>> static int catpt_restore_fwimage(struct catpt_dev *cdev,
> 
>>   		off = catpt_to_host_offset(info->offset);
>> -		if (off < cdev->dram.start || off > cdev->dram.end)
>> -			continue;
>> -
>>   		r2.start = off;
>>   		r2.end = r2.start + info->size - 1;
>>   
>> +		if (r2.start < cdev->dram.start || r2.end > cdev->dram.end)
>> +			continue;
> 
> See above, but here we already good for use the respective resource_*() API.
> 
>>   		if (!resource_intersection(&r2, &r1, &common))
>>   			continue;
> 

Re: [PATCH 1/6] ASoC: Intel: catpt: Fix offset checks

[Andy Shevchenko]

On Mon, Nov 24, 2025 at 07:20:48PM +0100, Cezary Rojewski wrote:
> On 2025-11-24 3:31 PM, Andy Shevchenko wrote:
> > On Fri, Nov 21, 2025 at 12:43:26PM +0100, Cezary Rojewski wrote:
> > > Verify if the entire block is found within DRAM, not just the start of
> > > it.
> > 
> > I would rewrap this to have less distant lengths.
> > 
> > E.g.,
> > 
> > Verify if the entire block is found within DRAM, not just
> > the start of it.
> 
> Sure, will do. Typically I just do 72-chars per line and whatever happens,
> happens.

Understand, but here is a corner case, I usually rewrap a bit when it's line
out of only a couple of characters.

...

> > > static int catpt_restore_memdumps(struct catpt_dev *cdev, struct dma_chan *chan)
> > 
> > >   		off = catpt_to_host_offset(info->offset);
> > > -		if (off < cdev->dram.start || off > cdev->dram.end)
> > > +		if (off < cdev->dram.start || off + info->size >= cdev->dram.end)
> > >   			continue;
> > 
> > Hmm... I would rather do something like
> > 
> > 		struct resource r;
> > 		...
> > 		resource_set_range(catpt_to_host_offset(info->offset), info->size);
> > 		if (!resource_contains())
> > 			continue;
> > 
> > >   		dev_dbg(cdev->dev, "restoring memdump: off 0x%08x size %d\n",
> > 
> > OTOH it seems more invasive change for kinda a fix.
> 
> Looks elegant though. My idea was to avoid additional operations (which
> resource_contains() does), perhaps unnecessarily as readability suffered
> because of that. Ack.

Note, it's defined as static inline, if compiler can prove constiness of
something, it will eliminate the code. TL;DR: let compiler to do its job.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] ASoC: Intel: catpt: Fix offset checks

[Andy Shevchenko]

On Mon, Nov 24, 2025 at 09:05:06PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 07:20:48PM +0100, Cezary Rojewski wrote:
> > On 2025-11-24 3:31 PM, Andy Shevchenko wrote:
> > > On Fri, Nov 21, 2025 at 12:43:26PM +0100, Cezary Rojewski wrote:

...

> > > > static int catpt_restore_memdumps(struct catpt_dev *cdev, struct dma_chan *chan)
> > > 
> > > >   		off = catpt_to_host_offset(info->offset);
> > > > -		if (off < cdev->dram.start || off > cdev->dram.end)
> > > > +		if (off < cdev->dram.start || off + info->size >= cdev->dram.end)
> > > >   			continue;
> > > 
> > > Hmm... I would rather do something like

> > > 		struct resource r;

You might need to nullify this as well:

		struct resource r = {}; // or any equivalent

> > > 		...
> > > 		resource_set_range(catpt_to_host_offset(info->offset), info->size);
> > > 		if (!resource_contains())
> > > 			continue;
> > > 
> > > >   		dev_dbg(cdev->dev, "restoring memdump: off 0x%08x size %d\n",
> > > 
> > > OTOH it seems more invasive change for kinda a fix.
> > 
> > Looks elegant though. My idea was to avoid additional operations (which
> > resource_contains() does), perhaps unnecessarily as readability suffered
> > because of that. Ack.
> 
> Note, it's defined as static inline, if compiler can prove constiness of
> something, it will eliminate the code. TL;DR: let compiler to do its job.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] ASoC: Intel: catpt: Do not block the system from suspending

[Andy Shevchenko]

On Mon, Nov 24, 2025 at 07:05:55PM +0100, Cezary Rojewski wrote:
> On 2025-11-24 3:42 PM, Andy Shevchenko wrote:
> > On Fri, Nov 21, 2025 at 12:43:30PM +0100, Cezary Rojewski wrote:

...

> > > +/* Do not block the system from suspending, recover on resume() if needed. */
> > > +static int catpt_suspend(struct device *dev)
> > > +{
> > > +	__catpt_suspend(dev);
> > 
> > Can we use catpt_do_suspend() name? Or something with a better suffix?
> > 
> > Wouldn't be useful to have some debug message to be printed in case of failure?
> > 
> > 	ret = catpt_do_suspend(dev);
> > 	if (ret)
> > 		dev_dbg(dev, ..., ret);
> > 
> > ?
> 
> No problem renaming the function in v2. As for the debug print,
> __catpt_suspend/catpt_do_suspend() would dump enough information already. If
> I get failure from powering up DRAM/IRAM banks or an IPC failure, additional
> "failed" message on top won't add anything useful.

Okay, then no need to add a noise, indeed.

> > > +	return 0;
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] ASoC: Intel: catpt: Fix offset checks

[Cezary Rojewski]

On 2025-11-24 8:06 PM, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 09:05:06PM +0200, Andy Shevchenko wrote:
>> On Mon, Nov 24, 2025 at 07:20:48PM +0100, Cezary Rojewski wrote:
>>> On 2025-11-24 3:31 PM, Andy Shevchenko wrote:
>>>> On Fri, Nov 21, 2025 at 12:43:26PM +0100, Cezary Rojewski wrote:
> 
> ...
> 
>>>>> static int catpt_restore_memdumps(struct catpt_dev *cdev, struct dma_chan *chan)
>>>>
>>>>>    		off = catpt_to_host_offset(info->offset);
>>>>> -		if (off < cdev->dram.start || off > cdev->dram.end)
>>>>> +		if (off < cdev->dram.start || off + info->size >= cdev->dram.end)
>>>>>    			continue;
>>>>
>>>> Hmm... I would rather do something like
> 
>>>> 		struct resource r;
> 
> You might need to nullify this as well:
> 
> 		struct resource r = {}; // or any equivalent

This made me realize variables 'r1', 'r2' and 'common' that are part of 
catpt_restore_fwimage() are not zeroed and utilizing resource_xxx() API 
without doing so is bogus. My plan is to the following: leave this very 
fix as-is (without refactoring) and follow it up immediately with patch 
that swaps manual manipulation with resource_xxx(), as you suggested.

Sounds good?

>>>> 		...
>>>> 		resource_set_range(catpt_to_host_offset(info->offset), info->size);
>>>> 		if (!resource_contains())
>>>> 			continue;
>>>>
>>>>>    		dev_dbg(cdev->dev, "restoring memdump: off 0x%08x size %d\n",
>>>>
>>>> OTOH it seems more invasive change for kinda a fix.
>>>
>>> Looks elegant though. My idea was to avoid additional operations (which
>>> resource_contains() does), perhaps unnecessarily as readability suffered
>>> because of that. Ack.
>>
>> Note, it's defined as static inline, if compiler can prove constiness of
>> something, it will eliminate the code. TL;DR: let compiler to do its job.
> 

Re: [PATCH 1/6] ASoC: Intel: catpt: Fix offset checks

[Andy Shevchenko]

On Tue, Nov 25, 2025 at 12:31:04PM +0100, Cezary Rojewski wrote:
> On 2025-11-24 8:06 PM, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 09:05:06PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 24, 2025 at 07:20:48PM +0100, Cezary Rojewski wrote:
> > > > On 2025-11-24 3:31 PM, Andy Shevchenko wrote:
> > > > > On Fri, Nov 21, 2025 at 12:43:26PM +0100, Cezary Rojewski wrote:

...

> > > > > > static int catpt_restore_memdumps(struct catpt_dev *cdev, struct dma_chan *chan)
> > > > > 
> > > > > >    		off = catpt_to_host_offset(info->offset);
> > > > > > -		if (off < cdev->dram.start || off > cdev->dram.end)
> > > > > > +		if (off < cdev->dram.start || off + info->size >= cdev->dram.end)
> > > > > >    			continue;
> > > > > 
> > > > > Hmm... I would rather do something like
> > 
> > > > > 		struct resource r;
> > 
> > You might need to nullify this as well:
> > 
> > 		struct resource r = {}; // or any equivalent
> 
> This made me realize variables 'r1', 'r2' and 'common' that are part of
> catpt_restore_fwimage() are not zeroed and utilizing resource_xxx() API
> without doing so is bogus. My plan is to the following: leave this very fix
> as-is (without refactoring) and follow it up immediately with patch that
> swaps manual manipulation with resource_xxx(), as you suggested.
> 
> Sounds good?

Yep! Thanks.

> > > > > 		...
> > > > > 		resource_set_range(catpt_to_host_offset(info->offset), info->size);
> > > > > 		if (!resource_contains())
> > > > > 			continue;
> > > > > 
> > > > > >    		dev_dbg(cdev->dev, "restoring memdump: off 0x%08x size %d\n",
> > > > > 
> > > > > OTOH it seems more invasive change for kinda a fix.
> > > > 
> > > > Looks elegant though. My idea was to avoid additional operations (which
> > > > resource_contains() does), perhaps unnecessarily as readability suffered
> > > > because of that. Ack.
> > > 
> > > Note, it's defined as static inline, if compiler can prove constiness of
> > > something, it will eliminate the code. TL;DR: let compiler to do its job.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] ASoC: Intel: catpt: Fix offset checks

[Andy Shevchenko]

On Tue, Nov 25, 2025 at 08:24:35PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 25, 2025 at 12:31:04PM +0100, Cezary Rojewski wrote:
> > On 2025-11-24 8:06 PM, Andy Shevchenko wrote:
> > > On Mon, Nov 24, 2025 at 09:05:06PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Nov 24, 2025 at 07:20:48PM +0100, Cezary Rojewski wrote:
> > > > > On 2025-11-24 3:31 PM, Andy Shevchenko wrote:
> > > > > > On Fri, Nov 21, 2025 at 12:43:26PM +0100, Cezary Rojewski wrote:

...

> > > > > > > static int catpt_restore_memdumps(struct catpt_dev *cdev, struct dma_chan *chan)
> > > > > > 
> > > > > > >    		off = catpt_to_host_offset(info->offset);
> > > > > > > -		if (off < cdev->dram.start || off > cdev->dram.end)
> > > > > > > +		if (off < cdev->dram.start || off + info->size >= cdev->dram.end)
> > > > > > >    			continue;
> > > > > > 
> > > > > > Hmm... I would rather do something like
> > > 
> > > > > > 		struct resource r;
> > > 
> > > You might need to nullify this as well:
> > > 
> > > 		struct resource r = {}; // or any equivalent
> > 
> > This made me realize variables 'r1', 'r2' and 'common' that are part of
> > catpt_restore_fwimage() are not zeroed and utilizing resource_xxx() API
> > without doing so is bogus. My plan is to the following: leave this very fix
> > as-is (without refactoring) and follow it up immediately with patch that
> > swaps manual manipulation with resource_xxx(), as you suggested.
> > 
> > Sounds good?
> 
> Yep! Thanks.

Just to add, you may also consider using the form

	r = DEFINE_RES_*(...);

which effectively will nullify untouched fields.
So, either it or {} or memset() whichever fits
better.

> > > > > > 		...
> > > > > > 		resource_set_range(catpt_to_host_offset(info->offset), info->size);
> > > > > > 		if (!resource_contains())
> > > > > > 			continue;
> > > > > > 
> > > > > > >    		dev_dbg(cdev->dev, "restoring memdump: off 0x%08x size %d\n",
> > > > > > 
> > > > > > OTOH it seems more invasive change for kinda a fix.
> > > > > 
> > > > > Looks elegant though. My idea was to avoid additional operations (which
> > > > > resource_contains() does), perhaps unnecessarily as readability suffered
> > > > > because of that. Ack.
> > > > 
> > > > Note, it's defined as static inline, if compiler can prove constiness of
> > > > something, it will eliminate the code. TL;DR: let compiler to do its job.

-- 
With Best Regards,
Andy Shevchenko