| 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: <38f09c4d-70d1-f65f-6e9b-4ad84eda4059@roeck-us.net>
Date: Mon, 30 Jan 2023 22:16:06 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: David Rau <david.rau.zg@...esas.com>
Cc: "perex@...ex.cz" <perex@...ex.cz>,
"lgirdwood@...il.com" <lgirdwood@...il.com>,
"broonie@...nel.org" <broonie@...nel.org>,
"tiwai@...e.com" <tiwai@...e.com>,
"support.opensource@...semi.com" <support.opensource@...semi.com>,
"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ASoC: da7219: Fix pole orientation detection on OMTP
headsets when playing music
On 1/30/23 19:58, David Rau wrote:
> Thanks for the kind feedback.
> Would you please let me know what kinds of environment such error appears you ever meet?
> Ex: da7219_aad->gnd_switch_delay = ?
>
We are seeing the problem on various Chromebooks.
Never mind, though. I really don't have time to keep arguing about this.
I would have assumed that it is obvious that a long msleep() in an
interrupt handler is not appropriate, but obviously I was wrong.
I'll see if I can implement a downstream fix.
Guenter
>
> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Friday, January 20, 2023 00:12
> To: David Rau <david.rau.zg@...esas.com>
> Cc: David Rau <we730128@...il.com>; perex@...ex.cz; lgirdwood@...il.com; broonie@...nel.org; tiwai@...e.com; support.opensource@...semi.com; alsa-devel@...a-project.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] ASoC: da7219: Fix pole orientation detection on OMTP headsets when playing music
>
> On Thu, Jan 19, 2023 at 11:02:25AM +0000, David Rau wrote:
>> Would you please provide me the related error messages when hung task crashes in da7219_aad_irq_thread()?
>> BTW, "gnd_switch_delay = 256" is an unusual use case of the longer jack detection latency.
>>
>
> Here is a typical traceback.
>
> <3>[ 246.919057] INFO: task irq/105-da7219-:2854 blocked for more than 122 seconds.
> <3>[ 246.919065] Not tainted 5.10.159-20927-g317f62e2494d #1 <3>[ 246.919068] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> <6>[ $PHONE_NUMBER] task:irq/105-da7219- state:D stack: 0 pid: 2854 ppid: 2 flags:0x00004080 <6>[ 246.919075] Call Trace:
> <6>[ 246.919084] __schedule+0x3b0/0xdaf
> <6>[ 246.919090] schedule+0x44/0xa8
> <6>[ 246.919093] schedule_timeout+0xb6/0x290 <6>[ 246.919098] ? run_local_timers+0x4e/0x4e <6>[ 246.919102] msleep+0x2c/0x38 <6>[ 246.919108] da7219_aad_irq_thread+0x66/0x2b0 [snd_soc_da7219 cd5a76eef6e777074216b9d61f7918f7561bf7ec]
> <6>[ 246.919113] ? irq_forced_thread_fn+0x5f/0x5f <6>[ 246.919116] irq_thread_fn+0x22/0x4d <6>[ 246.919120] irq_thread+0x120/0x19d <6>[ 246.919123] ? irq_thread_fn+0x4d/0x4d <6>[ 246.919128] kthread+0x142/0x153 <6>[ 246.919132] ? irq_forced_secondary_handler+0x21/0x21
> <6>[ 246.919135] ? kthread_blkcg+0x31/0x31 <6>[ 246.919139] ret_from_fork+0x1f/0x30
>
> The underlying question is if it really appropriate to have an
> msleep() of any kind in an interrupt handler. If this is about debouncing a signal, it should be handled with a delayed timer.
>
> Guenter
>
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
>> Sent: Wednesday, January 18, 2023 03:57
>> To: David Rau <we730128@...il.com>
>> Cc: perex@...ex.cz; lgirdwood@...il.com; broonie@...nel.org;
>> tiwai@...e.com; support.opensource@...semi.com;
>> alsa-devel@...a-project.org; linux-kernel@...r.kernel.org; David Rau
>> <david.rau.zg@...esas.com>
>> Subject: Re: [PATCH] ASoC: da7219: Fix pole orientation detection on
>> OMTP headsets when playing music
>>
>> On Mon, Nov 21, 2022 at 05:07:44AM +0000, David Rau wrote:
>>> The OMTP pin define headsets can be mis-detected as line out instead
>>> of OMTP, causing obvious issues with audio quality.
>>> This patch is to put increased resistances within the device at a
>>> suitable point.
>>>
>>> To solve this issue better, the new mechanism setup ground switches
>>> with conditional delay control and these allow for more stabile
>>> detection process to operate as intended. This conditional delay
>>> control will not impact the hardware process but use extra system
>>> resource.
>>>
>>> This commit improves control of ground switches in the AAD logic.
>>>
>>> Signed-off-by: David Rau <david.rau.zg@...esas.com>
>>> ---
>>> sound/soc/codecs/da7219-aad.c | 42
>>> ++++++++++++++++++++++++++++++-----
>>> sound/soc/codecs/da7219-aad.h | 1 +
>>> 2 files changed, 37 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/sound/soc/codecs/da7219-aad.c
>>> b/sound/soc/codecs/da7219-aad.c index bba73c44c219..08200ec259f9
>>> 100644
>>> --- a/sound/soc/codecs/da7219-aad.c
>>> +++ b/sound/soc/codecs/da7219-aad.c
>>> @@ -352,9 +352,14 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void *data)
>>> struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
>>> struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
>>> u8 events[DA7219_AAD_IRQ_REG_MAX];
>>> - u8 statusa;
>>> + u8 statusa, srm_st;
>>> int i, report = 0, mask = 0;
>>>
>>> + srm_st = snd_soc_component_read(component, DA7219_PLL_SRM_STS) & DA7219_PLL_SRM_STS_MCLK;
>>> + msleep(da7219_aad->gnd_switch_delay * ((srm_st == 0x0) ? 2 : 1) -
>>> +4);
>>
>> Ever since this patch was applied to ChromeOS, we have observed hung task crashes in da7219_aad_irq_thread().
>>
>> Is it really appropriate to sleep up to (256 * 2) - 4 = 508 ms in an interrupt handler ?
>>
>> Thanks,
>> Guenter
>>
>>> + /* Enable ground switch */
>>> + snd_soc_component_update_bits(component, 0xFB, 0x01, 0x01);
>>> +
>>> /* Read current IRQ events */
>>> regmap_bulk_read(da7219->regmap, DA7219_ACCDET_IRQ_EVENT_A,
>>> events, DA7219_AAD_IRQ_REG_MAX); @@ -454,8 +459,8 @@ static
>>> irqreturn_t da7219_aad_irq_thread(int irq, void *data)
>>> snd_soc_dapm_disable_pin(dapm, "Mic Bias");
>>> snd_soc_dapm_sync(dapm);
>>>
>>> - /* Enable ground switch */
>>> - snd_soc_component_update_bits(component, 0xFB, 0x01, 0x01);
>>> + /* Disable ground switch */
>>> + snd_soc_component_update_bits(component, 0xFB, 0x01, 0x00);
>>> }
>>> }
>>>
>>> @@ -831,6 +836,32 @@ static void da7219_aad_handle_pdata(struct snd_soc_component *component)
>>> }
>>> }
>>>
>>> +static void da7219_aad_handle_gnd_switch_time(struct
>>> +snd_soc_component *component) {
>>> + struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
>>> + struct da7219_aad_priv *da7219_aad = da7219->aad;
>>> + u8 jack_det;
>>> +
>>> + jack_det = snd_soc_component_read(component, DA7219_ACCDET_CONFIG_2)
>>> + & DA7219_JACK_DETECT_RATE_MASK;
>>> + switch (jack_det) {
>>> + case 0x00:
>>> + da7219_aad->gnd_switch_delay = 32;
>>> + break;
>>> + case 0x10:
>>> + da7219_aad->gnd_switch_delay = 64;
>>> + break;
>>> + case 0x20:
>>> + da7219_aad->gnd_switch_delay = 128;
>>> + break;
>>> + case 0x30:
>>> + da7219_aad->gnd_switch_delay = 256;
>>> + break;
>>> + default:
>>> + da7219_aad->gnd_switch_delay = 32;
>>> + break;
>>> + }
>>> +}
>>>
>>> /*
>>> * Suspend/Resume
>>> @@ -908,9 +939,6 @@ int da7219_aad_init(struct snd_soc_component *component)
>>> snd_soc_component_update_bits(component, DA7219_ACCDET_CONFIG_1,
>>> DA7219_BUTTON_CONFIG_MASK, 0);
>>>
>>> - /* Enable ground switch */
>>> - snd_soc_component_update_bits(component, 0xFB, 0x01, 0x01);
>>> -
>>> INIT_WORK(&da7219_aad->btn_det_work, da7219_aad_btn_det_work);
>>> INIT_WORK(&da7219_aad->hptest_work, da7219_aad_hptest_work);
>>>
>>> @@ -928,6 +956,8 @@ int da7219_aad_init(struct snd_soc_component *component)
>>> regmap_bulk_write(da7219->regmap, DA7219_ACCDET_IRQ_MASK_A,
>>> &mask, DA7219_AAD_IRQ_REG_MAX);
>>>
>>> + da7219_aad_handle_gnd_switch_time(component);
>>> +
>>> return 0;
>>> }
>>> EXPORT_SYMBOL_GPL(da7219_aad_init);
>>> diff --git a/sound/soc/codecs/da7219-aad.h
>>> b/sound/soc/codecs/da7219-aad.h index f48a12012ef3..21fdf53095cc
>>> 100644
>>> --- a/sound/soc/codecs/da7219-aad.h
>>> +++ b/sound/soc/codecs/da7219-aad.h
>>> @@ -187,6 +187,7 @@ enum da7219_aad_event_regs { struct
>>> da7219_aad_priv {
>>> struct snd_soc_component *component;
>>> int irq;
>>> + int gnd_switch_delay;
>>>
>>> u8 micbias_pulse_lvl;
>>> u32 micbias_pulse_time;
>>> --
>>> 2.17.1
>>>
Powered by blists - more mailing lists