Re: [PATCH 4/9] USB: Convert from tasklet to BH workqueue
On Wed, Mar 27, 2024 at 04:03:09PM +, Allen Pais wrote:
> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
>
> This patch converts drivers/infiniband/* from tasklet to BH workqueue.
>
> Based on the work done by Tejun Heo
> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
>
> Signed-off-by: Allen Pais
> ---
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index c0e005670d67..88d8e1c366cd 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1662,10 +1663,9 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
> usb_put_urb(urb);
> }
>
> -static void usb_giveback_urb_bh(struct work_struct *work)
> +static void usb_giveback_urb_bh(struct work_struct *t)
> {
> - struct giveback_urb_bh *bh =
> - container_of(work, struct giveback_urb_bh, bh);
> + struct giveback_urb_bh *bh = from_work(bh, t, bh);
> struct list_head local_list;
>
> spin_lock_irq(>lock);
Is there any reason for this apparently pointless change of a local
variable's name?
Alan Stern
Re: [PATCH v3] drivers/usb/host/ehci-fsl: Fix interrupt setup in host mode.
On Tue, Jul 05, 2022 at 10:29:53AM -0600, Rob Herring wrote: > On Sat, Jul 2, 2022 at 3:04 PM Darren Stevens wrote: > > > > In patch a1a2b7125e10 (Drop static setup of IRQ resource from DT > > core) we stopped platform_get_resource() from returning the IRQ, as all > > drivers were supposed to have switched to platform_get_irq() > > Unfortunately the Freescale EHCI driver in host mode got missed. Fix > > it. > > > > Fixes: a1a2b7125e10 (Drop static setup of IRQ resource from DT core) > > Reported-by: Christian Zigotzky > > Suggested-by: Rob Herring > > Signed-off-by: Darren Stevens > > --- > > v3 - Corrected resource allocation in fsl-mph-dr-of.c > > > > v2 - Fixed coding style, removed a couple of unneeded initializations, > > cc'd Layerscape maintainers. > > > > Tested on AmigaOne X5000/20 and X5000/40 Contains code by Rob Herring > > (in fsl-mph-dr-of.c) > > Thanks for fixing. > > Acked-by: Rob Herring Okay for me too. Acked-by: Alan Stern
Re: [PATCH v0 42/42] notifier: Return an error when callback is already registered
On Mon, Nov 08, 2021 at 05:21:45PM +0100, Borislav Petkov wrote: > On Mon, Nov 08, 2021 at 05:12:16PM +0100, Geert Uytterhoeven wrote: > > Returning void is the other extreme ;-) > > > > There are 3 levels (ignoring BUG_ON()/panic () inside the callee): > > 1. Return void: no one can check success or failure, > > 2. Return an error code: up to the caller to decide, > > 3. Return a __must_check error code: every caller must check. > > > > I'm in favor of 2, as there are several places where it cannot fail. > > Makes sense to me. I'll do that in the next iteration. Is there really any reason for returning an error code? For example, is it anticipated that at some point in the future these registration calls might fail? Currently, the only reason for failing to register a notifier callback is because the callback is already registered. In a sense this isn't even an actual failure -- after the registration returns the callback _will_ still be registered. So if the call can never really fail, why bother with a return code? Especially since the caller can't do anything with such a code value. Given the current state of affairs, I vote in favor of 1 (plus a WARN or something similar to generate a stack dump in the callee, since double registration really is a bug). Alan Stern
Re: [PATCH v0 00/42] notifiers: Return an error when callback is already registered
On Mon, Nov 08, 2021 at 11:19:24AM +0100, Borislav Petkov wrote: > From: Borislav Petkov > > Hi all, > > this is a huge patchset for something which is really trivial - it > changes the notifier registration routines to return an error value > if a notifier callback is already present on the respective list of > callbacks. For more details scroll to the last patch. > > Everything before it is converting the callers to check the return value > of the registration routines and issue a warning, instead of the WARN() > notifier_chain_register() does now. What reason is there for moving the check into the callers? It seems like pointless churn. Why not add the error return code, change the WARN to pr_warn, and leave the callers as they are? Wouldn't that end up having exactly the same effect? For that matter, what sort of remedial action can a caller take if the return code is -EEXIST? Is there any point in forcing callers to check the return code if they can't do anything about it? > Before the last patch has been applied, though, that checking is a > NOP which would make the application of those patches trivial - every > maintainer can pick a patch at her/his discretion - only the last one > enables the build warnings and that one will be queued only after the > preceding patches have all been merged so that there are no build > warnings. Why should there be _any_ build warnings? The real problem occurs when a notifier callback is added twice, not when a caller fails to check the return code. Double-registration is not the sort of thing that can be detected at build time. Alan Stern > Due to the sheer volume of the patches, I have addressed the respective > patch and the last one, which enables the warning, with addressees for > each maintained area so as not to spam people unnecessarily. > > If people prefer I carry some through tip, instead, I'll gladly do so - > your call. > > And, if you think the warning messages need to be more precise, feel > free to adjust them before committing. > > Thanks!
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
On Fri, Jul 17, 2020 at 12:22:49PM -0400, Mathieu Desnoyers wrote: > - On Jul 17, 2020, at 12:11 PM, Alan Stern st...@rowland.harvard.edu > wrote: > > >> > I agree with Nick: A memory barrier is needed somewhere between the > >> > assignment at 6 and the return to user mode at 8. Otherwise you end up > >> > with the Store Buffer pattern having a memory barrier on only one side, > >> > and it is well known that this arrangement does not guarantee any > >> > ordering. > >> > >> Yes, I see this now. I'm still trying to wrap my head around why the memory > >> barrier at the end of membarrier() needs to be paired with a scheduler > >> barrier though. > > > > The memory barrier at the end of membarrier() on CPU0 is necessary in > > order to enforce the guarantee that any writes occurring on CPU1 before > > the membarrier() is executed will be visible to any code executing on > > CPU0 after the membarrier(). Ignoring the kthread issue, we can have: > > > > CPU0CPU1 > > x = 1 > > barrier() > > y = 1 > > r2 = y > > membarrier(): > > a: smp_mb() > > b: send IPI IPI-induced mb > > c: smp_mb() > > r1 = x > > > > The writes to x and y are unordered by the hardware, so it's possible to > > have r2 = 1 even though the write to x doesn't execute until b. If the > > memory barrier at c is omitted then "r1 = x" can be reordered before b > > (although not before a), so we get r1 = 0. This violates the guarantee > > that membarrier() is supposed to provide. > > > > The timing of the memory barrier at c has to ensure that it executes > > after the IPI-induced memory barrier on CPU1. If it happened before > > then we could still end up with r1 = 0. That's why the pairing matters. > > > > I hope this helps your head get properly wrapped. :-) > > It does help a bit! ;-) > > This explains this part of the comment near the smp_mb at the end of > membarrier: > > * Memory barrier on the caller thread _after_ we finished > * waiting for the last IPI. [...] > > However, it does not explain why it needs to be paired with a barrier in the > scheduler, clearly for the case where the IPI is skipped. I wonder whether > this part > of the comment is factually correct: > > * [...] Matches memory barriers around rq->curr modification in > scheduler. The reasoning is pretty much the same as above: CPU0CPU1 x = 1 barrier() y = 1 r2 = y membarrier(): a: smp_mb() switch to kthread (includes mb) b: read rq->curr == kthread switch to user (includes mb) c: smp_mb() r1 = x Once again, it is possible that x = 1 doesn't become visible to CPU0 until shortly before b. But if c is omitted then "r1 = x" can be reordered before b (to any time after a), so we can have r1 = 0. Here the timing requirement is that c executes after the first memory barrier on CPU1 -- which is one of the ones around the rq->curr modification. (In fact, in this scenario CPU1's switch back to the user process is irrelevant.) Alan Stern
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
> > I agree with Nick: A memory barrier is needed somewhere between the > > assignment at 6 and the return to user mode at 8. Otherwise you end up > > with the Store Buffer pattern having a memory barrier on only one side, > > and it is well known that this arrangement does not guarantee any > > ordering. > > Yes, I see this now. I'm still trying to wrap my head around why the memory > barrier at the end of membarrier() needs to be paired with a scheduler > barrier though. The memory barrier at the end of membarrier() on CPU0 is necessary in order to enforce the guarantee that any writes occurring on CPU1 before the membarrier() is executed will be visible to any code executing on CPU0 after the membarrier(). Ignoring the kthread issue, we can have: CPU0CPU1 x = 1 barrier() y = 1 r2 = y membarrier(): a: smp_mb() b: send IPI IPI-induced mb c: smp_mb() r1 = x The writes to x and y are unordered by the hardware, so it's possible to have r2 = 1 even though the write to x doesn't execute until b. If the memory barrier at c is omitted then "r1 = x" can be reordered before b (although not before a), so we get r1 = 0. This violates the guarantee that membarrier() is supposed to provide. The timing of the memory barrier at c has to ensure that it executes after the IPI-induced memory barrier on CPU1. If it happened before then we could still end up with r1 = 0. That's why the pairing matters. I hope this helps your head get properly wrapped. :-) Alan Stern
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
On Fri, Jul 17, 2020 at 09:39:25AM -0400, Mathieu Desnoyers wrote: > - On Jul 16, 2020, at 5:24 PM, Alan Stern st...@rowland.harvard.edu wrote: > > > On Thu, Jul 16, 2020 at 02:58:41PM -0400, Mathieu Desnoyers wrote: > >> - On Jul 16, 2020, at 12:03 PM, Mathieu Desnoyers > >> mathieu.desnoy...@efficios.com wrote: > >> > >> > - On Jul 16, 2020, at 11:46 AM, Mathieu Desnoyers > >> > mathieu.desnoy...@efficios.com wrote: > >> > > >> >> - On Jul 16, 2020, at 12:42 AM, Nicholas Piggin npig...@gmail.com > >> >> wrote: > >> >>> I should be more complete here, especially since I was complaining > >> >>> about unclear barrier comment :) > >> >>> > >> >>> > >> >>> CPU0 CPU1 > >> >>> a. user stuff1. user stuff > >> >>> b. membarrier() 2. enter kernel > >> >>> c. smp_mb() 3. smp_mb__after_spinlock(); // in __schedule > >> >>> d. read rq->curr 4. rq->curr switched to kthread > >> >>> e. is kthread, skip IPI 5. switch_to kthread > >> >>> f. return to user6. rq->curr switched to user thread > >> >>> g. user stuff7. switch_to user thread > >> >>> 8. exit kernel > >> >>> 9. more user stuff ... > >> Requiring a memory barrier between update of rq->curr (back to current > >> process's > >> thread) and following user-space memory accesses does not seem to guarantee > >> anything more than what the initial barrier at the beginning of __schedule > >> already > >> provides, because the guarantees are only about accesses to user-space > >> memory. ... > > Is it correct to say that the switch_to operations in 5 and 7 include > > memory barriers? If they do, then skipping the IPI should be okay. > > > > The reason is as follows: The guarantee you need to enforce is that > > anything written by CPU0 before the membarrier() will be visible to CPU1 > > after it returns to user mode. Let's say that a writes to X and 9 > > reads from X. > > > > Then we have an instance of the Store Buffer pattern: > > > > CPU0CPU1 > > a. Write X 6. Write rq->curr for user thread > > c. smp_mb() 7. switch_to memory barrier > > d. Read rq->curr9. Read X > > > > In this pattern, the memory barriers make it impossible for both reads > > to miss their corresponding writes. Since d does fail to read 6 (it > > sees the earlier value stored by 4), 9 must read a. > > > > The other guarantee you need is that g on CPU0 will observe anything > > written by CPU1 in 1. This is easier to see, using the fact that 3 is a > > memory barrier and d reads from 4. > > Right, and Nick's reply involving pairs of loads/stores on each side > clarifies the situation even further. The key part of my reply was the question: "Is it correct to say that the switch_to operations in 5 and 7 include memory barriers?" From the text quoted above and from Nick's reply, it seems clear that they do not. I agree with Nick: A memory barrier is needed somewhere between the assignment at 6 and the return to user mode at 8. Otherwise you end up with the Store Buffer pattern having a memory barrier on only one side, and it is well known that this arrangement does not guarantee any ordering. One thing I don't understand about all this: Any context switch has to include a memory barrier somewhere, but both you and Nick seem to be saying that steps 6 and 7 don't include (or don't need) any memory barriers. What am I missing? Alan Stern
Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
On Thu, Jul 16, 2020 at 02:58:41PM -0400, Mathieu Desnoyers wrote:
> - On Jul 16, 2020, at 12:03 PM, Mathieu Desnoyers
> mathieu.desnoy...@efficios.com wrote:
>
> > - On Jul 16, 2020, at 11:46 AM, Mathieu Desnoyers
> > mathieu.desnoy...@efficios.com wrote:
> >
> >> - On Jul 16, 2020, at 12:42 AM, Nicholas Piggin npig...@gmail.com
> >> wrote:
> >>> I should be more complete here, especially since I was complaining
> >>> about unclear barrier comment :)
> >>>
> >>>
> >>> CPU0 CPU1
> >>> a. user stuff1. user stuff
> >>> b. membarrier() 2. enter kernel
> >>> c. smp_mb() 3. smp_mb__after_spinlock(); // in __schedule
> >>> d. read rq->curr 4. rq->curr switched to kthread
> >>> e. is kthread, skip IPI 5. switch_to kthread
> >>> f. return to user6. rq->curr switched to user thread
> >>> g. user stuff7. switch_to user thread
> >>> 8. exit kernel
> >>> 9. more user stuff
> >>>
> >>> What you're really ordering is a, g vs 1, 9 right?
> >>>
> >>> In other words, 9 must see a if it sees g, g must see 1 if it saw 9,
> >>> etc.
> >>>
> >>> Userspace does not care where the barriers are exactly or what kernel
> >>> memory accesses might be being ordered by them, so long as there is a
> >>> mb somewhere between a and g, and 1 and 9. Right?
> >>
> >> This is correct.
> >
> > Actually, sorry, the above is not quite right. It's been a while
> > since I looked into the details of membarrier.
> >
> > The smp_mb() at the beginning of membarrier() needs to be paired with a
> > smp_mb() _after_ rq->curr is switched back to the user thread, so the
> > memory barrier is between store to rq->curr and following user-space
> > accesses.
> >
> > The smp_mb() at the end of membarrier() needs to be paired with the
> > smp_mb__after_spinlock() at the beginning of schedule, which is
> > between accesses to userspace memory and switching rq->curr to kthread.
> >
> > As to *why* this ordering is needed, I'd have to dig through additional
> > scenarios from https://lwn.net/Articles/573436/. Or maybe Paul remembers ?
>
> Thinking further about this, I'm beginning to consider that maybe we have been
> overly cautious by requiring memory barriers before and after store to
> rq->curr.
>
> If CPU0 observes a CPU1's rq->curr->mm which differs from its own process
> (current)
> while running the membarrier system call, it necessarily means that CPU1 had
> to issue smp_mb__after_spinlock when entering the scheduler, between any
> user-space
> loads/stores and update of rq->curr.
>
> Requiring a memory barrier between update of rq->curr (back to current
> process's
> thread) and following user-space memory accesses does not seem to guarantee
> anything more than what the initial barrier at the beginning of __schedule
> already
> provides, because the guarantees are only about accesses to user-space memory.
>
> Therefore, with the memory barrier at the beginning of __schedule, just
> observing that
> CPU1's rq->curr differs from current should guarantee that a memory barrier
> was issued
> between any sequentially consistent instructions belonging to the current
> process on
> CPU1.
>
> Or am I missing/misremembering an important point here ?
Is it correct to say that the switch_to operations in 5 and 7 include
memory barriers? If they do, then skipping the IPI should be okay.
The reason is as follows: The guarantee you need to enforce is that
anything written by CPU0 before the membarrier() will be visible to CPU1
after it returns to user mode. Let's say that a writes to X and 9
reads from X.
Then we have an instance of the Store Buffer pattern:
CPU0CPU1
a. Write X 6. Write rq->curr for user thread
c. smp_mb() 7. switch_to memory barrier
d. Read rq->curr9. Read X
In this pattern, the memory barriers make it impossible for both reads
to miss their corresponding writes. Since d does fail to read 6 (it
sees the earlier value stored by 4), 9 must read a.
The other guarantee you need is that g on CPU0 will observe anything
written by CPU1 in 1. This is easier to see, using the fact that 3 is a
memory barrier and d reads from 4.
Alan Stern
Re: next take at setting up a dma mask by default for platform devices
On Thu, 15 Aug 2019, Christoph Hellwig wrote: > On Thu, Aug 15, 2019 at 03:23:18PM +0200, Greg Kroah-Hartman wrote: > > I've taken the first 2 patches for 5.3-final. Given that patch 3 needs > > to be fixed, I'll wait for a respin of these before considering them. > > I have a respun version ready, but I'd really like to hear some > comments from usb developers about the approach before spamming > everyone again.. I didn't see any problems with your approach at first glance; it looked like a good idea. Alan Stern
Re: [PATCH -next] usb: host: drop pointless static qualifier
On Wed, 23 Jan 2019, YueHaibing wrote:
> There is no need to have the 'dummy_mask' variable static since new
> value always be assigned before use it.
>
> Signed-off-by: YueHaibing
> ---
> drivers/usb/host/ehci-ps3.c | 2 +-
> drivers/usb/host/ohci-ps3.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
> index 454d8c6..91cee02 100644
> --- a/drivers/usb/host/ehci-ps3.c
> +++ b/drivers/usb/host/ehci-ps3.c
> @@ -86,7 +86,7 @@ static int ps3_ehci_probe(struct ps3_system_bus_device *dev)
> int result;
> struct usb_hcd *hcd;
> unsigned int virq;
> - static u64 dummy_mask;
> + u64 dummy_mask;
>
> if (usb_disabled()) {
> result = -ENODEV;
> diff --git a/drivers/usb/host/ohci-ps3.c b/drivers/usb/host/ohci-ps3.c
> index 395f9d3..a1c1bdf 100644
> --- a/drivers/usb/host/ohci-ps3.c
> +++ b/drivers/usb/host/ohci-ps3.c
> @@ -69,7 +69,7 @@ static int ps3_ohci_probe(struct ps3_system_bus_device *dev)
> int result;
> struct usb_hcd *hcd;
> unsigned int virq;
> - static u64 dummy_mask;
> + u64 dummy_mask;
>
> if (usb_disabled()) {
> result = -ENODEV;
No. You need to read the code and understand how a variable is used
before you decide to modify it.
In this case, a suitable approach would be to change the declaration
so that it says:
status u64 dummy_mask = DMA_BIT_MASK(32);
and remove the line that does the assignment dynamically.
Alan Stern
Re: [PATCH] powerpc/ps3: Set driver coherent_dma_mask
On Thu, 19 Jul 2018, Geoff Levand wrote:
> Hi Alan,
>
> On 07/19/2018 07:33 AM, Alan Stern wrote:
> > On Wed, 18 Jul 2018, Geoff Levand wrote:
> >
> >> diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
> >> index 8c733492d8fe..454d8c624a3f 100644
> >> --- a/drivers/usb/host/ehci-ps3.c
> >> +++ b/drivers/usb/host/ehci-ps3.c
> >> @@ -86,7 +86,7 @@ static int ps3_ehci_probe(struct ps3_system_bus_device
> >> *dev)
> >>int result;
> >>struct usb_hcd *hcd;
> >>unsigned int virq;
> >> - static u64 dummy_mask = DMA_BIT_MASK(32);
> >> + static u64 dummy_mask;
> >>
> >>if (usb_disabled()) {
> >>result = -ENODEV;
> >> @@ -131,7 +131,9 @@ static int ps3_ehci_probe(struct ps3_system_bus_device
> >> *dev)
> >>goto fail_irq;
> >>}
> >>
> >> - dev->core.dma_mask = _mask; /* FIXME: for improper usb code */
> >> + dummy_mask = DMA_BIT_MASK(32);
> >> + dev->core.dma_mask = _mask;
> >> + dma_set_coherent_mask(>core, dummy_mask);
> >
> > What is the reason for changing a static initialization to a dynamic
> > one? As far as I can see, the patch touches four lines of code but the
> > only real difference is addition of a single line (and removal of a
> > comment).
>
> I thought it would be better if all the setting was done in
> one place, that's the only reason.
All right; in that case (for the EHCI and OHCI pieces):
Acked-by: Alan Stern
Alan Stern
Re: [PATCH] powerpc/ps3: Set driver coherent_dma_mask
On Wed, 18 Jul 2018, Geoff Levand wrote:
> Set the coherent_dma_mask for the PS3 ehci, ohci, and snd devices.
>
> Silences WARN_ON_ONCE messages emitted by the dma_alloc_attrs() routine.
>
> Reported-by: Fredrik Noring
> Signed-off-by: Geoff Levand
> ---
> Hi Michael,
>
> This just silences some warnings. Can you take it through the powerpc
> tree?
>
> -Geoff
>
>
> drivers/usb/host/ehci-ps3.c | 6 --
> drivers/usb/host/ohci-ps3.c | 6 --
> sound/ppc/snd_ps3.c | 5 +
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
> index 8c733492d8fe..454d8c624a3f 100644
> --- a/drivers/usb/host/ehci-ps3.c
> +++ b/drivers/usb/host/ehci-ps3.c
> @@ -86,7 +86,7 @@ static int ps3_ehci_probe(struct ps3_system_bus_device *dev)
> int result;
> struct usb_hcd *hcd;
> unsigned int virq;
> - static u64 dummy_mask = DMA_BIT_MASK(32);
> + static u64 dummy_mask;
>
> if (usb_disabled()) {
> result = -ENODEV;
> @@ -131,7 +131,9 @@ static int ps3_ehci_probe(struct ps3_system_bus_device
> *dev)
> goto fail_irq;
> }
>
> - dev->core.dma_mask = _mask; /* FIXME: for improper usb code */
> + dummy_mask = DMA_BIT_MASK(32);
> + dev->core.dma_mask = _mask;
> + dma_set_coherent_mask(>core, dummy_mask);
What is the reason for changing a static initialization to a dynamic
one? As far as I can see, the patch touches four lines of code but the
only real difference is addition of a single line (and removal of a
comment).
> diff --git a/drivers/usb/host/ohci-ps3.c b/drivers/usb/host/ohci-ps3.c
> index 20a23d795adf..395f9d3bc849 100644
> --- a/drivers/usb/host/ohci-ps3.c
> +++ b/drivers/usb/host/ohci-ps3.c
> @@ -69,7 +69,7 @@ static int ps3_ohci_probe(struct ps3_system_bus_device *dev)
> int result;
> struct usb_hcd *hcd;
> unsigned int virq;
> - static u64 dummy_mask = DMA_BIT_MASK(32);
> + static u64 dummy_mask;
>
> if (usb_disabled()) {
> result = -ENODEV;
> @@ -115,7 +115,9 @@ static int ps3_ohci_probe(struct ps3_system_bus_device
> *dev)
> goto fail_irq;
> }
>
> - dev->core.dma_mask = _mask; /* FIXME: for improper usb code */
> + dummy_mask = DMA_BIT_MASK(32);
> + dev->core.dma_mask = _mask;
> + dma_set_coherent_mask(>core, dummy_mask);
Same here.
Alan Stern
RE: PROBLEM: USB isochronous urb leak on EHCI driver
On Tue, 10 Feb 2015, Michael Tessier wrote: Okay; I did my homeworks. We've loaded kernel V3.16 (Oct 14th, 2015) on an i.MX51 plattform and the problem is still there. Unless an important change occured in V3.19, it appears that the latest kernel is not the solution for us. So we're still not able to use 4 codecs on our i.MX plattform. So just to make things clearer: - We have customers waiting for a solution with that hardware (this hardware is already delivred AND used); - We have important comittments and severe penalties ($$$) if we're not able to deliver on time (due for March 15th); - We've already looked at a hardware solution, which corresponds to replace current units ($), so that is not really an option for us; So as a last resort, I'm wondering, where is the USB expert who could help us solving our problem? Any suggestions? That would be me. Great. What do you need to make it a priority? Let's put it this way: Fixing bugs in the ehci-hcd driver already is a priority for me. But it takes second place to other priorities, such as my day job. (And incidentally, finding and fixing bugs in kernels as old as 2.6.31 is _not_ a priority, since so many have already been found and fixed. That is one important reason for my suggestion that you do the debugging with the most recent kernel version you can.) If we are to get into debugging the USB driver, we would do it with the current kernel used (V2.6.31). What are the better tools to get into that? I guess a USB analyzer (hardware) would be the smart thing? Any brand name to suggest? It really would be better to start by debugging the most recent kernel possible. Once the problem has been solved there, it should be fairly straightforward to port it back. How much time do you think you'd spend solving that kind of issue? Few days? Few weeks, or few months? No idea. In the past, such things have taken a few days to a few weeks, depending on lots of factors -- when they are solvable at all. Could you commit on a certain number of hours? (We are trying to evaluate a possible date where we could start delivering products) No, I can't commit to anything specific. When you say straightforward, again do you have an idea of the amount of time to do such work? Again, it all depends. For example, it might turn out that the hardware you are using contains a bug of a sort I have seen in the past. In that case, programming a workaround wouldn't take more than a few hours, once I knew what was going on. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: PROBLEM: USB isochronous urb leak on EHCI driver
On Mon, 9 Feb 2015, Michael Tessier wrote: Okay; I did my homeworks. We've loaded kernel V3.16 (Oct 14th, 2015) on an i.MX51 plattform and the problem is still there. Unless an important change occured in V3.19, it appears that the latest kernel is not the solution for us. So we're still not able to use 4 codecs on our i.MX plattform. So just to make things clearer: - We have customers waiting for a solution with that hardware (this hardware is already delivred AND used); - We have important comittments and severe penalties ($$$) if we're not able to deliver on time (due for March 15th); - We've already looked at a hardware solution, which corresponds to replace current units ($), so that is not really an option for us; So as a last resort, I'm wondering, where is the USB expert who could help us solving our problem? Any suggestions? That would be me. If we are to get into debugging the USB driver, we would do it with the current kernel used (V2.6.31). What are the better tools to get into that? I guess a USB analyzer (hardware) would be the smart thing? Any brand name to suggest? It really would be better to start by debugging the most recent kernel possible. Once the problem has been solved there, it should be fairly straightforward to port it back. Any other ideas for a solution will be really appreciated. You should begin by using usbmon during a short test (one or two seconds ought to be enough). See the instructions in the kernel source file Documentation/usb/usbmon.txt. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: PROBLEM: USB isochronous urb leak on EHCI driver
On Tue, 6 Jan 2015, Michael Tessier wrote: That is interresting, however, I have an older kernel running an OHCI driver which is able to handle 4 codecs. Same usb hardware (codecs and hub), but older kernel on a different CPU, with much less power. This makes me believe that there's a solution to make it work... Of course there is: Install an OHCI host controller and use it to drive your codecs. It should work fine. The periodic scheduling algorithm for OHCI is very different from the algorithm for EHCI. According to your knowledge, how much time would you think it takes to change the EHCI driver with an OHCI one? I don't understand the question. And can you tell if the OHCI driver will work on my hardware given that the Host controller of the i.MX512 is a USB2.0 host controller? (OHCI was implemented for USB 1.x from what I understand) The OHCI driver works with OHCI hardware and the EHCI driver works with EHCI hardware. OHCI is USB-1.1 and EHCI is USB-2. They are not interchangeable. I tried to do so several days ago with the built-in configurator (I am using ltib), but the configurator does not allow selecting the OHCI driver; I tried manually but it turned into compiler errors... It looks like the configurator is smart; it won't let you select the wrong driver for your hardware. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: PROBLEM: USB isochronous urb leak on EHCI driver
On Tue, 6 Jan 2015, Michael Tessier wrote: That is interresting, however, I have an older kernel running an OHCI driver which is able to handle 4 codecs. Same usb hardware (codecs and hub), but older kernel on a different CPU, with much less power. This makes me believe that there's a solution to make it work... Of course there is: Install an OHCI host controller and use it to drive your codecs. It should work fine. What do you mean by that? The host controller is embedded in the i.MX CPU... Changing the CPU is not really an option to me. Unless I am missing something? I didn't realize you were talking about an i.MX-based system. On a computer with a free PCI slot, it's easy to add an OHCI controller. iMX isn't as accomodating. If there's no way to add an extra USB controller to your system then the only choice is to upgrade the driver software. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: PROBLEM: USB isochronous urb leak on EHCI driver
using only 1 codec and becomes choppy when adding a second codec, then it means that this issue is still in the 3.x kernel. This answer will tell me if it is worth working on using a newer kernel or not. I have to say that I'm not a linux expert, so I see the migration to a newer kernel as a quite big amount of work... Why don't you try this yourself? It's easy to do; borrow a regular PC with a USB-2 host controller, boot it from a Live-CD version of Linux, plug in your hub with the codecs, and see what happens. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: PROBLEM: USB isochronous urb leak on EHCI driver
driver is a contradiction in terms. Moving to an earlier driver would be a _downgrade_. a fairly big amount of work), I would really like to know if this problem would still be in the 3.x kernels. Has anyone seen that issue in 3.x kernels? It depends a lot on the system hardware. Many people are using USB audio in 3.x kernels with no problem. On the other hand, some people have reported a bug (quite different from yours) so recently that the patch to fix it has not yet been merged. I am pretty new to USB driver debugging, so any ideas of where/how to find solutions will be appreciated. Thank you very much in advance for the support. Also don't hesitate to redirect me if I'm not at the right place to ask these questions. I can also provide some code if someone need it to help. Your first step should be to use an up-to-date kernel, as recommended by other people. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH 0/2] usb: Reuse fsl driver code for synopsys usb controller
On Sun, 20 Apr 2014, Punnaiah Choudary Kalluri wrote: Zynq soc contains a dual role usb controller and this IP is from synopsys. We observed that there is driver available for this controller from freescale and decided to reuse this driver for zynq use. Here is the link for zynq soc TRM. Please refer chapter 15 for usb controller related information. http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf The following series of patches add initial support for zynq soc in fsl gadget controller driver and fsl host controller driver. Based on these patches, I have the following concerns and sugesstions Since the freescale usb driver is for synopsys IP, Please consider rebranding this driver name and config options to reflect that it is a sysnopsys IP. So that other vendors who using this IP can reuse thie driver. Also the ehci-fsl.c is for powerpc based soc's, and zynq is ARM based, i have protected the code which is specifc to freescale with CONFIG_FSL_SOC. Please suggest if there is a better way of doing this? Filling the code with #ifdef lines is definitely not a good way to go. Ordinary if statements would be a lot better, if you can't figure out a reasonable way to encapsulate the differences. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/7] IBM Akebono: Add support to the OHCI platform driver for PPC476GTR
On Fri, 21 Feb 2014, Alistair Popple wrote: The IBM Akebono board uses the PPC476GTR SoC which has a OHCI compliant USB host interface. This patch adds support for it to the OHCI platform driver. As we use device tree to pass platform specific data instead of platform data we remove the check for platform data and instead provide reasonable defaults if no platform data is present. This is similar to what is currently done in ehci-platform.c. Signed-off-by: Alistair Popple alist...@popple.id.au Acked-by: Alan Stern st...@rowland.harvard.edu As Arnd pointed out, this patch is out of date. See https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/drivers/usb/host/ohci-platform.c?h=usb-nextid=ca52a17ba975dbf47e87c9bc63086aca0cf92806 and https://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/drivers/usb/host/ohci-platform.c?h=usb-nextid=ce149c30b9f89d0c9addd1d71ccdb57c1051553b Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/7] ECHI Platform: Merge ppc-of EHCI driver into the ehci-platform driver
On Fri, 21 Feb 2014, Alistair Popple wrote: Currently the ppc-of driver uses the compatibility string usb-ehci. This means platforms that use device-tree and implement an EHCI compatible interface have to either use the ppc-of driver or add a compatible line to the ehci-platform driver. It would be more appropriate for the platform driver to be compatible with usb-ehci as non-powerpc platforms are also beginning to utilise device-tree. This patch merges the device tree property parsing from ehci-ppc-of into the platform driver and adds a usb-ehci compatibility string. The existing ehci-ppc-of driver is removed and the 440EPX specific quirks are added to the ehci-platform driver. Signed-off-by: Alistair Popple alist...@popple.id.au This patch is also out of date. The compatibility string used by the ehci-platform driver is generic-ehci. There remains the question of whether to merge ehci-ppc-of into ehci-platform. This would be a rather invasive change, but I suppose we could do it. With adjustments along the lines suggested by Mark Rutland. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/7] IBM Akebono: Add support to the OHCI platform driver for Akebono
On Thu, 7 Nov 2013, Alistair Popple wrote: Thanks. Based on the discussion for the EHCI driver I would like to change the compatibility string to usb-ochi (instead of ibm,akebono-ohci). Are you still happy for me to add the Acked-by with the alternate compatibility (and of course the formatting fix)? No other drivers currently use usb-ochi so it shouldn't require any merging of drivers. Yes, go ahead (as long as you use the right spelling, as Ben pointed out). Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH] ehci-platform: Merge ppc-of EHCI driver into the ehci-platform driver
On Wed, 6 Nov 2013, Alistair Popple wrote:
Currently the ppc-of driver uses the compatibility string
usb-ehci. This means platforms that use device-tree and implement an
EHCI compatible interface have to either use the ppc-of driver or add
a compatible line to the ehci-platform driver. It would be more
appropriate for the platform driver to be compatible with usb-ehci
as non-powerpc platforms are also beginning to utilise device-tree.
This patch merges the device tree property parsing from ehci-ppc-of
into the platform driver and adds a usb-ehci compatibility
string. The existing ehci-ppc-of driver is renamed to ehci-440epx as
it contains platform specific work arounds for the 440EPX SoC.
Signed-off-by: Alistair Popple alist...@popple.id.au
---
So I could submit something like this that essentially merges the ppc-of
driver into the platform driver instead of adding the ibm,akebono-ehci
compatible line to the platform driver.
However I'm still fairly new to device-tree so I'm not sure what (if any) the
broader impact would be. A quick grep for usb-ehci turned up a couple of
ARM
device tree's using it however they all provided their own drivers and don't
select CONFIG_USB_EHCI_HCD_PLATFORM so I'm guess they shouldn't be impacted.
I have attempted to fix up any PowerPC device trees/configs, although I
wasn't
sure if usb-ehci should remain in sequoia.dts or not given that it needs to
use the 440EPX specific driver.
Also this hasn't been tested (beyond compilation) yet.
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-
platform.c
index f6b790c..027f368 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -77,6 +77,7 @@ static int ehci_platform_probe(struct platform_device *dev)
struct usb_hcd *hcd;
struct resource *res_mem;
struct usb_ehci_pdata *pdata;
+ struct device_node *dn = dev-dev.of_node;
int irq;
int err = -ENOMEM;
@@ -96,6 +97,18 @@ static int ehci_platform_probe(struct platform_device *dev)
pdata = dev_get_platdata(dev-dev);
+ /* Initialise platform data from device tree if available. */
+ if (!dn) {
Shouldn't this be if (dn)?
+ if (of_get_property(dn, big-endian, NULL)) {
+ pdata-big_endian_mmio = 1;
+ pdata-big_endian_desc = 1;
+ }
+ if (of_get_property(dn, big-endian-regs, NULL))
+ pdata-big_endian_mmio = 1;
+ if (of_get_property(dn, big-endian-desc, NULL))
+ pdata-big_endian_desc = 1;
+ }
+
This isn't good if there is more than one EHCI controller using
ehci-platform. To accomodate such cases, it would be necessary to
allocate a separate copy of ehci_platform_defaults for each controller.
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 4/7] IBM Akebono: Add support to the OHCI platform driver for Akebono
On Tue, 5 Nov 2013, Alistair Popple wrote:
The IBM Akebono board has a OHCI compliant USB host interface. This
patch adds support for it to the OHCI platform driver.
As we use device tree to pass platform specific data instead of
platform data we remove the check for platform data and instead
provide reasonable defaults if no platform data is present. This is
similar to what is currently done in ehci-platform.c.
Signed-off-by: Alistair Popple alist...@popple.id.au
Cc: Alan Stern st...@rowland.harvard.edu
Cc: linux-...@vger.kernel.org
---
drivers/usb/host/ohci-platform.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/ohci-platform.c
b/drivers/usb/host/ohci-platform.c
index a4c6410..4331454 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -23,6 +23,8 @@
#include linux/usb/ohci_pdriver.h
#include linux/usb.h
#include linux/usb/hcd.h
+#include linux/slab.h
+#include linux/of.h
#include ohci.h
@@ -55,6 +57,8 @@ static const struct ohci_driver_overrides
platform_overrides __initconst = {
.reset =ohci_platform_reset,
};
+static struct usb_ohci_pdata ohci_platform_defaults;
+
static int ohci_platform_probe(struct platform_device *dev)
{
struct usb_hcd *hcd;
@@ -63,14 +67,14 @@ static int ohci_platform_probe(struct platform_device
*dev)
int irq;
int err = -ENOMEM;
- if (!pdata) {
- WARN_ON(1);
- return -ENODEV;
- }
-
if (usb_disabled())
return -ENODEV;
+ /* Platforms using DT don't always provide platform data.
+ * This should provide reasonable defaults. */
/*
* The accepted format for multi-line
* comments is like this.
*/
+ if (!pdata)
+ dev-dev.platform_data = pdata = ohci_platform_defaults;
+
irq = platform_get_irq(dev, 0);
if (irq 0) {
dev_err(dev-dev, no irq provided);
@@ -171,6 +175,11 @@ static int ohci_platform_resume(struct device *dev)
#define ohci_platform_resume NULL
#endif /* CONFIG_PM */
+static const struct of_device_id ohci_of_match[] = {
+ { .compatible = ibm,akebono-ohci, },
+ {},
+};
+
static const struct platform_device_id ohci_platform_table[] = {
{ ohci-platform, 0 },
{ }
@@ -191,6 +200,7 @@ static struct platform_driver ohci_platform_driver = {
.owner = THIS_MODULE,
.name = ohci-platform,
.pm = ohci_platform_pm_ops,
+ .of_match_table = ohci_of_match,
}
};
Update the comment formatting, and then you can resubmit with
Acked-by: Alan Stern st...@rowland.harvard.edu
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/7] IBM Akebono: Add support to the EHCI platform driver for Akebono
On Tue, 5 Nov 2013, Alistair Popple wrote:
The IBM Akebono board has an EHCI compliant USB host interface. This
patch adds support for it to the EHCI platform driver.
Signed-off-by: Alistair Popple alist...@popple.id.au
Cc: Alan Stern st...@rowland.harvard.edu
Cc: linux-...@vger.kernel.org
---
drivers/usb/host/ehci-platform.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/ehci-platform.c
b/drivers/usb/host/ehci-platform.c
index f6b790c..0a67616 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -203,9 +203,10 @@ static int ehci_platform_resume(struct device *dev)
#define ehci_platform_resume NULL
#endif /* CONFIG_PM */
-static const struct of_device_id vt8500_ehci_ids[] = {
+static const struct of_device_id ehci_platform_ids[] = {
{ .compatible = via,vt8500-ehci, },
{ .compatible = wm,prizm-ehci, },
+ { .compatible = ibm,akebono-ehci, },
{}
};
@@ -229,7 +230,7 @@ static struct platform_driver ehci_platform_driver = {
.owner = THIS_MODULE,
.name = ehci-platform,
.pm = ehci_platform_pm_ops,
- .of_match_table = vt8500_ehci_ids,
+ .of_match_table = ehci_platform_ids,
}
};
Acked-by: Alan Stern st...@rowland.harvard.edu
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 36/51] DMA-API: usb: use dma_set_coherent_mask()
On Thu, 19 Sep 2013, Russell King wrote: The correct way for a driver to specify the coherent DMA mask is not to directly access the field in the struct device, but to use dma_set_coherent_mask(). Only arch and bus code should access this member directly. Convert all direct write accesses to using the correct API. Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c index f6b790c..5b0cd2d 100644 --- a/drivers/usb/host/ehci-platform.c +++ b/drivers/usb/host/ehci-platform.c @@ -91,8 +91,9 @@ static int ehci_platform_probe(struct platform_device *dev) dev-dev.platform_data = ehci_platform_defaults; if (!dev-dev.dma_mask) dev-dev.dma_mask = dev-dev.coherent_dma_mask; - if (!dev-dev.coherent_dma_mask) - dev-dev.coherent_dma_mask = DMA_BIT_MASK(32); + err = dma_set_coherent_mask(dev-dev, DMA_BIT_MASK(32)); + if (err) + return err; pdata = dev_get_platdata(dev-dev); ehci-platform.c is a generic file, intended for use by multiple platforms. Is it not possible that on some platforms, the arch or bus code may already have initialized the DMA masks? Isn't something like this (i.e., DMA bindings) planned for Device Tree? By eliminating the tests above and calling dma_set_coherent_mask or dma_coerce_mask_and_coherent unconditionally, this patch (and the next) would overwrite those initial settings. I don't know to what extent the same may be true for the other, platform-specific, drivers changed by this patch. But it's something to be aware of. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] usb: remove redundant tdi_reset
On Wed, 17 Apr 2013, Shengzhou Liu wrote: We remove the redundant tdi_reset in ehci_setup since there is already it in ehci_reset. It was observed that the duplicated tdi_reset was causing the PHY_CLK_VALID bit unstable. Signed-off-by: Shengzhou Liu shengzhou@freescale.com --- drivers/usb/host/ehci-hcd.c |3 --- 1 files changed, 0 insertions(+), 3 deletions(-) diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 416a6dc..83b5a17 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -670,9 +670,6 @@ int ehci_setup(struct usb_hcd *hcd) if (retval) return retval; - if (ehci_is_TDI(ehci)) - tdi_reset(ehci); - ehci_reset(ehci); return 0; Acked-by: Alan Stern st...@rowland.harvard.edu This should be applied to stable kernels going back to 3.6. In case you are wondering why that redudant call was added, I did it because some of the PCI drivers (Intel and TDI) already had calls to tdi_reset. The commit removed those calls, so the new one was added in. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 159/493] usb: remove use of __devinit
On Mon, 19 Nov 2012, Bill Pemberton wrote: CONFIG_HOTPLUG is going away as an option so __devinit is no longer needed. Signed-off-by: Bill Pemberton wf...@virginia.edu For all the __devinit* annotations and all the EHCI, OHCI, and UHCI drivers: Acked-by: Alan Stern st...@rowland.harvard.edu ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc/usb: fix bug of CPU hang when missing USB PHY clock
On Mon, 20 Aug 2012, Kumar Gala wrote: Subject: Re: [PATCH 2/2] powerpc/usb: fix bug of CPU hang when missing USB PHY clock On Aug 10, 2012, at 5:48 AM, Shengzhou Liu wrote: when missing USB PHY clock, kernel booting up will hang during USB initialization. We should check USBGP[PHY_CLK_VALID] bit to avoid CPU hanging in this case. Signed-off-by: Shengzhou Liu shengzhou@freescale.com --- drivers/usb/host/ehci-fsl.c | 63 ++--- - drivers/usb/host/ehci-fsl.h |1 + 2 files changed, 46 insertions(+), 18 deletions(-) I assume this should be considered a bug fix and be looked at for inclusion in v3.6? - k [Shengzhou] Yes. Greg, ping? Greg is away on vacation for the rest of this week. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: Problem with full speed devices on PowerPC MPC5121 host port
On Fri, 6 Jan 2012, Matthias Fuchs wrote: For my eyes it does not really look like a general USB issue. It looks like a problem with the Freescale EHCI implementation that is influenced by high interrupt or internal bus load caused by the flood ping. Indeed, it might be a problem with the built-in Transaction Translator. That would explain why it affect full-speed devices. However, I would expect the resetting the controller hardware (which happens when you reload the ehci-fsl driver) would fix any such issues. It's hard to imagine how a problem could survive a reset like that. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] usb:gadget: use min_t() macro instead of min()
On Mon, 13 Jun 2011, Tanya Brokhman wrote: On Sun, Jun 12, 2011 at 02:14:46PM +0300, Tatyana Brokhman wrote: Signed-off-by: Tatyana Brokhman tlin...@codeaurora.org I need a sensible commit log for this. Why do we need to change all min() to min_t() ? Actually, Alan asked me to make this change in one place in dummy_hcd. I wasn't aware of the min_t macro before that. So when I searched the code for other places I found quite a few and just thought that it would be nicer to use min_t() instead of min() with casting. So we don't need to make this change. Everything works as is. This patch only makes the code look nicer, nothing more. I can elaborate the above in the commit log if you want. The change I suggested involved replacing two typecasts with a single min_t. All (or almost all) the places this patch touches currently contain only one typecast, so the motivation for changing them is a lot weaker. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] usb:gadget: use min_t() macro instead of min()
On Mon, 13 Jun 2011, Tanya Brokhman wrote: The change I suggested involved replacing two typecasts with a single min_t. All (or almost all) the places this patch touches currently contain only one typecast, so the motivation for changing them is a lot weaker. You're right. So what's the final call on this one? Do you think it can be merged or you prefer not change anything? I personally think the code looks nicer using min_t instead of min with casting but that's just my opinion and of course there are arguments against this patch. I don't care either way. It's up to you and Felipe. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] PM: Hide CONFIG_PM from users
On Mon, 7 Feb 2011, Mark Brown wrote: On Tue, Feb 08, 2011 at 02:19:16AM +1100, Stephen Rothwell wrote: At least some of the powerpc defconfigs were added with CONFIG_PM disabled. I assume that was on purpose (though it may not have been). I'd not be so sure - since it's a bool without an explicit default set Kconfig will default to disabling it and if anything enabling it is the option that requires special effort. This may be a naive suggestion, but have you considered simply _asking_ the people who added those defconfigs? Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V7 07/10] USB/ppc4xx: Add Synopsys DWC OTG PCD function
On Tue, 18 Jan 2011 tma...@apm.com wrote:
From: Tirumala Marri tma...@apm.com
The PCD is responsible for translating requests from the gadget driver
to appropriate actions on the DWC OTG controller.
Signed-off-by: Tirumala R Marri tma...@apm.com
Signed-off-by: Fushen Chen fc...@apm.com
Signed-off-by: Mark Miesfeld mmiesf...@apm.com
---
drivers/usb/dwc_otg/dwc_otg_pcd.c | 1752
+
drivers/usb/dwc_otg/dwc_otg_pcd.h | 139 +++
2 files changed, 1891 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/dwc_otg/dwc_otg_pcd.c
b/drivers/usb/dwc_otg/dwc_otg_pcd.c
new file mode 100644
index 000..857dcee
--- /dev/null
+++ b/drivers/usb/dwc_otg/dwc_otg_pcd.c
...
+static struct usb_ep_ops dwc_otg_pcd_ep_ops = {
+ .enable = dwc_otg_pcd_ep_enable,
+ .disable = dwc_otg_pcd_ep_disable,
+ .alloc_request = dwc_otg_pcd_alloc_request,
+ .free_request = dwc_otg_pcd_free_request,
+ .queue = dwc_otg_pcd_ep_queue,
+ .dequeue = dwc_otg_pcd_ep_dequeue,
+ .set_halt = dwc_otg_pcd_ep_set_halt,
+ .fifo_status = NULL,
+ .fifo_flush = NULL,
+};
This is missing a .set_wedge method.
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 00/10] wii: add usb 2.0 support
On Fri, 19 Mar 2010, Albert Herranz wrote:
Alan: I think you are also working in a patchset to make {un}map_urb_for_dma
remember how the urb was mapped, right?
Yes; you can see an initial version here:
http://marc.info/?l=linux-usbm=126901183419219w=2
This is a bug fix, so it's likely to be merged before your work.
It's a messy situation, with two people changing the same code at the
same time. I think the best approach will be to wait until the bug-fix
patch is tested and accepted; then I can create a version of your 5/10
and 6/10 patches adding HCD_NO_COHERENT_MEM and the corresponding
behavior. That can stand by itself, and once it is accepted the rest
of your series should go through with no difficulty (at least, no
difficulties involving the USB core!).
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 8/9] USB: add HCD_NO_COHERENT_MEM host controller driver flag
On Mon, 1 Mar 2010, Albert Herranz wrote: If transfer_buffer_length is 0 then do nothing. Otherwise if num_sgs 0 then do nothing. Otherwise if URB_NO_TRANSFER_DMA_MAP and transfer_dma are both set (this avoids your HCD_NO_COHERENT_MEM case) then do nothing. I see. This case would include the PIO case too (for which dma_handle is set to all 1s). The test above should be transfer_dma != ~0, not transfer_dma != 0, since ~0 means the DMA address isn't set. In fact I forgot to include the PIO case; it should be handled by changing the remaining tests as follows: Otherwise if hcd-self.uses_dma is set then If this URB doesn't require PIO then call dma_map_single Otherwise if HCD_LOCAL_MEM is set then call hcd_alloc_coherent Otherwise do nothing (PIO case). Currently this URB doesn't require PIO is always true, but in the future it won't be. So this assumes that transfer_dma should be set initially to 0 when allocating USB buffers for HCD_NO_COHERENT_MEM. No, it should be set to ~0, the same as when buffers are allocated for a PIO-based controller. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 8/9] USB: add HCD_NO_COHERENT_MEM host controller driver flag
On Tue, 2 Mar 2010, Albert Herranz wrote: Currently this URB doesn't require PIO is always true, but in the future it won't be. Can this be currently tested? Not yet, but soon. You can follow the gory details in this email thread: http://marc.info/?l=linux-usbm=126477569707174w=2 See especially this email: http://marc.info/?l=linux-usbm=126630714002200w=2 Should I make provisions for this check now too? Don't worry about it. If you structure the code as I described, it will be easy to insert the check later on. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 8/9] USB: add HCD_NO_COHERENT_MEM host controller driver flag
On Sun, 28 Feb 2010, Albert Herranz wrote:
The HCD_NO_COHERENT_MEM USB host controller driver flag can be enabled
to instruct the USB stack to avoid allocating coherent memory for USB
buffers.
This flag is useful to overcome some esoteric memory access restrictions
found in some platforms.
For example, the Nintendo Wii video game console is a NOT_COHERENT_CACHE
platform that is unable to safely perform non-32 bit uncached writes
to RAM because the byte enables are not connected to the bus.
Thus, in that platform, coherent DMA buffers cannot be directly used
by the kernel code unless it guarantees that all write accesses
to said buffers are done in 32 bit chunks (which is not the case in the
USB subsystem).
To avoid this unwanted behaviour HCD_NO_COHERENT_MEM can be enabled at
the HCD controller, causing USB buffer allocations to be satisfied from
normal kernel memory. In this case, the USB stack will make sure that
the buffers get properly mapped/unmapped for DMA transfers using the DMA
streaming mapping API.
Note that other parts of the USB stack may also use coherent memory,
like for example the hardware descriptors used in the EHCI controllers.
This needs to be checked and addressed separately. In the EHCI example,
hardware descriptors are accessed in 32-bit (or 64-bit) chunks, causing
no problems in the described scenario.
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1260,6 +1260,34 @@ static void hcd_free_coherent(struct usb_bus *bus,
dma_addr_t *dma_handle,
*dma_handle = 0;
}
+static int urb_needs_setup_dma_map(struct usb_hcd *hcd, struct urb *urb)
+{
+ return !(urb-transfer_flags URB_NO_SETUP_DMA_MAP) ||
+((hcd-driver-flags HCD_NO_COHERENT_MEM)
+ urb-setup_dma == ~(dma_addr_t)0);
+}
+
+static int urb_needs_setup_dma_unmap(struct usb_hcd *hcd, struct urb *urb)
+{
+ return !(urb-transfer_flags URB_NO_SETUP_DMA_MAP) ||
+((hcd-driver-flags HCD_NO_COHERENT_MEM)
+ urb-setup_dma urb-setup_dma != ~(dma_addr_t)0);
+}
+
+static int urb_needs_transfer_dma_map(struct usb_hcd *hcd, struct urb *urb)
+{
+ return !(urb-transfer_flags URB_NO_TRANSFER_DMA_MAP) ||
+((hcd-driver-flags HCD_NO_COHERENT_MEM)
+ urb-transfer_dma == ~(dma_addr_t)0);
+}
+
+static int urb_needs_transfer_dma_unmap(struct usb_hcd *hcd, struct urb *urb)
+{
+ return !(urb-transfer_flags URB_NO_TRANSFER_DMA_MAP) ||
+((hcd-driver-flags HCD_NO_COHERENT_MEM)
+ urb-transfer_dma urb-transfer_dma != ~(dma_addr_t)0);
+}
+
These functions would be a lot easier to understand if they were
expanded into multiple test and return statements, rather than
squeezing all the Boolean manipulations into single expressions. (Not
to mention the fact that other developement is going to make them even
more complicated than they are now...)
Also, I can't help thinking that the corresponding *_map() and
*_unmap() routines are so similar, it ought to be possible to combine
them. The only difference is a check for a NULL DMA address, and it's
not clear to me why it is present. It's also not clear why the test
for a DMA address of all ones is present. Maybe they both can be
removed.
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 8/9] USB: add HCD_NO_COHERENT_MEM host controller driver flag
On Mon, 1 Mar 2010, Albert Herranz wrote: Also, I can't help thinking that the corresponding *_map() and *_unmap() routines are so similar, it ought to be possible to combine them. The only difference is a check for a NULL DMA address, and it's not clear to me why it is present. It's also not clear why the test for a DMA address of all ones is present. Maybe they both can be removed. I think too that I can simplify that logic. I added those checks in a defensive way seeking robustness while I familiarize with the USB stack innards. So far, those cases are just avoiding mappings when urb_needs_transfer_dma_map()/urb_needs_transfer_dma_unmap() are called with urb-transfer_buffer == 0 and urb-transfer_dma == 0. I guess that those cases are related to scatterlist-based urb requests. What should be the correct way to check if a urb has already been scatter/gather-mapped? If urb-num_sgs 0 then urb has been s-g mapped. Although we don't currently check for it, quite a few URBs have transfer_buffer_length == 0 (a number of control requests are like this, for example) so they don't need a mapping either. The final logic would be something like: - map if URB_NO_TRANSFER_DMA_MAP is cleared - otherwise (URB_TRANSFER_NO_DMA_MAP is set so) map if HCD_NO_COHERENT_MEM is set _and_ it's not a scatter/gather request (as that should have been mapped already by usb_buffer_map_sg()) Am I on the right path? More or less. I would do it somewhat differently: If URB_NO_TRANSFER_DMA_MAP is set then no map is needed. Otherwise if num_sgs 0 then no map is needed. Otherwise if HCD_NO_COHERENT_MEM is set then use hcd_alloc_coherent(). Otherwise if transfer_buffer_length 0 then use dma_map_single(). Similar logic is needed for the setup buffer mapping, but you can assume that control URBs never use scatter-gather so there's no need to check num_sgs (and there's no need to check the transfer length, since setup packets are always 8 bytes long). In fact, I think URB_NO_SETUP_DMA_MAP doesn't really offer any worthwhile advantages. (About the only place where multiple control requests are used in rapid succession is during firmware transfers, and those aren't time-constrained.) It is currently used in a few drivers, but we ought to be able to remove it without too much effort. That might make a good project. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 8/9] USB: add HCD_NO_COHERENT_MEM host controller driver flag
On Mon, 1 Mar 2010, Albert Herranz wrote:
Am I on the right path?
More or less. I would do it somewhat differently:
If URB_NO_TRANSFER_DMA_MAP is set then no map is needed.
Otherwise if num_sgs 0 then no map is needed.
Otherwise if HCD_NO_COHERENT_MEM is set then use
hcd_alloc_coherent().
Otherwise if transfer_buffer_length 0 then use
dma_map_single().
I think that logic is not quite right.
Remember that the final goal is to avoid allocating USB buffers from coherent
memory (because the USB drivers access USB buffers without access
restrictions but the platform I'm working on can't write to coherent memory
unless it is done in 32-bit chunks).
Actually the final goal is to make the mapping/unmapping algorithms
clear and correct. One of the subgoals involves avoiding coherent USB
buffers, but there are others as well (if you look back the through the
linux-usb mailing list for the last few weeks you'll see a discussion
about a controller which has to use PIO for control transfers but can
use DMA for other types).
And we want to avoid bouncing at the USB layer too (that's what v1 did).
The strategy so far is:
- we have modified the USB buffer allocation routines
hcd_buffer_alloc()/hcd_buffer_free() to always return normal kernel memory
when HCD_NO_COHERENT_MEM is set on the host controller.
- during map_urb_for_dma()/unmap_urb_for_dma() we need to make sure that
those USB buffers are sync'ed, even if we are told
USB_NO_{SETUP,TRANSFER}_DMA_MAP
So the logic would be:
If URB_NO_TRANSFER_DMA_MAP is _cleared_ then do the mapping
No, that's wrong because it ignores the HCD_LOCAL_MEM flag.
- this case covers normal kernel memory used as a buffer and not
already DMA mapped by a USB driver
Otherwise if HCD_NO_COHERENT_MEM is set _and_ num_sgs == 0 _and_
transfer_buffer_length 0 then do the mapping too
- this case covers USB buffers allocated via usb_buffer_alloc() and
marked URB_NO_TRANSFER_DMA_MAP by a USB driver, which are allocated from
normal kernel memory when HCD_NO_COHERENT_MEM is set (we avoid bouncing
buffers here too, at least if they sit already within MEM2 in the Wii, but
anyway that's part of the platform DMA mapping code)
s-g urbs do not need a mapping as they have already been mapped, marked
URB_NO_TRANSFER_DMA_MAP and have num_sgs 0
Actually the test for transfer_buffer_length == 0 should be done first,
since obviously no mapping is needed if there's no data. (And in fact
the current code does do this; I was wrong earlier when I said it
doesn't.)
So let's make things a little easier by first testing the conditions
under which no mapping is needed:
If transfer_buffer_length is 0 then do nothing.
Otherwise if num_sgs 0 then do nothing.
Otherwise if URB_NO_TRANSFER_DMA_MAP and transfer_dma
are both set (this avoids your HCD_NO_COHERENT_MEM
case) then do nothing.
The remaining cases all need mapping and/or bounce buffers:
Otherwise if HCD_LOCAL_MEM is set then call hcd_alloc_coherent.
Otherwise call dma_map_single.
Finally, the unmapping tests can be simplified greatly if the kind of
mapping is recorded in the URB flags.
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH 1/2] USB: add HCD_BOUNCE_BUFFERS host controller driver flag
On Sun, 7 Feb 2010, Albert Herranz wrote: The wii has no uhci, but has 2 ohci controllers. For ohci we need a similar approach as done for ehci. So you'll need to write a patch splitting up the OHCI data structures in the same way the EHCI qh was split up. If you do it as described above then the buffers you're worried about won't be allocated in coherent memory to begin with, so no problems will arise. It turns out that we have more limitations. The wii has 2 discontiguous memory areas (usually called MEM1 and MEM2). I have checked that the ehci controller doesn't work properly when performing dma to buffers allocated in MEM1 (it corrupts part of the data) but has no problems if the buffers sit within MEM2. So usb buffers will need to be bounced anyway if they are part of MEM1. This sounds like the sort of restriction that dma_map_single() should be capable of handling. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH 1/2] USB: add HCD_BOUNCE_BUFFERS host controller driver flag
On Thu, 4 Feb 2010, Albert Herranz wrote:
Hi Alan,
Alan Stern wrote:
This description sounds hopelessly confused. Maybe you're just
misusing the term coherent. The patch itself doesn't affect the
coherent DMA mappings anyway; it affects the streaming mappings. Or to
put it another way, what's the justification for replacing a call to
dma_map_single() with a call to dma_alloc_coherent()?
Since the patch doesn't affect any of the coherent mappings (see for
example the calls to dma_pool_create() in ehci-mem.c), I don't see how
it can possibly do what you claim.
Thanks for your comments. Let's try to hopefully clarify this a bit.
I've used the term coherent as described in Documentation/DMA-API.txt (aka
consistent as used in PCI-related functions).
I've tried to describe first the limitations of the platform that I'm working
on. Basically, one of the annoying things of that platform is that writes to
uncached memory (as used in coherent memory) can only be reliably performed
in 32-bit accesses.
The USB subsystem ends up using coherent memory for buffers and/or other
structures in different ways.
The coherent memory allocated in dma_pool_create() in ehci-mem.c that you
report is not a problem at all because it is always accessed in 32-bit chunks
(it hasn't been always like that but since commit
3807e26d69b9ad3864fe03224ebebc9610d5802e USB: EHCI: split ehci_qh into hw
and sw parts this got addressed as a side effect, so I didn't need to post
another patch for that).
On a 64-bit processor, some of the accesses will be 64 bits wide
instead of 32. Does that matter for your purposes?
What about ohci-hcd and uhci-hcd? They both use non-32-bit accesses to
structures in coherent memory.
Other possible interactions with coherent memory are those involving
buffers used in USB transactions, which may be allocated via the USB
subsystem (at usb_buffer_alloc() or when bounced via hcd_alloc_coherent()) or
which may come already allocated and ready for use
(URB_NO_{SETUP,TRANSFER}_DMA_MAP).
Ah yes, quite correct. And this indicates that you need to concentrate
on usb_buffer_alloc(). On your system (or rather, whenever the
HCD_NO_COHERENT_MEM flag is set) it should allocate normal memory and
set the DMA pointer to NULL.
Then map_urb_for_dma() should check the urb-setup_dma and
urb-transfer_dma pointers; if a pointer is NULL then the
corresponding urb-URB_NO_SETUP_DMA_MAP or urb-NO_TRANSFER_DMA_MAP
flag should be ignored (and probably should be cleared so as to
avoid confusing unmap_urb_for_dma()).
The patch, as posted, allocates normal memory for USB buffers _within_ the
USB subsystem and invariably bounces all buffers to new coherent buffers.
So, basically, what the patch claims (avoid 32-bit writes for coherent
memory within the USB subsystem) is done (hey, it actually works ;-).
But I think you have raised valid points here :)
If the coherent memory is already allocated and passed (as already
dma-mapped) to the USB subsystem then there is no gain in bouncing the buffer:
- if a non-32 bit write was done to that coherent memory the damage is
already done
- if the coherent memory was written always in 32-bit accesses then we can
just safely use it
So bouncing here should be avoided as it is unneeded.
On the other hand, we can control USB buffers managed by the USB subsystem
itself.
That's what the patch does. It avoids access restrictions to USB buffers by
allocating them from normal memory (leaving USB drivers free to access those
buffers in whatever bus width they need, as they do today) ... and bouncing
them.
The thing here is that it makes no sense to bounce them to coherent memory
if they can be dma-mapped directly (as you point in your
dma_map_single-vs-dma_alloc_coherent comment).
So... that's what RFCs are for :)
If you do it as described above then the buffers you're worried about
won't be allocated in coherent memory to begin with, so no problems
will arise.
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Bugme-new] [Bug 13304] New: ehci_hcd module causing problems in using usb head phone
On Fri, 26 Jun 2009, abhishekkumar wrote: PFA I am attaching dmesg file taken from /var/log/ and also the lines which I got from dmesg command in different file named dmesgafter.txt. I still don't see any lines about the headphone device in the log. Was the headphone plugged in during boot, or did you plug it in later? Then , periodic and registers files and also the usbmon log. Here's what your registers file says: bus ps3_system_bus, device sb_05 (driver 10 Dec 2004) PS3 EHCI Host Controller EHCI ff.ff, hcd state 1 structural params 0x capability params 0x status Async Periodic Recl Halt IAA FATAL FLR PCD ERR INT command park=3 ithresh=63 LReset IAAD Async Periodic period=?? Reset R intrenable IAA FATAL FLR PCD ERR INT uframe port 1 status POWER OWNER sig=? RESET SUSPEND RESUME OCC OC PEC PE CSC port 2 status POWER OWNER sig=? RESET SUSPEND RESUME OCC OC PEC PE CSC irq normal 30832 err 30 reclaim 84 (lost 1) complete 31221 unlink 10 This is very bad. It indicates that the CPU was unable to communicate with the EHCI controller at all! All the memory-mapped I/O reads returned 0x. No wonder the keyboard and mouse stopped working. I have no idea what could have caused this to happen. Even if the controller had suffered a fatal error, you wouldn't see this. It looks like the bus's connection to the controller was turned off. I'm CC-ing the PS3 maintainer and mailing list. Maybe people there can help. I am even not able to kill the application because it's not showing when I use top command. I addition to that I get some messages during boot up . they are Unable to accept address for port 1 (error -62) usb cable may be bad Unable to accept address for port 2 (error -62) Those are normal. They occur because your system loads ohci-hcd before ehci-hcd. It should load ehci-hcd first. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Bugme-new] [Bug 13304] New: ehci_hcd module causing problems in using usb head phone
On Fri, 26 Jun 2009, Geoff Levand wrote: There is a known but yet unfixed bug for the PS3's EHCI Async Periodic (typically audio recording) device handling. There are a few related hardware errata that I have not yet implemented driver fixes for that I think are causing it. I guess it is the same problem as reported by Abhishek since Andrew's dmesg (http://www.osl.iu.edu/~afriedle/dmesg.txt) shows similar results. Yes, it definitely looks the same. More info is here: http://ozlabs.org/pipermail/cbe-oss-dev/2009-February/006365.html I have bought a ART Tube USB device but have not had time to fix this bug. It is on my todo list. Please feel free to make an attempt. Where is the information about the hardware errata you mentioned? Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Bugme-new] [Bug 13304] New: ehci_hcd module causing problems in using usb head phone
On Fri, 26 Jun 2009, Geoff Levand wrote: Where is the information about the hardware errata you mentioned? Sorry, I should have mentioned it. Unfortunately, that info is not in the public as far as I know. I think only someone with access to those will be able to work on this fix. Okay, then I guess there's nothing more I can do regarding Bug #13304. Can you take it over? Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpointinterfaces
On Mon, 18 May 2009, K.Prasad wrote:
+int __kprobes hw_breakpoint_handler(struct die_args *args)
+{
+ int rc = NOTIFY_STOP;
+ struct hw_breakpoint *bp;
+ struct pt_regs *regs = args-regs;
+ unsigned long dar;
+ int cpu, stepped, is_kernel;
+
+ /* Disable breakpoints during exception handling */
+ set_dabr(0);
+
+ dar = regs-dar (~HW_BREAKPOINT_ALIGN);
+ is_kernel = (dar = TASK_SIZE) ? 1 : 0;
is_kernel_addr() ?
Ok.
Shouldn't this test hbp_kernel_pos instead?
Testing hbp_kernel_pos should be sufficient for PPC64 with just one
breakpoint register. However the above code is more extensible to other
PowerPC implementations which have more than one breakpoint register.
Then maybe you don't want to test this at all. Just compare the dar
value with each of the breakpoint addresses. That's more like what the
x86 code does.
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC Patch 2/6] Introduce PPC64 specific Hardware Breakpointinterfaces
On Fri, 15 May 2009, K.Prasad wrote:
I see that you're referring to this code in __switch_to() :
if (unlikely(__get_cpu_var(current_dabr) != new-thread.dabr))
set_dabr(new-thread.dabr);
arch_install_thread_hw_breakpoint()--switch_to_thread_hw_breakpoint()
--__switch_to() implementation is also similar.
In __switch_to(),
if (unlikely(test_tsk_thread_flag(new, TIF_DEBUG)))
switch_to_thread_hw_breakpoint(new);
happens only when TIF_DEBUG flag is set. This flag is cleared when the
process unregisters any breakpoints it had requested earlier. So, the
set_dabr() call is avoided for processes not using the debug register.
In the x86 code, shouldn't arch_update_user_hw_breakpoint set or clear
TIF_DEBUG, depending on whether or not there are any user breakpoints
remaining?
+int __kprobes hw_breakpoint_handler(struct die_args *args)
+{
+ int rc = NOTIFY_STOP;
+ struct hw_breakpoint *bp;
+ struct pt_regs *regs = args-regs;
+ unsigned long dar;
+ int cpu, stepped, is_kernel;
+
+ /* Disable breakpoints during exception handling */
+ set_dabr(0);
+
+ dar = regs-dar (~HW_BREAKPOINT_ALIGN);
+ is_kernel = (dar = TASK_SIZE) ? 1 : 0;
is_kernel_addr() ?
Ok.
Shouldn't this test hbp_kernel_pos instead?
+ if (is_kernel)
+ bp = hbp_kernel[0];
+ else {
+ bp = current-thread.hbp[0];
+ /* Lazy debug register switching */
+ if (!bp)
+ return rc;
Shouldn't this test be moved outside the if statement, as in the x86
code?
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC Patch 6/6] Adapt kexec and samples code to recognise PPC64 hardware breakpoint usage
On Thu, 14 May 2009, K.Prasad wrote: Index: linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c === --- linux-2.6-tip.hbkpt.orig/samples/hw_breakpoint/data_breakpoint.c 2009-05-14 00:17:24.0 +0530 +++ linux-2.6-tip.hbkpt/samples/hw_breakpoint/data_breakpoint.c 2009-05-14 00:58:06.0 +0530 @@ -54,6 +54,10 @@ sample_hbp.info.type = HW_BREAKPOINT_WRITE; sample_hbp.info.len = HW_BREAKPOINT_LEN_4; #endif /* CONFIG_X86 */ +#ifdef CONFIG_PPC64 + sample_hbp.info.name = ksym_name; + sample_hbp.info.type = DABR_DATA_WRITE; This should be HW_BREAKPOINT_WRITE, not DABR_DATA_WRITE. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH -mm 1/3] USB: FHCI: Driver should be responsible for managing endpoint queues
On Wed, 24 Dec 2008, Anton Vorontsov wrote: Follow these changes for the FHCI driver: commit e9df41c5c5899259541dc928872cad4d07b82076 Author: Alan Stern st...@rowland.harvard.edu Date: Wed Aug 8 11:48:02 2007 -0400 USB: make HCDs responsible for managing endpoint queues On the whole this looks good. --- a/drivers/usb/host/fhci-q.c +++ b/drivers/usb/host/fhci-q.c @@ -200,6 +200,9 @@ void urb_complete_free(struct fhci_hcd *fhci, struct urb *urb) else urb-status = 0; } + + usb_hcd_unlink_urb_from_ep(fhci_to_hcd(fhci), urb); + spin_unlock(fhci-lock); usb_hcd_giveback_urb(fhci_to_hcd(fhci), urb, urb-status); For the future you might think about not using urb-status at all. The intention is eventually to remove that field. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] USB: Driver for Freescale QUICC Engine USB Host Controller
On Wed, 24 Dec 2008, Anton Vorontsov wrote: This patch adds support for the FHCI USB controller, as found in the Freescale MPC836x and MPC832x processors. It can support Full or Low speed modes. Quite a lot the hardware is doing by itself (SOF generation, CRC generation and checking), though scheduling and retransmission is on software's shoulders. This controller does not integrate the root hub, so this driver also fakes one-port hub. External hub is required to support more than one device. It looks like the kernel this was written for is several versions out of date. The driver is missing some critical calls to functions like usb_hcd_link_urb_to_ep(). Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb/fsl_qe_udc: Report disconnect before unbinding
On Wed, 12 Nov 2008, Anton Vorontsov wrote: Gadgets disable endpoints in their disconnect callbacks, so we must call disconnect before unbinding. This also fixes muram memory leak, since we free muram in the qe_ep_disable(). --- a/drivers/usb/gadget/fsl_qe_udc.c +++ b/drivers/usb/gadget/fsl_qe_udc.c @@ -2382,6 +2382,9 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) nuke(loop_ep, -ESHUTDOWN); spin_unlock_irqrestore(udc_controller-lock, flags); + /* report disconnect; the driver is already quiesced */ + driver-disconnect(udc_controller-gadget); + /* unbind gadget and unhook driver. */ driver-unbind(udc_controller-gadget); udc_controller-gadget.dev.driver = NULL; Wouldn't it be better to do this before nuking the existing requests? The comment is wrong; the gadget driver is _not_ quiesced at this point. In fact the disconnect call is what quiesces the driver! And wouldn't it be better to _skip_ doing this if the gadget wasn't connected before? Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb/fsl_qe_udc: Report disconnect before unbinding
On Wed, 12 Nov 2008, Anton Vorontsov wrote: On Wed, Nov 12, 2008 at 11:12:18PM +0300, Anton Vorontsov wrote: On Wed, Nov 12, 2008 at 10:59:16PM +0300, Anton Vorontsov wrote: [...] And wouldn't it be better to _skip_ doing this if the gadget wasn't connected before? Composite framework handles this. If there were no connections, then the disconnect() is a nop (except the spin lock/unlock pair). I'm not sure how the controller driver could tell if there was a connection or not: it doesn't operate these terms. What the udc controller knows is: how to report bus reset and how to receive or transmit the data... It seems I lied. Did you mean something like this patch? diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c index c7de671..fd44cf4 100644 --- a/drivers/usb/gadget/fsl_qe_udc.c +++ b/drivers/usb/gadget/fsl_qe_udc.c @@ -2368,6 +2368,9 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) /* stop usb controller, disable intr */ qe_usb_disable(); + if (udc-usb_state == USB_STATE_DEFAULT) Should be also || usb_state == USB_STATE_ATTACHED, since this is the initial value after the usb_gadget_register_driver(). And the _DEFAULT state is we're just after the bus reset (also means that we already called the disconnect()). That sounds right. Although come to think of it, I guess there really is no harm in calling the disconnect method twice in a row. But it's better to do what the other UDC drivers do. + goto skip_quiesce; + /* in fact, no needed */ udc_controller-usb_state = USB_STATE_ATTACHED; udc_controller-ep0_state = WAIT_FOR_SETUP; @@ -2385,6 +2388,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) /* report disconnect; the driver is already quiesced */ driver-disconnect(udc_controller-gadget); I would update this last comment slightly. The word driver is ambiguous, and since this function is named usb_gadget_unregister_driver(), it looks like you're talking about the gadget driver. +skip_quiesce: /* unbind gadget and unhook driver. */ driver-unbind(udc_controller-gadget); udc_controller-gadget.dev.driver = NULL; Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [USB] powerpc: Workaround for the PPC440EPX USBH_23 errata [take 3]
On Mon, 3 Nov 2008, Vitaly Bordug wrote: A published errata for ppc440epx states, that when running Linux with both EHCI and OHCI modules loaded, the EHCI module experiences a fatal error when a high-speed device is connected to the USB2.0, and functions normally if OHCI module is not loaded. There used to be recommendation to use only hi-speed or full-speed devices with specific conditions, when respective module was unloaded. Later, it was observed that ohci suspend is enough to keep things going, and it was turned into workaround, as explained below. Please fix this patch so that it does not cause a build error on non-PowerPC architectures. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: gigE 2.6.27 USB
On Wed, 15 Oct 2008, Kevin Diggs wrote: I cut a deal with my 8600 to get it to boot 2.6.27. It does not seem to work right either. The error message that shows up in dmesg is (the messages that show up for the gigE appear to be for two different busses (hubs?): hub 1-0:1.0: state 7 ports 2 chg evt hub 1-0:1.0: state 7 ports 2 chg evt hub 1-0:1.0: state 7 ports 2 chg evt hub 1-0:1.0: state 7 ports 2 chg evt A bug was introduced in 2.6.27 and the fix is queued for 2.6.27.stable. The fix takes the form of two patches, which you can find here: http://mirrors.med.harvard.edu/linux/kernel/people/gregkh/gregkh-2.6/gregkh-04-usb/ohci-allow-broken-controllers-to-auto-stop.patch http://mirrors.med.harvard.edu/linux/kernel/people/gregkh/gregkh-2.6/gregkh-04-usb/usb-ohci-fix-endless-polling-behavior.patch The second depends on the first, so you have to apply them both. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata [take 2]
On Tue, 14 Oct 2008, Vitaly Bordug wrote: A published errata for ppc440epx states, that when running Linux with both EHCI and OHCI modules loaded, the EHCI module experiences a fatal error when a high-speed device is connected to the USB2.0, and functions normally if OHCI module is not loaded. There used to be recommendation to use only hi-speed or full-speed devices with specific conditions, when respective module was unloaded. Later, it was observed that ohci suspend is enough to keep things going, and it was turned into workaround, as explained below. Have you tried building this on a non-PPC440EPx system? It looks like this patch would cause a compile error. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
On Fri, 5 Sep 2008, Vitaly Bordug wrote: Not every hub will work (none of available did so far), and it is often not an option for embedded device without rewiring. It's odd that your hubs don't work. What's wrong with them? well, they do not have transaction translators then. Nothing really wrong No. If the hubs run at high speed then they must have transaction translators; it's required by the USB 2.0 specification. Did you try using them with ohci-hcd not loaded? Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
On Fri, 5 Sep 2008, Vitaly Bordug wrote: I've started looking this way. Sorry, but this approach is a little bit fragile, and unreliable. First, it just did not work out in case of usb2 hub with memory stick and keybord plugged - OHCI did not suspend, ehci is hosed and we're happily using hi-speed device on full-speed: Secondly, we may *not* rely on the fact, that OHCI will always have the same suspend policy. Even kicking the code up to the shape when it will automagically suspend in proper timing to get the HW issue around, we cannot be sure that it will persist along kernel lifecycle, and won't require concerned people to kick suspend timings back to the working state subsequently each rc release. Thirdly, PM is disabled by Kconfig explicitly in case of 44x. Reasoning is not clear at the moment, but I believe that isn't there just in case. I assume that's the reason the suggested approach failed. What to do when CONFIG_PM is off is a separate matter. Let's not worry about it for now -- especially since, as Matthias suggested, you can use a USB 2.0 hub. Not every hub will work (none of available did so far), and it is often not an option for embedded device without rewiring. It's odd that your hubs don't work. What's wrong with them? As this touches powerpc stuff only, are there any objections to let powerpc peolple consider if approach suggested earlier is applicable or not? I don't mind doing that, provided the changes are cleaned up so that they don't affect people who aren't building kernels for 44x systems. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
On Fri, 29 Aug 2008, Benjamin Herrenschmidt wrote: I suppose some embedded platforms don't use CONFIG_PM, is this still a requirement for autosuspend ? Or do that happen always on an empty port nowadays ? ohci-hcd doesn't automatically suspend the root hub if CONFIG_PM isn't set. However it isn't necessary to set CONFIG_SLEEP, CONFIG_HIBERNATION, or CONFIG_USB_SUSPEND. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Fri, 29 Aug 2008, Arnd Bergmann wrote: Does building a kernel image that can run on different hardware without rebuilding also violate the relevant standards? No. That isn't what Arnd was concerned about. He noted that even if you did build multiple modules, only one of them could be loaded at any time. Well, actually it was exactly what I was concerned about ;-) The way I understand the code, it is layered into the hardware specific part and the protocol specific part, which are connected through the interfaces I pointed out. That's right. The standard requires that there can only be one protocol handler per physical interface, which is a reasonable limitation. No, you've got it exactly backward. There can be multiple protocol handlers per physical interface, but there must be only one physical interface per device. However, what the Linux implementation actually enforces is that there can only be one hardware specific driver built or loaded into the kernel, which just looks like an arbitrary restriction that does not actually help. Not at all -- it is an implementation of the constraint that there be only one physical interface. If the gadget hardware drivers were registering the device with a gadget_bus_type, you could still enforce the only one protocol rule by binding every protocol to every device in that bus type. There is no such rule. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Fri, 29 Aug 2008, Arnd Bergmann wrote:
On Friday 29 August 2008, Alan Stern wrote:
I thought you _were_ arguing against that. Unless I misunderstood,
your original complaint was that since each peripheral controller
driver defines usb_gadget_{un}register_driver, there can be only one
controller driver loaded at a time.
That's probably where the misunderstanding came from: I did not expect
more than one driver to actually be useful on a given system, but that
should IMHO not prevent you from loading the drivers using modprobe, or
building them all into the kernel.
All right, then we're in agreement. :-)
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
On Fri, 29 Aug 2008, Vitaly Bordug wrote: But even assuming PM set, common use-case of embedded systems to have stuff on USB bus that is never plugged off; and in case of compiled-in ehci and ohci there is just Initializing USB Mass Storage driver... usb 1-1: new high speed USB device using ppc-of-ehci and address 2 usb 1-1: device descriptor read/all, error -110 hub 1-0:1.0: unable to enumerate USB device on port 1 (not touching PM here to be clear) even 1-second delay would be enough for root hub to be hosed... So, is the patch (with all the issues addressed) acceptable for mainline? Try doing this instead. First, make sure CONFIG_PM is set! :-) Then replace your patch with something that adds a 2-second delay to the end of ehci_start() when running on a 440EPx system. That should give the OHCI controller time to suspend before the EHCI root hub is registered. A little more tweaking will be needed to handle system sleeps. But this should be a good start. What to do when CONFIG_PM is off is a separate matter. Let's not worry about it for now -- especially since, as Matthias suggested, you can use a USB 2.0 hub. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, 28 Aug 2008, Arnd Bergmann wrote: Not addressing this driver in particular, but the USB gadget layer in general: This is a horrible interface, since every gadget driver exports the same symbols, you can never build a kernel that includes more than one gadget driver. Even if the drivers are all built as modules, simply loading one of them prevents loading another one. This was done deliberately. The relevant standards state that a USB device can have no more than one peripheral interface. Now, I don't claim to fully support this decision. But there is a reason behind it; it's not just a case of bad design. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
On Thu, 28 Aug 2008, Vitaly Bordug wrote: This doesn't explain why the fatal error occurs. On certain 44x set of SoCs, only one controller is able to function, e.g. technically they are mutually exclusive. There used to be recommendation to use only hi-speed or full-speed devices with specific conditions, when respective module was unloaded. Later, it was observed that ohci suspend is enough to keep things going, and it was turned into workaround, as explained below. Okay, good. _This_ is what you should have put in the patch description, instead of all that other stuff. I have some doubts about parts of this patch. What stands about noted gotchas, I agree and will fix them, thanks for looking through the patch. I agree this doesn't look pleasant, but unfortunately is the only way to say use USB keyboard, and hi-speed memory stick at the same time. Your original post mentioned that the 440EPx has only one USB 2.0 host port. Then how can you use a keyboard and memory stick at the same time? You'd have to plug them into a hub -- in which case only one controller would be needed, the one driving the hub. The patch would be unnecessary. I doubt this will interact properly with usbcore and the rest of ohci-hcd. They do not expect the root hub to be suspended unless they know about it. I need to reemphasize, that upper is not normal, but unfortunately the only way to have both full and hi-speed usb live in peace with 44xEPX family. Quirks are not going to be executed on other chip anyway, and on chip in question, it was tested and works stable enough. I can add an explicit warning, that workaround is being utilised, to make things clear if it will be considered appropriate. Do these systems support CONFIG_PM? If they do then your patch shouldn't be needed, because then ohci-hcd will automatically suspend the OHCI root hub 1 second after the last full/low-speed device is unplugged. You could reduce that 1 second value if you wanted to decrease the probability of conflicts. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, 28 Aug 2008, Scott Wood wrote: Alan Stern wrote: This was done deliberately. The relevant standards state that a USB device can have no more than one peripheral interface. Does building a kernel image that can run on different hardware without rebuilding also violate the relevant standards? No. That isn't what Arnd was concerned about. He noted that even if you did build multiple modules, only one of them could be loaded at any time. And who's to say that there aren't multiple USB devices on a single board, that just happen to share a CPU and memory? :-) That's why I don't fully support this decision. But I wanted to point out that there _was_ a conscious decision, as opposed to bad programming through sheer carelessness. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
On Thu, 28 Aug 2008, Steven A. Falco wrote: Alan Stern wrote: Your original post mentioned that the 440EPx has only one USB 2.0 host port. Then how can you use a keyboard and memory stick at the same time? You'd have to plug them into a hub -- in which case only one controller would be needed, the one driving the hub. The patch would be unnecessary. I have one of these processors on a Sequoia board. What happens is that if you build the kernel with both EHCI and OHCI support, then plug in a modern USB memory stick, it initially tries EHCI, the driver fails, and the whole thing falls back to OHCI. So you wind up running at 12 Mbps. The only way to make high speed work is to turn off the OHCI driver, and then you cannot support slow devices with that kernel. So, hile you cannot plug two devices in at one time, you can plug in different speed devices at different times, and my understanding is that this patch will let that work seamlessly. Is there some reason why it doesn't work already? All the patch does is suspend the OHCI root hub when you plug in the memory stick -- but the root hub should already be suspended. Unless the memory stick is already plugged in when the kernel boots. In which case the root hub won't be suspended -- it won't suspend until 1 second after ohci-hcd initializes the controller. Is that the scenario you're worried about? Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC][USB] powerpc: Workaround for the PPC440EPX USBH_23 errata
-ehci-440epx);
+
+ /* Work around - At this point ohci_run has executed, the
+ * controller is running, everything, the root ports, etc., is
+ * set up. If the ehci driver is loaded, put the ohci core in
+ * the suspended state. The ehci driver will bring it out of
+ * suspended state when / if a non-high speed USB device is
+ * attached to the USB Host port. If the ehci driver is not
+ * loaded, do nothing. request_mem_region is used to test if
+ * the ehci driver is loaded.
+ */
+ if (np != NULL) {
+ ohci-flags |= OHCI_QUIRK_AMCC_USB23;
+ if (!of_address_to_resource(np, 0, res))
+ if (!request_mem_region(res.start, 0x4, hcd_name)) {
+ writel_be((readl_be(ohci-regs-control) |
+ OHCI_USB_SUSPEND), ohci-regs-control);
+ (void) readl_be(ohci-regs-control);
Wrong indentation level.
+ } else {
+ release_mem_region(res.start, 0x4);
+ }
+ else
+ pr_debug(__FILE__ : cannot get ehci offset from fdt\n);
Wrong indentation amount.
+ }
iounmap(hcd-regs);
err_ioremap:
irq_dispose_mapping(irq);
I doubt this will interact properly with usbcore and the rest of
ohci-hcd. They do not expect the root hub to be suspended unless they
know about it.
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [patch v6 3/4] USB: add Cypress c67x00 OTG controller HCD driver
On Wed, 30 Jan 2008, Peter Korsgaard wrote:
+static void c67x00_sched_done(unsigned long __c67x00)
+{
+ struct c67x00_hcd *c67x00 = (struct c67x00_hcd *)__c67x00;
+ struct c67x00_urb_priv *urbp, *tmp;
+ struct urb *urb;
+
+ spin_lock(c67x00-lock);
+
+ /* Loop over the done list and give back all the urbs */
+ list_for_each_entry_safe(urbp, tmp, c67x00-done_list, hep_node) {
+ urb = urbp-urb;
+ c67x00_release_urb(c67x00, urb);
+ if (!usb_hcd_check_unlink_urb(c67x00_hcd_to_hcd(c67x00),
+ urb, urbp-status)) {
Alan The function call above is completely wrong. It is meant to be used
only
Alan from within the dequeue method.
Ahh, so should I just unconditionally do the unlink_urb_from_ep and
giveback_urb?
Yes, that's right. The check_unlink_urb routine merely verifies that
an unlink is valid. If you're about to giveback an URB then you
already know it's valid to do so.
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [linux-usb-devel] [PATCH v2 3/4] USB: add Cypress c67x00 OTG controller HCD driver
On Fri, 28 Dec 2007, Grant Likely wrote:
From: Grant Likely [EMAIL PROTECTED]
This patch adds HDC support for the Cypress c67x00 family of devices.
One minor correction:
+static void c67x00_sched_done(unsigned long __c67x00)
+{
+ struct c67x00_hcd *c67x00 = (struct c67x00_hcd *)__c67x00;
+ struct c67x00_urb_priv *urbp, *tmp;
+ struct usb_hcd *hcd = c67x00_hcd_to_hcd(c67x00);
+ struct urb *urb;
+ int status;
+ unsigned long flags;
+
+ spin_lock_irqsave(c67x00-lock, flags);
+
+ /* Loop over the done list and give back all the urbs */
+ list_for_each_entry_safe(urbp, tmp, c67x00-done_list, hep_node) {
+ urb = urbp-urb;
+ status = urbp-status;
+
+ c67x00_release_urb(c67x00, urb);
+
+ usb_hcd_unlink_urb_from_ep(hcd, urb);
+
+ spin_unlock_irqrestore(c67x00-lock, flags);
+ usb_hcd_giveback_urb(hcd, urb, status);
+ spin_lock_irqsave(c67x00-lock, flags);
The giveback routine is supposed to be called with interrupts disabled.
Consequently you should use spin_unlock() and spin_lock() here, not the
_irqsave/_irqrestore variants.
+ }
+ spin_unlock_irqrestore(c67x00-lock, flags);
+}
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [linux-pm] Re: [RFC] powermac: proper sleep management
On Mon, 12 Nov 2007, Johannes Berg wrote: Looks good to me, +/- a couple of things: - We _REALLY_ want the freezer to be optional and not enabled by default on PowerPC. Maybe make it a compile option ? Well, Alan is going to tell you that USB will break. If we need confirmation for that I can do the test he suggested to you or Paul a while ago. Isn't it true that the freezer _already_ isn't enabled on PPC? So leaving it off wouldn't break anything more than it is broken now. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [linux-pm] Re: [RFC] powermac: proper sleep management
On Tue, 13 Nov 2007, Benjamin Herrenschmidt wrote: On Mon, 2007-11-12 at 17:32 +0100, Johannes Berg wrote: Looks good to me, +/- a couple of things: - We _REALLY_ want the freezer to be optional and not enabled by default on PowerPC. Maybe make it a compile option ? Well, Alan is going to tell you that USB will break. If we need confirmation for that I can do the test he suggested to you or Paul a while ago. Then USB is broken today on powermacs and need to be fixed. We had a clear agreement at KS this year that the freezer was at best a band-aid and that drivers -had- to be fixed to cope regardless. More accurately, freezing user tasks is at best a band-aid. However some kernel threads do need to be frozen, and keeping the freezer around for their use makes sense. It has less overhead -- I think -- than adding new code to do the freezing in each of these threads. (It's true that USB drivers in general aren't written to operate in a freezerless system-suspend environment. That's a harder problem and it will have to be fixed driver-by-driver, over time. The same may be true for lots of non-USB char device drivers as well. Pick your favorite char device driver: How will it behave if a user task submits an I/O request after the device has been suspended?) Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [linux-pm] Re: [RFC] powermac: proper sleep management
On Tue, 13 Nov 2007, Benjamin Herrenschmidt wrote: I remember fixing various issues so that khubd would be safe when non frozen (among other things) a while ago. Did you guys break it all again ? Sorry, I don't know what fixes you're referring to. But come to think of it, some changes I added to the PM core a few months ago (locking every device prior to system sleep) would also make khubd and ksuspend_usbd safe, since they acquire the device lock before trying to resume anything. So I take back what I said. It's no longer true that those two USB kernel threads need to be frozen for system sleep. (I should test and make sure that really works...) But aren't there other kernel threads scattered around that still want to be frozen? For instance, drivers/misc/tifm_core.c calls create_freezeable_workqueue(). And set_freezable() gets called in lots of places. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
