Taniya Das (2):
clk: qcom: common: Handle invalid index error
clk: qcom: lpass: Initialize start_index
drivers/clk/qcom/common.c | 12 ++++++++----
drivers/clk/qcom/common.h | 1 +
drivers/clk/qcom/lpassaudiocc-sc7280.c | 1 +
3 files changed, 10 insertions(+), 4 deletions(-)
--
2.17.1
Initialize start_index for lpass_audio_cc_sc7280_desc
to fix invalid index error.
Signed-off-by: Taniya Das <[email protected]>
---
drivers/clk/qcom/lpassaudiocc-sc7280.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c
index 1339f9211a14..0567007da8bd 100644
--- a/drivers/clk/qcom/lpassaudiocc-sc7280.c
+++ b/drivers/clk/qcom/lpassaudiocc-sc7280.c
@@ -702,6 +702,7 @@ static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = {
.config = &lpass_audio_cc_sc7280_regmap_config,
.clks = lpass_audio_cc_sc7280_clocks,
.num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks),
+ .start_index = LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC;
};
static const struct qcom_reset_map lpass_audio_cc_sc7280_resets[] = {
--
2.17.1
Introduce start_index to handle invalid index error
seen when there are two clock descriptors assigned
to the same clock controller.
[ 3.600604] qcom_cc_clk_hw_get: invalid index 5
[ 3.625251] qcom_cc_clk_hw_get: invalid index 6
[ 3.648190] qcom_cc_clk_hw_get: invalid index 7
Fixes: 120c15528390 ("clk: qcom: Migrate to clk_hw based registration and OF APIs")
Signed-off-by: Taniya Das <[email protected]>
---
drivers/clk/qcom/common.c | 12 ++++++++----
drivers/clk/qcom/common.h | 1 +
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 75f09e6e057e..0e80535b61f2 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -21,6 +21,7 @@ struct qcom_cc {
struct qcom_reset_controller reset;
struct clk_regmap **rclks;
size_t num_rclks;
+ u32 rclks_start_index;
};
const
@@ -226,12 +227,13 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
struct qcom_cc *cc = data;
unsigned int idx = clkspec->args[0];
- if (idx >= cc->num_rclks) {
+ if (idx >= cc->rclks_start_index && idx < cc->num_rclks)
+ return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
+ else if (idx < cc->rclks_start_index && idx >= cc->num_rclks)
pr_err("%s: invalid index %u\n", __func__, idx);
- return ERR_PTR(-EINVAL);
- }
- return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
+ return ERR_PTR(-EINVAL);
+
}
int qcom_cc_really_probe(struct platform_device *pdev,
@@ -281,6 +283,8 @@ int qcom_cc_really_probe(struct platform_device *pdev,
cc->rclks = rclks;
cc->num_rclks = num_clks;
+ if (desc->start_index)
+ cc->rclks_start_index = desc->start_index;
qcom_cc_drop_protected(dev, cc);
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index 9c8f7b798d9f..924f36af55b3 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -23,6 +23,7 @@ struct qcom_cc_desc {
const struct regmap_config *config;
struct clk_regmap **clks;
size_t num_clks;
+ u32 start_index;
const struct qcom_reset_map *resets;
size_t num_resets;
struct gdsc **gdscs;
--
2.17.1
On Fri, 3 Mar 2023 at 11:30, Taniya Das <[email protected]> wrote:
>
> Introduce start_index to handle invalid index error
> seen when there are two clock descriptors assigned
> to the same clock controller.
Please provide details of the exact case that you are trying to solve
(this might go to the cover letter). I think the commit message is
slightly misleading here. Are you trying to add error messages or to
prevent them from showing up?
I'm asking because error messages do not seem to correspond to patch
2. You add start_index to make the kernel warn for the clock indices
less than LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC = 4, while quoted
messages show indices 5,6,7.
Nit: please don't overwrap the commit message, the recommended line
width is about 72-77 chars.
>
> [ 3.600604] qcom_cc_clk_hw_get: invalid index 5
> [ 3.625251] qcom_cc_clk_hw_get: invalid index 6
> [ 3.648190] qcom_cc_clk_hw_get: invalid index 7
>
> Fixes: 120c15528390 ("clk: qcom: Migrate to clk_hw based registration and OF APIs")
> Signed-off-by: Taniya Das <[email protected]>
> ---
> drivers/clk/qcom/common.c | 12 ++++++++----
> drivers/clk/qcom/common.h | 1 +
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 75f09e6e057e..0e80535b61f2 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -21,6 +21,7 @@ struct qcom_cc {
> struct qcom_reset_controller reset;
> struct clk_regmap **rclks;
> size_t num_rclks;
> + u32 rclks_start_index;
> };
>
> const
> @@ -226,12 +227,13 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> struct qcom_cc *cc = data;
> unsigned int idx = clkspec->args[0];
>
> - if (idx >= cc->num_rclks) {
> + if (idx >= cc->rclks_start_index && idx < cc->num_rclks)
> + return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> + else if (idx < cc->rclks_start_index && idx >= cc->num_rclks)
> pr_err("%s: invalid index %u\n", __func__, idx);
> - return ERR_PTR(-EINVAL);
> - }
>
> - return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> + return ERR_PTR(-EINVAL);
> +
> }
>
> int qcom_cc_really_probe(struct platform_device *pdev,
> @@ -281,6 +283,8 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>
> cc->rclks = rclks;
> cc->num_rclks = num_clks;
> + if (desc->start_index)
> + cc->rclks_start_index = desc->start_index;
>
> qcom_cc_drop_protected(dev, cc);
>
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 9c8f7b798d9f..924f36af55b3 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -23,6 +23,7 @@ struct qcom_cc_desc {
> const struct regmap_config *config;
> struct clk_regmap **clks;
> size_t num_clks;
> + u32 start_index;
> const struct qcom_reset_map *resets;
> size_t num_resets;
> struct gdsc **gdscs;
> --
> 2.17.1
>
--
With best wishes
Dmitry
Hi Taniya,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on clk/clk-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Taniya-Das/clk-qcom-common-Handle-invalid-index-error/20230303-173158
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
patch link: https://lore.kernel.org/r/20230303092859.22094-3-quic_tdas%40quicinc.com
patch subject: [PATCH 2/2] clk: qcom: lpass: Initialize start_index
config: hexagon-randconfig-r021-20230302 (https://download.01.org/0day-ci/archive/20230303/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d26ce274e7b0af8a6c6985630d1da8e257c9031d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Taniya-Das/clk-qcom-common-Handle-invalid-index-error/20230303-173158
git checkout d26ce274e7b0af8a6c6985630d1da8e257c9031d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/clk/qcom/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from drivers/clk/qcom/lpassaudiocc-sc7280.c:13:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/clk/qcom/lpassaudiocc-sc7280.c:13:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/clk/qcom/lpassaudiocc-sc7280.c:13:
In file included from include/linux/regmap.h:20:
In file included from include/linux/iopoll.h:14:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/clk/qcom/lpassaudiocc-sc7280.c:705:56: error: unexpected ';' before '}'
.start_index = LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC;
^
6 warnings and 1 error generated.
vim +705 drivers/clk/qcom/lpassaudiocc-sc7280.c
700
701 static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = {
702 .config = &lpass_audio_cc_sc7280_regmap_config,
703 .clks = lpass_audio_cc_sc7280_clocks,
704 .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks),
> 705 .start_index = LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC;
706 };
707
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Dmitry,
Thanks for the comments.
On 3/3/2023 4:14 PM, Dmitry Baryshkov wrote:
> On Fri, 3 Mar 2023 at 11:30, Taniya Das <[email protected]> wrote:
>>
>> Introduce start_index to handle invalid index error
>> seen when there are two clock descriptors assigned
>> to the same clock controller.
>
> Please provide details of the exact case that you are trying to solve
> (this might go to the cover letter). I think the commit message is
> slightly misleading here. Are you trying to add error messages or to
> prevent them from showing up?
>
We are trying to avoid error messages from showing up.
> I'm asking because error messages do not seem to correspond to patch
> 2. You add start_index to make the kernel warn for the clock indices
> less than LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC = 4, while quoted
> messages show indices 5,6,7.
>
Right, we want the kernel to warn if the clock index is less than
start_index, along with that we also want to handle the case where
num_rclks is uninitialized because of same clock descriptor being
assigned to two clock controllers.
Earlier Invalid index error was showing up for valid indices 5,6,7
because of the simple if check(idx >= num_rclks), hence we enhanced the
checks to handle the above case and compare the index to the start_index
+ num_rclks, instead of simply comparing it with num_clks.
> Nit: please don't overwrap the commit message, the recommended line
> width is about 72-77 chars.
>
Done.
>>
>> [ 3.600604] qcom_cc_clk_hw_get: invalid index 5
>> [ 3.625251] qcom_cc_clk_hw_get: invalid index 6
>> [ 3.648190] qcom_cc_clk_hw_get: invalid index 7
>
>>
>> Fixes: 120c15528390 ("clk: qcom: Migrate to clk_hw based registration and OF APIs")
>> Signed-off-by: Taniya Das <[email protected]>
>> ---
>> drivers/clk/qcom/common.c | 12 ++++++++----
>> drivers/clk/qcom/common.h | 1 +
>> 2 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>> index 75f09e6e057e..0e80535b61f2 100644
>> --- a/drivers/clk/qcom/common.c
>> +++ b/drivers/clk/qcom/common.c
>> @@ -21,6 +21,7 @@ struct qcom_cc {
>> struct qcom_reset_controller reset;
>> struct clk_regmap **rclks;
>> size_t num_rclks;
>> + u32 rclks_start_index;
>> };
>>
>> const
>> @@ -226,12 +227,13 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
>> struct qcom_cc *cc = data;
>> unsigned int idx = clkspec->args[0];
>>
>> - if (idx >= cc->num_rclks) {
>> + if (idx >= cc->rclks_start_index && idx < cc->num_rclks)
>> + return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
>> + else if (idx < cc->rclks_start_index && idx >= cc->num_rclks)
>> pr_err("%s: invalid index %u\n", __func__, idx);
>> - return ERR_PTR(-EINVAL);
>> - }
>>
>> - return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
>> + return ERR_PTR(-EINVAL);
>> +
>> }
>>
>> int qcom_cc_really_probe(struct platform_device *pdev,
>> @@ -281,6 +283,8 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>>
>> cc->rclks = rclks;
>> cc->num_rclks = num_clks;
>> + if (desc->start_index)
>> + cc->rclks_start_index = desc->start_index;
>>
>> qcom_cc_drop_protected(dev, cc);
>>
>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>> index 9c8f7b798d9f..924f36af55b3 100644
>> --- a/drivers/clk/qcom/common.h
>> +++ b/drivers/clk/qcom/common.h
>> @@ -23,6 +23,7 @@ struct qcom_cc_desc {
>> const struct regmap_config *config;
>> struct clk_regmap **clks;
>> size_t num_clks;
>> + u32 start_index;
>> const struct qcom_reset_map *resets;
>> size_t num_resets;
>> struct gdsc **gdscs;
>> --
>> 2.17.1
>>
>
>
> --
> With best wishes
>
>
>
> Dmitry
--
Thanks & Regards,
Taniya Das.
On 17/04/2023 07:40, Taniya Das wrote:
> Hi Dmitry,
>
> Thanks for the comments.
>
>
> On 3/3/2023 4:14 PM, Dmitry Baryshkov wrote:
>> On Fri, 3 Mar 2023 at 11:30, Taniya Das <[email protected]> wrote:
>>>
>>> Introduce start_index to handle invalid index error
>>> seen when there are two clock descriptors assigned
>>> to the same clock controller.
>>
>> Please provide details of the exact case that you are trying to solve
>> (this might go to the cover letter). I think the commit message is
>> slightly misleading here. Are you trying to add error messages or to
>> prevent them from showing up?
>>
>
> We are trying to avoid error messages from showing up.
>
>> I'm asking because error messages do not seem to correspond to patch
>> 2. You add start_index to make the kernel warn for the clock indices
>> less than LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC = 4, while quoted
>> messages show indices 5,6,7.
>>
>
> Right, we want the kernel to warn if the clock index is less than
> start_index,
This is arguable but logical. Usually we do not warn for absent clocks.
> along with that we also want to handle the case where
> num_rclks is uninitialized because of same clock descriptor being
> assigned to two clock controllers.
Hmm, but num_rclks is always initialized, isn't it? In the worst case it
will default to 0 meaning that
> Earlier Invalid index error was showing up for valid indices 5,6,7
> because of the simple if check(idx >= num_rclks), hence we enhanced the
> checks to handle the above case and compare the index to the start_index
> + num_rclks, instead of simply comparing it with num_clks.
This is not a part of the patch and it will be incorrect anyway, since
num_rclks = desc->num_clks = ARRAY_SIZE(some_cc_clocks).
Checking idx against `start_index + num_rclks` will allow one to get
clocks after the end of rclks array.
For lpass_audio_cc_sc7280_desc num_rclks should get the value of 16, as
the last entry in llpass_audio_cc_sc7280_clocks has index
LPASS_AUDIO_CC_RX_MCLK_CLK_SRC = 15.
My analysis might be completely wrong, but I can only assume that
somehow wrong clock controller got used. Could you please give it a try
with the
https://lore.kernel.org/linux-clk/[email protected]/
being applied?
>
>> Nit: please don't overwrap the commit message, the recommended line
>> width is about 72-77 chars.
>>
>
> Done.
>
>>>
>>> [ 3.600604] qcom_cc_clk_hw_get: invalid index 5
>>> [ 3.625251] qcom_cc_clk_hw_get: invalid index 6
>>> [ 3.648190] qcom_cc_clk_hw_get: invalid index 7
>>
>>>
>>> Fixes: 120c15528390 ("clk: qcom: Migrate to clk_hw based registration
>>> and OF APIs")
>>> Signed-off-by: Taniya Das <[email protected]>
>>> ---
>>> drivers/clk/qcom/common.c | 12 ++++++++----
>>> drivers/clk/qcom/common.h | 1 +
>>> 2 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>> index 75f09e6e057e..0e80535b61f2 100644
>>> --- a/drivers/clk/qcom/common.c
>>> +++ b/drivers/clk/qcom/common.c
>>> @@ -21,6 +21,7 @@ struct qcom_cc {
>>> struct qcom_reset_controller reset;
>>> struct clk_regmap **rclks;
>>> size_t num_rclks;
>>> + u32 rclks_start_index;
>>> };
>>>
>>> const
>>> @@ -226,12 +227,13 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct
>>> of_phandle_args *clkspec,
>>> struct qcom_cc *cc = data;
>>> unsigned int idx = clkspec->args[0];
>>>
>>> - if (idx >= cc->num_rclks) {
>>> + if (idx >= cc->rclks_start_index && idx < cc->num_rclks)
>>> + return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
>>> + else if (idx < cc->rclks_start_index && idx >= cc->num_rclks)
>>> pr_err("%s: invalid index %u\n", __func__, idx);
>>> - return ERR_PTR(-EINVAL);
>>> - }
>>>
>>> - return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
>>> + return ERR_PTR(-EINVAL);
>>> +
>>> }
>>>
>>> int qcom_cc_really_probe(struct platform_device *pdev,
>>> @@ -281,6 +283,8 @@ int qcom_cc_really_probe(struct platform_device
>>> *pdev,
>>>
>>> cc->rclks = rclks;
>>> cc->num_rclks = num_clks;
>>> + if (desc->start_index)
>>> + cc->rclks_start_index = desc->start_index;
>>>
>>> qcom_cc_drop_protected(dev, cc);
>>>
>>> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
>>> index 9c8f7b798d9f..924f36af55b3 100644
>>> --- a/drivers/clk/qcom/common.h
>>> +++ b/drivers/clk/qcom/common.h
>>> @@ -23,6 +23,7 @@ struct qcom_cc_desc {
>>> const struct regmap_config *config;
>>> struct clk_regmap **clks;
>>> size_t num_clks;
>>> + u32 start_index;
>>> const struct qcom_reset_map *resets;
>>> size_t num_resets;
>>> struct gdsc **gdscs;
>>> --
>>> 2.17.1
>>>
>>
>>
>> --
>> With best wishes
>>
>>
>>
>> Dmitry
>
--
With best wishes
Dmitry
Quoting Taniya Das (2023-04-16 21:40:48)
> Hi Dmitry,
>
> Thanks for the comments.
>
>
> On 3/3/2023 4:14 PM, Dmitry Baryshkov wrote:
> > On Fri, 3 Mar 2023 at 11:30, Taniya Das <[email protected]> wrote:
> >>
> >> Introduce start_index to handle invalid index error
> >> seen when there are two clock descriptors assigned
> >> to the same clock controller.
> >
> > Please provide details of the exact case that you are trying to solve
> > (this might go to the cover letter). I think the commit message is
> > slightly misleading here. Are you trying to add error messages or to
> > prevent them from showing up?
> >
>
> We are trying to avoid error messages from showing up.
>
> > I'm asking because error messages do not seem to correspond to patch
> > 2. You add start_index to make the kernel warn for the clock indices
> > less than LPASS_AUDIO_CC_CDIV_RX_MCLK_DIV_CLK_SRC = 4, while quoted
> > messages show indices 5,6,7.
> >
>
> Right, we want the kernel to warn if the clock index is less than
> start_index, along with that we also want to handle the case where
> num_rclks is uninitialized because of same clock descriptor being
> assigned to two clock controllers.
The start_index should be 0.