| 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: <731330d0-e177-f9cb-50d2-91701f8dff0a@mentor.com>
Date: Tue, 6 Aug 2019 16:21:00 +0900
From: Jiada Wang <jiada_wang@...tor.com>
To: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
CC: <lgirdwood@...il.com>, <broonie@...nel.org>, <perex@...ex.cz>,
<tiwai@...e.com>, <twischer@...adit-jv.com>,
<alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/3] ASoC: rsnd: Allow reconfiguration of clock rate
Hi Morimoto-san
Sorry for the delayed response
On 2019/07/22 17:41, Kuninori Morimoto wrote:
>
> Hi Jiada
>
> The solution looks very over-kill to me,
> especiallyq [3/3] patch is too much to me.
>
> 1st, can we start clock at .hw_param, instead of .prepare ?
> and stop it at .hw_free ?
>
the reasoning to move start of clock from .init to .prepare by
commit 4d230d1271064 ("ASoC: rsnd: fixup not to call clk_get/set under
non-atomic")
is to prevent clk_{get, set_rate} be called from atomic context,
since .hw_params is non atomic context,
so I think start of clock can be moved from .prepare to .hw_params
> 2nd, can we keep usrcnt setup as-is ?
> I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not 0 ?
I don't fully understand your 2nd question,
in case of rsnd_ssi_master_clk_stop(), if avoid
rsnd_ssi_master_clk_stop() when ssi->rate is 0 by apply following change
static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod,
struct rsnd_dai_stream *io)
{
...
- if (ssi->usrcnt > 1)
+ if (ssi->rate == 0)
return;
...
}
then when any IO stream with same SSI calls .hw_free,
the other IO stream's clock will be stopped too.
Thanks,
Jiada
> similar for rsnd_ssi_master_clk_stop()
>
> static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod,
> struct rsnd_dai_stream *io)
> {
> ...
> if (ssi->rate)
> return 0;
> ...
> }
>
> static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod,
> struct rsnd_dai_stream *io)
> {
> ...
> - if (ssi->usrcnt > 1)
> + if (ssi->rate == 0)
> return;
> ...
> }
>
>
>> From: Timo Wischer <twischer@...adit-jv.com>
>>
>> Currently after clock rate is started in .prepare reconfiguration
>> of clock rate is not allowed, unless the stream is stopped.
>>
>> But there is use case in Gstreamer ALSA sink, in case of changed caps
>> Gsreatmer reconfigs and it calls snd_pcm_hw_free() before snd_pcm_prepre().
>> See gstreamer1.0-plugins-base/
>> gst-libs/gst/audio/gstaudiobasesink.c: gst_audio_base_sink_setcaps():
>> - gst_audio_ring_buffer_release()
>> - gst_audio_sink_ring_buffer_release()
>> - gst_alsasink_unprepare()
>> - snd_pcm_hw_free()
>> is called before
>> - gst_audio_ring_buffer_acquire()
>> - gst_audio_sink_ring_buffer_acquire()
>> - gst_alsasink_prepare()
>> - set_hwparams()
>> - snd_pcm_hw_params()
>> - snd_pcm_prepare()
>>
>> The issue mentioned in this commit can be reproduced with the following
>> aplay patch:
>>
>> >diff --git a/aplay/aplay.c b/aplay/aplay.c
>> >@@ -2760,6 +2760,8 @@ static void playback_go(int fd, size_t loaded,
>> > header(rtype, name);
>> > set_params();
>> >+ hwparams.rate = (hwparams.rate == 48000) ? 44100 : 48000;
>> >+ set_params();
>> >
>> > while (loaded > chunk_bytes && written < count && !in_aborting)
>> > {
>> > if (pcm_write(audiobuf + written, chunk_size) <= 0)
>>
>> $ aplay -Dplughw:0,0,0 -c8 -fS24_LE -r48000 /dev/zero
>> rcar_sound ec500000.sound: SSI parent/child should use same rate
>> rcar_sound ec500000.sound: ssi[3] : prepare error -22
>> rcar_sound ec500000.sound: ASoC: cpu DAI prepare error: -22
>> rsnd_link0: ASoC: prepare FE rsnd_link0 failed
>>
>> this patch address the issue by stop clock in .hw_free,
>> to allow reconfiguration of clock rate without stop of the stream.
>>
>> Signed-off-by: Timo Wischer <twischer@...adit-jv.com>
>> Signed-off-by: Jiada Wang <jiada_wang@...tor.com>
>> ---
>> sound/soc/sh/rcar/ssi.c | 53 +++++++++++++++++++++++++++++------------
>> 1 file changed, 38 insertions(+), 15 deletions(-)
>>
>> diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
>> index f6a7466622ea..f43937d2c588 100644
>> --- a/sound/soc/sh/rcar/ssi.c
>> +++ b/sound/soc/sh/rcar/ssi.c
>> @@ -286,7 +286,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod,
>> if (rsnd_ssi_is_multi_slave(mod, io))
>> return 0;
>>
>> - if (ssi->usrcnt > 0) {
>> + if (ssi->rate) {
>> if (ssi->rate != rate) {
>> dev_err(dev, "SSI parent/child should use same rate\n");
>> return -EINVAL;
>> @@ -471,13 +471,9 @@ static int rsnd_ssi_init(struct rsnd_mod *mod,
>> struct rsnd_dai_stream *io,
>> struct rsnd_priv *priv)
>> {
>> - struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
>> -
>> if (!rsnd_ssi_is_run_mods(mod, io))
>> return 0;
>>
>> - ssi->usrcnt++;
>> -
>> rsnd_mod_power_on(mod);
>>
>> rsnd_ssi_config_init(mod, io);
>> @@ -505,18 +501,8 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod,
>> return -EIO;
>> }
>>
>> - rsnd_ssi_master_clk_stop(mod, io);
>> -
>> rsnd_mod_power_off(mod);
>>
>> - ssi->usrcnt--;
>> -
>> - if (!ssi->usrcnt) {
>> - ssi->cr_own = 0;
>> - ssi->cr_mode = 0;
>> - ssi->wsr = 0;
>> - }
>> -
>> return 0;
>> }
>>
>> @@ -525,6 +511,7 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod,
>> struct snd_pcm_substream *substream,
>> struct snd_pcm_hw_params *params)
>> {
>> + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
>> struct rsnd_dai *rdai = rsnd_io_to_rdai(io);
>> unsigned int fmt_width = snd_pcm_format_width(params_format(params));
>>
>> @@ -536,6 +523,11 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod,
>> return -EINVAL;
>> }
>>
>> + if (!rsnd_ssi_is_run_mods(mod, io))
>> + return 0;
>> +
>> + ssi->usrcnt++;
>> +
>> return 0;
>> }
>>
>> @@ -913,6 +905,35 @@ static int rsnd_ssi_prepare(struct rsnd_mod *mod,
>> return rsnd_ssi_master_clk_start(mod, io);
>> }
>>
>> +static int rsnd_ssi_hw_free(struct rsnd_mod *mod, struct rsnd_dai_stream *io,
>> + struct snd_pcm_substream *substream)
>> +{
>> + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
>> +
>> + if (!rsnd_ssi_is_run_mods(mod, io))
>> + return 0;
>> +
>> + if (!ssi->usrcnt) {
>> + struct rsnd_priv *priv = rsnd_mod_to_priv(mod);
>> + struct device *dev = rsnd_priv_to_dev(priv);
>> +
>> + dev_err(dev, "%s usrcnt error\n", rsnd_mod_name(mod));
>> + return -EIO;
>> + }
>> +
>> + rsnd_ssi_master_clk_stop(mod, io);
>> +
>> + ssi->usrcnt--;
>> +
>> + if (!ssi->usrcnt) {
>> + ssi->cr_own = 0;
>> + ssi->cr_mode = 0;
>> + ssi->wsr = 0;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static struct rsnd_mod_ops rsnd_ssi_pio_ops = {
>> .name = SSI_NAME,
>> .probe = rsnd_ssi_common_probe,
>> @@ -926,6 +947,7 @@ static struct rsnd_mod_ops rsnd_ssi_pio_ops = {
>> .pcm_new = rsnd_ssi_pcm_new,
>> .hw_params = rsnd_ssi_hw_params,
>> .prepare = rsnd_ssi_prepare,
>> + .hw_free = rsnd_ssi_hw_free,
>> .get_status = rsnd_ssi_get_status,
>> };
>>
>> @@ -1012,6 +1034,7 @@ static struct rsnd_mod_ops rsnd_ssi_dma_ops = {
>> .pcm_new = rsnd_ssi_pcm_new,
>> .fallback = rsnd_ssi_fallback,
>> .hw_params = rsnd_ssi_hw_params,
>> + .hw_free = rsnd_ssi_hw_free,
>> .prepare = rsnd_ssi_prepare,
>> .get_status = rsnd_ssi_get_status,
>> };
>> --
>> 2.19.2
>>
Powered by blists - more mailing lists