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: <TYZPR04MB632148D739338EC2ABB1C035966C9@TYZPR04MB6321.apcprd04.prod.outlook.com>
Date:   Thu, 4 May 2023 02:44:45 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH v3 2/2] clk: tests: Add missing test cases for mux
 determine_rate
Content-Language: en-US
To:     Stephen Boyd <sboyd@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Yang Xiwen via B4 Relay 
        <devnull+forbidden405.outlook.com@kernel.org>
Cc:     linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230421-clk-v3-0-9ff79e7e7fed@outlook.com>
 <20230421-clk-v3-2-9ff79e7e7fed@outlook.com>
 <954b051546fe936db16353fc58a68438.sboyd@kernel.org>
From:   Yang Xiwen <forbidden405@outlook.com>
In-Reply-To: <954b051546fe936db16353fc58a68438.sboyd@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: TYZPR04MB6321:EE_|PUZPR04MB6649:EE_
X-MS-Office365-Filtering-Correlation-Id: e290769c-fb5f-4976-073b-08db4c067ab1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: a68TSertaFiZjGZbhyG9wonu2wV+31QkDv4qpySJ9gPpPEXl+/OI7qA9vUMyqe+SfsOQ0lJu5SDggL4l/8HmnWSqBd4B15gW4WiT2XyWewXFgliCtRultBB58v+yckAj2aQYUz/XBryUn8vRWV299MgUg+Ygbd5PYA0U6/jmtCsUORo1nOVjwhX4hnRryxwVlgQqXa7+aNaWBs7g11r3R8sfrKszOFFeHNt0j26nTzwSSpGOYa8fO7Aiy+sDDwGzpO8p47orwj95wX4y+k8f6eBNxITlVQK1uUG6na2/eP2Wa+k/V+lIiEsZSlJ2OZpxW94C8hhHh5OMQxLnMBJFZHTLt19IvmCWeX9F8RiP8+GEcrg2dacv1nOSHsYLjUBZkOj0pA+Knli0sHJlztcdmjNTtpHilf618CPowtACoBmQUQIMqQJk/VRhqyb36S2nCthqeWrGvD/TvwP+9jecm6J1srTDjU+OiViYRArwd/9ZK+3GBSRntH9QKP3shi+ojRvhkSuZ5zRcrcOKt30G1ZvabCNIeKFeAQuz280Mu1wDETCSzulsOCv7P2esqlYqHQwp9NlBrCSCKOeT6+3Pm2T+pU2LtfAWzGEqC17zj/XrGT36yW/5akTmIOFW178Q
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MHljTit4Myt0Vm1UMGFLVnFWQnk5U2xDK2FIVTNBdm9qR2wyZVVUQmJRN2tu?=
 =?utf-8?B?bXdIalZ6NlhtaGgrN3ZucTU4Wm1IVy8xOXhVYmVRRWU5c1FBa2l4ekhZWkNK?=
 =?utf-8?B?MmVBNWZKSWduVTVKZ2RlaGhTNW5qblhlbDRqMTkvRGJDRG1MT3BNSmtYRHFD?=
 =?utf-8?B?ajF6aG9XTHlQUVpLMUVJL2xqUGovYzIrbkpMTzJTaEZKZk1IMXhVU0lMQ3RU?=
 =?utf-8?B?aDlZSmVIU0ZvNGVBbjVLMlRQTlVCMDM0R2I4WXM2M0hjb3VRNmVEVGVtbGpJ?=
 =?utf-8?B?N2lZWUt0WTNDZWFiVndCU1BhS1RKMnd2cDdZcS9PdFNJSjZMNk01bVNpbWtH?=
 =?utf-8?B?OVZxL056QVNtZlZtaENxbFNNamprbUVVWDVYQVVwVWh3RFA4ekJZSXhObEw3?=
 =?utf-8?B?Ukw1WVl6ZnRJbkFhVGhGNytWN3Uyd2hkNFgrcXlSNURzcFRuTVlKY0JSM3hP?=
 =?utf-8?B?dmVlY3VsdUpCd0YwYXVadlRxazN4RUpsNk9Fd2JCVCtRTHVvNTdBV3d2S0Ro?=
 =?utf-8?B?UXYvWFYvelUvSTRLa1Y4Nm9IK05haUZpVXk1S2NURmJNTUJhcWtwTWxnZEJU?=
 =?utf-8?B?anVuY0o4eVI2MEdtOWRrNzJVSk9KRUYzUGE4aVN3Qnp4aGxWaUZ4elY1T1dj?=
 =?utf-8?B?eDFxUlN5YmZhUkgrdVFmcXltWi9RY1NLbFoxYkl2ZExkVXp5RGg3SUVlVmRU?=
 =?utf-8?B?QTF3WlUwT0hlay9IVzlCdmxOVHZrZ0wxbVRGVm90VlZhamNsUVpWWHZVRTcy?=
 =?utf-8?B?V2laNUxsUENwZCtCQ0xNYnFTSVhTOXB5WS91VEtTbDd2cWxsMFltZW9xajVV?=
 =?utf-8?B?Q2RIYno1SW9oN01Oc0RyR25zUTdheWsrTDhvb0RVRlh1RzFFQVEvRGtCY0J4?=
 =?utf-8?B?elFiVFZWanliQThZaEs1SnZOdS9CRUt6WW5Bb2JFZ1lENDVVdnQ3eU9Mak1r?=
 =?utf-8?B?OUFCYXFzcGY0d0tpYXpoMG5CT2plcGhLNmlpU1FaT2pSQUdUTVQ4SG5OL2hn?=
 =?utf-8?B?dVg2UHcyQmpUQTJ6TzdIbWRGelh1T0ozUTJ0QjFmYmZBOTcwblNjZW9LeXlN?=
 =?utf-8?B?cjY1TTlhWkU0UHlJSldjU1JRM0h3SUFTenY0Y0Y2aDhUakFCOXF2dTZMVUhk?=
 =?utf-8?B?WHJNc2ptMVI3VHp4VXFJc0NZVkYvYlI0YUFpVDZ2ZS9sTTI4dmh2T3c1L3N6?=
 =?utf-8?B?MWxVVVYxcVorOE9icVJmQWxMYTNrQWlKZUtvT0lWOFZaaExhQ2NJcjdOcFRt?=
 =?utf-8?B?OVVIa0JhZGRXZ3FUdTZVOStDejJKNG4zdElsZHhzVVR5QWxLcWs3VzBRWnpu?=
 =?utf-8?B?cjYzdGI4dUE5eElsZnhnY2dTWFBHREM3SkJzdDJGM1lJTVg4Ymx6VlVZZVRt?=
 =?utf-8?B?clo5N01XUjBpKzE5YjRJZGd6WExNTWZBL2RxSmNmbEZQRHJweFpaeUZGOUdC?=
 =?utf-8?B?Tmg4TUlQUjJ2MDh1MkRsaDdtb3JlcVZGWm12U05DekVDYUN0VzRic1l0S01p?=
 =?utf-8?B?NGRhZko3ZjBPQnRnblZnY0NzUnY0YkdaUkRTMHhNMGVpU0MrTXdXNU1vVzdr?=
 =?utf-8?B?TlRYWDRLYVg5RjVqNnE0a2NVWHVZZ1JrM0N3TjFxT2NDc1ZCYUNYZVAybHF6?=
 =?utf-8?B?N3JXcjZUUDNvcHpsODNKMjQyOTV0Wk80OVhDdElkU3o4S0srSkhhWkptb0pX?=
 =?utf-8?B?MlRNUlBJclh0L2RRbU45OXM1NzFSNnlkaHoxQlVTVmljUlAydG5hbGdsMkRl?=
 =?utf-8?Q?urQo7iFr/nGibE7a20=3D?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e290769c-fb5f-4976-073b-08db4c067ab1
X-MS-Exchange-CrossTenant-AuthSource: TYZPR04MB6321.apcprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2023 18:44:51.8000
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PUZPR04MB6649
X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_MUA_MOZILLA,
        FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE 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 5/3/2023 11:08 AM, Stephen Boyd wrote:
> Quoting Yang Xiwen via B4 Relay (2023-04-26 12:34:17)
>> diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
>> index f9a5c2964c65d..4f7f9a964637a 100644
>> --- a/drivers/clk/clk_test.c
>> +++ b/drivers/clk/clk_test.c
>> @@ -2194,7 +2194,47 @@ static void clk_leaf_mux_set_rate_parent_test_exit(struct kunit *test)
>>   * parent, the rate request structure returned by __clk_determine_rate
>>   * is sane and will be what we expect.
>>   */
>> -static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
> 
> Just leave this one alone and put the other test cases right after it.
> Don't rename it and also move it lower down. It makes the diff hard to
> read.
I don't quite understand. In this patch, I renamed it to case3. Here I
think you'd like it to remain as-is. But I think the comments below said
I should rename it to clk_leaf_mux_determine_rate_exactly_parent1()?
Actually what this test has done should be included in this series of
test cases as one of them.
> 
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case1(struct kunit *test)
> 
> Please add a comment above each test case like there is for
> clk_leaf_mux_set_rate_parent_determine_rate() that describes what is
> being tested.
> 
>> +{
>> +       struct clk_leaf_mux_ctx *ctx = test->priv;
>> +       struct clk_hw *hw = &ctx->hw;
>> +       struct clk *clk = clk_hw_get_clk(hw, NULL);
>> +       struct clk_rate_request req;
>> +       unsigned long rate;
>> +       int ret;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, 0);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case2(struct kunit *test)
>> +{
>> +       struct clk_leaf_mux_ctx *ctx = test->priv;
>> +       struct clk_hw *hw = &ctx->hw;
>> +       struct clk *clk = clk_hw_get_clk(hw, NULL);
>> +       struct clk_rate_request req;
>> +       unsigned long rate;
>> +       int ret;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_INIT_RATE);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);
> 
> There should be some KUNIT_EXPECT statement in each test.
> 
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case3(struct kunit *test)
>>  {
>>         struct clk_leaf_mux_ctx *ctx = test->priv;
>>         struct clk_hw *hw = &ctx->hw;
>> @@ -2218,8 +2258,95 @@ static void clk_leaf_mux_set_rate_parent_determine_rate(struct kunit *test)
>>         clk_put(clk);
>>  }
>>  
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case4(struct kunit *test)
>> +{
>> +       struct clk_leaf_mux_ctx *ctx = test->priv;
>> +       struct clk_hw *hw = &ctx->hw;
>> +       struct clk *clk = clk_hw_get_clk(hw, NULL);
>> +       struct clk_rate_request req;
>> +       unsigned long rate;
>> +       int ret;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, (DUMMY_CLOCK_RATE_1 + DUMMY_CLOCK_RATE_2) / 2);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_1);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_1);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case5(struct kunit *test)
>> +{
>> +       struct clk_leaf_mux_ctx *ctx = test->priv;
>> +       struct clk_hw *hw = &ctx->hw;
>> +       struct clk *clk = clk_hw_get_clk(hw, NULL);
>> +       struct clk_rate_request req;
>> +       unsigned long rate;
>> +       int ret;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, DUMMY_CLOCK_RATE_2 + 100000);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +static void clk_leaf_mux_set_rate_parent_determine_rate_case6(struct kunit *test)
>> +{
>> +       struct clk_leaf_mux_ctx *ctx = test->priv;
>> +       struct clk_hw *hw = &ctx->hw;
>> +       struct clk *clk = clk_hw_get_clk(hw, NULL);
>> +       struct clk_rate_request req;
>> +       unsigned long rate;
>> +       int ret;
>> +
>> +       rate = clk_get_rate(clk);
>> +       KUNIT_ASSERT_EQ(test, rate, DUMMY_CLOCK_RATE_1);
>> +
>> +       clk_hw_init_rate_request(hw, &req, ULONG_MAX);
>> +
>> +       ret = __clk_determine_rate(hw, &req);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +       KUNIT_EXPECT_EQ(test, req.rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_EQ(test, req.best_parent_rate, DUMMY_CLOCK_RATE_2);
>> +       KUNIT_EXPECT_PTR_EQ(test, req.best_parent_hw, &ctx->mux_ctx.hw);
>> +
>> +       clk_put(clk);
>> +}
>> +
>> +/* We test 6 cases here:
>> + * 1. The requested rate is 0;
>> + * 2. The requested rate is not 0 but lower than any rate that parents could offer;
>> + * 3. The requested rate is exactly one of the parents' clock rate;
>> + * 4. The requested rate is between the lowest clock rate and the highest clock rate that the parents could offer;
>> + * 5. The requested rate is larger than all rates that parents could offer;
>> + * 6. The requested rate is ULONG_MAX.
>> + *
>> + * Hopefully they covered all cases.
>> + */
> 
> Please remove this comment and name the cases better.
> 
>>  static struct kunit_case clk_leaf_mux_set_rate_parent_test_cases[] = {
>> -       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate),
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case1),
> 
> Maybe call it clk_leaf_mux_determine_rate_request_zero?
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case2),
> 
> clk_leaf_mux_determine_rate_lower_than_parents_fails
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case3),
> 
> clk_leaf_mux_determine_rate_exactly_parent1
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case4),
> 
> I'm not sure I understand what is being tested in this case. Are we
> testing that __clk_determine_rate() with a rate between parent0 and
> parent1 picks parent1?
I think I have to speak more about how these tests are arranged. Imagine
that there is a segment starting at 0, ending at ULONG_MAX. Now add two
points on it, assuming DUMMY_CLOCK_RATE_1(142MHz) and
DUMMY_CLOCK_RATE_2(242MHz). We split the segment into 3 segments, and we
have 4 endpoints in total. They are: 0, 0-142MHz, 142MHz, 142MHz-242MHz,
242MHz-ULONG_MAX, ULONG_MAX. Ideally, we need to test all these 7 cases.
For those 4 points, the tests should be straightforward. But for the 3
segments, we can only extract a random point on it to test. Besides, I
think requesting for 142MHz or 242MHz has no big difference, so I
omitted one of them. The original
clk_leaf_mux_set_rate_parent_determine_rate() is considered to be one of
the cases here, for which I think should be absorbed and renamed.

And as said in the previous email, the situation here would become even
more complicated if CLK_MUX_ROUND_CLOSEST is true. I guess it should be
in another patchset.
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case5),
> 
> clk_leaf_mux_determine_rate_larger_than_parents
> 
>> +       KUNIT_CASE(clk_leaf_mux_set_rate_parent_determine_rate_case6),
> 
> clk_leaf_mux_determine_rate_ULONG_MAX_picks_parent1

-- 
Best regards,
Yang Xiwen