Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <423e3575-e8d2-f100-1f9c-43c195139f6b@seco.com>
Date:   Thu, 15 Dec 2022 11:08:28 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [v2] pwm: xilinx: Fix u32 overflow issue in 32-bit width PWM
 mode.
Content-Language: en-US
To:     Kenneth Sloat <ksloat@designlinxhs.com>,
        "michal.simek@xilinx.com" <michal.simek@xilinx.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <SJ0P222MB0107490C5371B848EF04351CA1E19@SJ0P222MB0107.NAMP222.PROD.OUTLOOK.COM>
From:   Sean Anderson <sean.anderson@seco.com>
In-Reply-To: <SJ0P222MB0107490C5371B848EF04351CA1E19@SJ0P222MB0107.NAMP222.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TC9YMkZjbDNSZTdEdS9yc05tako2djBqd3lHRmhtL0JaYVpDdzZaU0V6ZUJJ?=
 =?utf-8?B?Rm5YNnplNTkxUUFsR1o5K1p6Qk1aNFB0RGdMbm1xUFdqTUNHaTM1Z3hsZnVm?=
 =?utf-8?B?Y0RqQUprNkF0ZE91MEdXTUh3UnZwSEcrd0hCdGxtY3ZEeFBIdjh5VVpEN3RB?=
 =?utf-8?B?RXFTQnFHbklud2w3SXhDcEFDVDlrUUtENnRjRG8rZXVCYkFvYW54NGF1Y0pt?=
 =?utf-8?B?U3JkdVZwL0o0SHhwMHl1ZzJzTXA5SVhYd0ZZampPdllvaVJtQTBnRktrZ1dH?=
 =?utf-8?B?TE84b1dlM0M2bWpoMmR3MUdBbHNDWit6UUxVckNBMk92SmtWVGZTQ3VPTHZa?=
 =?utf-8?B?RUZkeERyaGJOQUZwZktBMWc5VW9QaEwwd2l3bnlaU0FHZlh0QlUwYmJKdDVz?=
 =?utf-8?B?N2FBVUJzeHJySHVvUW9ZQ0hEcUZ5WGVJTm56ODR6RXd1YzZwK0kzVkJTaTJ0?=
 =?utf-8?B?S0NkWkMvTXBvNUtzdDJLRzZQd0VhSlFYUkExYUltM1VJY3dNUndPSUIyajJM?=
 =?utf-8?B?bzk2Z3FIWVZaZDFLOHU1SnFNS0d6eUlBNVRNdjVqb05wdmlJcE1XSHdPenEy?=
 =?utf-8?B?NzJSRWRvQWt4aUJwSFYrU2tESjg3V0JOWU5nS29lR0pkNVNlcU1QTzdYU0dw?=
 =?utf-8?B?QjcwRW1zbVBtN2hIZVpLNVVjUFYzdkU1aHF2d0wwempicWdzdXRiMXpvdmNO?=
 =?utf-8?B?SjUydXVRSzRURUdlZTBKQksvVWtoaVphS2U5SjIzTmVpOFlaOWt3NGkyVEMr?=
 =?utf-8?B?S1IzMklrR3o3amlZMSs2TlhwSk9GVmNjeTNwYXhiWHFHUUJ0VHNGMVpESnZt?=
 =?utf-8?B?STNwTkYxclgzRytFeEFWZnZJd1VXK3cxajRzdm5UTHZqTXRmV1ZOQTZvSTJq?=
 =?utf-8?B?M2tDQmFwcVprTitUWXpudmFjRUlsOE93SjZwUUlSQkwyWXRFQ08wRUsxNlNl?=
 =?utf-8?B?U0dNOFByNFVOczJvbzUrb09kUEgwOE11cm9temxlcUtsS1RaWW8xTXVnNTVm?=
 =?utf-8?B?NStRaE1wU0tYVXdjaFFQcGlBSmUxUTcrZlYwdkc5eGplalBYd0l6bC9yUmkx?=
 =?utf-8?B?blU0ak9EZzhGRUdjVWFRcWxBUXMrNGIwRXVNOVN4Z0ZQbDFLNW5CRk0wMmhE?=
 =?utf-8?B?ay9iOHpMNUwrbUhVYlZ2NEswN3M5VUloT252RlE3VW8xWWlyQUNLQWd0emF1?=
 =?utf-8?B?RjkwRXRtSi9SV1JjeFBNcUozYjk3c2lxb1NmTWQyU0RxcjhpQy8yTjRESW5G?=
 =?utf-8?B?QVI3VlB4bkNTNXFKTTNmS3RQeDdMclB1QnFXTTZTRVZwMDBoZGtmRGFXWXRy?=
 =?utf-8?B?cnRkSHVzUWFzUVc5Z1M2OERLTFVZQWRscW9Yb0l5TGtIc1BNSGFCWm94NkdZ?=
 =?utf-8?B?WHhtMEIydzQxbGp3bW5MdG5PNmYvemU4T3dJbnR2L3RRUzhvYW90blFwbXh6?=
 =?utf-8?B?TkZrVHpMSzV3NjI0V2t6M1BVaVlSdlNOa0NlUnFuUzdaN2tvVnBPNVcrUjc1?=
 =?utf-8?B?Z1BKaTd3VVd1Qk8vMmJVODFOYlU1K1FYT3kyZUNaN2xpcEd6SGtIUS9Faldq?=
 =?utf-8?B?dzJTVWhGZ3FtTC81b25HSEdLN0oyWjZGbnpYNWVVbUxDcVp4V0Rxd0JTRXdr?=
 =?utf-8?B?NnZodExOT3JZSE83ZjFwTVZoYVNqTzhGdlpUUllBb29NVzlMSHVpTEZhSllt?=
 =?utf-8?B?a1hIN29ncm5RbzAzbWxHKzhuUHpoM1BTci9leVl2NjhmRkpiL29lUHpPNFNq?=
 =?utf-8?B?OW9zWnRQZ1JvZ2NVTDhwblF2RWl2N1hqNmUrUjFmaW56WWd6Q0t1cmZDVXNM?=
 =?utf-8?B?MnZ2Rkp1TE9hUm55amx0SWtBSWFZOTZONzRKYmFXaCtMRktzbnNGcTFaNjFW?=
 =?utf-8?B?QXMrYmNDY3Y5T1E5YmRpZkYxS2ZtRnpXTGFaVU9WNXoxaVZyQUR4TGsvWkNn?=
 =?utf-8?B?aVVISVdBVFdWRk0vYzA1TDRDR0hjZnEwMEdCL3YxOURxakFwU0xDSFMySzBh?=
 =?utf-8?B?eFJvT0pMa042Vmk2MG9qYXE4aFQzNkVlMU9VbTVvcm9BZ1E2WTFrQyt1cGZu?=
 =?utf-8?B?cmd5TkVqSjE5WVB3UDgxZ0FWa04xclc3Ty9KUlNPY1FhMHh3aXJPN2NWaXh3?=
 =?utf-8?B?RWNqOW1qOERGUmo0V0gxQlBOOWY5cWFyRThqU3VHTm1ZQ3BPR0diM1hJY1JQ?=
 =?utf-8?B?Z2c9PQ==?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 06c1e0ce-98d3-4f2b-33dc-08dadeb69cdc
X-MS-Exchange-CrossTenant-AuthSource: DB9PR03MB8847.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Dec 2022 16:08:32.4999
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cMpHfQUcgkVnrwnOJAR0xunr4bwsJ1c33xu4HjTh5qKlxWU5r83pSDui5DtJIOQZXnfpHIUytoNAuHPXsmkmTw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR03MB7790
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: 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>