linux-kernel - Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event

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]

Message-ID: <CAMPhdO9sGWTomtyMSTe_jC2GfvtdLR=M-z3Uzpesm9efBzhSqA@mail.gmail.com>
Date:	Wed, 11 Jan 2012 14:37:08 +0800
From:	Eric Miao <eric.miao@...aro.org>
To:	Richard Zhao <richard.zhao@...escale.com>
Cc:	Shawn Guo <shawn.guo@...aro.org>, patches@...aro.org,
	vinod.koul@...el.com, linux-kernel@...r.kernel.org,
	kernel@...gutronix.de, dan.j.williams@...el.com,
	Richard Zhao <richard.zhao@...aro.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 6/6] dma/imx-sdma: check whether event_id0 < 32 when set event_mask

On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao
<richard.zhao@...escale.com> wrote:
> On Wed, Jan 11, 2012 at 08:53:23AM +0800, Richard Zhao wrote:
>> On Tue, Jan 10, 2012 at 11:38:39PM +0800, Shawn Guo wrote:
>> > On Tue, Jan 10, 2012 at 10:29:42PM +0800, Richard Zhao wrote:
>> > > On Tue, Jan 10, 2012 at 10:20:10PM +0800, Shawn Guo wrote:
>> > > > On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote:
>> > > > > Signed-off-by: Richard Zhao <richard.zhao@...aro.org>
>> > > > > ---
>> > > >
>> > > > I think it deserves a sensible commit message explaining why the patch
>> > > > is needed.
>> > > If event_id0 < 32, 1 << (sdmac->event_id0 - 32) is not zero.
>> This meant to make you clear about the patch. I'll add it in commit
>> message.
> unsigned int t = 31;
> printf("%d %08x\n", t, 1 << (t-32));
>
> I test above code on both x86 and arm. They shows different results.
> x86: 31 80000000
> arm: 31 00000000
>
> I think we still need this patch. we shoud not let it depends on gcc's
> behavior.
>
> Thanks
> Richard
>> > >
>> > My point is you may explain the exact problem you are seeing without
>> > this patch
>> The kernel don't have event_id < 32 case yet. I found the bug when
>> I review the code.
>> > and how the patch helps here.  In general, doing so would
>> > win a warm feeling from reviewers much more easily than leaving the
>> > commit message empty there.
>> I understand your point that comment as much as possible.
>>

Shawn,

I think Richard has made the issue quite clear here, the original
code does seem to have some problems even to me, who do not
understand the very details of the SDMA:

-                       sdmac->event_mask0 = 1 << sdmac->event_id0;
-                       sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32);

1. if sdmac->event_id0 >= 32, which will cause event_mask0 to be incorrect
2. if sdmac->event_id < 32, sdmac->event_mask1 will be incorrect

An alternate way is to use the standard bit operations:

struct sdma_channel {

	...

	unsigned long event_mask[2];

	...
};

	set_bit(sdmac->event_id0, event_mask);

Which avoids branch instructions and add a bit protection for the operation
to be atomic enough (event_mask0/1 won't be inconsistent).
--
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/