linux-kernel - Re: [PATCH] extcon: arizona: Wait for any running HPDETs to complete on jack removal

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: <5887F8B8.60409@samsung.com>
Date:   Wed, 25 Jan 2017 10:00:40 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
Cc:     myungjoo.ham@...sung.com, linux-kernel@...r.kernel.org,
        patches@...nsource.wolfsonmicro.com
Subject: Re: [PATCH] extcon: arizona: Wait for any running HPDETs to complete
 on jack removal

Hi Charles,

On 2017년 01월 24일 19:17, Charles Keepax wrote:
> As the HPDET can't be aborted mid way through we should not allow any new
> insertion to be processed until the previous HPDET has finished. It is very
> unlikely but with low enough debounce settings you could start a new HPDET
> before the old one has completed, which results in an erroneous reading.
> 
> Signed-off-by: Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
> ---
>  drivers/extcon/extcon-arizona.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index ed78b7c..218e378 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -1049,6 +1049,39 @@ static void arizona_hpdet_work(struct work_struct *work)
>  	mutex_unlock(&info->lock);
>  }
>  
> +static int arizona_hpdet_wait(struct arizona_extcon_info *info)
> +{
> +	struct arizona *arizona = info->arizona;
> +	unsigned int val;
> +	int i, ret;
> +
> +	for (i = 0; i < 15; i++) {

I recommend that you define the separate constant definition
instead of using interger value (15) directly as following:
	
	#define HPDET_WAIT_COUNT	15

> +		ret = regmap_read(arizona->regmap, ARIZONA_HEADPHONE_DETECT_2,
> +				  &val);
> +		if (ret) {
> +			dev_err(arizona->dev,
> +				"Failed to read HPDET state: %d\n", ret);
> +			return ret;
> +		}
> +
> +		switch (info->hpdet_ip_version) {
> +		case 0:
> +			if (val & ARIZONA_HP_DONE)
> +				return 0;
> +			break;
> +		default:
> +			if (val & ARIZONA_HP_DONE_B)
> +				return 0;
> +			break;
> +		}
> +
> +		msleep(20);

ditto.

	#define HPDET_WAIT_DELAY_MS	20

> +	}
> +
> +	dev_err(arizona->dev, "HPDET did not appear to complete\n");
> +	return -ETIMEDOUT;
> +}
> +
>  static irqreturn_t arizona_jackdet(int irq, void *data)
>  {
>  	struct arizona_extcon_info *info = data;
> @@ -1155,6 +1188,8 @@ static irqreturn_t arizona_jackdet(int irq, void *data)
>  					"Removal report failed: %d\n", ret);
>  		}
>  

Sometimes I met this similiar case on specific h/w.
I agree to add some delay according to the h/w.
So, you better to add the detailed comment before
calling the arizona_hpdet_wait() to make it easy
to understand why it is necessary.

> +		arizona_hpdet_wait(info);

Is not necessary to check the return value of this call?
When arizona_hpdet_wait() return the -ETIMEDOUT,
Does not this error affect the next some code?

> +

>  		regmap_update_bits(arizona->regmap,
>  				   ARIZONA_JACK_DETECT_DEBOUNCE,
>  				   ARIZONA_MICD_CLAMP_DB | ARIZONA_JD1_DB,
> 
-- 
Best Regards,
Chanwoo Choi
Samsung Electronics