All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: "jbrunet@baylibre.com" <jbrunet@baylibre.com>,
	"mojha@codeaurora.org" <mojha@codeaurora.org>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>,
	"sboyd@codeaurora.org" <sboyd@codeaurora.org>,
	"soren.brinkmann@xilinx.com" <soren.brinkmann@xilinx.com>,
	Nixiaoming <nixiaoming@huawei.com>
Cc: "linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3 1/2] clk:Fix divide-by-zero in divider_ro_round_rate_parent
Date: Wed, 24 Apr 2019 14:27:32 -0700	[thread overview]
Message-ID: <155614125279.15276.7728769195271989477@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <E490CD805F7529488761C40FD9D26EF12AB29F5F@dggemm507-mbx.china.huawei.com>

Quoting Nixiaoming (2019-04-24 08:31:23)
> On Wed, Apr 24, 2019 at 6:52 AM Stephen Boyd <sboyd@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 <nixiaoming@huawei.com>
> >> Reviewed-by: Mukesh Ojha <mojha@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.


      reply	other threads:[~2019-04-24 21:27 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-30 13:54 [PATCH v3 1/2] clk:Fix divide-by-zero in divider_ro_round_rate_parent nixiaoming
2019-04-23 22:51 ` Stephen Boyd
2019-04-24 15:31   ` Nixiaoming
2019-04-24 21:27     ` Stephen Boyd [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=155614125279.15276.7728769195271989477@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=jbrunet@baylibre.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mojha@codeaurora.org \
    --cc=mturquette@baylibre.com \
    --cc=nixiaoming@huawei.com \
    --cc=sboyd@codeaurora.org \
    --cc=soren.brinkmann@xilinx.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.