Shimoda-san
On Tue, 31 Jan 2012, Shimoda, Yoshihiro wrote:
> Hi Guennadi-san,
>
> 2012/01/27 17:48, Guennadi Liakhovetski wrote:
> > Hi Shimoda-san
> >
> > On Fri, 27 Jan 2012, Shimoda, Yoshihiro wrote:
> [ snip ]
> >>>
> >>> 1. switch from using a tasklet to threaded IRQ, which allowed to
> >>
> >> I tested this driver on 2 environment, but it could not work correctly.
> >> - renesas_usbhs driver with shdma driver on the mackerel board
> >> - renesas_usbhs driver with experimental SUDMAC driver
> >>
> >> In this case, should we modify the renesas_usbhs driver?
> >
> > Yes, you do. Please, see the respective version of the shdma patch. It
> > doesn't request channel IRQs any more directly, instead, it uses a
> > dma-simple wrapper function. Also operations change a bit.
>
> Thank you for your comment.
>
> I investigaed the issue. I found out the renesas_usbhs driver may
> call tx_submit() in the callback() of the dma-simple finally.
Sorry, in my first reply to this your email I misread the renesas_usbhs
for an issue with your new SUDMAC driver. Since this is not the case and
my patch seems to be causing a regression, I'll look at it.
Thanks
Guennadi
> 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.
>
>
> 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;
> ==============
>
> 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 <[email protected]>
> EC No.
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Hi Guennadi-san,
2012/02/03 7:19, Guennadi Liakhovetski wrote:
[ snip ]
>>
>> Thank you for your comment.
>>
>> I investigaed the issue. I found out the renesas_usbhs driver may
>> call tx_submit() in the callback() of the dma-simple finally.
>
> Sorry, in my first reply to this your email I misread the renesas_usbhs
> for an issue with your new SUDMAC driver. Since this is not the case and
> my patch seems to be causing a regression, I'll look at it.
Thank you for the comment.
I found out that if the "power_up" is 0 in the sample_tx_submit(),
the pm_state will be changed to "PENDING".
And then, even if the renesas_usbhs calls dma_async_issue_pending(),
the dma-simple driver doesn't call the simple_chan_xfer_ld_queue().
This is because the driver will call the simple_chan_xfer_ld_queue()
when the pm_state is "ESTABLISHED" only.
So I guess that we have to modify the simple_issue_pending().
Best regards,
Yoshihiro Shimoda
> Thanks
> Guennadi
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
--
Yoshihiro Shimoda <[email protected]>
On Fri, 3 Feb 2012, Shimoda, Yoshihiro wrote:
> Hi Guennadi-san,
>
> 2012/02/03 7:19, Guennadi Liakhovetski wrote:
> [ snip ]
> >>
> >> Thank you for your comment.
> >>
> >> I investigaed the issue. I found out the renesas_usbhs driver may
> >> call tx_submit() in the callback() of the dma-simple finally.
> >
> > Sorry, in my first reply to this your email I misread the renesas_usbhs
> > for an issue with your new SUDMAC driver. Since this is not the case and
> > my patch seems to be causing a regression, I'll look at it.
>
> Thank you for the comment.
>
> I found out that if the "power_up" is 0 in the sample_tx_submit(),
> the pm_state will be changed to "PENDING".
> And then, even if the renesas_usbhs calls dma_async_issue_pending(),
> the dma-simple driver doesn't call the simple_chan_xfer_ld_queue().
> This is because the driver will call the simple_chan_xfer_ld_queue()
> when the pm_state is "ESTABLISHED" only.
>
> So I guess that we have to modify the simple_issue_pending().
Ok, I looked at this code, there's now a comment in simple_tx_submit():
if (power_up) {
...
} else {
/*
* Tell .device_issue_pending() not to run the queue, interrupts
* will do it anyway
*/
schan->pm_state = DMA_SIMPLE_PM_PENDING;
}
And that's exactly what should be happening: an interrupt should trigger
the IRQ thread should call simple_chan_xfer_ld_queue(), so, looks like the
interrupt is not coming? It's also strange, why and how it worked without
dma-simple? I don't think I changed that with the transition...
Investigating.
Thanks
Guennadi
---
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 <[email protected]>
---
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,
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. 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)
> 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
Hi Guennadi-san,
2012/02/04 0:43, 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.
>
> Signed-off-by: Guennadi Liakhovetski <[email protected]>
> ---
>
> 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.
Thank you for the patch and comments.
I also tested this patch using the g_mass_storage and g_ether.
About your points, I think so, too.
So, I will write such a code.
Best regards,
Yoshihiro Shimoda
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/
Hi,
On Mon, Feb 06, 2012 at 11:11:45AM +0100, Guennadi Liakhovetski wrote:
> 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...
please do so ;-)
> > 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.
I see.. so please just split the list changes to separate patch and
resend.
--
balbi