[PATCHv2 0/4] dw_dmac: few cleanups to the driver

[PATCHv2 0/4] dw_dmac: few cleanups to the driver

[Andy Shevchenko]

There are few cleanups to the driver which partialy acked and reviewed.

Since v1:
- patch 1/4 ("use helper macro module_platform_driver()") is removed from the
  series: it brings issues rather than cleans up
- patch 2/4 ("add module alias") is postponed

Andy Shevchenko (2):
  dw_dmac: change {dev_}printk() to corresponding macros
  dw_dmac: don't call platform_get_drvdata twice

Heikki Krogerus (2):
  dmaengine: dw_dmac: remove CLK dependency
  dmaengine: dw_dmac: amend description and indentation

 drivers/dma/Kconfig   |    1 -
 drivers/dma/dw_dmac.c |   25 +++++++++++--------------
 2 files changed, 11 insertions(+), 15 deletions(-)

-- 
1.7.10.4

[PATCHv2 1/4] dmaengine: dw_dmac: remove CLK dependency

[Andy Shevchenko]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

This driver could be used on different platforms. Thus, the HAVE_CLK dependency
is dropped away.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Felipe Balbi <balbi@ti.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/dma/Kconfig |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 677cd6e..df32537 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -83,7 +83,6 @@ config INTEL_IOP_ADMA
 
 config DW_DMAC
 	tristate "Synopsys DesignWare AHB DMA support"
-	depends on HAVE_CLK
 	select DMA_ENGINE
 	default y if CPU_AT32AP7000
 	help
-- 
1.7.10.4

[PATCHv2 2/4] dmaengine: dw_dmac: amend description and indentation

[Andy Shevchenko]

From: Heikki Krogerus <heikki.krogerus@linux.intel.com>

The driver will be used as a core part for various implementations of the
DesignWare DMA device. The patch adjusts description on the top and corrects
paragraph indentation in few places across the code.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/dma/dw_dmac.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index c4b0eb3..c27c125 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1,6 +1,5 @@
 /*
- * Driver for the Synopsys DesignWare DMA Controller (aka DMACA on
- * AVR32 systems.)
+ * Core driver for the Synopsys DesignWare DMA Controller
  *
  * Copyright (C) 2007-2008 Atmel Corporation
  * Copyright (C) 2010-2011 ST Microelectronics
@@ -9,6 +8,7 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -222,7 +222,6 @@ static inline void dwc_dump_chan_regs(struct dw_dma_chan *dwc)
 		channel_readl(dwc, CTL_LO));
 }
 
-
 static inline void dwc_chan_disable(struct dw_dma *dw, struct dw_dma_chan *dwc)
 {
 	channel_clear_bit(dw, CH_EN, dwc->mask);
@@ -1679,6 +1678,7 @@ static int dw_resume_noirq(struct device *dev)
 
 	clk_prepare_enable(dw->clk);
 	dma_writel(dw, CFG, DW_CFG_DMA_EN);
+
 	return 0;
 }
 
-- 
1.7.10.4

[PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index c27c125..60b172a 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -456,9 +456,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
 
 static inline void dwc_dump_lli(struct dw_dma_chan *dwc, struct dw_lli *lli)
 {
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
-			lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
+	dev_crit(chan2dev(&dwc->chan), "  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
+		 lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
 }
 
 static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
@@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
 	 * controller flagged an error instead of scribbling over
 	 * random memory locations.
 	 */
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"Bad descriptor submitted for DMA!\n");
-	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
-			"  cookie: %d\n", bad_desc->txd.cookie);
+	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
+	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);
 	dwc_dump_lli(dwc, &bad_desc->lli);
 	list_for_each_entry(child, &bad_desc->tx_list, desc_node)
 		dwc_dump_lli(dwc, &child->lli);
@@ -1625,8 +1622,8 @@ static int __devinit dw_probe(struct platform_device *pdev)
 
 	dma_writel(dw, CFG, DW_CFG_DMA_EN);
 
-	printk(KERN_INFO "%s: DesignWare DMA Controller, %d channels\n",
-			dev_name(&pdev->dev), nr_channels);
+	pr_info("%s: DesignWare DMA Controller, %d channels\n",
+		dev_name(&pdev->dev), nr_channels);
 
 	dma_async_device_register(&dw->dma);
 
-- 
1.7.10.4

[PATCHv2 4/4] dw_dmac: don't call platform_get_drvdata twice

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/dma/dw_dmac.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
index 60b172a..7d2b438 100644
--- a/drivers/dma/dw_dmac.c
+++ b/drivers/dma/dw_dmac.c
@@ -1653,7 +1653,7 @@ static void dw_shutdown(struct platform_device *pdev)
 {
 	struct dw_dma	*dw = platform_get_drvdata(pdev);
 
-	dw_dma_off(platform_get_drvdata(pdev));
+	dw_dma_off(dw);
 	clk_disable_unprepare(dw->clk);
 }
 
@@ -1662,7 +1662,7 @@ static int dw_suspend_noirq(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_dma	*dw = platform_get_drvdata(pdev);
 
-	dw_dma_off(platform_get_drvdata(pdev));
+	dw_dma_off(dw);
 	clk_disable_unprepare(dw->clk);
 
 	return 0;
-- 
1.7.10.4

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[viresh kumar]

On Wed, Oct 17, 2012 at 4:01 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw_dmac.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCHv2 4/4] dw_dmac: don't call platform_get_drvdata twice

[viresh kumar]

On Wed, Oct 17, 2012 at 4:01 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw_dmac.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

Hi,

On Wed, Oct 17, 2012 at 01:31:17PM +0300, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/dma/dw_dmac.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index c27c125..60b172a 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -456,9 +456,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  
>  static inline void dwc_dump_lli(struct dw_dma_chan *dwc, struct dw_lli *lli)
>  {
> -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> -			"  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
> -			lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
> +	dev_crit(chan2dev(&dwc->chan), "  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
> +		 lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);

is this really critical ? To me it looks more like a debugging message.

>  }
>  
>  static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> @@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  	 * controller flagged an error instead of scribbling over
>  	 * random memory locations.
>  	 */
> -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> -			"Bad descriptor submitted for DMA!\n");
> -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> -			"  cookie: %d\n", bad_desc->txd.cookie);
> +	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
> +	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);

now this is critical, indeed. I would suggest using dev_WARN_ONCE() so
that it's noisy enough to catch the failing user.

>  	dwc_dump_lli(dwc, &bad_desc->lli);
>  	list_for_each_entry(child, &bad_desc->tx_list, desc_node)
>  		dwc_dump_lli(dwc, &child->lli);
> @@ -1625,8 +1622,8 @@ static int __devinit dw_probe(struct platform_device *pdev)
>  
>  	dma_writel(dw, CFG, DW_CFG_DMA_EN);
>  
> -	printk(KERN_INFO "%s: DesignWare DMA Controller, %d channels\n",
> -			dev_name(&pdev->dev), nr_channels);
> +	pr_info("%s: DesignWare DMA Controller, %d channels\n",
> +		dev_name(&pdev->dev), nr_channels);

you have a struct device available in platform_device, please use
dev_info() or dev_dbg().

>  	dma_async_device_register(&dw->dma);
>  
> -- 
> 1.7.10.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 4/4] dw_dmac: don't call platform_get_drvdata twice

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1159 bytes --]

On Wed, Oct 17, 2012 at 01:31:18PM +0300, Andy Shevchenko wrote:

I would suggest adding a commit log, but if maintainer is fine without,
I'm fine too :-p

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

looks good:

Reviewed-by: Felipe Balbi <balbi@ti.com>

> ---
>  drivers/dma/dw_dmac.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> index 60b172a..7d2b438 100644
> --- a/drivers/dma/dw_dmac.c
> +++ b/drivers/dma/dw_dmac.c
> @@ -1653,7 +1653,7 @@ static void dw_shutdown(struct platform_device *pdev)
>  {
>  	struct dw_dma	*dw = platform_get_drvdata(pdev);
>  
> -	dw_dma_off(platform_get_drvdata(pdev));
> +	dw_dma_off(dw);
>  	clk_disable_unprepare(dw->clk);
>  }
>  
> @@ -1662,7 +1662,7 @@ static int dw_suspend_noirq(struct device *dev)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dw_dma	*dw = platform_get_drvdata(pdev);
>  
> -	dw_dma_off(platform_get_drvdata(pdev));
> +	dw_dma_off(dw);
>  	clk_disable_unprepare(dw->clk);
>  
>  	return 0;
> -- 
> 1.7.10.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Andy Shevchenko]

On Wed, 2012-10-17 at 16:09 +0300, Felipe Balbi wrote: 
> Hi,
> 
> On Wed, Oct 17, 2012 at 01:31:17PM +0300, Andy Shevchenko wrote:
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/dma/dw_dmac.c |   15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> > index c27c125..60b172a 100644
> > --- a/drivers/dma/dw_dmac.c
> > +++ b/drivers/dma/dw_dmac.c
> > @@ -456,9 +456,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >  
> >  static inline void dwc_dump_lli(struct dw_dma_chan *dwc, struct dw_lli *lli)
> >  {
> > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > -			"  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
> > -			lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
> > +	dev_crit(chan2dev(&dwc->chan), "  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
> > +		 lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
> 
> is this really critical ? To me it looks more like a debugging message.
This one is used in two cases, where one is marked as "error", another -
"critical"

> 
> >  }
> >  
> >  static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > @@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> >  	 * controller flagged an error instead of scribbling over
> >  	 * random memory locations.
> >  	 */
> > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > -			"Bad descriptor submitted for DMA!\n");
> > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > -			"  cookie: %d\n", bad_desc->txd.cookie);
> > +	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
> > +	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);
> 
> now this is critical, indeed. I would suggest using dev_WARN_ONCE() so
> that it's noisy enough to catch the failing user.
To this and upper comment, there is an explanation why it's critical. I
guess the WARN_ONCE is not good enough, for example if we have more than
one user making such noise.

> 
> >  	dwc_dump_lli(dwc, &bad_desc->lli);
> >  	list_for_each_entry(child, &bad_desc->tx_list, desc_node)
> >  		dwc_dump_lli(dwc, &child->lli);
> > @@ -1625,8 +1622,8 @@ static int __devinit dw_probe(struct platform_device *pdev)
> >  
> >  	dma_writel(dw, CFG, DW_CFG_DMA_EN);
> >  
> > -	printk(KERN_INFO "%s: DesignWare DMA Controller, %d channels\n",
> > -			dev_name(&pdev->dev), nr_channels);
> > +	pr_info("%s: DesignWare DMA Controller, %d channels\n",
> > +		dev_name(&pdev->dev), nr_channels);
> 
> you have a struct device available in platform_device, please use
> dev_info() or dev_dbg().
Agreed.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]

On Wed, Oct 17, 2012 at 04:36:58PM +0300, Andy Shevchenko wrote:
> On Wed, 2012-10-17 at 16:09 +0300, Felipe Balbi wrote: 
> > Hi,
> > 
> > On Wed, Oct 17, 2012 at 01:31:17PM +0300, Andy Shevchenko wrote:
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/dma/dw_dmac.c |   15 ++++++---------
> > >  1 file changed, 6 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> > > index c27c125..60b172a 100644
> > > --- a/drivers/dma/dw_dmac.c
> > > +++ b/drivers/dma/dw_dmac.c
> > > @@ -456,9 +456,8 @@ static void dwc_scan_descriptors(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > >  
> > >  static inline void dwc_dump_lli(struct dw_dma_chan *dwc, struct dw_lli *lli)
> > >  {
> > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > -			"  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
> > > -			lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
> > > +	dev_crit(chan2dev(&dwc->chan), "  desc: s0x%x d0x%x l0x%x c0x%x:%x\n",
> > > +		 lli->sar, lli->dar, lli->llp, lli->ctlhi, lli->ctllo);
> > 
> > is this really critical ? To me it looks more like a debugging message.
> This one is used in two cases, where one is marked as "error", another -
> "critical"

fair enough

> > >  static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > > @@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > >  	 * controller flagged an error instead of scribbling over
> > >  	 * random memory locations.
> > >  	 */
> > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > -			"Bad descriptor submitted for DMA!\n");
> > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > -			"  cookie: %d\n", bad_desc->txd.cookie);
> > > +	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
> > > +	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);
> > 
> > now this is critical, indeed. I would suggest using dev_WARN_ONCE() so
> > that it's noisy enough to catch the failing user.
> To this and upper comment, there is an explanation why it's critical. I
> guess the WARN_ONCE is not good enough, for example if we have more than
> one user making such noise.

then use dev_WARN()

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 4/4] dw_dmac: don't call platform_get_drvdata twice

[viresh kumar]

On Wed, Oct 17, 2012 at 6:44 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Oct 17, 2012 at 01:31:18PM +0300, Andy Shevchenko wrote:
>
> I would suggest adding a commit log, but if maintainer is fine without,
> I'm fine too :-p

Should be there. But i am bored of giving this review comment now :)

--
viresh

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Andy Shevchenko]

On Wed, 2012-10-17 at 16:53 +0300, Felipe Balbi wrote: 
> On Wed, Oct 17, 2012 at 04:36:58PM +0300, Andy Shevchenko wrote:
> > On Wed, 2012-10-17 at 16:09 +0300, Felipe Balbi wrote: 
> > > Hi,
> > > 
> > > > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

> > > > @@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > > >  	 * controller flagged an error instead of scribbling over
> > > >  	 * random memory locations.
> > > >  	 */
> > > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > > -			"Bad descriptor submitted for DMA!\n");
> > > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > > -			"  cookie: %d\n", bad_desc->txd.cookie);
> > > > +	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
> > > > +	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);
> > > 
> > > now this is critical, indeed. I would suggest using dev_WARN_ONCE() so
> > > that it's noisy enough to catch the failing user.
> > To this and upper comment, there is an explanation why it's critical. I
> > guess the WARN_ONCE is not good enough, for example if we have more than
> > one user making such noise.
> 
> then use dev_WARN()
I can't see how dev_WARN could be more useful here than the dev_crit. In
current message we have channel and cookie to link back to the user.
What does WARN add meaningful?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On Thu, Oct 18, 2012 at 11:15:31AM +0300, Andy Shevchenko wrote:
> On Wed, 2012-10-17 at 16:53 +0300, Felipe Balbi wrote: 
> > On Wed, Oct 17, 2012 at 04:36:58PM +0300, Andy Shevchenko wrote:
> > > On Wed, 2012-10-17 at 16:09 +0300, Felipe Balbi wrote: 
> > > > Hi,
> > > > 
> > > > > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> 
> > > > > @@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > > > >  	 * controller flagged an error instead of scribbling over
> > > > >  	 * random memory locations.
> > > > >  	 */
> > > > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > > > -			"Bad descriptor submitted for DMA!\n");
> > > > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > > > -			"  cookie: %d\n", bad_desc->txd.cookie);
> > > > > +	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
> > > > > +	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);
> > > > 
> > > > now this is critical, indeed. I would suggest using dev_WARN_ONCE() so
> > > > that it's noisy enough to catch the failing user.
> > > To this and upper comment, there is an explanation why it's critical. I
> > > guess the WARN_ONCE is not good enough, for example if we have more than
> > > one user making such noise.
> > 
> > then use dev_WARN()
> I can't see how dev_WARN could be more useful here than the dev_crit. In
> current message we have channel and cookie to link back to the user.
> What does WARN add meaningful?

a dump_stack()

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Andy Shevchenko]

On Thu, 2012-10-18 at 13:34 +0300, Felipe Balbi wrote: 
> On Thu, Oct 18, 2012 at 11:15:31AM +0300, Andy Shevchenko wrote:
> > On Wed, 2012-10-17 at 16:53 +0300, Felipe Balbi wrote: 
> > > On Wed, Oct 17, 2012 at 04:36:58PM +0300, Andy Shevchenko wrote:
> > > > On Wed, 2012-10-17 at 16:09 +0300, Felipe Balbi wrote: 

> > > > > > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

> > > > > > @@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > > > > >  	 * controller flagged an error instead of scribbling over
> > > > > >  	 * random memory locations.
> > > > > >  	 */
> > > > > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > > > > -			"Bad descriptor submitted for DMA!\n");
> > > > > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > > > > -			"  cookie: %d\n", bad_desc->txd.cookie);
> > > > > > +	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
> > > > > > +	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);
> > > > > 
> > > > > now this is critical, indeed. I would suggest using dev_WARN_ONCE() so
> > > > > that it's noisy enough to catch the failing user.
> > > > To this and upper comment, there is an explanation why it's critical. I
> > > > guess the WARN_ONCE is not good enough, for example if we have more than
> > > > one user making such noise.
> > > 
> > > then use dev_WARN()
> > I can't see how dev_WARN could be more useful here than the dev_crit. In
> > current message we have channel and cookie to link back to the user.
> > What does WARN add meaningful?
> 
> a dump_stack()

How could it be useful? The dwc_handle_error is called from a tasklet
that is called from scheduler asynchronously. The tasklet is queued in
interrupt handler.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

Hi,

On Thu, Oct 18, 2012 at 02:09:43PM +0300, Andy Shevchenko wrote:
> On Thu, 2012-10-18 at 13:34 +0300, Felipe Balbi wrote: 
> > On Thu, Oct 18, 2012 at 11:15:31AM +0300, Andy Shevchenko wrote:
> > > On Wed, 2012-10-17 at 16:53 +0300, Felipe Balbi wrote: 
> > > > On Wed, Oct 17, 2012 at 04:36:58PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, 2012-10-17 at 16:09 +0300, Felipe Balbi wrote: 
> 
> > > > > > > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
> 
> > > > > > > @@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
> > > > > > >  	 * controller flagged an error instead of scribbling over
> > > > > > >  	 * random memory locations.
> > > > > > >  	 */
> > > > > > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > > > > > -			"Bad descriptor submitted for DMA!\n");
> > > > > > > -	dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
> > > > > > > -			"  cookie: %d\n", bad_desc->txd.cookie);
> > > > > > > +	dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
> > > > > > > +	dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);
> > > > > > 
> > > > > > now this is critical, indeed. I would suggest using dev_WARN_ONCE() so
> > > > > > that it's noisy enough to catch the failing user.
> > > > > To this and upper comment, there is an explanation why it's critical. I
> > > > > guess the WARN_ONCE is not good enough, for example if we have more than
> > > > > one user making such noise.
> > > > 
> > > > then use dev_WARN()
> > > I can't see how dev_WARN could be more useful here than the dev_crit. In
> > > current message we have channel and cookie to link back to the user.
> > > What does WARN add meaningful?
> > 
> > a dump_stack()
> 
> How could it be useful? The dwc_handle_error is called from a tasklet
> that is called from scheduler asynchronously. The tasklet is queued in
> interrupt handler.

even if it's not useful, it's a lot more verbose and more likely to get
user's attention. If someone's passing broken DMA descriptor, it should
be a really big fat warning so we can get user reports early enoough.

Anyway, it's your call, I don't mind really.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 3/4] dw_dmac: change {dev_}printk() to corresponding macros

[Andy Shevchenko]

On Thu, Oct 18, 2012 at 4:59 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Oct 18, 2012 at 02:09:43PM +0300, Andy Shevchenko wrote:
>> On Thu, 2012-10-18 at 13:34 +0300, Felipe Balbi wrote:
>> > On Thu, Oct 18, 2012 at 11:15:31AM +0300, Andy Shevchenko wrote:
>> > > On Wed, 2012-10-17 at 16:53 +0300, Felipe Balbi wrote:
>> > > > On Wed, Oct 17, 2012 at 04:36:58PM +0300, Andy Shevchenko wrote:
>> > > > > On Wed, 2012-10-17 at 16:09 +0300, Felipe Balbi wrote:
>>
>> > > > > > > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c
>>
>> > > > > > > @@ -492,10 +491,8 @@ static void dwc_handle_error(struct dw_dma *dw, struct dw_dma_chan *dwc)
>> > > > > > >    * controller flagged an error instead of scribbling over
>> > > > > > >    * random memory locations.
>> > > > > > >    */
>> > > > > > > - dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
>> > > > > > > -                 "Bad descriptor submitted for DMA!\n");
>> > > > > > > - dev_printk(KERN_CRIT, chan2dev(&dwc->chan),
>> > > > > > > -                 "  cookie: %d\n", bad_desc->txd.cookie);
>> > > > > > > + dev_crit(chan2dev(&dwc->chan), "Bad descriptor submitted for DMA!\n");
>> > > > > > > + dev_crit(chan2dev(&dwc->chan), "  cookie: %d\n", bad_desc->txd.cookie);
>> > > > > >
>> > > > > > now this is critical, indeed. I would suggest using dev_WARN_ONCE() so
>> > > > > > that it's noisy enough to catch the failing user.
>> > > > > To this and upper comment, there is an explanation why it's critical. I
>> > > > > guess the WARN_ONCE is not good enough, for example if we have more than
>> > > > > one user making such noise.
>> > > >
>> > > > then use dev_WARN()
>> > > I can't see how dev_WARN could be more useful here than the dev_crit. In
>> > > current message we have channel and cookie to link back to the user.
>> > > What does WARN add meaningful?
>> >
>> > a dump_stack()
>>
>> How could it be useful? The dwc_handle_error is called from a tasklet
>> that is called from scheduler asynchronously. The tasklet is queued in
>> interrupt handler.
>
> even if it's not useful, it's a lot more verbose and more likely to get
> user's attention. If someone's passing broken DMA descriptor, it should
> be a really big fat warning so we can get user reports early enoough.

I will do separate patch for this. If Viresh and/or Vinoud is okay
with it, user will see fat message.

-- 
With Best Regards,
Andy Shevchenko