Hi Guennadi-san,

2012/02/01 9:52, Guennadi Liakhovetski wrote:

Hi Shimoda-san

On Tue, 31 Jan 2012, Shimoda, Yoshihiro wrote:

[ snip ]

...

I investigaed the issue. I found out the renesas_usbhs driver may call tx_submit() in the callback()

Well, actually, this is not something, that shdma officially supports ATM. Originally it was prohibited by the API, but later a correction has been added to Documentation/dmaengine.txt, explicitly allowing this. So, good, if this works for you, but watch out for possible issues.

...

of the dma-simple finally. In this case, the value of power_up in the simple_tx_submit is 0, the start_xfer() is not called. And then, even if the ld_queue is not empty, the DMA doesn't start.

This is correct. As soon as all the transfers, currently queued in the shdma driver, has been processed and completed, to restart DMA you need to call dma_async_issue_pending().

...

So, if I add codes like the following in the dma-simple.c, the renesas_usbhs driver can work correctly without the driver changes. If you think the code is good, would you merge it to your code? ======= chan_irqt() ======= simple_chan_ld_cleanup(schan, false);

• spin_lock_irq(&schan->chan_lock);

• /* Next desc */

• simple_chan_xfer_ld_queue(schan);

• spin_unlock_irq(&schan->chan_lock);

  return IRQ_HANDLED;

==============

Therefore, no, this shouldn't be needed.

Thank you very much for your comment. I understood it. I will investigate the renesas_usbhs driver more.

Best regards, Yoshihiro Shimoda

Thanks Guennadi

...

Best regards, Yoshihiro Shimoda

...

Thanks Guennadi

...

For reference, if I changed the threaded IRQ to tasklet, the 2 environment worked correctly: ============== diff --git a/drivers/dma/dma-simple.c b/drivers/dma/dma-simple.c index 49d8f7d..65a5170 100644 --- a/drivers/dma/dma-simple.c +++ b/drivers/dma/dma-simple.c @@ -715,16 +715,18 @@ static irqreturn_t chan_irq(int irq, void *dev)

spin_lock(&schan->chan_lock);

• ret = ops->chan_irq(schan, irq) ? IRQ_WAKE_THREAD : IRQ_NONE;

• ret = ops->chan_irq(schan, irq) ? IRQ_HANDLED : IRQ_NONE;

• if (ret == IRQ_HANDLED)

• tasklet_schedule(&schan->tasklet);
  
  

  spin_unlock(&schan->chan_lock);

  return ret;

}

-static irqreturn_t chan_irqt(int irq, void *dev) +static void chan_irqt(unsigned long dev) {

• struct dma_simple_chan *schan = dev;

• struct dma_simple_chan *schan = (struct dma_simple_chan *)dev; const struct dma_simple_ops *ops = to_simple_dev(schan->dma_chan.device)->ops; struct dma_simple_desc *sdesc;

@@ -744,15 +746,13 @@ static irqreturn_t chan_irqt(int irq, void *dev) spin_unlock_irq(&schan->chan_lock);

simple_chan_ld_cleanup(schan, false);

• return IRQ_HANDLED;

}

int dma_simple_request_irq(struct dma_simple_chan *schan, int irq, unsigned long flags, const char *name) {

• int ret = request_threaded_irq(irq, chan_irq, chan_irqt,

• 		       flags, name, schan);
  

• int ret = request_irq(irq, chan_irq, flags, name, schan);

• tasklet_init(&schan->tasklet, chan_irqt, (unsigned long)schan);

  schan->irq = ret < 0 ? ret : irq;

diff --git a/include/linux/dma-simple.h b/include/linux/dma-simple.h index 5336674..beb1489 100644 --- a/include/linux/dma-simple.h +++ b/include/linux/dma-simple.h @@ -68,6 +68,7 @@ struct dma_simple_chan { int id; /* Raw id of this channel */ int irq; /* Channel IRQ */ enum dma_simple_pm_state pm_state;

• struct tasklet_struct tasklet;

};

/**

Best regards, Yoshihiro Shimoda

---

Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/

-- Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com EC No.

---

Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/

The current renesas_usbhs driver triggers

BUG: scheduling while atomic: ksoftirqd/0/3/0x00000102

with enabled CONFIG_DEBUG_ATOMIC_SLEEP, by submitting DMA transfers from an atomic (tasklet) context, which is not supported by the shdma dmaengine driver. Fix it by switching to a work. Also simplify some list manipulations.

Signed-off-by: Guennadi Liakhovetski g.liakhovetski@gmx.de ---

Shimoda-san, this is the problem, that you were observing. However, it exists with the present version of shdma just as well as with the new one - on top of the simple DMA library. I marked it an RFC because (1) I only lightly tested it with the gadget device on mackerel with the mass storage gadget, and (2) I am somewhat concerned about races. Currently the work function runs with no locking and there are no usual cancel_work_sync() points in the patch. However, it has also been like this before with the tasklet implementation, which is not much better, and it looks like there are no asynchronous operations on the same packets like timeouts. Only asynchronous events, that I can think about are things like unloading the driver or unplugging the cable, but these have been there before too. It would become worse on SMP, I think. Comments welcome.

diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c index 72339bd..4d739ec 100644 --- a/drivers/usb/renesas_usbhs/fifo.c +++ b/drivers/usb/renesas_usbhs/fifo.c @@ -75,8 +75,7 @@ void usbhs_pkt_push(struct usbhs_pipe *pipe, struct usbhs_pkt *pkt, pipe->handler = &usbhsf_null_handler; }

- list_del_init(&pkt->node); - list_add_tail(&pkt->node, &pipe->list); + list_move_tail(&pkt->node, &pipe->list);

/* * each pkt must hold own handler. @@ -106,7 +105,7 @@ static struct usbhs_pkt *__usbhsf_pkt_get(struct usbhs_pipe *pipe) if (list_empty(&pipe->list)) return NULL;

- return list_entry(pipe->list.next, struct usbhs_pkt, node); + return list_first_entry(&pipe->list, struct usbhs_pkt, node); }

struct usbhs_pkt *usbhs_pkt_pop(struct usbhs_pipe *pipe, struct usbhs_pkt *pkt) @@ -762,9 +761,9 @@ static int __usbhsf_dma_map_ctrl(struct usbhs_pkt *pkt, int map) }

static void usbhsf_dma_complete(void *arg); -static void usbhsf_dma_prepare_tasklet(unsigned long data) +static void xfer_work(struct work_struct *work) { - struct usbhs_pkt *pkt = (struct usbhs_pkt *)data; + struct usbhs_pkt *pkt = container_of(work, struct usbhs_pkt, work); struct usbhs_pipe *pipe = pkt->pipe; struct usbhs_fifo *fifo = usbhs_pipe_to_fifo(pipe); struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe); @@ -844,11 +843,8 @@ static int usbhsf_dma_prepare_push(struct usbhs_pkt *pkt, int *is_done)

pkt->trans = len;

- tasklet_init(&fifo->tasklet, - usbhsf_dma_prepare_tasklet, - (unsigned long)pkt); - - tasklet_schedule(&fifo->tasklet); + INIT_WORK(&pkt->work, xfer_work); + schedule_work(&pkt->work);

return 0;

@@ -938,11 +934,8 @@ static int usbhsf_dma_try_pop(struct usbhs_pkt *pkt, int *is_done)

pkt->trans = len;

- tasklet_init(&fifo->tasklet, - usbhsf_dma_prepare_tasklet, - (unsigned long)pkt); - - tasklet_schedule(&fifo->tasklet); + INIT_WORK(&pkt->work, xfer_work); + schedule_work(&pkt->work);

return 0;

diff --git a/drivers/usb/renesas_usbhs/fifo.h b/drivers/usb/renesas_usbhs/fifo.h index f68609c..c31731a 100644 --- a/drivers/usb/renesas_usbhs/fifo.h +++ b/drivers/usb/renesas_usbhs/fifo.h @@ -19,6 +19,7 @@

#include <linux/interrupt.h> #include <linux/sh_dma.h> +#include <linux/workqueue.h> #include <asm/dma.h> #include "pipe.h"

@@ -31,7 +32,6 @@ struct usbhs_fifo { u32 ctr; /* xFIFOCTR */

struct usbhs_pipe *pipe; - struct tasklet_struct tasklet;

struct dma_chan *tx_chan; struct dma_chan *rx_chan; @@ -53,6 +53,7 @@ struct usbhs_pkt { struct usbhs_pkt_handle *handler; void (*done)(struct usbhs_priv *priv, struct usbhs_pkt *pkt); + struct work_struct work; dma_addr_t dma; void *buf; int length;

Hi Felipe

On Sun, 5 Feb 2012, Felipe Balbi wrote:

Hi,

On Fri, Feb 03, 2012 at 04:43:20PM +0100, Guennadi Liakhovetski wrote:

...

The current renesas_usbhs driver triggers

BUG: scheduling while atomic: ksoftirqd/0/3/0x00000102

with enabled CONFIG_DEBUG_ATOMIC_SLEEP, by submitting DMA transfers from an atomic (tasklet) context, which is not supported by the shdma dmaengine driver. Fix it by switching to a work. Also simplify some list manipulations.

you are doing much more than what you say.

Are those two list macro changes what you refer to as "a lot?" ;-) You're right in principle, they are not directly related to the purpose of this patch, they are just something that occurred to me, while tracking down DMA packets. But yes, it can be extracted to a separate cosmetic patch...

Also, instead of using a workqueue, have you considered using threaded_irqs ?

(I didn't go over the driver again to see if it makes sense to use threaded_irqs in this case, but doesn't hurt asking)

_From a first glance these tasklets are not directly enough related to IRQs, so, doing that is either impossible, or would require a _much_ deeper change to the driver and _this_ would indeed be a much bigger change than just fixing the Oops.

Thanks Guennadi

...

Shimoda-san, this is the problem, that you were observing. However, it exists with the present version of shdma just as well as with the new one

• on top of the simple DMA library. I marked it an RFC because (1) I only

lightly tested it with the gadget device on mackerel with the mass storage gadget, and (2) I am somewhat concerned about races. Currently the work function runs with no locking and there are no usual cancel_work_sync() points in the patch. However, it has also been like this before with the tasklet implementation, which is not much better, and it looks like there are no asynchronous operations on the same packets like timeouts. Only asynchronous events, that I can think about are things like unloading the driver or unplugging the cable, but these have been there before too. It would become worse on SMP, I think. Comments welcome.

diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c index 72339bd..4d739ec 100644 --- a/drivers/usb/renesas_usbhs/fifo.c +++ b/drivers/usb/renesas_usbhs/fifo.c @@ -75,8 +75,7 @@ void usbhs_pkt_push(struct usbhs_pipe *pipe, struct usbhs_pkt *pkt, pipe->handler = &usbhsf_null_handler; }

• list_del_init(&pkt->node);
• list_add_tail(&pkt->node, &pipe->list);

• list_move_tail(&pkt->node, &pipe->list);

  /*

  • each pkt must hold own handler.

@@ -106,7 +105,7 @@ static struct usbhs_pkt *__usbhsf_pkt_get(struct usbhs_pipe *pipe) if (list_empty(&pipe->list)) return NULL;

• return list_entry(pipe->list.next, struct usbhs_pkt, node);

• return list_first_entry(&pipe->list, struct usbhs_pkt, node);

these two hunks are not part of $SUBJECT

-- balbi

--- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/