linux-kernel - Re: [PATCH] ASoC: codecs: Fix WCD_MBHC_HPH_PA

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: <c01b6669-d0f7-aab5-3aca-02f19be8a319@wanadoo.fr>
Date:   Tue, 19 Oct 2021 19:39:04 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        lgirdwood@...il.com, broonie@...nel.org, perex@...ex.cz,
        tiwai@...e.com, yang.lee@...ux.alibaba.com
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] ASoC: codecs: Fix WCD_MBHC_HPH_PA_EN usage

Le 19/10/2021 à 15:47, Srinivas Kandagatla a écrit :
> 
> 
> On 17/10/2021 08:31, Christophe JAILLET wrote:
>> 'hphpa_on' is known to be false, so the if block at the end of the 
>> function
>> is dead code.
> 
> Yes, this is a dead code we should remove it.

Ok, thanks for the clarification.

> 
> This code was part of moisture detection logic which is not enabled in 
> upstream code yet.

If 'yet' is the important word of the sentence, maybe the best is to 
leave the code as-is.
If you prefer it to be removed, I can send a patch if it helps.

> 
> During Moisture detection if the PA is on then we switch it off and do 
> moisture measurements and at the end of the function we restore the 
> state of PA.
> 
>>
>> Turn it into a meaningful code by having 'hphpa_on' be static. Use is 
>> as a
>> flip-flop variable.
> 
> No, It does not.
> 
> Have you even tested this patch in anyway?

No, as said below the ---, the purpose of this patch was not to be 
correct (or tested). It was only to draw attention on odd things.

CJ



> 
>>
>> Fixes: 0e5c9e7ff899 ("ASoC: codecs: wcd: add multi button Headset 
>> detection support")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> ---
>> The purpose of this patch is not to be correct (!) but to draw attention
>> on several points:
>>     - in 'wcd_mbhc_adc_hs_rem_irq()', the "if (hphpa_on)" path is dead 
>> code
>>       because 'hphpa_on' is known to be false
>>     - What is this magic number '3'?
>>       All 'wcd_mbhc_read_field()' look for 0 or non-0
>>     - a 'mutex_[un]lock()' in an IRQ handler looks spurious to me
>>
>> Instead of this (likely broken) patch, it is likely that something is
>> missing elsewhere. Maybe in 'wcd_mbhc_adc_hs_ins_irq()'.
>> I also guess that 'hphpa_on' should be read for somewhere else.
>> ---
>>   sound/soc/codecs/wcd-mbhc-v2.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/codecs/wcd-mbhc-v2.c 
>> b/sound/soc/codecs/wcd-mbhc-v2.c
>> index 405128ccb4b0..783d8c35bc1b 100644
>> --- a/sound/soc/codecs/wcd-mbhc-v2.c
>> +++ b/sound/soc/codecs/wcd-mbhc-v2.c
>> @@ -1176,7 +1176,7 @@ static irqreturn_t wcd_mbhc_adc_hs_rem_irq(int 
>> irq, void *data)
>>       struct wcd_mbhc *mbhc = data;
>>       unsigned long timeout;
>>       int adc_threshold, output_mv, retry = 0;
>> -    bool hphpa_on = false;
>> +    static bool hphpa_on = false;
>>       mutex_lock(&mbhc->lock);
>>       timeout = jiffies + 
>> msecs_to_jiffies(WCD_FAKE_REMOVAL_MIN_PERIOD_MS);
>> @@ -1212,6 +1212,9 @@ static irqreturn_t wcd_mbhc_adc_hs_rem_irq(int 
>> irq, void *data)
>>       if (hphpa_on) {
>>           hphpa_on = false;
>> +        wcd_mbhc_write_field(mbhc, WCD_MBHC_HPH_PA_EN, 0);
>> +    } else {
>> +        hphpa_on = true;
>>           wcd_mbhc_write_field(mbhc, WCD_MBHC_HPH_PA_EN, 3);
> 
> Just remove this dead code.
> 
> --srini
>>       }
>>   exit:
>>
>