| Message ID | 1388167599-23525-3-git-send-email-valentine.barshak@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Valentine, On Friday 27 December 2013 22:06:39 Valentine Barshak wrote: > The clks member of the clk_onecell_data structure should > point to a valid clk array (no NULL entries allowed), > and the clk_num should be equal to the number > of elements in the clks array. > > The MSTP driver fails to satisfy the above conditions. > The clks array may contain NULL entries if not all > clock-indices are initialized in the device tree. > Thus, if the clock indices are interleaved we end up > with NULL pointers in-between. I don't think that's an issue in practice as long as no reference to a NULL clock exists in the device tree, but it should of course be fixed. > The other problem is the driver uses maximum clock index > as the number of clocks, which is incorrect (less than > the actual number of clocks by 1). Good catch. > Fix the first issue by pre-setting the whole clks array > with ERR_PTR(-ENOENT) pointers instead of zeros; and > use maximum clkidx + 1 as the number of clocks to fix > the other one. > > This should make of_clk_src_onecell_get() return the following: > * valid clk pointers for all clocks registered; > * ERR_PTR(-EINVAL) if (idx >= clk_data->clk_num); > * ERR_PTR(-ENOENT) if the clock at the selected index was not > initialized in the device tree (and was not registered). > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > --- > drivers/clk/shmobile/clk-mstp.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/shmobile/clk-mstp.c > b/drivers/clk/shmobile/clk-mstp.c index be7d017..14b91ad 100644 > --- a/drivers/clk/shmobile/clk-mstp.c > +++ b/drivers/clk/shmobile/clk-mstp.c > @@ -160,7 +160,7 @@ static void __init cpg_mstp_clocks_init(struct > device_node *np) unsigned int i; > > group = kzalloc(sizeof(*group), GFP_KERNEL); > - clks = kzalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); > + clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); > if (group == NULL || clks == NULL) { > kfree(group); > kfree(clks); > @@ -182,6 +182,10 @@ static void __init cpg_mstp_clocks_init(struct > device_node *np) } > > for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { > + clks[i] = ERR_PTR(-ENOENT); > + } No need for brackets here. With this fixed, Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { > const char *parent_name; > const char *name; > u32 clkidx; > @@ -208,7 +212,8 @@ static void __init cpg_mstp_clocks_init(struct > device_node *np) clks[clkidx] = cpg_mstp_clock_register(name, parent_name, > clkidx, group); > if (!IS_ERR(clks[clkidx])) { > - group->data.clk_num = max(group->data.clk_num, clkidx); > + group->data.clk_num = max(group->data.clk_num, > + clkidx + 1); > /* > * Register a clkdev to let board code retrieve the > * clock by name and register aliases for non-DT
Hi Valentine, On Saturday 28 December 2013 12:35:31 Laurent Pinchart wrote: > On Friday 27 December 2013 22:06:39 Valentine Barshak wrote: > > The clks member of the clk_onecell_data structure should > > point to a valid clk array (no NULL entries allowed), > > and the clk_num should be equal to the number > > of elements in the clks array. > > > > The MSTP driver fails to satisfy the above conditions. > > The clks array may contain NULL entries if not all > > clock-indices are initialized in the device tree. > > Thus, if the clock indices are interleaved we end up > > with NULL pointers in-between. > > I don't think that's an issue in practice as long as no reference to a NULL > clock exists in the device tree, but it should of course be fixed. > > > The other problem is the driver uses maximum clock index > > as the number of clocks, which is incorrect (less than > > the actual number of clocks by 1). > > Good catch. > > > Fix the first issue by pre-setting the whole clks array > > with ERR_PTR(-ENOENT) pointers instead of zeros; and > > use maximum clkidx + 1 as the number of clocks to fix > > the other one. > > > > This should make of_clk_src_onecell_get() return the following: > > * valid clk pointers for all clocks registered; > > * ERR_PTR(-EINVAL) if (idx >= clk_data->clk_num); > > * ERR_PTR(-ENOENT) if the clock at the selected index was not > > > > initialized in the device tree (and was not registered). > > > > Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > > --- > > > > drivers/clk/shmobile/clk-mstp.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/clk/shmobile/clk-mstp.c > > b/drivers/clk/shmobile/clk-mstp.c index be7d017..14b91ad 100644 > > --- a/drivers/clk/shmobile/clk-mstp.c > > +++ b/drivers/clk/shmobile/clk-mstp.c > > @@ -160,7 +160,7 @@ static void __init cpg_mstp_clocks_init(struct > > device_node *np) > > unsigned int i; > > > > group = kzalloc(sizeof(*group), GFP_KERNEL); > > - clks = kzalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); > > + clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); > > if (group == NULL || clks == NULL) { > > kfree(group); > > kfree(clks); > > @@ -182,6 +182,10 @@ static void __init cpg_mstp_clocks_init(struct > > device_node *np) } > > > > for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { > > + clks[i] = ERR_PTR(-ENOENT); > > + } > > No need for brackets here. > > With this fixed, > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Could you please resubmit the series with this fixed and the Reviewed-by line from Ben picked for patch 1/2, and ask Mike to apply it for v3.14 in the cover letter ? > > + > > + for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { > > const char *parent_name; > > const char *name; > > u32 clkidx; > > @@ -208,7 +212,8 @@ static void __init cpg_mstp_clocks_init(struct > > device_node *np) > > clks[clkidx] = cpg_mstp_clock_register(name, parent_name, > > clkidx, group); > > > > if (!IS_ERR(clks[clkidx])) { > > - group->data.clk_num = max(group->data.clk_num, clkidx); > > + group->data.clk_num = max(group->data.clk_num, > > + clkidx + 1); > > /* > > * Register a clkdev to let board code retrieve the > > * clock by name and register aliases for non-DT
On 01/09/2014 09:46 PM, Laurent Pinchart wrote: > Hi Valentine, > > On Saturday 28 December 2013 12:35:31 Laurent Pinchart wrote: >> On Friday 27 December 2013 22:06:39 Valentine Barshak wrote: >>> The clks member of the clk_onecell_data structure should >>> point to a valid clk array (no NULL entries allowed), >>> and the clk_num should be equal to the number >>> of elements in the clks array. >>> >>> The MSTP driver fails to satisfy the above conditions. >>> The clks array may contain NULL entries if not all >>> clock-indices are initialized in the device tree. >>> Thus, if the clock indices are interleaved we end up >>> with NULL pointers in-between. >> >> I don't think that's an issue in practice as long as no reference to a NULL >> clock exists in the device tree, but it should of course be fixed. >> >>> The other problem is the driver uses maximum clock index >>> as the number of clocks, which is incorrect (less than >>> the actual number of clocks by 1). >> >> Good catch. >> >>> Fix the first issue by pre-setting the whole clks array >>> with ERR_PTR(-ENOENT) pointers instead of zeros; and >>> use maximum clkidx + 1 as the number of clocks to fix >>> the other one. >>> >>> This should make of_clk_src_onecell_get() return the following: >>> * valid clk pointers for all clocks registered; >>> * ERR_PTR(-EINVAL) if (idx >= clk_data->clk_num); >>> * ERR_PTR(-ENOENT) if the clock at the selected index was not >>> >>> initialized in the device tree (and was not registered). >>> >>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> >>> --- >>> >>> drivers/clk/shmobile/clk-mstp.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/clk/shmobile/clk-mstp.c >>> b/drivers/clk/shmobile/clk-mstp.c index be7d017..14b91ad 100644 >>> --- a/drivers/clk/shmobile/clk-mstp.c >>> +++ b/drivers/clk/shmobile/clk-mstp.c >>> @@ -160,7 +160,7 @@ static void __init cpg_mstp_clocks_init(struct >>> device_node *np) >>> unsigned int i; >>> >>> group = kzalloc(sizeof(*group), GFP_KERNEL); >>> - clks = kzalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); >>> + clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); >>> if (group == NULL || clks == NULL) { >>> kfree(group); >>> kfree(clks); >>> @@ -182,6 +182,10 @@ static void __init cpg_mstp_clocks_init(struct >>> device_node *np) } >>> >>> for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { >>> + clks[i] = ERR_PTR(-ENOENT); >>> + } >> >> No need for brackets here. >> >> With this fixed, >> >> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Could you please resubmit the series with this fixed and the Reviewed-by line > from Ben picked for patch 1/2, and ask Mike to apply it for v3.14 in the cover > letter ? I did respin these here: http://marc.info/?l=linux-sh&m=138823255807611&w=2 Mike said he had taken them in clk-next, Thanks, Val. > >>> + >>> + for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { >>> const char *parent_name; >>> const char *name; >>> u32 clkidx; >>> @@ -208,7 +212,8 @@ static void __init cpg_mstp_clocks_init(struct >>> device_node *np) >>> clks[clkidx] = cpg_mstp_clock_register(name, parent_name, >>> clkidx, group); >>> >>> if (!IS_ERR(clks[clkidx])) { >>> - group->data.clk_num = max(group->data.clk_num, clkidx); >>> + group->data.clk_num = max(group->data.clk_num, >>> + clkidx + 1); >>> /* >>> * Register a clkdev to let board code retrieve the >>> * clock by name and register aliases for non-DT -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Valentine, On Thursday 09 January 2014 21:51:05 Valentine wrote: > On 01/09/2014 09:46 PM, Laurent Pinchart wrote: > > On Saturday 28 December 2013 12:35:31 Laurent Pinchart wrote: > >> On Friday 27 December 2013 22:06:39 Valentine Barshak wrote: > >>> The clks member of the clk_onecell_data structure should > >>> point to a valid clk array (no NULL entries allowed), > >>> and the clk_num should be equal to the number > >>> of elements in the clks array. > >>> > >>> The MSTP driver fails to satisfy the above conditions. > >>> The clks array may contain NULL entries if not all > >>> clock-indices are initialized in the device tree. > >>> Thus, if the clock indices are interleaved we end up > >>> with NULL pointers in-between. > >> > >> I don't think that's an issue in practice as long as no reference to a > >> NULL clock exists in the device tree, but it should of course be fixed. > >> > >>> The other problem is the driver uses maximum clock index > >>> as the number of clocks, which is incorrect (less than > >>> the actual number of clocks by 1). > >> > >> Good catch. > >> > >>> Fix the first issue by pre-setting the whole clks array > >>> with ERR_PTR(-ENOENT) pointers instead of zeros; and > >>> use maximum clkidx + 1 as the number of clocks to fix > >>> the other one. > >>> > >>> This should make of_clk_src_onecell_get() return the following: > >>> * valid clk pointers for all clocks registered; > >>> * ERR_PTR(-EINVAL) if (idx >= clk_data->clk_num); > >>> * ERR_PTR(-ENOENT) if the clock at the selected index was not > >>> > >>> initialized in the device tree (and was not registered). > >>> > >>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> > >>> --- > >>> > >>> drivers/clk/shmobile/clk-mstp.c | 9 +++++++-- > >>> 1 file changed, 7 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/clk/shmobile/clk-mstp.c > >>> b/drivers/clk/shmobile/clk-mstp.c index be7d017..14b91ad 100644 > >>> --- a/drivers/clk/shmobile/clk-mstp.c > >>> +++ b/drivers/clk/shmobile/clk-mstp.c > >>> @@ -160,7 +160,7 @@ static void __init cpg_mstp_clocks_init(struct > >>> device_node *np) > >>> > >>> unsigned int i; > >>> > >>> group = kzalloc(sizeof(*group), GFP_KERNEL); > >>> > >>> - clks = kzalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); > >>> + clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); > >>> > >>> if (group == NULL || clks == NULL) { > >>> > >>> kfree(group); > >>> kfree(clks); > >>> > >>> @@ -182,6 +182,10 @@ static void __init cpg_mstp_clocks_init(struct > >>> device_node *np) } > >>> > >>> for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { > >>> > >>> + clks[i] = ERR_PTR(-ENOENT); > >>> + } > >> > >> No need for brackets here. > >> > >> With this fixed, > >> > >> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Could you please resubmit the series with this fixed and the Reviewed-by > > line from Ben picked for patch 1/2, and ask Mike to apply it for v3.14 in > > the cover letter ? > > I did respin these here: > http://marc.info/?l=linux-sh&m=138823255807611&w=2 Oops, my bad, I've missed that. > Mike said he had taken them in clk-next, Great, thank you.
On 01/09/2014 09:53 PM, Laurent Pinchart wrote: > Hi Valentine, > > On Thursday 09 January 2014 21:51:05 Valentine wrote: >> On 01/09/2014 09:46 PM, Laurent Pinchart wrote: >>> On Saturday 28 December 2013 12:35:31 Laurent Pinchart wrote: >>>> On Friday 27 December 2013 22:06:39 Valentine Barshak wrote: >>>>> The clks member of the clk_onecell_data structure should >>>>> point to a valid clk array (no NULL entries allowed), >>>>> and the clk_num should be equal to the number >>>>> of elements in the clks array. >>>>> >>>>> The MSTP driver fails to satisfy the above conditions. >>>>> The clks array may contain NULL entries if not all >>>>> clock-indices are initialized in the device tree. >>>>> Thus, if the clock indices are interleaved we end up >>>>> with NULL pointers in-between. >>>> >>>> I don't think that's an issue in practice as long as no reference to a >>>> NULL clock exists in the device tree, but it should of course be fixed. >>>> >>>>> The other problem is the driver uses maximum clock index >>>>> as the number of clocks, which is incorrect (less than >>>>> the actual number of clocks by 1). >>>> >>>> Good catch. >>>> >>>>> Fix the first issue by pre-setting the whole clks array >>>>> with ERR_PTR(-ENOENT) pointers instead of zeros; and >>>>> use maximum clkidx + 1 as the number of clocks to fix >>>>> the other one. >>>>> >>>>> This should make of_clk_src_onecell_get() return the following: >>>>> * valid clk pointers for all clocks registered; >>>>> * ERR_PTR(-EINVAL) if (idx >= clk_data->clk_num); >>>>> * ERR_PTR(-ENOENT) if the clock at the selected index was not >>>>> >>>>> initialized in the device tree (and was not registered). >>>>> >>>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> >>>>> --- >>>>> >>>>> drivers/clk/shmobile/clk-mstp.c | 9 +++++++-- >>>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/clk/shmobile/clk-mstp.c >>>>> b/drivers/clk/shmobile/clk-mstp.c index be7d017..14b91ad 100644 >>>>> --- a/drivers/clk/shmobile/clk-mstp.c >>>>> +++ b/drivers/clk/shmobile/clk-mstp.c >>>>> @@ -160,7 +160,7 @@ static void __init cpg_mstp_clocks_init(struct >>>>> device_node *np) >>>>> >>>>> unsigned int i; >>>>> >>>>> group = kzalloc(sizeof(*group), GFP_KERNEL); >>>>> >>>>> - clks = kzalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); >>>>> + clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); >>>>> >>>>> if (group == NULL || clks == NULL) { >>>>> >>>>> kfree(group); >>>>> kfree(clks); >>>>> >>>>> @@ -182,6 +182,10 @@ static void __init cpg_mstp_clocks_init(struct >>>>> device_node *np) } >>>>> >>>>> for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { >>>>> >>>>> + clks[i] = ERR_PTR(-ENOENT); >>>>> + } >>>> >>>> No need for brackets here. >>>> >>>> With this fixed, >>>> >>>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>> Could you please resubmit the series with this fixed and the Reviewed-by >>> line from Ben picked for patch 1/2, and ask Mike to apply it for v3.14 in >>> the cover letter ? >> >> I did respin these here: >> http://marc.info/?l=linux-sh&m=138823255807611&w=2 > > Oops, my bad, I've missed that. No problem. > >> Mike said he had taken them in clk-next, > > Great, thank you. > I just don't seem to find it in https://git.linaro.org/people/mike.turquette/linux.git/shortlog/refs/heads/clk-next Is it the right repo? Thanks, Val. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c index be7d017..14b91ad 100644 --- a/drivers/clk/shmobile/clk-mstp.c +++ b/drivers/clk/shmobile/clk-mstp.c @@ -160,7 +160,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) unsigned int i; group = kzalloc(sizeof(*group), GFP_KERNEL); - clks = kzalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); + clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); if (group == NULL || clks == NULL) { kfree(group); kfree(clks); @@ -182,6 +182,10 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) } for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { + clks[i] = ERR_PTR(-ENOENT); + } + + for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { const char *parent_name; const char *name; u32 clkidx; @@ -208,7 +212,8 @@ static void __init cpg_mstp_clocks_init(struct device_node *np) clks[clkidx] = cpg_mstp_clock_register(name, parent_name, clkidx, group); if (!IS_ERR(clks[clkidx])) { - group->data.clk_num = max(group->data.clk_num, clkidx); + group->data.clk_num = max(group->data.clk_num, + clkidx + 1); /* * Register a clkdev to let board code retrieve the * clock by name and register aliases for non-DT
The clks member of the clk_onecell_data structure should point to a valid clk array (no NULL entries allowed), and the clk_num should be equal to the number of elements in the clks array. The MSTP driver fails to satisfy the above conditions. The clks array may contain NULL entries if not all clock-indices are initialized in the device tree. Thus, if the clock indices are interleaved we end up with NULL pointers in-between. The other problem is the driver uses maximum clock index as the number of clocks, which is incorrect (less than the actual number of clocks by 1). Fix the first issue by pre-setting the whole clks array with ERR_PTR(-ENOENT) pointers instead of zeros; and use maximum clkidx + 1 as the number of clocks to fix the other one. This should make of_clk_src_onecell_get() return the following: * valid clk pointers for all clocks registered; * ERR_PTR(-EINVAL) if (idx >= clk_data->clk_num); * ERR_PTR(-ENOENT) if the clock at the selected index was not initialized in the device tree (and was not registered). Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com> --- drivers/clk/shmobile/clk-mstp.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)