linux-kernel - Re: [PATCH] pwm: meson: fix harware duty calculation

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] [day] [month] [year] [list]

Message-ID: <1511940871.27425.4.camel@baylibre.com>
Date:   Wed, 29 Nov 2017 08:34:31 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Yixun Lan <yixun.lan@...ogic.com>,
        Thierry Reding <thierry.reding@...il.com>,
        linux-pwm@...r.kernel.org, linux-amlogic@...ts.infradead.org
Cc:     Neil Armstrong <narmstrong@...libre.com>,
        Carlo Caione <carlo@...one.org>,
        Kevin Hilman <khilman@...libre.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Jian Hu <jian.hu@...ogic.com>
Subject: Re: [PATCH] pwm: meson: fix harware duty calculation

On Wed, 2017-11-29 at 11:03 +0800, Yixun Lan wrote:
> From: Jian Hu <jian.hu@...ogic.com>
> 
> The actual HIGH/LOW signal output from the PWM is equal to
> the value programed to HW register plus one, this is designed by HW.
> 
> This fix should apply to all Meson SoC(include GX/GXL/GXBB, Meson6,8)
> 
> Fixes: 211ed630753d ("pwm: Add support for Meson PWM Controller")
> Signed-off-by: Jian Hu <jian.hu@...ogic.com>
> Signed-off-by: Yixun Lan <yixun.lan@...ogic.com>
> ---
>  drivers/pwm/pwm-meson.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index d589331d1884..78d9b8c1a4bc 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -193,6 +193,11 @@ static int meson_pwm_calc(struct meson_pwm *meson,
>  			break;
>  	}
>  
> +	if (cnt < 2) {
> +		dev_err(meson->chip.dev, "invalid period\n");
> +		return -EINVAL;
> +	}
> +
>  	if (pre_div == MISC_CLK_DIV_MASK) {
>  		dev_err(meson->chip.dev, "unable to get period pre_div\n");
>  		return -EINVAL;
> @@ -201,19 +206,23 @@ static int meson_pwm_calc(struct meson_pwm *meson,
>  	dev_dbg(meson->chip.dev, "period=%u pre_div=%u cnt=%u\n", period,
>  		pre_div, cnt);
>  
> +	/*
> +	 * Due to the design of hardware, values of 'hi', 'lo' are 1 based
> +	 * which mean the actual output from hardware is 'hi' + 1, 'lo' + 1
> +	 */
>  	if (duty == period) {
>  		channel->pre_div = pre_div;
> -		channel->hi = cnt;
> +		channel->hi = cnt - 1;
>  		channel->lo = 0;
>  	} else if (duty == 0) {
>  		channel->pre_div = pre_div;
>  		channel->hi = 0;
> -		channel->lo = cnt;
> +		channel->lo = cnt - 1;
>  	} else {
>  		/* Then check is we can have the duty with the same pre_div
> */
>  		duty_cnt = DIV_ROUND_CLOSEST_ULL((u64)duty * 1000,
>  						 fin_ps * (pre_div + 1));
> -		if (duty_cnt > 0xffff) {
> +		if (duty_cnt > 0xffff || !duty_cnt) {

duty_cnt = 0 is a valid value here. It will be the case for duty != 0 but low
enough for the HW (calculation) to approximate the duty cycle to zero.

>  			dev_err(meson->chip.dev, "unable to get duty
> cycle\n");
>  			return -EINVAL;
>  		}
> @@ -222,8 +231,8 @@ static int meson_pwm_calc(struct meson_pwm *meson,
>  			duty, pre_div, duty_cnt);
>  
>  		channel->pre_div = pre_div;
> -		channel->hi = duty_cnt;
> -		channel->lo = cnt - duty_cnt;
> +		channel->hi = duty_cnt - 1;

As explained above, duty_cnt could be zero, you need to take care of this here

> +		channel->lo = cnt - duty_cnt - 1;

Same here, it is possible duty_cnt to be egual to cnt so you also need to be
careful here

>  	}
>  
>  	return 0;