* [v2] pwm: xilinx: Fix u32 overflow issue in 32-bit width PWM mode.
@ 2022-12-15 16:07 ` Kenneth Sloat
0 siblings, 0 replies; 8+ messages in thread
From: Kenneth Sloat @ 2022-12-15 16:07 UTC (permalink / raw)
To: sean.anderson@seco.com, michal.simek@xilinx.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Kenneth Sloat
From: Ken Sloat <ksloat@designlinxhs.com>
This timer HW supports 8, 16 and 32-bit timer widths. This
driver currently uses a u32 to store the max possible value
of the timer. However, statements perform addition of 2 in
xilinx_pwm_apply() when calculating the period_cycles and
duty_cycles values. Since priv->max is a u32, this will
result in an overflow to 1 which will not only be incorrect
but fail on range comparison. This results in making it
impossible to set the PWM in this timer mode.
There are two obvious solutions to the current problem:
1. Cast each instance where overflow occurs to u64.
2. Change priv->max from a u32 to a u64.
Solution #1 requires more code modifications, and leaves
opportunity to introduce similar overflows if other math
statements are added in the future. These may also go
undetected if running in non 32-bit timer modes.
Solution #2 is the much smaller and cleaner approach and
thus the chosen method in this patch.
This was tested on a Zynq UltraScale+ with multiple
instances of the PWM IP.
Signed-off-by: Ken Sloat <ksloat@designlinxhs.com>
---
Changes in v2:
-Update commit comments to explain specifically where this
problem occurs as well as compare solutions.
---
include/clocksource/timer-xilinx.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
index c0f56fe6d22a..d116f18de899 100644
--- a/include/clocksource/timer-xilinx.h
+++ b/include/clocksource/timer-xilinx.h
@@ -41,7 +41,7 @@ struct regmap;
struct xilinx_timer_priv {
struct regmap *map;
struct clk *clk;
- u32 max;
+ u64 max;
};
/**
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [v2] pwm: xilinx: Fix u32 overflow issue in 32-bit width PWM mode.
2022-12-15 16:07 ` Kenneth Sloat
@ 2022-12-15 16:08 ` Sean Anderson
-1 siblings, 0 replies; 8+ messages in thread
From: Sean Anderson @ 2022-12-15 16:08 UTC (permalink / raw)
To: Kenneth Sloat, michal.simek@xilinx.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On 12/15/22 11:07, Kenneth Sloat wrote:
> From: Ken Sloat <ksloat@designlinxhs.com>
>
> This timer HW supports 8, 16 and 32-bit timer widths. This
> driver currently uses a u32 to store the max possible value
> of the timer. However, statements perform addition of 2 in
> xilinx_pwm_apply() when calculating the period_cycles and
> duty_cycles values. Since priv->max is a u32, this will
> result in an overflow to 1 which will not only be incorrect
> but fail on range comparison. This results in making it
> impossible to set the PWM in this timer mode.
>
> There are two obvious solutions to the current problem:
> 1. Cast each instance where overflow occurs to u64.
> 2. Change priv->max from a u32 to a u64.
>
> Solution #1 requires more code modifications, and leaves
> opportunity to introduce similar overflows if other math
> statements are added in the future. These may also go
> undetected if running in non 32-bit timer modes.
>
> Solution #2 is the much smaller and cleaner approach and
> thus the chosen method in this patch.
>
> This was tested on a Zynq UltraScale+ with multiple
> instances of the PWM IP.
>
> Signed-off-by: Ken Sloat <ksloat@designlinxhs.com>
> ---
> Changes in v2:
> -Update commit comments to explain specifically where this
> problem occurs as well as compare solutions.
>
> ---
> include/clocksource/timer-xilinx.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
> index c0f56fe6d22a..d116f18de899 100644
> --- a/include/clocksource/timer-xilinx.h
> +++ b/include/clocksource/timer-xilinx.h
> @@ -41,7 +41,7 @@ struct regmap;
> struct xilinx_timer_priv {
> struct regmap *map;
> struct clk *clk;
> - u32 max;
> + u64 max;
> };
>
> /**
Reviewed-by: Sean Anderson <sean.anderson@seco.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [v2] pwm: xilinx: Fix u32 overflow issue in 32-bit width PWM mode.
@ 2022-12-15 16:08 ` Sean Anderson
0 siblings, 0 replies; 8+ messages in thread
From: Sean Anderson @ 2022-12-15 16:08 UTC (permalink / raw)
To: Kenneth Sloat, michal.simek@xilinx.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On 12/15/22 11:07, Kenneth Sloat wrote:
> From: Ken Sloat <ksloat@designlinxhs.com>
>
> This timer HW supports 8, 16 and 32-bit timer widths. This
> driver currently uses a u32 to store the max possible value
> of the timer. However, statements perform addition of 2 in
> xilinx_pwm_apply() when calculating the period_cycles and
> duty_cycles values. Since priv->max is a u32, this will
> result in an overflow to 1 which will not only be incorrect
> but fail on range comparison. This results in making it
> impossible to set the PWM in this timer mode.
>
> There are two obvious solutions to the current problem:
> 1. Cast each instance where overflow occurs to u64.
> 2. Change priv->max from a u32 to a u64.
>
> Solution #1 requires more code modifications, and leaves
> opportunity to introduce similar overflows if other math
> statements are added in the future. These may also go
> undetected if running in non 32-bit timer modes.
>
> Solution #2 is the much smaller and cleaner approach and
> thus the chosen method in this patch.
>
> This was tested on a Zynq UltraScale+ with multiple
> instances of the PWM IP.
>
> Signed-off-by: Ken Sloat <ksloat@designlinxhs.com>
> ---
> Changes in v2:
> -Update commit comments to explain specifically where this
> problem occurs as well as compare solutions.
>
> ---
> include/clocksource/timer-xilinx.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
> index c0f56fe6d22a..d116f18de899 100644
> --- a/include/clocksource/timer-xilinx.h
> +++ b/include/clocksource/timer-xilinx.h
> @@ -41,7 +41,7 @@ struct regmap;
> struct xilinx_timer_priv {
> struct regmap *map;
> struct clk *clk;
> - u32 max;
> + u64 max;
> };
>
> /**
Reviewed-by: Sean Anderson <sean.anderson@seco.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [v2] pwm: xilinx: Fix u32 overflow issue in 32-bit width PWM mode.
2022-12-15 16:07 ` Kenneth Sloat
@ 2022-12-15 16:13 ` Michal Simek
-1 siblings, 0 replies; 8+ messages in thread
From: Michal Simek @ 2022-12-15 16:13 UTC (permalink / raw)
To: Kenneth Sloat, sean.anderson@seco.com, michal.simek@xilinx.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On 12/15/22 17:07, Kenneth Sloat wrote:
> From: Ken Sloat <ksloat@designlinxhs.com>
>
> This timer HW supports 8, 16 and 32-bit timer widths. This
> driver currently uses a u32 to store the max possible value
> of the timer. However, statements perform addition of 2 in
> xilinx_pwm_apply() when calculating the period_cycles and
> duty_cycles values. Since priv->max is a u32, this will
> result in an overflow to 1 which will not only be incorrect
> but fail on range comparison. This results in making it
> impossible to set the PWM in this timer mode.
>
> There are two obvious solutions to the current problem:
> 1. Cast each instance where overflow occurs to u64.
> 2. Change priv->max from a u32 to a u64.
>
> Solution #1 requires more code modifications, and leaves
> opportunity to introduce similar overflows if other math
> statements are added in the future. These may also go
> undetected if running in non 32-bit timer modes.
>
> Solution #2 is the much smaller and cleaner approach and
> thus the chosen method in this patch.
>
> This was tested on a Zynq UltraScale+ with multiple
> instances of the PWM IP.
>
> Signed-off-by: Ken Sloat <ksloat@designlinxhs.com>
> ---
> Changes in v2:
> -Update commit comments to explain specifically where this
> problem occurs as well as compare solutions.
>
> ---
> include/clocksource/timer-xilinx.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
> index c0f56fe6d22a..d116f18de899 100644
> --- a/include/clocksource/timer-xilinx.h
> +++ b/include/clocksource/timer-xilinx.h
> @@ -41,7 +41,7 @@ struct regmap;
> struct xilinx_timer_priv {
> struct regmap *map;
> struct clk *clk;
> - u32 max;
> + u64 max;
> };
>
> /**
> --
> 2.17.1
Reviewed-by: Michal Simek <michal.simek@amd.com>
Thanks,
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [v2] pwm: xilinx: Fix u32 overflow issue in 32-bit width PWM mode.
@ 2022-12-15 16:13 ` Michal Simek
0 siblings, 0 replies; 8+ messages in thread
From: Michal Simek @ 2022-12-15 16:13 UTC (permalink / raw)
To: Kenneth Sloat, sean.anderson@seco.com, michal.simek@xilinx.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
On 12/15/22 17:07, Kenneth Sloat wrote:
> From: Ken Sloat <ksloat@designlinxhs.com>
>
> This timer HW supports 8, 16 and 32-bit timer widths. This
> driver currently uses a u32 to store the max possible value
> of the timer. However, statements perform addition of 2 in
> xilinx_pwm_apply() when calculating the period_cycles and
> duty_cycles values. Since priv->max is a u32, this will
> result in an overflow to 1 which will not only be incorrect
> but fail on range comparison. This results in making it
> impossible to set the PWM in this timer mode.
>
> There are two obvious solutions to the current problem:
> 1. Cast each instance where overflow occurs to u64.
> 2. Change priv->max from a u32 to a u64.
>
> Solution #1 requires more code modifications, and leaves
> opportunity to introduce similar overflows if other math
> statements are added in the future. These may also go
> undetected if running in non 32-bit timer modes.
>
> Solution #2 is the much smaller and cleaner approach and
> thus the chosen method in this patch.
>
> This was tested on a Zynq UltraScale+ with multiple
> instances of the PWM IP.
>
> Signed-off-by: Ken Sloat <ksloat@designlinxhs.com>
> ---
> Changes in v2:
> -Update commit comments to explain specifically where this
> problem occurs as well as compare solutions.
>
> ---
> include/clocksource/timer-xilinx.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
> index c0f56fe6d22a..d116f18de899 100644
> --- a/include/clocksource/timer-xilinx.h
> +++ b/include/clocksource/timer-xilinx.h
> @@ -41,7 +41,7 @@ struct regmap;
> struct xilinx_timer_priv {
> struct regmap *map;
> struct clk *clk;
> - u32 max;
> + u64 max;
> };
>
> /**
> --
> 2.17.1
Reviewed-by: Michal Simek <michal.simek@amd.com>
Thanks,
Michal
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [v2] pwm: xilinx: Fix u32 overflow issue in 32-bit width PWM mode.
2022-12-15 16:07 ` Kenneth Sloat
@ 2024-05-09 12:31 ` Michal Simek
-1 siblings, 0 replies; 8+ messages in thread
From: Michal Simek @ 2024-05-09 12:31 UTC (permalink / raw)
To: Kenneth Sloat, sean.anderson@seco.com, michal.simek@xilinx.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Thierry Reding
On 12/15/22 17:07, Kenneth Sloat wrote:
> From: Ken Sloat <ksloat@designlinxhs.com>
>
> This timer HW supports 8, 16 and 32-bit timer widths. This
> driver currently uses a u32 to store the max possible value
> of the timer. However, statements perform addition of 2 in
> xilinx_pwm_apply() when calculating the period_cycles and
> duty_cycles values. Since priv->max is a u32, this will
> result in an overflow to 1 which will not only be incorrect
> but fail on range comparison. This results in making it
> impossible to set the PWM in this timer mode.
>
> There are two obvious solutions to the current problem:
> 1. Cast each instance where overflow occurs to u64.
> 2. Change priv->max from a u32 to a u64.
>
> Solution #1 requires more code modifications, and leaves
> opportunity to introduce similar overflows if other math
> statements are added in the future. These may also go
> undetected if running in non 32-bit timer modes.
>
> Solution #2 is the much smaller and cleaner approach and
> thus the chosen method in this patch.
>
> This was tested on a Zynq UltraScale+ with multiple
> instances of the PWM IP.
>
> Signed-off-by: Ken Sloat <ksloat@designlinxhs.com>
> ---
> Changes in v2:
> -Update commit comments to explain specifically where this
> problem occurs as well as compare solutions.
>
> ---
> include/clocksource/timer-xilinx.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
> index c0f56fe6d22a..d116f18de899 100644
> --- a/include/clocksource/timer-xilinx.h
> +++ b/include/clocksource/timer-xilinx.h
> @@ -41,7 +41,7 @@ struct regmap;
> struct xilinx_timer_priv {
> struct regmap *map;
> struct clk *clk;
> - u32 max;
> + u64 max;
> };
>
> /**
It looks like it was forgotten for quite a while.
Let me take it via my tree.
Applied.
Michal
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [v2] pwm: xilinx: Fix u32 overflow issue in 32-bit width PWM mode.
@ 2024-05-09 12:31 ` Michal Simek
0 siblings, 0 replies; 8+ messages in thread
From: Michal Simek @ 2024-05-09 12:31 UTC (permalink / raw)
To: Kenneth Sloat, sean.anderson@seco.com, michal.simek@xilinx.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Thierry Reding
On 12/15/22 17:07, Kenneth Sloat wrote:
> From: Ken Sloat <ksloat@designlinxhs.com>
>
> This timer HW supports 8, 16 and 32-bit timer widths. This
> driver currently uses a u32 to store the max possible value
> of the timer. However, statements perform addition of 2 in
> xilinx_pwm_apply() when calculating the period_cycles and
> duty_cycles values. Since priv->max is a u32, this will
> result in an overflow to 1 which will not only be incorrect
> but fail on range comparison. This results in making it
> impossible to set the PWM in this timer mode.
>
> There are two obvious solutions to the current problem:
> 1. Cast each instance where overflow occurs to u64.
> 2. Change priv->max from a u32 to a u64.
>
> Solution #1 requires more code modifications, and leaves
> opportunity to introduce similar overflows if other math
> statements are added in the future. These may also go
> undetected if running in non 32-bit timer modes.
>
> Solution #2 is the much smaller and cleaner approach and
> thus the chosen method in this patch.
>
> This was tested on a Zynq UltraScale+ with multiple
> instances of the PWM IP.
>
> Signed-off-by: Ken Sloat <ksloat@designlinxhs.com>
> ---
> Changes in v2:
> -Update commit comments to explain specifically where this
> problem occurs as well as compare solutions.
>
> ---
> include/clocksource/timer-xilinx.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
> index c0f56fe6d22a..d116f18de899 100644
> --- a/include/clocksource/timer-xilinx.h
> +++ b/include/clocksource/timer-xilinx.h
> @@ -41,7 +41,7 @@ struct regmap;
> struct xilinx_timer_priv {
> struct regmap *map;
> struct clk *clk;
> - u32 max;
> + u64 max;
> };
>
> /**
It looks like it was forgotten for quite a while.
Let me take it via my tree.
Applied.
Michal
^ permalink raw reply [flat|nested] 8+ messages in thread