Message ID | 20230215232712.17072-1-ansuelsmth@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp149wrn; Wed, 15 Feb 2023 15:50:59 -0800 (PST) X-Google-Smtp-Source: AK7set8RkTu3MAklpdFLiJ966dXJWbaoeFXXrQ7MXw8fm92X+7ni6/+cnFxbQsYwUIcS4CYDA1gH X-Received: by 2002:a17:90a:e7cd:b0:233:a7bc:95c1 with SMTP id kb13-20020a17090ae7cd00b00233a7bc95c1mr240416pjb.14.1676505058720; Wed, 15 Feb 2023 15:50:58 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1676505058; cv=none; d=google.com; s=arc-20160816; b=pIfNnJyE1T8k9w0h8AfjdV5fE/k8pkC9A33ZoDBePTSNHQB1yzZEN+Qgzd4DwfGVQZ khCDQy8ZHg3O4tIdgRlSBniRVgv1jmS+BgE0Q7W8tdFHcbyQTxiANNct0CrGGj+B0uvT aDR5xMxMmLqipAbVnqAPW9itDs2cUhI/yXovcTEJk0ot4VViH4XEBZoBQKZ6eMb7nW7o BjfVQGd7ycW4wfgPxJvmCPhmXuLb27YDWcuuwx+oKHHUjUKG7oYm4wxilhQ/e80sA4oM eEwNDzclc2P74e4AYgi9p94SCxAs1en6JxsV2RsftjqdJ/pQ9tn4yEcGMjUcE/qQwFs0 p2hw== 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 :message-id:date:subject:cc:to:from:dkim-signature; bh=KLWxglH8TvO+YjEnp0OsBiwCevBxF+hBx7Ea32u4U1s=; b=i8glPSvV5iDYbk7qfxxTv74woLksNQmPunRyj4SCMpYf1Fi6RZvdJlvgtjS+Wi2e7b 4xU2qngzx0j9RXxThiF7FKCswFTbrLSZ3mhit2HYjAsV7AlKvkmEcE/DnSfFx6g2UU+s Nc/BqQCUlI8mdwOv/l5URJyMb6IDRnMDgPhuAw322HsmRWFjBwDgHThUBt3j/nGkQ/7x Illqn/bQDMheOG8NoXVz8Zdo7F31RQXFIoSaIG81L2iWs5y6iNd/WTGlJMKEnc8q7PqS VCdzxttONvEHujQ3yOcuAXtzz7O5qVilrleOGEhecfliBQkZFL0iR0HHqRg2apMKRK5Z 1o0Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=XqP2qTHN; 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=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id d19-20020a637353000000b004fb8e72c37fsi13322415pgn.16.2023.02.15.15.50.40; Wed, 15 Feb 2023 15:50:58 -0800 (PST) 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=@gmail.com header.s=20210112 header.b=XqP2qTHN; 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=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229658AbjBOXgv (ORCPT <rfc822;hadasmailinglist@gmail.com> + 99 others); Wed, 15 Feb 2023 18:36:51 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42048 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229532AbjBOXgu (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 15 Feb 2023 18:36:50 -0500 Received: from mail-wm1-x335.google.com (mail-wm1-x335.google.com [IPv6:2a00:1450:4864:20::335]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BFA9543469; Wed, 15 Feb 2023 15:36:48 -0800 (PST) Received: by mail-wm1-x335.google.com with SMTP id bg5-20020a05600c3c8500b003e00c739ce4so282434wmb.5; Wed, 15 Feb 2023 15:36:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=KLWxglH8TvO+YjEnp0OsBiwCevBxF+hBx7Ea32u4U1s=; b=XqP2qTHN3FxCW03V1fZuaqLV9dSsKSa5UCeHysLTZ6Z2ilvx3KPGTDbe0f1HqCMTrh 0QzhOvWMipm4pTrDTiEIKI6ATFUUAFDJkApr6U/0/xIgVwIpaCMYXlqx8/323YIG9sab YoDWAJTWSVW+YexHbB9kS92EdUQxsO3p8HoQwlbG1xpHS8qWfizGZ76FpnYzjI58we6u XI67WcJHrLfkxdJWpmDo7iC8gMO6oWc/lzkiDh1+VsiCWKg0FJpdgGjbB3KpLhmvo3Di hDnJlFreRzTOXeRyc8LPVw2J7Eem2LfC9l3xss+UfCBIni4PmOyzBeLbTauIfKpGI9YU yrog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=KLWxglH8TvO+YjEnp0OsBiwCevBxF+hBx7Ea32u4U1s=; b=FNBg2GofetG2wrbkKYjdr3CEvl/u/v2pN3aRBuUoBo6hMsECD+9Jk2/mmo+3nQuYg2 makUCCca/BnbrVSLw5RYHmYmbQ1wcBmY1sFv0gvuYGCOQ2CczUJBCF1TPNWbFSt8kB5b n2dFfqpqnVUtqv0UPYA9NE+oaOXbxFZu9HQDf7uw2NK6Qp1NYSUyOjkDWtpMj93gWzN1 m0eMSZA6LPYX3+k1C29YPL6800R5gN1Tcvw1zco6wKAtCrwor6QP1phKwgVC+pxwKyhd AI7husgMjZoV0LChgW45WsYKvvZA2QVVIoMtX8PWj/7URsf84k4uj4dRyWpQdRlq6Fb/ iPuQ== X-Gm-Message-State: AO0yUKWcj14FyU5zaE0cCmjOF2LitulhVkZAkFf1MKfFojT8+LiGP/Xn QFnfBTmrgKFJh1javsiVyn4= X-Received: by 2002:a05:600c:44c4:b0:3da:fc30:bfc5 with SMTP id f4-20020a05600c44c400b003dafc30bfc5mr3259702wmo.13.1676504207100; Wed, 15 Feb 2023 15:36:47 -0800 (PST) Received: from localhost.localdomain (93-34-91-73.ip49.fastwebnet.it. [93.34.91.73]) by smtp.googlemail.com with ESMTPSA id m17-20020a05600c3b1100b003dd1bd0b915sm3702281wms.22.2023.02.15.15.36.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Feb 2023 15:36:46 -0800 (PST) From: Christian Marangi <ansuelsmth@gmail.com> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Christian Marangi <ansuelsmth@gmail.com>, Miquel Raynal <miquel.raynal@bootlin.com>, Jerome Brunet <jbrunet@baylibre.com>, Russell King <linux@armlinux.org.uk>, Jeffrey Hugo <jhugo@codeaurora.org>, Chen-Yu Tsai <wens@csie.org> Subject: [PATCH] clk: Fix wrong clock returned in parent_data with .name and no .index Date: Thu, 16 Feb 2023 00:27:12 +0100 Message-Id: <20230215232712.17072-1-ansuelsmth@gmail.com> X-Mailer: git-send-email 2.38.1 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,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1757942968027270195?= X-GMAIL-MSGID: =?utf-8?q?1757942968027270195?= |
Series |
clk: Fix wrong clock returned in parent_data with .name and no .index
|
|
Commit Message
Christian Marangi
Feb. 15, 2023, 11:27 p.m. UTC
Commit 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
introduced a regression due to a "fragile" implementation present in some very
corner case.
Such commit introduced the support for parents to be specified using
clkspec index. The index is an int and should be -1 if the feature
should not be used. This is the case with parent_hws or legacy
parent_names used and the index value is set to -1 by default.
With parent_data the situation is different, since it's a struct that
can have multiple value (.index, .name, .fw_name), it's init to all 0 by
default. This cause the index value to be set to 0 everytime even if not
intended to be defined and used.
This simple "fragile" implementation cause side-effect and unintended
behaviour.
Assuming the following scenario (to repro the corner case and doesn't
reflect real code):
In dt we have a node like this:
acc1: clock-controller@2098000 {
compatible = "qcom,kpss-acc-v1";
reg = <0x02098000 0x1000>, <0x02008000 0x1000>;
clock-output-names = "acpu1_aux";
clocks = <&pxo_board>;
clock-names = "pxo";
#clock-cells = <0>;
};
And on the relevant driver we have the parent data defined as such:
static const struct clk_parent_data aux_parents[] = {
{ .name = "pll8_vote" },
{ .fw_name = "pxo", .name = "pxo_board" },
};
Someone would expect the first parent to be globally searched and set to
point to the clock named "pll8_vote".
But this is not the case and instead under the hood, the parent point to
the pxo clock. This happen without any warning and was discovered on
another platform while the gcc driver was converted to parent_data and
only .name was defined.
The reason is hard to discover but very simple.
Due to the introduction of index support, clk_core_get() won't return
-ENOENT but insted will correctly return a clock.
This is because of_parse_clkspec() will use the index (that is set to 0
due to how things are allocated) and correctly find in the DT node a
clock at index 0. That in the provided example is exactly the phandle to
pxo_board.
Clock is found so the parent is now wrongly linked to the pxo_board
clock.
This only happens in this specific scenario but it's still worth to be
handled and currently there are some driver that hardcode the
parent_data and may suffer from this.
To fix this and handle it correctly we can use the following logic:
1. With a .fw_name not defined (index searching is skipped if a named
clock is provided)
2. Check if .name is provided
3. Compare the provided .name with what clockspec found
4. Return -ENOENT if the name doesn't match, return the clock if it does.
Returning -ENOENT permit clk core code flow to fallback to global
searching and correctly search the right clock.
Fixes: 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
drivers/clk/clk.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
Comments
On Thu, Feb 16, 2023 at 12:27:12AM +0100, Christian Marangi wrote: > Commit 601b6e93304a ("clk: Allow parents to be specified via clkspec index") > introduced a regression due to a "fragile" implementation present in some very > corner case. > > Such commit introduced the support for parents to be specified using > clkspec index. The index is an int and should be -1 if the feature > should not be used. This is the case with parent_hws or legacy > parent_names used and the index value is set to -1 by default. > With parent_data the situation is different, since it's a struct that > can have multiple value (.index, .name, .fw_name), it's init to all 0 by > default. This cause the index value to be set to 0 everytime even if not > intended to be defined and used. > > This simple "fragile" implementation cause side-effect and unintended > behaviour. > > Assuming the following scenario (to repro the corner case and doesn't > reflect real code): > > In dt we have a node like this: > acc1: clock-controller@2098000 { > compatible = "qcom,kpss-acc-v1"; > reg = <0x02098000 0x1000>, <0x02008000 0x1000>; > clock-output-names = "acpu1_aux"; > clocks = <&pxo_board>; > clock-names = "pxo"; > #clock-cells = <0>; > }; > > And on the relevant driver we have the parent data defined as such: > static const struct clk_parent_data aux_parents[] = { > { .name = "pll8_vote" }, > { .fw_name = "pxo", .name = "pxo_board" }, > }; > > Someone would expect the first parent to be globally searched and set to > point to the clock named "pll8_vote". > But this is not the case and instead under the hood, the parent point to > the pxo clock. This happen without any warning and was discovered on > another platform while the gcc driver was converted to parent_data and > only .name was defined. > > The reason is hard to discover but very simple. > > Due to the introduction of index support, clk_core_get() won't return > -ENOENT but insted will correctly return a clock. > This is because of_parse_clkspec() will use the index (that is set to 0 > due to how things are allocated) and correctly find in the DT node a > clock at index 0. That in the provided example is exactly the phandle to > pxo_board. > > Clock is found so the parent is now wrongly linked to the pxo_board > clock. > > This only happens in this specific scenario but it's still worth to be > handled and currently there are some driver that hardcode the > parent_data and may suffer from this. > > To fix this and handle it correctly we can use the following logic: > 1. With a .fw_name not defined (index searching is skipped if a named > clock is provided) > 2. Check if .name is provided > 3. Compare the provided .name with what clockspec found > 4. Return -ENOENT if the name doesn't match, return the clock if it does. > > Returning -ENOENT permit clk core code flow to fallback to global > searching and correctly search the right clock. > > Fixes: 601b6e93304a ("clk: Allow parents to be specified via clkspec index") > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > Cc: Jerome Brunet <jbrunet@baylibre.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Michael Turquette <mturquette@baylibre.com> > Cc: Jeffrey Hugo <jhugo@codeaurora.org> > Cc: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Think this should also be backported to stable kernel just like it was done with 4f8c6aba37da199155a121c6cdc38505a9eb0259 ? > --- > drivers/clk/clk.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 998676d78029..42e297fcfe45 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -395,6 +395,7 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec) > */ > static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index) > { > + const char *global_name = core->parents[p_index].name; > const char *name = core->parents[p_index].fw_name; > int index = core->parents[p_index].index; > struct clk_hw *hw = ERR_PTR(-ENOENT); > @@ -407,6 +408,23 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index) > !of_parse_clkspec(np, index, name, &clkspec)) { > hw = of_clk_get_hw_from_clkspec(&clkspec); > of_node_put(clkspec.np); > + > + /* > + * The returned hw may be incorrect and extra check are required in > + * some corner case. > + * > + * In case a .fw_name is not set of_parse_clkspec will use the index > + * to search the related clock. > + * But index may be never set and actually never intended to be used > + * in the defined parent_data since a 0 value is also accepted and that > + * is what by default each struct is initialized. > + * > + * In the following case check if we have .name and check if the returned > + * clock name match the globally name defined for the parent in the > + * parent_data .name value. > + */ > + if (!name && global_name && strcmp(global_name, hw->core->name)) > + return ERR_PTR(-ENOENT); > } else if (name) { > /* > * If the DT search above couldn't find the provider fallback to > -- > 2.38.1 >
On Fri, Feb 17, 2023 at 02:11:20PM -0800, Stephen Boyd wrote: > Quoting Christian Marangi (2023-02-15 15:27:12) > > Commit 601b6e93304a ("clk: Allow parents to be specified via clkspec index") > > introduced a regression due to a "fragile" implementation present in some very > > corner case. > > > > Such commit introduced the support for parents to be specified using > > clkspec index. The index is an int and should be -1 if the feature > > should not be used. This is the case with parent_hws or legacy > > parent_names used and the index value is set to -1 by default. > > With parent_data the situation is different, since it's a struct that > > can have multiple value (.index, .name, .fw_name), it's init to all 0 by > > default. This cause the index value to be set to 0 everytime even if not > > It's only initialized to all 0 because that's what you've decided to do. > It could be on the stack and have random stack junk values. > Yes and that itself is problematic on his own. The index value may be set to an unintended value and we really can't update each parent_data to -1. And as you can see in the example index is used as an alternative source to search the parent. Hope it's clear what is the problem here. > > intended to be defined and used. > > > > This simple "fragile" implementation cause side-effect and unintended > > behaviour. > > > > Assuming the following scenario (to repro the corner case and doesn't > > reflect real code): > > > > In dt we have a node like this: > > acc1: clock-controller@2098000 { > > compatible = "qcom,kpss-acc-v1"; > > reg = <0x02098000 0x1000>, <0x02008000 0x1000>; > > clock-output-names = "acpu1_aux"; > > clocks = <&pxo_board>; > > clock-names = "pxo"; > > #clock-cells = <0>; > > }; > > > > And on the relevant driver we have the parent data defined as such: > > static const struct clk_parent_data aux_parents[] = { > > { .name = "pll8_vote" }, > > { .fw_name = "pxo", .name = "pxo_board" }, > > }; > > > > Someone would expect the first parent to be globally searched and set to > > point to the clock named "pll8_vote". > > But this is not the case and instead under the hood, the parent point to > > the pxo clock. This happen without any warning and was discovered on > > another platform while the gcc driver was converted to parent_data and > > only .name was defined. > > You didn't set .index explicitly to zero, but it is zero because of the > use of static struct initializers here. If the struct was on the stack > nobody knows what the value would be. Set -1 if you don't want to use > the index lookup path. There is at least one driver that use .name for global searching and it's clear that he didn't intend to use index lookup. Are you totally against this or you are suggesting I should use a different word for this? To me this looks very sensible and something we should take care since sounds a bit fragile to me. (I know 99% of the time it would be a dev error but we could have case where things works by luck and for example someone starts adding an additional parent in later changes and regression happens.)
Quoting Christian Marangi (2023-02-15 15:27:12) > Commit 601b6e93304a ("clk: Allow parents to be specified via clkspec index") > introduced a regression due to a "fragile" implementation present in some very > corner case. > > Such commit introduced the support for parents to be specified using > clkspec index. The index is an int and should be -1 if the feature > should not be used. This is the case with parent_hws or legacy > parent_names used and the index value is set to -1 by default. > With parent_data the situation is different, since it's a struct that > can have multiple value (.index, .name, .fw_name), it's init to all 0 by > default. This cause the index value to be set to 0 everytime even if not It's only initialized to all 0 because that's what you've decided to do. It could be on the stack and have random stack junk values. > intended to be defined and used. > > This simple "fragile" implementation cause side-effect and unintended > behaviour. > > Assuming the following scenario (to repro the corner case and doesn't > reflect real code): > > In dt we have a node like this: > acc1: clock-controller@2098000 { > compatible = "qcom,kpss-acc-v1"; > reg = <0x02098000 0x1000>, <0x02008000 0x1000>; > clock-output-names = "acpu1_aux"; > clocks = <&pxo_board>; > clock-names = "pxo"; > #clock-cells = <0>; > }; > > And on the relevant driver we have the parent data defined as such: > static const struct clk_parent_data aux_parents[] = { > { .name = "pll8_vote" }, > { .fw_name = "pxo", .name = "pxo_board" }, > }; > > Someone would expect the first parent to be globally searched and set to > point to the clock named "pll8_vote". > But this is not the case and instead under the hood, the parent point to > the pxo clock. This happen without any warning and was discovered on > another platform while the gcc driver was converted to parent_data and > only .name was defined. You didn't set .index explicitly to zero, but it is zero because of the use of static struct initializers here. If the struct was on the stack nobody knows what the value would be. Set -1 if you don't want to use the index lookup path.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 998676d78029..42e297fcfe45 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -395,6 +395,7 @@ of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec) */ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index) { + const char *global_name = core->parents[p_index].name; const char *name = core->parents[p_index].fw_name; int index = core->parents[p_index].index; struct clk_hw *hw = ERR_PTR(-ENOENT); @@ -407,6 +408,23 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index) !of_parse_clkspec(np, index, name, &clkspec)) { hw = of_clk_get_hw_from_clkspec(&clkspec); of_node_put(clkspec.np); + + /* + * The returned hw may be incorrect and extra check are required in + * some corner case. + * + * In case a .fw_name is not set of_parse_clkspec will use the index + * to search the related clock. + * But index may be never set and actually never intended to be used + * in the defined parent_data since a 0 value is also accepted and that + * is what by default each struct is initialized. + * + * In the following case check if we have .name and check if the returned + * clock name match the globally name defined for the parent in the + * parent_data .name value. + */ + if (!name && global_name && strcmp(global_name, hw->core->name)) + return ERR_PTR(-ENOENT); } else if (name) { /* * If the DT search above couldn't find the provider fallback to