| Message ID | 1326178910-14044-6-git-send-email-richard.zhao@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Jan 10, 2012 at 03:01:50PM +0800, Richard Zhao wrote: > Signed-off-by: Richard Zhao <richard.zhao@linaro.org> > --- I think it deserves a sensible commit message explaining why the patch is needed. Regards, Shawn > drivers/dma/imx-sdma.c | 7 +++++-- > 1 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 22fd561..1d5b6ab 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -753,8 +753,11 @@ static int sdma_config_channel(struct sdma_channel *sdmac) > if (sdmac->event_id0 > 31) > sdmac->watermark_level |= 1 << 30; > } else { > - sdmac->event_mask0 = 1 << sdmac->event_id0; > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > + if (sdmac->event_id0 < 32) > + sdmac->event_mask0 = 1 << sdmac->event_id0; > + else > + sdmac->event_mask1 = > + 1 << (sdmac->event_id0 - 32); > } > /* Watermark Level */ > sdmac->watermark_level |= sdmac->watermark_level; > -- > 1.7.5.4 > >
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@linaro.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. Thanks Richard > > Regards, > Shawn > > > drivers/dma/imx-sdma.c | 7 +++++-- > > 1 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > > index 22fd561..1d5b6ab 100644 > > --- a/drivers/dma/imx-sdma.c > > +++ b/drivers/dma/imx-sdma.c > > @@ -753,8 +753,11 @@ static int sdma_config_channel(struct sdma_channel *sdmac) > > if (sdmac->event_id0 > 31) > > sdmac->watermark_level |= 1 << 30; > > } else { > > - sdmac->event_mask0 = 1 << sdmac->event_id0; > > - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); > > + if (sdmac->event_id0 < 32) > > + sdmac->event_mask0 = 1 << sdmac->event_id0; > > + else > > + sdmac->event_mask1 = > > + 1 << (sdmac->event_id0 - 32); > > } > > /* Watermark Level */ > > sdmac->watermark_level |= sdmac->watermark_level; > > -- > > 1.7.5.4 > > > >
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@linaro.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. > My point is you may explain the exact problem you are seeing without this patch 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.
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@linaro.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. > > > 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. Thanks Richard > > -- > Regards, > Shawn > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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@linaro.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. > > Thanks > Richard > > > > -- > > Regards, > > Shawn > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao <richard.zhao@freescale.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@linaro.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).
On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao > <richard.zhao@freescale.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@linaro.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). It's a good idea. Thanks Richard
On Wed, Jan 11, 2012 at 08:35:57PM +0800, Richard Zhao wrote: > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > > On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao > > <richard.zhao@freescale.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@linaro.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). > It's a good idea. I'm not sure whether I can always use bitops for every bit operation case, event it don't need atomic. bitops has locks to be atomic. Thanks Richard > Thanks > Richard >
On Wed, Jan 11, 2012 at 09:16:17PM +0800, Shawn Guo wrote: > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > > I think Richard has made the issue quite clear here, the original > > Yes, he has made it clear, but only after I asked for more comments, > not with the empty commit message. > > > 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 > > > My testing tells this is not the case. The event_mask0 will be 0 in > case 1) and event_mask1 will be 0 in case 2), which is quite what we > expect. And I do not believe you will see any functionality bug with > the existing code. Please see my mail mentioned "we shoud not let it depends on gcc's behavior." > > See, that's why we need verbose commit message to make the patch and > the problem it's trying to address very clear. I never doubt it. Thanks Richard > > Regards, > Shawn >
On Wed, Jan 11, 2012 at 08:53:23AM +0800, Richard Zhao wrote: > The kernel don't have event_id < 32 case yet. I found the bug when > I review the code. I tested audio on imx51, in which case SSI0 TX event number is 29. I do not see any problem without applying this patch.
On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > I think Richard has made the issue quite clear here, the original Yes, he has made it clear, but only after I asked for more comments, not with the empty commit message. > 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 > My testing tells this is not the case. The event_mask0 will be 0 in case 1) and event_mask1 will be 0 in case 2), which is quite what we expect. And I do not believe you will see any functionality bug with the existing code. See, that's why we need verbose commit message to make the patch and the problem it's trying to address very clear. Regards, Shawn
On Wed, Jan 11, 2012 at 09:09:11PM +0800, Richard Zhao wrote: > On Wed, Jan 11, 2012 at 09:16:17PM +0800, Shawn Guo wrote: > > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > > > I think Richard has made the issue quite clear here, the original > > > > Yes, he has made it clear, but only after I asked for more comments, > > not with the empty commit message. > > > > > 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 > > > > > My testing tells this is not the case. The event_mask0 will be 0 in > > case 1) and event_mask1 will be 0 in case 2), which is quite what we > > expect. And I do not believe you will see any functionality bug with > > the existing code. > Please see my mail mentioned "we shoud not let it depends on gcc's > behavior." In this case, I would rather believe that the author is smart enough to write the code intentionally based on his good understanding on behavior of arm-gcc.
On Wed, Jan 11, 2012 at 9:35 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Wed, Jan 11, 2012 at 09:09:11PM +0800, Richard Zhao wrote: >> On Wed, Jan 11, 2012 at 09:16:17PM +0800, Shawn Guo wrote: >> > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: >> > > I think Richard has made the issue quite clear here, the original >> > >> > Yes, he has made it clear, but only after I asked for more comments, >> > not with the empty commit message. >> > >> > > 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 >> > > >> > My testing tells this is not the case. The event_mask0 will be 0 in >> > case 1) and event_mask1 will be 0 in case 2), which is quite what we >> > expect. And I do not believe you will see any functionality bug with >> > the existing code. >> Please see my mail mentioned "we shoud not let it depends on gcc's >> behavior." > > In this case, I would rather believe that the author is smart enough > to write the code intentionally based on his good understanding on > behavior of arm-gcc. Either using the tricky gcc behavior which _will_ vary along version changes, or using "hard to understand code" is BAD idea. We don't write good code depending on author's "smartness", we write code so that it's understandable and maintainable.
On Wed, Jan 11, 2012 at 08:53:38PM +0800, Richard Zhao wrote: > On Wed, Jan 11, 2012 at 08:35:57PM +0800, Richard Zhao wrote: > > On Wed, Jan 11, 2012 at 02:37:08PM +0800, Eric Miao wrote: > > > On Wed, Jan 11, 2012 at 9:47 AM, Richard Zhao > > > <richard.zhao@freescale.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@linaro.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). > > It's a good idea. > I'm not sure whether I can always use bitops for every bit operation case, > event it don't need atomic. bitops has locks to be atomic. I'll use non-atomic bit ops, __set_bit etc. > > Thanks > Richard > > Thanks > > Richard > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 22fd561..1d5b6ab 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -753,8 +753,11 @@ static int sdma_config_channel(struct sdma_channel *sdmac) if (sdmac->event_id0 > 31) sdmac->watermark_level |= 1 << 30; } else { - sdmac->event_mask0 = 1 << sdmac->event_id0; - sdmac->event_mask1 = 1 << (sdmac->event_id0 - 32); + if (sdmac->event_id0 < 32) + sdmac->event_mask0 = 1 << sdmac->event_id0; + else + sdmac->event_mask1 = + 1 << (sdmac->event_id0 - 32); } /* Watermark Level */ sdmac->watermark_level |= sdmac->watermark_level;
Signed-off-by: Richard Zhao <richard.zhao@linaro.org> --- drivers/dma/imx-sdma.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)