linux-kernel - Re: [PATCH v4 1/1] pwm: Convert period and duty cycle to u64

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: <20200114074710.kxkz4664oap3r752@pengutronix.de>
Date:   Tue, 14 Jan 2020 08:47:10 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Guru Das Srinagesh <gurus@...eaurora.org>
Cc:     linux-pwm@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Subbaraman Narayanamurthy <subbaram@...eaurora.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/1] pwm: Convert period and duty cycle to u64

On Mon, Jan 13, 2020 at 03:53:31PM -0800, Guru Das Srinagesh wrote:
> Because period and duty cycle are defined as ints with units of
> nanoseconds, the maximum time duration that can be set is limited to
> ~2.147 seconds. Change their definitions to u64 in the structs of the
> PWM framework so that higher durations may be set.
> 
> Also make the relevant fixes to those drivers that use the period and
> duty_cycle struct members in division operations, viz. replacing the
> division operations with 64-bit division macros as appropriate.

I like the goal of this patch (i.e. expanding the domain of period and
duty-cycle). I wonder however if we need a more sophisticated approach
here. The problem I see is that if something breaks (and the fix isn't
easy) we will have to revert the whole patch touching (now) 15 files.

I didn't thought about that much, but it would be great if we could
prepare the affected drivers to work with both, int and u64 and switch
in a separate commit. Reverting would then become cheaper.
The conversion to 64-bit division macros could be done even without
actually converting period and duty cycle, couldn't it?

> Signed-off-by: Guru Das Srinagesh <gurus@...eaurora.org>
> Reported-by: kbuild test robot <lkp@...el.com>

This Reported-by: looks wrong. It gave some hints about what had to be
improved in an earlier revision of this patch, but usually this means
that the patch is a fix for an earlier commit. So I would put this in
the text, something like:

	The kbuild test robot helped to improve this patch series to
	(hopefully) catch all code sites having to be adapted.

> [...]
> diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
> index 924d39a..ba9500a 100644
> --- a/drivers/pwm/pwm-clps711x.c
> +++ b/drivers/pwm/pwm-clps711x.c
> @@ -43,7 +43,7 @@ static void clps711x_pwm_update_val(struct clps711x_chip *priv, u32 n, u32 v)
>  static unsigned int clps711x_get_duty(struct pwm_device *pwm, unsigned int v)
>  {
>  	/* Duty cycle 0..15 max */
> -	return DIV_ROUND_CLOSEST(v * 0xf, pwm->args.period);
> +	return DIV64_U64_ROUND_CLOSEST(v * 0xf, pwm->args.period);
>  }

In ir-rx51.c you used DIV_ROUND_CLOSEST_ULL to replace
DIV_ROUND_CLOSEST, here it is DIV64_U64_ROUND_CLOSEST. Maybe it is worth
to describe the relevant difference shortly in the commit log.

>  static int clps711x_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |