Quoting Nixiaoming (2019-04-24 08:31:23) > On Wed, Apr 24, 2019 at 6:52 AM Stephen Boyd <sb...@kernel.org> wrote: > >Quoting nixiaoming (2019-03-30 06:54:50) > >> In the function divider_recalc_rate() The judgment of the return value of > >> _get_div() indicates that the return value of _get_div() can be 0. > > > >When does _get_div() return 0? It can't be CLK_DIVIDER_MAX_AT_ZERO or > >CLK_DIVIDER_POWER_OF_TWO. I suppose it could be CLK_DIVIDER_ONE_BASED if > >CLK_DIVIDER_ALLOW_ZERO is set? Or just CLK_DIVIDER_ALLOW_ZERO is set? Or > >a table that has 0 in it for some odd reason. > > > divider_ro_round_rate_parent() is an exported function. > There is no parameter check or return value check before > and after calling _get_div(), which may result in a divide by zero error. > > Case1: The "flags" contains CLK_DIVIDER_ONE_BASED, and "val" is 0. > Case2: The "flags" does not contain CLK_DIVIDER_ONE_BASED, > CLK_DIVIDER_POWER_OF_TWO, CLK_DIVIDER_MAX_AT_ZERO, > "table" is NULL. "val" is 0xffffffff > In both cases _get_div() returns 0
Alright, so this is all theoretical and not happening in practice? Best to do nothing then I think. > > >> In order to avoid the divide-by-zero error, add check for return value > >> of _get_div() in the divider_ro_round_rate_parent() > >> > >> Signed-off-by: nixiaoming <nixiaom...@huawei.com> > >> Reviewed-by: Mukesh Ojha <mo...@codeaurora.org> > >> --- > >> drivers/clk/clk-divider.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > >> index e5a1726..f4bf7a4 100644 > >> --- a/drivers/clk/clk-divider.c > >> +++ b/drivers/clk/clk-divider.c > >> @@ -347,6 +347,9 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, > >> struct clk_hw *parent, > >> int div; > >> > >> div = _get_div(table, val, flags, width); > >> + /* avoid divide-by-zero */ > >> + if (!div) > >> + return -EINVAL; > > > >Can you please give more details on what's happening here? Who's the > >caller? What are the arguments being passed in? Shouldn't we check for > >CLK_DIVIDER_ALLOW_ZERO and then return prate as it comes in instead of > >returning an error? > > > I found that there may be a divide-by-zero error by code review, > for example: "flags" is CLK_DIVIDER_ONE_BASED and "val" is 0. > So simply add a return value check to avoid divide-by-zero > > thanks for your suggestion, > I will resend the patch later > refer to your advice and divider_recalc_rate() to add a check for > CLK_DIVIDER_ALLOW_ZERO > Ok. I'm actually wondering why divider_ro_round_rate_parent() doesn't use clk_divider_bestdiv() instead of _get_div(). I think we did this to make things simpler. We could make clk_divider_bestdiv() take a 'val' argument to start the divider loop (and make that 0 for the "normal" case) and then look at the 'flags' for CLK_DIVIDER_READ_ONLY to restrict the 'maxdiv' and 'i' arguments there to be whatever the val is passed in to be. It would require passing the val into _get_maxdiv() too, and only caring about the val when the read only flag is set. Then I think we would naturally get div-by-zero avoidance code and we could collapse the divider code paths substantially into clk_divider_bestdiv(). It would be even better if that patch could come after moving the clk-divider ops to use .determine_rate() instead of .round_rate() clk_ops. If we did that then we could enhance the divider code to handle rate request clamping.
