Message ID | 20230526171057.66876-3-sebastian.reichel@collabora.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp644831vqr; Fri, 26 May 2023 10:34:55 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5Bb5Da4hQxKHVexg6wvtR0tLaH3Ke5q39KnWoIzxVF3SvTJcTY1KJFBkYjOxaGwX254ef7 X-Received: by 2002:a05:6a00:1489:b0:627:6328:79f1 with SMTP id v9-20020a056a00148900b00627632879f1mr5012397pfu.34.1685122495361; Fri, 26 May 2023 10:34:55 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685122495; cv=none; d=google.com; s=arc-20160816; b=vDqmu1W4n3Rjs/eKTFXdEJMgSJhr86xcjoINPhiGMUoAabcErpKMBMVLBfZ9AxDsL3 NqVAPu2detrhHQVLwD6Mq1RwU5k9iKnyVDpy6ZypMgdaKyZbPPXmvZXSj9BZqmF4JH4d 1RegSgeodzUINP7ICyrReeEr4KCi4rBHKBJ7VGWbxapt6rYesPBf4fmc5aoVQcM7fW/a ZtYAbIwI9zRI6FtJ+u/ohxDY9/2ZGFeeeQ6RuUkyPwzc2mEN2HREFHzXJSxqht9Ig7bC uVvuly0WBMlQI9R7jLF/38ueHtSYKQWGzWvnR5DFtHG/HUC5/1wcME1+oAJgiZpTxIgi s+JQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=s8FZPGrJifbBYWWLsATIfRWcLbtvfuP7HSuuFpuFDrY=; b=mrn4J52hMitlr8ebXLItarKEsZmivK3njsNS5AQjKZz+jYYaS/br5q78ZMbQ2wO4Fz gyu0gEY5dEI+Nyrl6RVSE+W2CV5Dv1mRBGnKrOlvsP75S9W4AnwzkMUVg/mZt9AIZ/Oi 9yeEsMsyUQl+s3ZTnQbk4GdgcuZi6nVQ2y311gMfdjT7HjZrGIsy3JaOGPE5oGMppF64 5kQaPS3/olZG6kBb2iZwJkAewNgUFqTnRFk/jueHZwW93X0PDEHn3lNmw9Z9W8U7ko2q jO5ZCIYF0f7owOhAx5U+t9PRgSu/pPnAK0FVLYHQVsDfW6KIs4mtWbTWcJxcu6LXbdVN TkEg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=CQMmdb+l; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id g5-20020a636b05000000b005289dd0ef00si3983118pgc.568.2023.05.26.10.34.42; Fri, 26 May 2023 10:34:55 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=CQMmdb+l; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242258AbjEZRLF (ORCPT <rfc822;zhanglyra.2023@gmail.com> + 99 others); Fri, 26 May 2023 13:11:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33258 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230144AbjEZRLC (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 26 May 2023 13:11:02 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 00EA0F3; Fri, 26 May 2023 10:11:00 -0700 (PDT) Received: from jupiter.universe (dyndsl-091-248-132-021.ewe-ip-backbone.de [91.248.132.21]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: sre) by madras.collabora.co.uk (Postfix) with ESMTPSA id 918536606E95; Fri, 26 May 2023 18:10:59 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1685121059; bh=T4lpsRVLH6x7IReX3htoV7ctOeRMnG2Z8G+kSXW+NWE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=CQMmdb+lt47Nfyk60vuz43mxBRGxAFCuth1e7EdEYjEs4yei18dyATChn8dt2sqYz mTXJthprnnFujc8KXNsnukL+v6lc7oOLF1EH5FWqiCCNsILbqkBG82sQAlOGNiTFRU iGZ37UJspuNS561holzqTqRltGh7PCKgwi7lfznv7aS8LDOQhfH6tohw5YhezK1UzG kU/ifwIaMRaWzDU432gRJTscGZZpQXOkmRjlp5B1R/Bq/js+2gjnwLC2hUXzEVub70 pmxTBTJTW42CuKlTRDs5pRSyQR/ez5QtzAZK1kqx83gUfFLY0iDBfSIIaIuilxB7xw fy1IZcNMojGdA== Received: by jupiter.universe (Postfix, from userid 1000) id D41714807E3; Fri, 26 May 2023 19:10:57 +0200 (CEST) From: Sebastian Reichel <sebastian.reichel@collabora.com> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Christopher Obbard <chris.obbard@collabora.com>, David Laight <David.Laight@ACULAB.COM>, Sebastian Reichel <sebastian.reichel@collabora.com>, kernel@collabora.com Subject: [PATCH v2 2/2] clk: divider: Fix divisions Date: Fri, 26 May 2023 19:10:57 +0200 Message-Id: <20230526171057.66876-3-sebastian.reichel@collabora.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230526171057.66876-1-sebastian.reichel@collabora.com> References: <20230526171057.66876-1-sebastian.reichel@collabora.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766979005706679046?= X-GMAIL-MSGID: =?utf-8?q?1766979005706679046?= |
Series |
Fix 64 bit issues in common clock framework
|
|
Commit Message
Sebastian Reichel
May 26, 2023, 5:10 p.m. UTC
The clock framework handles clock rates as "unsigned long", so u32 on
32-bit architectures and u64 on 64-bit architectures.
The current code pointlessly casts the dividend to u64 on 32-bit
architectures and thus pointlessly reducing the performance.
On the other hand on 64-bit architectures the divisor is masked and only
the lower 32-bit are used. Thus requesting a frequency >= 4.3GHz results
in incorrect values. For example requesting 4300000000 (4.3 GHz) will
effectively request ca. 5 MHz. Requesting clk_round_rate(clk, ULONG_MAX)
is a bit of a special case, since that still returns correct values as
long as the parent clock is below 8.5 GHz.
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
drivers/clk/clk-divider.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Comments
Il 26/05/23 19:10, Sebastian Reichel ha scritto: > The clock framework handles clock rates as "unsigned long", so u32 on > 32-bit architectures and u64 on 64-bit architectures. > > The current code pointlessly casts the dividend to u64 on 32-bit > architectures and thus pointlessly reducing the performance. > > On the other hand on 64-bit architectures the divisor is masked and only > the lower 32-bit are used. Thus requesting a frequency >= 4.3GHz results > in incorrect values. For example requesting 4300000000 (4.3 GHz) will > effectively request ca. 5 MHz. Requesting clk_round_rate(clk, ULONG_MAX) > is a bit of a special case, since that still returns correct values as > long as the parent clock is below 8.5 GHz. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Quoting Sebastian Reichel (2023-05-26 10:10:57) > The clock framework handles clock rates as "unsigned long", so u32 on > 32-bit architectures and u64 on 64-bit architectures. > > The current code pointlessly casts the dividend to u64 on 32-bit > architectures and thus pointlessly reducing the performance. It looks like that was done to make the DIV_ROUND_UP() macro not overflow the dividend on 32-bit machines (from 9556f9dad8f5): DIV_ROUND_UP(3000000000, 1500000000) = (3.0G + 1.5G - 1) / 1.5G = OVERFLOW / 1.5G but I agree, the u64 cast is not necessary if DIV_ROUND_UP_ULL() is used as that macro casts the dividend to unsigned long long anyway. > > On the other hand on 64-bit architectures the divisor is masked and only > the lower 32-bit are used. Thus requesting a frequency >= 4.3GHz results > in incorrect values. For example requesting 4300000000 (4.3 GHz) will > effectively request ca. 5 MHz. Nice catch. But I'm concerned that the case above is broken by changing to DIV_ROUND_UP(). As this code is generic, I fear we'll have to change this code that divides rates to use DIV64_U64_ROUND_UP() because we don't know how large the rate is (i.e. it could be larger than 32-bits on a 64-bit machine). > Requesting clk_round_rate(clk, ULONG_MAX) > is a bit of a special case, since that still returns correct values as > long as the parent clock is below 8.5 GHz. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > drivers/clk/clk-divider.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index a2c2b5203b0a..c38e8aa60e54 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -220,7 +220,7 @@ static int _div_round_up(const struct clk_div_table *table, > unsigned long parent_rate, unsigned long rate, > unsigned long flags) > { > - int div = DIV_ROUND_UP_ULL((u64)parent_rate, rate); > + int div = DIV_ROUND_UP(parent_rate, rate); > > if (flags & CLK_DIVIDER_POWER_OF_TWO) > div = __roundup_pow_of_two(div); > @@ -237,7 +237,7 @@ static int _div_round_closest(const struct clk_div_table *table, > int up, down; > unsigned long up_rate, down_rate; > > - up = DIV_ROUND_UP_ULL((u64)parent_rate, rate); > + up = DIV_ROUND_UP(parent_rate, rate); > down = parent_rate / rate; > > if (flags & CLK_DIVIDER_POWER_OF_TWO) { > @@ -473,7 +473,7 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate, > { > unsigned int div, value; > > - div = DIV_ROUND_UP_ULL((u64)parent_rate, rate); > + div = DIV_ROUND_UP(parent_rate, rate); > > if (!_is_valid_div(table, div, flags)) > return -EINVAL; This is undoing parts of commit 9556f9dad8f5 ("clk: divider: handle integer overflow when dividing large clock rates"). Please pair this patch with extensive kunit tests in a new test suite clk-divider_test.c file. I don't know if UML supports changing sizeof(long), but that would be a cool feature to tease out these sorts of issues. I suppose we'll just have to run the kunit tests on various architectures to cover the possibilities.
From: Stephen Boyd > Sent: 13 June 2023 01:42 > > Quoting Sebastian Reichel (2023-05-26 10:10:57) > > The clock framework handles clock rates as "unsigned long", so u32 on > > 32-bit architectures and u64 on 64-bit architectures. > > > > The current code pointlessly casts the dividend to u64 on 32-bit > > architectures and thus pointlessly reducing the performance. > > It looks like that was done to make the DIV_ROUND_UP() macro not > overflow the dividend on 32-bit machines (from 9556f9dad8f5): > > DIV_ROUND_UP(3000000000, 1500000000) = (3.0G + 1.5G - 1) / 1.5G > = OVERFLOW / 1.5G Maybe add: #define DIV_ROUND_UP_NZ(x, y) (((x) - 1)/(y) + 1) which doesn't overflow but requires x != 0. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index a2c2b5203b0a..c38e8aa60e54 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -220,7 +220,7 @@ static int _div_round_up(const struct clk_div_table *table, unsigned long parent_rate, unsigned long rate, unsigned long flags) { - int div = DIV_ROUND_UP_ULL((u64)parent_rate, rate); + int div = DIV_ROUND_UP(parent_rate, rate); if (flags & CLK_DIVIDER_POWER_OF_TWO) div = __roundup_pow_of_two(div); @@ -237,7 +237,7 @@ static int _div_round_closest(const struct clk_div_table *table, int up, down; unsigned long up_rate, down_rate; - up = DIV_ROUND_UP_ULL((u64)parent_rate, rate); + up = DIV_ROUND_UP(parent_rate, rate); down = parent_rate / rate; if (flags & CLK_DIVIDER_POWER_OF_TWO) { @@ -473,7 +473,7 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate, { unsigned int div, value; - div = DIV_ROUND_UP_ULL((u64)parent_rate, rate); + div = DIV_ROUND_UP(parent_rate, rate); if (!_is_valid_div(table, div, flags)) return -EINVAL;