diff mbox series

ASoC: atmel: mchp-pdmc: Skip ALSA restoration if substream runtime is uninitialized

Message ID 20240924081237.50046-1-andrei.simion@microchip.com (mailing list archive)
State Accepted
Commit 09cfc6a532d249a51d3af5022d37ebbe9c3d31f6
Headers show
Series ASoC: atmel: mchp-pdmc: Skip ALSA restoration if substream runtime is uninitialized | expand

Commit Message

Andrei Simion Sept. 24, 2024, 8:12 a.m. UTC
Update the driver to prevent alsa-restore.service from failing when
reading data from /var/lib/alsa/asound.state at boot. Ensure that the
restoration of ALSA mixer configurations is skipped if substream->runtime
is NULL.

Fixes: 50291652af52 ("ASoC: atmel: mchp-pdmc: add PDMC driver")
Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
---
 sound/soc/atmel/mchp-pdmc.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 4d0326b60bb753627437fff0f76bf1525bcda422

Comments

Mark Brown Sept. 24, 2024, 8:59 a.m. UTC | #1
On Tue, Sep 24, 2024 at 11:12:38AM +0300, Andrei Simion wrote:

> Update the driver to prevent alsa-restore.service from failing when
> reading data from /var/lib/alsa/asound.state at boot. Ensure that the
> restoration of ALSA mixer configurations is skipped if substream->runtime
> is NULL.

> +++ b/sound/soc/atmel/mchp-pdmc.c
> @@ -302,6 +302,9 @@ static int mchp_pdmc_chmap_ctl_put(struct snd_kcontrol *kcontrol,
>  	if (!substream)
>  		return -ENODEV;
>  
> +	if (!substream->runtime)
> +		return 0; /* just for avoiding error from alsactl restore */
> +

This then means that control writes are just discarded which presumably
is going to upset things if they actually saved a value here.  Why is
that a good choice, rather than either fixing the race so the card
doesn't come up too early or removing the need for the runtime?
Andrei Simion Sept. 24, 2024, 11:25 a.m. UTC | #2
On 24.09.2024 11:59, Mark Brown wrote:
> 
> 
> On Tue, Sep 24, 2024 at 11:12:38AM +0300, Andrei Simion wrote:
> 
>> Update the driver to prevent alsa-restore.service from failing when
>> reading data from /var/lib/alsa/asound.state at boot. Ensure that the
>> restoration of ALSA mixer configurations is skipped if substream->runtime
>> is NULL.
>> +++ b/sound/soc/atmel/mchp-pdmc.c
>> @@ -302,6 +302,9 @@ static int mchp_pdmc_chmap_ctl_put(struct snd_kcontrol *kcontrol,
>>	if (!substream)
>>		return -ENODEV;
>>
>> +	if (!substream->runtime)
>> +		return 0; /* just for avoiding error from alsactl restore */
>> +
> This then means that control writes are just discarded which presumably
> is going to upset things if they actually saved a value here.  Why is
> that a good choice, rather than either fixing the race so the card
> doesn't come up too early or removing the need for the runtime?

Ok. I understand. My first intention was to follow the
https://github.com/torvalds/linux/blob/master/sound/hda/hdmi_chmap.c#L794
but after your point of view, I intend to return -EAGAIN in V2
to specify the substream->runtime is not ready.

I retested: configured pdmc, then reboot the board and the configuration:
as a result the configuration kept.

alsa-restore.service status success.

Do you think this solution is enough?

Thank you and best regards,
Andrei Simion
Mark Brown Sept. 24, 2024, 11:34 a.m. UTC | #3
On Tue, Sep 24, 2024 at 02:25:44PM +0300, Andrei Simion wrote:
> On 24.09.2024 11:59, Mark Brown wrote:
> > On Tue, Sep 24, 2024 at 11:12:38AM +0300, Andrei Simion wrote:

> > This then means that control writes are just discarded which presumably
> > is going to upset things if they actually saved a value here.  Why is
> > that a good choice, rather than either fixing the race so the card
> > doesn't come up too early or removing the need for the runtime?

> Ok. I understand. My first intention was to follow the
> https://github.com/torvalds/linux/blob/master/sound/hda/hdmi_chmap.c#L794
> but after your point of view, I intend to return -EAGAIN in V2
> to specify the substream->runtime is not ready.

> I retested: configured pdmc, then reboot the board and the configuration:
> as a result the configuration kept.

> alsa-restore.service status success.

> Do you think this solution is enough?

Ah, I'm a little surprised that we baked retries like that into the API
but if userspace already understands that then that'd be fine.  Though
now I see that the HDA code already does what your existing patch does
I'm happy to just queue this version, HDA is so widely deployed that I'd
guess everything will DTRT and it's probably best to be consistent.
Sorry, I hadn't seen these nuances of the channel map interface before.
Mark Brown Sept. 24, 2024, 3:28 p.m. UTC | #4
On Tue, 24 Sep 2024 11:12:38 +0300, Andrei Simion wrote:
> Update the driver to prevent alsa-restore.service from failing when
> reading data from /var/lib/alsa/asound.state at boot. Ensure that the
> restoration of ALSA mixer configurations is skipped if substream->runtime
> is NULL.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: atmel: mchp-pdmc: Skip ALSA restoration if substream runtime is uninitialized
      commit: 09cfc6a532d249a51d3af5022d37ebbe9c3d31f6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
diff mbox series

Patch

diff --git a/sound/soc/atmel/mchp-pdmc.c b/sound/soc/atmel/mchp-pdmc.c
index 939cd44ebc8a..06dc3c48e7e8 100644
--- a/sound/soc/atmel/mchp-pdmc.c
+++ b/sound/soc/atmel/mchp-pdmc.c
@@ -302,6 +302,9 @@  static int mchp_pdmc_chmap_ctl_put(struct snd_kcontrol *kcontrol,
 	if (!substream)
 		return -ENODEV;
 
+	if (!substream->runtime)
+		return 0; /* just for avoiding error from alsactl restore */
+
 	map = mchp_pdmc_chmap_get(substream, info);
 	if (!map)
 		return -EINVAL;