[PATCH v8 01/12] clk: pwm: Use 64-bit division function

[PATCH v8 01/12] clk: pwm: Use 64-bit division function

[Guru Das Srinagesh]

Since the PWM framework is switching struct pwm_args.period's datatype
to u64, prepare for this transition by using div64_u64 to handle a
64-bit divisor.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org

Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
---
 drivers/clk/clk-pwm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index 87fe0b0e..7b1f7a0 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -89,7 +89,7 @@ static int clk_pwm_probe(struct platform_device *pdev)
 	}
 
 	if (of_property_read_u32(node, "clock-frequency", &clk_pwm->fixed_rate))
-		clk_pwm->fixed_rate = NSEC_PER_SEC / pargs.period;
+		clk_pwm->fixed_rate = div64_u64(NSEC_PER_SEC, pargs.period);
 
 	if (pargs.period != NSEC_PER_SEC / clk_pwm->fixed_rate &&
 	    pargs.period != DIV_ROUND_UP(NSEC_PER_SEC, clk_pwm->fixed_rate)) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

RE: [PATCH v8 01/12] clk: pwm: Use 64-bit division function

[David Laight]

From: Guru Das Srinagesh
> Sent: 11 March 2020 01:41
> 
> Since the PWM framework is switching struct pwm_args.period's datatype
> to u64, prepare for this transition by using div64_u64 to handle a
> 64-bit divisor.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> 
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
>  drivers/clk/clk-pwm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
> index 87fe0b0e..7b1f7a0 100644
> --- a/drivers/clk/clk-pwm.c
> +++ b/drivers/clk/clk-pwm.c
> @@ -89,7 +89,7 @@ static int clk_pwm_probe(struct platform_device *pdev)
>  	}
> 
>  	if (of_property_read_u32(node, "clock-frequency", &clk_pwm->fixed_rate))
> -		clk_pwm->fixed_rate = NSEC_PER_SEC / pargs.period;
> +		clk_pwm->fixed_rate = div64_u64(NSEC_PER_SEC, pargs.period);

That cannot be needed, a 32 bit division is fine.
More interesting would be whether pargs.period is sane (eg not zero).
I'd assign pargs.period to an 'unsigned int' variable
prior to the division (I hate casts - been bitten by them in the past.).

> 
>  	if (pargs.period != NSEC_PER_SEC / clk_pwm->fixed_rate &&
>  	    pargs.period != DIV_ROUND_UP(NSEC_PER_SEC, clk_pwm->fixed_rate)) {
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v8 01/12] clk: pwm: Use 64-bit division function

[Guru Das Srinagesh]

On Wed, Mar 11, 2020 at 04:58:24PM +0000, David Laight wrote:
> From: Guru Das Srinagesh
> > Sent: 11 March 2020 01:41
> > 
> > Since the PWM framework is switching struct pwm_args.period's datatype
> > to u64, prepare for this transition by using div64_u64 to handle a
> > 64-bit divisor.
> > 
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > 
> > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > ---
> >  drivers/clk/clk-pwm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
> > index 87fe0b0e..7b1f7a0 100644
> > --- a/drivers/clk/clk-pwm.c
> > +++ b/drivers/clk/clk-pwm.c
> > @@ -89,7 +89,7 @@ static int clk_pwm_probe(struct platform_device *pdev)
> >  	}
> > 
> >  	if (of_property_read_u32(node, "clock-frequency", &clk_pwm->fixed_rate))
> > -		clk_pwm->fixed_rate = NSEC_PER_SEC / pargs.period;
> > +		clk_pwm->fixed_rate = div64_u64(NSEC_PER_SEC, pargs.period);
> 
> That cannot be needed, a 32 bit division is fine.

Could you please explain why? I think the use of this function is
warranted in order to handle the division properly with a 64-bit
divisor.

> More interesting would be whether pargs.period is sane (eg not zero).

There is a non-zero check for pargs.period just prior to this line, so
the code is handling this case already.

> I'd assign pargs.period to an 'unsigned int' variable
> prior to the division (I hate casts - been bitten by them in the past.).

Wouldn't this truncate the 64-bit value? The intention behind this patch
is to allow the processing of 64-bit values in full.

Thank you.

Guru Das.

RE: [PATCH v8 01/12] clk: pwm: Use 64-bit division function

[David Laight]

From: Guru Das Srinagesh
> Sent: 12 March 2020 02:10
> On Wed, Mar 11, 2020 at 04:58:24PM +0000, David Laight wrote:
> > From: Guru Das Srinagesh
> > > Sent: 11 March 2020 01:41
> > >
> > > Since the PWM framework is switching struct pwm_args.period's datatype
> > > to u64, prepare for this transition by using div64_u64 to handle a
> > > 64-bit divisor.
> > >
...
> > > --- a/drivers/clk/clk-pwm.c
> > > +++ b/drivers/clk/clk-pwm.c
> > > @@ -89,7 +89,7 @@ static int clk_pwm_probe(struct platform_device *pdev)
> > >  	}
> > >
> > >  	if (of_property_read_u32(node, "clock-frequency", &clk_pwm->fixed_rate))
> > > -		clk_pwm->fixed_rate = NSEC_PER_SEC / pargs.period;
> > > +		clk_pwm->fixed_rate = div64_u64(NSEC_PER_SEC, pargs.period);
> >
> > That cannot be needed, a 32 bit division is fine.
> 
> Could you please explain why? I think the use of this function is
> warranted in order to handle the division properly with a 64-bit
> divisor.
...
> > I'd assign pargs.period to an 'unsigned int' variable
> > prior to the division (I hate casts - been bitten by them in the past.).
> 
> Wouldn't this truncate the 64-bit value? The intention behind this patch
> is to allow the processing of 64-bit values in full.

You are dividing a 32bit constant by a value.
If pargs.period is greater than 2^32 the result is zero.
I think you divide by 'fixed_rate' a bit later on - better not be zero.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v8 01/12] clk: pwm: Use 64-bit division function

[Guru Das Srinagesh]

On Thu, Mar 12, 2020 at 09:14:09AM +0000, David Laight wrote:
> From: Guru Das Srinagesh
> > Sent: 12 March 2020 02:10
> > On Wed, Mar 11, 2020 at 04:58:24PM +0000, David Laight wrote:
> > > From: Guru Das Srinagesh
> > > > Sent: 11 March 2020 01:41
> > > >
> > > > Since the PWM framework is switching struct pwm_args.period's datatype
> > > > to u64, prepare for this transition by using div64_u64 to handle a
> > > > 64-bit divisor.
> > > >
> ...
> > > > --- a/drivers/clk/clk-pwm.c
> > > > +++ b/drivers/clk/clk-pwm.c
> > > > @@ -89,7 +89,7 @@ static int clk_pwm_probe(struct platform_device *pdev)
> > > >  	}
> > > >
> > > >  	if (of_property_read_u32(node, "clock-frequency", &clk_pwm->fixed_rate))
> > > > -		clk_pwm->fixed_rate = NSEC_PER_SEC / pargs.period;
> > > > +		clk_pwm->fixed_rate = div64_u64(NSEC_PER_SEC, pargs.period);
> > >
> > > That cannot be needed, a 32 bit division is fine.
> > 
> > Could you please explain why? I think the use of this function is
> > warranted in order to handle the division properly with a 64-bit
> > divisor.
> ...
> > > I'd assign pargs.period to an 'unsigned int' variable
> > > prior to the division (I hate casts - been bitten by them in the past.).
> > 
> > Wouldn't this truncate the 64-bit value? The intention behind this patch
> > is to allow the processing of 64-bit values in full.
> 
> You are dividing a 32bit constant by a value.
> If pargs.period is greater than 2^32 the result is zero.

Thanks for the explanation. 

> I think you divide by 'fixed_rate' a bit later on - better not be zero.

Good point, but this issue exists with or without this patch, and fixing
it is beyond this patch's scope.

Just to check if this patch can be dropped, I tested out compilation
with this patch reverted and there were no errors, so I'm leaning
towards dropping this patch unless you have any further comments on how
to proceed.

Thank you.

Guru Das.

Re: [PATCH v8 01/12] clk: pwm: Use 64-bit division function

[Guru Das Srinagesh]

On Thu, Mar 12, 2020 at 12:09:12PM -0700, Guru Das Srinagesh wrote:
> On Thu, Mar 12, 2020 at 09:14:09AM +0000, David Laight wrote:
> > From: Guru Das Srinagesh
> > > Sent: 12 March 2020 02:10
> > > On Wed, Mar 11, 2020 at 04:58:24PM +0000, David Laight wrote:
> > > > From: Guru Das Srinagesh
> > > > > Sent: 11 March 2020 01:41
> > > > >
> > > > > Since the PWM framework is switching struct pwm_args.period's datatype
> > > > > to u64, prepare for this transition by using div64_u64 to handle a
> > > > > 64-bit divisor.
> > > > >
> > ...
> > > > > --- a/drivers/clk/clk-pwm.c
> > > > > +++ b/drivers/clk/clk-pwm.c
> > > > > @@ -89,7 +89,7 @@ static int clk_pwm_probe(struct platform_device *pdev)
> > > > >  	}
> > > > >
> > > > >  	if (of_property_read_u32(node, "clock-frequency", &clk_pwm->fixed_rate))
> > > > > -		clk_pwm->fixed_rate = NSEC_PER_SEC / pargs.period;
> > > > > +		clk_pwm->fixed_rate = div64_u64(NSEC_PER_SEC, pargs.period);
> > > >
> > > > That cannot be needed, a 32 bit division is fine.
> > > 
> > > Could you please explain why? I think the use of this function is
> > > warranted in order to handle the division properly with a 64-bit
> > > divisor.
> > ...
> > > > I'd assign pargs.period to an 'unsigned int' variable
> > > > prior to the division (I hate casts - been bitten by them in the past.).
> > > 
> > > Wouldn't this truncate the 64-bit value? The intention behind this patch
> > > is to allow the processing of 64-bit values in full.
> > 
> > You are dividing a 32bit constant by a value.
> > If pargs.period is greater than 2^32 the result is zero.
> 
> Thanks for the explanation. 
> 
> > I think you divide by 'fixed_rate' a bit later on - better not be zero.
> 
> Good point, but this issue exists with or without this patch, and fixing
> it is beyond this patch's scope.
> 
> Just to check if this patch can be dropped, I tested out compilation
> with this patch reverted and there were no errors, so I'm leaning
> towards dropping this patch unless you have any further comments on how
> to proceed.

Turns out I couldn't drop this patch after all - kbuild test robot
complained [1]. Accordingly, I've brought this patch back in my v10
patchset with the modifications you suggested. Could you kindly review it?

[1] https://www.spinics.net/lists/linux-pwm/msg11906.html

Thank you.

Guru Das.

Re: [PATCH v8 01/12] clk: pwm: Use 64-bit division function

[Guru Das Srinagesh]

On Thu, Mar 12, 2020 at 09:14:09AM +0000, David Laight wrote:
> From: Guru Das Srinagesh
> > Sent: 12 March 2020 02:10
> > On Wed, Mar 11, 2020 at 04:58:24PM +0000, David Laight wrote:
> > > From: Guru Das Srinagesh
> > > > Sent: 11 March 2020 01:41
> > > >
> > > > Since the PWM framework is switching struct pwm_args.period's datatype
> > > > to u64, prepare for this transition by using div64_u64 to handle a
> > > > 64-bit divisor.
> > > >
> ...
> > > > --- a/drivers/clk/clk-pwm.c
> > > > +++ b/drivers/clk/clk-pwm.c
> > > > @@ -89,7 +89,7 @@ static int clk_pwm_probe(struct platform_device *pdev)
> > > >  	}
> > > >
> > > >  	if (of_property_read_u32(node, "clock-frequency", &clk_pwm->fixed_rate))
> > > > -		clk_pwm->fixed_rate = NSEC_PER_SEC / pargs.period;
> > > > +		clk_pwm->fixed_rate = div64_u64(NSEC_PER_SEC, pargs.period);
> > >
> > > That cannot be needed, a 32 bit division is fine.
> > 
> > Could you please explain why? I think the use of this function is
> > warranted in order to handle the division properly with a 64-bit
> > divisor.
> ...
> > > I'd assign pargs.period to an 'unsigned int' variable
> > > prior to the division (I hate casts - been bitten by them in the past.).
> > 
> > Wouldn't this truncate the 64-bit value? The intention behind this patch
> > is to allow the processing of 64-bit values in full.
> 
> You are dividing a 32bit constant by a value.
> If pargs.period is greater than 2^32 the result is zero.

Correction: if pargs.period is greater than NSEC_PER_SEC, not 2^32.

> I think you divide by 'fixed_rate' a bit later on - better not be zero.

I am adding an explicit check in v12 to ensure fixed_rate is not zero. If
during the calculation it is found to be zero, probing will fail.

I think with this modification, this v8 version of this change makes
sense to use.

Thank you.

Guru Das.