linux-kernel - Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 27 Mar 2013 10:58:37 -0700
From:	Grant Grundler <grundler@...omium.org>
To:	Doug Anderson <dianders@...omium.org>
Cc:	Chris Ball <cjb@...top.org>, Will Newton <will.newton@...il.com>,
	Seungwon Jeon <tgih.jun@...sung.com>,
	Bing Zhao <bzhao@...vell.com>,
	Jaehoon Chung <jh80.chung@...sung.com>,
	Ashok Nagarajan <asnagarajan@...omium.org>,
	Olof Johansson <olof@...om.net>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

On Wed, Mar 27, 2013 at 8:07 AM, Doug Anderson <dianders@...omium.org> wrote:
> Grant,
>
> Thanks for posting!  See below...

thanks for reviewing/feedback! :)

> On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler <grundler@...omium.org> wrote:
>> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation.
>> But when debugging a related issue (http://crbug.com/221828) I found
>> the code unreadable.  This rewrite simplifies the computation and
>> explains each step.
>
> The fact that you mention a bug here is confusing.  I think this patch
> has nothing to do with fixing that bug and is just a readability
> patch.  Is that correct?

Correct.  "related" implies "not the same". But you are right - the
reference is causing confusion.

I will move the reference to the bug out of the commit log to below
the '---' area of the patch.

>  Please add to the description if so and
> maybe remove unrelated comment about the bug.

Thanks! Will do and repost later today.

...
>> +                       /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook"
>> +                        * Look for dwc_mobile_storage_db.pdf from Synopsys.
>> +                        * CLKDIV value 0 means divisor 1, val 1 -> 2, ...
>
> You are quoting exynos5250 docs here.  This driver is used for more
> than just exynos and so this could be confusing to users on other
> platforms.

I'm quoting Synopsys docs - that's the origin of this HW's ip.
You and I looked at exynos5250 docs originally and they say exactly
the same thing. But the section numbers are different.

>
>>                          */
>> -                       div += 1;
>> -
>> -               div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0;
>> +                       div /= 2;
>
> It does look like you're re-implementing DIV_ROUND_UP.

Yes, it does look like that but by breaking it out into simple steps
AND explaining why we do each step, the code becomes maintainable by
normal developers. The comments are key to *quickly* understanding the
code in this case.

> Maybe replace your "if" test and division with just a DIV_ROUND_UP?

No. I'd rather just drop the patch. This code can and should be stupid
simple. DIV_ROUND_UP just makes it harder to understand and impossible
to document as clearly.

cheers,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/