Re: [RFC] [PATCH] usb: gadget: fix dtd dma confusion
On Wed, May 09, 2012 at 02:02:22AM +0200, Christoph Fritz wrote:
>
> Hi to All,
>
> after a while of testing and searching I can come up with a patch
> that fixes g_ether <-> iperf for fsl_udc on ARM i.MX35.
>
> The sad part is that this kind of fix is already implemented for
> marvell mv_udc driver since last year but still _not_ in the ~15
> other *udc.c drivers.
>
> See here:
> daec765da767e4a6a30e1298862b28f2cae9a73f
> usb: gadget: mv_udc: fix dtd dma confusion
>
> So hereby I'm CC-ing all *udc.c maintainers to point out that this
> issue maybe affects you too.
>
>
> ---
> Subject: [PATCH] usb: gadget: fsl_udc: fix dtd dma confusion
>
> The controller will hang when doing testings with g_ether and iperf
> (tool for measuring maximum TCP and UDP bandwidth). This patch adds a
> delay to wait for controller to release dtd dma before freeing it.
>
> Signed-off-by: Christoph Fritz
> ---
> drivers/usb/gadget/fsl_udc_core.c |9 +
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/gadget/fsl_udc_core.c
> b/drivers/usb/gadget/fsl_udc_core.c
> index 55abfb6..fc86108 100644
> --- a/drivers/usb/gadget/fsl_udc_core.c
> +++ b/drivers/usb/gadget/fsl_udc_core.c
> @@ -1638,6 +1638,15 @@ static int process_ep_req(struct fsl_udc *udc, int
> pipe,
> status = REQ_UNCOMPLETE;
> return status;
> } else if (remaining_length) {
> + /* wait controller release dtd dma */
> + while ((curr_qh->curr_dtd_ptr == curr_td->td_dma)) {
> + if (curr_td->next_td_ptr ==
> + EP_QUEUE_HEAD_NEXT_TERMINATE) {
> + udelay(100);
> + break;
> + }
> + udelay(1);
> + }
> if (direction) {
> VDBG("Transmit dTD remaining length not zero");
> status = -EPROTO;
> --
> 1.7.2.5
ping - what about this patch? Will it be applied?
Thanks,
-- Christoph
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 03/17] powerpc: Add PFO support to the VIO bus
On Fri, 2012-05-11 at 07:58 +1000, Benjamin Herrenschmidt wrote: > > > > Having the timeout and retries in this function is the wrong thing to do. > > We'll resubmit this without the loop and the caller will be responsible for > > retrying the operations. > > > > I would rather have the caller cede the processor or alter thread > > priority where appropriate than doing that in this function. I don't > > think this should be done in this crypto driver. > > That sounds right indeed... as long as the upper crypto layer has a > concept of "try again later"... if it doesn't it will result in random > funny failures :-) Ping ? So I'm merging 1 to 5 (ie, up to and including the hwrng driver). I will still merge the rest if you send a fix for that in the next day or so but not much longer. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/irq: Fix another case of lazy IRQ state getting out of sync
On 2012年05月11日 10:12, Benjamin Herrenschmidt wrote:
> So we have another case of paca->irq_happened getting out of
> sync with the HW irq state. This can happen when a perfmon
> interrupt occurs while soft disabled, as it will return to a
> soft disabled but hard enabled context while leaving a stale
> PACA_IRQ_HARD_DIS flag set.
>
> This patch fixes it, and also adds a test for the condition
> of those flags being out of sync in arch_local_irq_restore()
> when CONFIG_TRACE_IRQFLAGS is enabled.
>
> This helps catching those gremlins faster (and so far I
> can't seem see any anymore, so that's good news).
This patch can work on my system.
Verified.
Thanks,
>
> Signed-off-by: Benjamin Herrenschmidt
> ---
>
> Please test ASAP as I need to send that to Linus today
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index f8a7a1a..293e283 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -588,23 +588,19 @@ _GLOBAL(ret_from_except_lite)
> fast_exc_return_irq:
> restore:
> /*
> - * This is the main kernel exit path, we first check if we
> - * have to change our interrupt state.
> + * This is the main kernel exit path. First we check if we
> + * are about to re-enable interrupts
>*/
> ld r5,SOFTE(r1)
> lbz r6,PACASOFTIRQEN(r13)
> - cmpwi cr1,r5,0
> - cmpwcr0,r5,r6
> - beq cr0,4f
> + cmpwi cr0,r5,0
> + beq restore_irq_off
>
> - /* We do, handle disable first, which is easy */
> - bne cr1,3f;
> - li r0,0
> - stb r0,PACASOFTIRQEN(r13);
> - TRACE_DISABLE_INTS
> - b 4f
> + /* We are enabling, were we already enabled ? Yes, just return */
> + cmpwi cr0,r6,1
> + beq cr0,do_restore
>
> -3: /*
> + /*
>* We are about to soft-enable interrupts (we are hard disabled
>* at this point). We check if there's anything that needs to
>* be replayed first.
> @@ -626,7 +622,7 @@ restore_no_replay:
> /*
>* Final return path. BookE is handled in a different file
>*/
> -4:
> +do_restore:
> #ifdef CONFIG_PPC_BOOK3E
> b .exception_return_book3e
> #else
> @@ -700,6 +696,25 @@ fast_exception_return:
> #endif /* CONFIG_PPC_BOOK3E */
>
> /*
> + * We are returning to a context with interrupts soft disabled.
> + *
> + * However, we may also about to hard enable, so we need to
> + * make sure that in this case, we also clear PACA_IRQ_HARD_DIS
> + * or that bit can get out of sync and bad things will happen
> + */
> +restore_irq_off:
> + ld r3,_MSR(r1)
> + lbz r7,PACAIRQHAPPENED(r13)
> + andi. r0,r3,MSR_EE
> + beq 1f
> + rlwinm r7,r7,0,~PACA_IRQ_HARD_DIS
> + stb r7,PACAIRQHAPPENED(r13)
> +1: li r0,0
> + stb r0,PACASOFTIRQEN(r13);
> + TRACE_DISABLE_INTS
> + b do_restore
> +
> + /*
>* Something did happen, check if a re-emit is needed
>* (this also clears paca->irq_happened)
>*/
> @@ -748,6 +763,9 @@ restore_check_irq_replay:
> #endif /* CONFIG_PPC_BOOK3E */
> 1: b .ret_from_except /* What else to do here ? */
>
> +
> +
> +3:
> do_work:
> #ifdef CONFIG_PREEMPT
> andi. r0,r3,MSR_PR/* Returning to user mode? */
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 5ec1b23..fe8cf8e 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -229,6 +229,19 @@ notrace void arch_local_irq_restore(unsigned long en)
>*/
> if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
> __hard_irq_disable();
> +#ifdef CONFIG_TRACE_IRQFLAG
> + else {
> + /*
> + * We should already be hard disabled here. We had bugs
> + * where that wasn't the case so let's dbl check it and
> + * warn if we are wrong. Only do that when IRQ tracing
> + * is enabled as mfmsr() can be costly.
> + */
> + if (WARN_ON(mfmsr() & MSR_EE))
> + __hard_irq_disable();
> + }
> +#endif /* CONFIG_TRACE_IRQFLAG */
> +
> set_soft_enabled(0);
>
> /*
>
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Patch][hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags
On Fri, 2012-05-11 at 14:13 +0530, K.Prasad wrote: > +#ifdef CONFIG_HAVE_HW_BREAKPOINT > + int ret, len = 0; > + struct thread_struct *thread = &(child->thread); > + struct perf_event *bp; > + struct perf_event_attr attr; > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ "ret" is unused in that function, causing a warning which breaks the build since we have -Werror. I'm fixing that locally but be more careful next time please. Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [RFC] [PATCH] usb: gadget: fix dtd dma confusion
> > See here:
> > daec765da767e4a6a30e1298862b28f2cae9a73f
> > usb: gadget: mv_udc: fix dtd dma confusion
> >
> > So hereby I'm CC-ing all *udc.c maintainers to point out that this
> > issue maybe affects you too.
> >
> >
> > ---
> > Subject: [PATCH] usb: gadget: fsl_udc: fix dtd dma confusion
> >
> > The controller will hang when doing testings with g_ether and iperf
> > (tool for measuring maximum TCP and UDP bandwidth). This patch adds a
> > delay to wait for controller to release dtd dma before freeing it.
> >
> > Signed-off-by: Christoph Fritz
> > ---
> > drivers/usb/gadget/fsl_udc_core.c |9 +
> > 1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/fsl_udc_core.c
> b/drivers/usb/gadget/fsl_udc_core.c
> > index 55abfb6..fc86108 100644
> > --- a/drivers/usb/gadget/fsl_udc_core.c
> > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > @@ -1638,6 +1638,15 @@ static int process_ep_req(struct fsl_udc *udc,
> int pipe,
> > status = REQ_UNCOMPLETE;
> > return status;
> > } else if (remaining_length) {
> > + /* wait controller release dtd dma */
> > + while ((curr_qh->curr_dtd_ptr == curr_td->td_dma)) {
> > + if (curr_td->next_td_ptr ==
> > + EP_QUEUE_HEAD_NEXT_TERMINATE) {
> > + udelay(100);
> > + break;
> > + }
> > + udelay(1);
> > + }
> > if (direction) {
> > VDBG("Transmit dTD remaining length not zero");
> > status = -EPROTO;
> > --
> > 1.7.2.5
>
> ping - what about this patch? Will it be applied?
>
Christoph, it is better use errata suggests workaround (postpone free dtd
memory),
and you can find related code at:
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/
drivers/usb/gadget/arcotg_udc.c?h=imx_3.0.15
The code is defined with POSTPONE_FREE_LAST_DTD
> Thanks,
> -- Christoph
>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC] [PATCH] usb: gadget: fix dtd dma confusion
On Mon, May 14, 2012 at 12:51:26AM +0200, Christoph Fritz wrote:
> On Wed, May 09, 2012 at 02:02:22AM +0200, Christoph Fritz wrote:
> >
> > Hi to All,
> >
> > after a while of testing and searching I can come up with a patch
> > that fixes g_ether <-> iperf for fsl_udc on ARM i.MX35.
> >
> > The sad part is that this kind of fix is already implemented for
> > marvell mv_udc driver since last year but still _not_ in the ~15
> > other *udc.c drivers.
> >
> > See here:
> > daec765da767e4a6a30e1298862b28f2cae9a73f
> > usb: gadget: mv_udc: fix dtd dma confusion
> >
> > So hereby I'm CC-ing all *udc.c maintainers to point out that this
> > issue maybe affects you too.
> >
> >
> > ---
> > Subject: [PATCH] usb: gadget: fsl_udc: fix dtd dma confusion
> >
> > The controller will hang when doing testings with g_ether and iperf
> > (tool for measuring maximum TCP and UDP bandwidth). This patch adds a
> > delay to wait for controller to release dtd dma before freeing it.
> >
> > Signed-off-by: Christoph Fritz
> > ---
> > drivers/usb/gadget/fsl_udc_core.c |9 +
> > 1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/fsl_udc_core.c
> > b/drivers/usb/gadget/fsl_udc_core.c
> > index 55abfb6..fc86108 100644
> > --- a/drivers/usb/gadget/fsl_udc_core.c
> > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > @@ -1638,6 +1638,15 @@ static int process_ep_req(struct fsl_udc *udc, int
> > pipe,
> > status = REQ_UNCOMPLETE;
> > return status;
> > } else if (remaining_length) {
> > + /* wait controller release dtd dma */
> > + while ((curr_qh->curr_dtd_ptr == curr_td->td_dma)) {
> > + if (curr_td->next_td_ptr ==
> > + EP_QUEUE_HEAD_NEXT_TERMINATE) {
> > + udelay(100);
> > + break;
> > + }
> > + udelay(1);
> > + }
> > if (direction) {
> > VDBG("Transmit dTD remaining length not zero");
> > status = -EPROTO;
> > --
> > 1.7.2.5
>
> ping - what about this patch? Will it be applied?
RFC patches are never applied, as you are asking for comments, not for
the patch to actually be applied...
Submit it properly if you wish it to be accepted.
thanks,
greg k-h
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
