Modify

Opened 7 years ago

Closed 4 years ago

Last modified 2 years ago

#6425 closed defect (fixed)

Fix broken ehci-ssb USB host (quilt updates)

Reported by: andi@… Owned by: hauke
Priority: high Milestone: Barrier Breaker 14.07
Component: kernel Version: Trunk
Keywords: usb ohci ehci host controller mipsel bcm5354 Cc: mb@…, sbrown@…

Description

Hi,

following are two parts:

One part is a new quilt series file (trunk/target/linux/brcm47xx/patches-2.6.31/285-ohci-ssb_fix_ohci-only_build.patch) to fix build in case ehci-ssb build (CONFIG_USB_EHCI_HCD_SSB) is disabled.

The other part is a diff to trunk/target/linux/brcm47xx/patches-2.6.31/270-ehci-ssb.patch
It un-isolates ssb_ehci_attach()/ssb_ehci_detach() as called by the ohci-ssb.c side of things.
It also adds all _missing_ updates to ehci-ssb.c since we last were in sync with mainline: mainline added some crucial callbacks which were then missing on our side and caused ehci_endpoint_disable() to loop due to a missing .clear_tt_buffer_complete callback not doing its thing. This caused all plugging/unplugging of USB devices to break after one device (such as my serial converters) had been open()ed.

While these patches are known to work on 2.6.31(.9), older kernels (something like 2.6.27+) most certainly suffer from the same problem (not sure whether one should backport or simply skip it).

Note that my svn tree is from around end of November since December killed kernel booting (perhaps due to lzma compression), thus I decided to skip updating in order to _finally_ have (and keep!) a configuration without any issues.

Set to priority high since the entire platform (WL-500gP v2, and other BCM5354 devices) is only "moderately" usable (almost(?) no USB gadgets working) without this fix.

Thanks,

Signed-off-by: Andreas Mohr <andi@…>

Attachments (2)

285-ohci-ssb_fix_ohci-only_build.patch (463 bytes) - added by andi@… 7 years ago.
Fix ohci-ssb.c build in case ehci-ssb.c build (CONFIG_USB_EHCI_HCD_SSB) is disabled.
270-ehci-ssb.patch.diff (4.0 KB) - added by andi@… 7 years ago.
Un-isolate ehci-ssb.c function exports required by ohci-ssb.c, re-sync to add missing mainline changes

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by andi@…

Fix ohci-ssb.c build in case ehci-ssb.c build (CONFIG_USB_EHCI_HCD_SSB) is disabled.

Changed 7 years ago by andi@…

Un-isolate ehci-ssb.c function exports required by ohci-ssb.c, re-sync to add missing mainline changes

comment:1 Changed 7 years ago by mb

What build problem does the build fix fix? Can you post the compilation error message, please?

comment:2 Changed 7 years ago by anonymous

Hmm, not sure whether I want to do this now. I'm currently doing all my builds on a 16GB SD card (don't ask...), and I already have a build finished to test usb-audio mmap fix - but only once I actually get to test it (quite soon, fortunately). Wouldn't really want to disrupt my current build into something unusable.
The problem (linker error, not compile error) probably is kind of obvious anyway, due to the missing but specified exports.

Thanks for your fast reply!

comment:3 Changed 7 years ago by mb

Well, the thing is that ifdefing out a function declaration is _discouraged_ by the kernel coding style and there's almost _never_ a real reason to ifdef out a function declaration.

Additionally I don't see how this could break compilation. Does it break because struct usb_hcd or something like that is undefined? If that's the case, we're simply _missing_ and #include or missing that declaration. In this case we'd just include the file or add
struct usb_hcd;
just in front of the function declarations.

comment:4 Changed 7 years ago by anonymous

OK, you're darn right. This stuff should be purely header-based (thus
we can cleanly choose whether to use this header and its interface or not)
and not some unreliable source-ifdef-extern-based crap which is unable to validate correctness due to its distributed nature. Hrmm, OTOH we don't have any headers in this area (yet?)...

I don't quite see how it can break compilation either, e.g. in this tiny sample:

$ cat import.c
extern void imported_blubb();

main()
{}

which works despite an undefined import.
However I guess once linking _multiple_ build units together the linker will start complaining about undefined imports, as it did in my case.

I will have to analyze it further soon, I'm afraid ;)
Will do so soon, once I tested my current build.
Thanks!

comment:5 Changed 7 years ago by mb

This stuff should be purely header-based (thus we can cleanly choose whether to use this header and its interface or not) and not some unreliable source-ifdef-extern-based crap which is unable to validate correctness due to its distributed nature. Hrmm, OTOH we don't have any headers in this area (yet?)...

Well, I didn't say that. It's OK to have function declarations in C files, if these are only two or three.

I don't quite see how it can break compilation either

A declaration does not affect the object code (besides debugging into).
So the only way I can see this could possibly fail is a missing struct usb_hcd declaration. If that's the case, do the following pseudo patch:

--- linux-2.6.30.9/drivers/usb/host/ohci-ssb.c.orig 2009-11-24 22:42:45.000000000 +0100
+++ linux-2.6.30.9/drivers/usb/host/ohci-ssb.c 2009-11-24 22:43:23.000000000 +0100
@@ -17,8 +17,10 @@

*/

#include <linux/ssb/ssb.h>


+struct usb_hcd;

extern int ssb_ehci_attach(struct ssb_device *dev, struct usb_hcd hcd);
extern void ssb_ehci_detach(struct ssb_device *dev, struct usb_hcd *hcd);


#define SSB_OHCI_TMSLOW_HOSTMODE (1 << 29)


comment:6 Changed 7 years ago by sbrown

I built only ohci both as a module and built-in and was unable to provoke a complaint from either the compiler or linker. Both builds flashed and ran on my wl520gu.

The ifdef's don't seem to be required.

comment:7 Changed 7 years ago by andi@…

Hmm, weird stuff. I definitely need to re-build this to provide proof.
However I wouldn't have patched it if it wasn't required on my side...
(and it definitely was required, I _did_ have failures there).
Or maybe the failure happens to be kernel-version-dependent? (quilt patch differences?). Anyway, will test it soon.

comment:8 Changed 7 years ago by mb

Note that I fixed an ehci related build issue (related to these two declarations) with r17535 in september. Maybe that already fixed the build issue for you, too.

comment:9 Changed 6 years ago by thepeople

  • Owner changed from developers to hauke
  • Status changed from new to assigned

comment:10 Changed 6 years ago by hauke

  • Resolution set to fixed
  • Status changed from assigned to closed

Thank you for your patch.

The compile error seems to be fixed.

The other problem should be fixed in r21427

You are right the ehci system changed and needs some additional function pointers now.

comment:11 Changed 6 years ago by anonymous

I am running Backifire on a Asus WL500GP v1:

Backfire (10.03.1-rc4, r24045)

When I install the ohci module, it does not load and complains:

ohci_hcd: Unknown symbol ssb_ehci_detach
ohci_hcd: Unknown symbol ssb_ehci_attach

Any idea how to solve that?

comment:12 Changed 6 years ago by hauke

You also have to install the ssb-ehci module, but the Asus WL500GP v1 uses a usb controller connected to the pci controller and not the one directly integrated into ssb, only v2 does so.

comment:13 Changed 5 years ago by anonymous

  • Resolution fixed deleted
  • Status changed from closed to reopened

Also Backfire (10.03.1-rc4, r24045) on WL-HDD (10.03 worked the same):

# insmod /lib/modules/2.6.32.25/ohci-hcd.ko
insmod: can't insert '/lib/modules/2.6.32.25/ohci-hcd.ko': unknown symbol in module, or unknown parameter
# dmesg |tail -n2
ohci_hcd: Unknown symbol ssb_ehci_detach
ohci_hcd: Unknown symbol ssb_ehci_attach

There's no module ssb-ehci in the feeds:

# opkg list|grep ssb
kmod-ocf-ubsec-ssb - 2.6.32.25+2009-02-21-1 - This package contains the OCF driver for the BCM5365p IPSec Core

Nor ehci:

# opkg list|grep ehci
#

comment:14 Changed 5 years ago by Paul Sokolovsky <pmiscml@…>

Previous post and reopen is by me. It would be nice to have this fixed. Thanks.

comment:15 Changed 5 years ago by Paul Sokolovsky <pmiscml@…>

Ok, so additional module which is needed here is *ehci-hcd.ko*, which lives in kmod-usb2 package. One that installed (tested on 10.03.1-rc4), USB devices get recognized.

So, the problem here is lack of proper package dependencies. And that's a bit tricky, because apparently such dependency (kmod-usb-ohci on kmod-usb2) only needed for few devices. And it's pretty funky, because ohci is USB1.1, so we kinda say that USB1.1 depends on USB2.0. That suggests that route cause and ultimate fix for that is factoring out offending functions (ssb_ehci_attach/ssb_ehci_detach) and putting in more appropriate place than ehci-hcd.

Short-term solution is of course document around this quirk - in all places where poor users could be stuck otherwise.

comment:16 Changed 5 years ago by florian

  • Status changed from reopened to assigned

comment:17 Changed 4 years ago by hauke

  • Resolution set to fixed
  • Status changed from assigned to closed

This was fixed in trunk in r29575.
Now it is possible to just load ehci or just ohci support.
I do not want to backport this fix to backfire, just install ehci and ohci driver.

comment:18 Changed 2 years ago by jow

  • Milestone changed from Attitude Adjustment 12.09 to Barrier Breaker 14.07

Milestone Attitude Adjustment 12.09 deleted

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.