Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Paul Mackerras]

My G4 powerbook gets a machine check on boot as a result of commit
478a3bab8c87a9ba4a4ba338314e32bb0c378e62.  Putting a return at the
start of quirk_usb_early_handoff fixes it.

The code in quirk_usb_handoff_ohci looks rather bogus in that it
doesn't do pci_enable_device before trying to access the device.

Paul.

Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Benjamin Herrenschmidt]

On Mon, 2005-10-31 at 16:23 +1100, Paul Mackerras wrote:
> My G4 powerbook gets a machine check on boot as a result of commit
> 478a3bab8c87a9ba4a4ba338314e32bb0c378e62.  Putting a return at the
> start of quirk_usb_early_handoff fixes it.
> 
> The code in quirk_usb_handoff_ohci looks rather bogus in that it
> doesn't do pci_enable_device before trying to access the device.

That and it doesn't test if the BARs are assigned at all, doesn't
request the resources, etc... 

I'm not sure it's legal to do pci_enable_device() from within a pci
quirk anyway. I really wonder what that code is doing in the quirks, I
don't think it's the right place, but I may be wrong.

What is the logic supposed to be there ?

Ben.

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[David Brownell]

On Monday 31 October 2005 4:16 pm, Benjamin Herrenschmidt wrote:
> On Mon, 2005-10-31 at 16:23 +1100, Paul Mackerras wrote:
> > My G4 powerbook gets a machine check on boot as a result of commit
> > 478a3bab8c87a9ba4a4ba338314e32bb0c378e62.  Putting a return at the
> > start of quirk_usb_early_handoff fixes it.
> > 
> > The code in quirk_usb_handoff_ohci looks rather bogus in that it
> > doesn't do pci_enable_device before trying to access the device.

No PCI quirk code has ever called pci_enable_device() AFAICT.

Of course the _need_ to do such a thing might be another PPC-specific
(or OpenFirmware-specific?) PCI thing ... we've hit other cases where
PPC breaks things that work on other PCI systems (and vice versa).


> That and it doesn't test if the BARs are assigned at all, doesn't
> request the resources, etc... 

If quirk code can't rely on BARs, then the PCI system needs some
basic overhauls ... yes?  I mean, how could quirk code ever work
if it can't access the relevant chip registers??


> I'm not sure it's legal to do pci_enable_device() from within a pci
> quirk anyway. I really wonder what that code is doing in the quirks, I
> don't think it's the right place, but I may be wrong.

Erm, what "code is doing" what, that you mean ??


> What is the logic supposed to be there ?

Which logic?  The fundamental thing those USB handoff functions do
is make sure that BIOS code lets go of the host controllers.  The
main reason it'd be using a controller is because of USB keyboards,
mice, or maybe boot disks.  Secondarily, that code needs to make
sure the controller is really quiesced before Linux starts using it.

- Dave

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Benjamin Herrenschmidt]

> No PCI quirk code has ever called pci_enable_device() AFAICT.

Most PCI quirks only do config space accesses

> Of course the _need_ to do such a thing might be another PPC-specific
> (or OpenFirmware-specific?) PCI thing ... we've hit other cases where
> PPC breaks things that work on other PCI systems (and vice versa).

"ppc" doens't do anything fancy that other archs don't do too, please
stop with your "ppc specific" thing all over the place.

It is illegal, whatever the platform is, to tap a PCI device MMIO like
that without calling pci_enable_device(), requesting resources etc... or
at the very least, testing if MMIO decoding is enabled on the chip.
Period. It has nothing to do with PPC and all to do with correctness.

> If quirk code can't rely on BARs, then the PCI system needs some
> basic overhauls ... yes?  I mean, how could quirk code ever work
> if it can't access the relevant chip registers??

Most quirk only ever use config space. BARs are _not_ guaranteed to be
set at quirk time. In fact, those devices are left disabled on purpose,
thus you should at least test if MMIO access is enabled, and if not,
avoid touching the device at all.

> > I'm not sure it's legal to do pci_enable_device() from within a pci
> > quirk anyway. I really wonder what that code is doing in the quirks, I
> > don't think it's the right place, but I may be wrong.
> 
> Erm, what "code is doing" what, that you mean ??

What _That_ code is doing in the quirks... shouldn't it be in the
{U,O,E}HCI drivers instead ?

> > What is the logic supposed to be there ?
> 
> Which logic?  The fundamental thing those USB handoff functions do
> is make sure that BIOS code lets go of the host controllers.  The
> main reason it'd be using a controller is because of USB keyboards,
> mice, or maybe boot disks.  Secondarily, that code needs to make
> sure the controller is really quiesced before Linux starts using it.

So you rant about "ppc specific" whatever while the entire point of this
code is to workaround x86 specific BIOS junk ...

Ben.

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[David Brownell]

On Monday 31 October 2005 6:41 pm, Benjamin Herrenschmidt wrote:
> 
> > No PCI quirk code has ever called pci_enable_device() AFAICT.
> 
> Most PCI quirks only do config space accesses

Some do I/O space access.  Few do memory space access (ioremap_nocache).


> > Of course the _need_ to do such a thing might be another PPC-specific
> > (or OpenFirmware-specific?) PCI thing ... we've hit other cases where
> > PPC breaks things that work on other PCI systems (and vice versa).
> 
> "ppc" doens't do anything fancy that other archs don't do too, please
> stop with your "ppc specific" thing all over the place.

When the only problem reports come from PPC hardware, it sure looks
PPC-specific to me.  If such issues get reported on non-PPC hardware
(with those unique-to-ppc changes to PCI enumeration) then I'll stop
thinking of it as PPC-specific.  Until then ... ;)


> It is illegal, whatever the platform is, to tap a PCI device MMIO like
> that without calling pci_enable_device(), requesting resources etc... or
> at the very least, testing if MMIO decoding is enabled on the chip.
> Period. It has nothing to do with PPC and all to do with correctness.

I could easily believe that all that quirk code has been buggy since
day one, yes.  Certainly it's always had bugs in how it dealt with the
USB functionality; so why shouldn't it have bugs in how it deals with
the PCI functionality too?  Even if it was being maintained by the
PCI maintainers!


> > > I'm not sure it's legal to do pci_enable_device() from within a pci
> > > quirk anyway. I really wonder what that code is doing in the quirks, I
> > > don't think it's the right place, but I may be wrong.
> > 
> > Erm, what "code is doing" what, that you mean ??
> 
> What _That_ code is doing in the quirks... shouldn't it be in the
> {U,O,E}HCI drivers instead ?

Not for PCI.  Vojtech, this is your cue to explain some of how late handoff
borks the input layer, as observed by SuSE on way too many BIOS/hardware combos
for me to remember ... :)


> > > What is the logic supposed to be there ?
> > 
> > Which logic?  The fundamental thing those USB handoff functions do
> > is make sure that BIOS code lets go of the host controllers.  The
> > main reason it'd be using a controller is because of USB keyboards,
> > mice, or maybe boot disks.  Secondarily, that code needs to make
> > sure the controller is really quiesced before Linux starts using it.
> 
> So you rant about "ppc specific" whatever while the entire point of this
> code is to workaround x86 specific BIOS junk ...

Actually any "sophisticated" boot loader nowadays will know something
about USB, to handle keyboards, mice, or maybe boot disks.  (Didn't I
just write that?)  On some platforms, u-Boot understands OHCI ... so that's
not just x86 BIOS or other closed-source firmware.  (Though to be sure,
that u-Boot code acts more like Linux 2.4 than anything else; it doesn't
follow the standard firmare-uses-USB rules.)  And I sure thought some of
the OpenFirmware systems had USB support too.  (Written in FORTH?)

- Dave

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Benjamin Herrenschmidt]

On Mon, 2005-10-31 at 19:09 -0800, David Brownell wrote:
> On Monday 31 October 2005 6:41 pm, Benjamin Herrenschmidt wrote:
> > 
> > > No PCI quirk code has ever called pci_enable_device() AFAICT.
> > 
> > Most PCI quirks only do config space accesses
> 
> Some do I/O space access.  Few do memory space access (ioremap_nocache).

The IO accesses probably work fine for some legacy stuff where the ISA
space is actually available, but they are broken if they don't check for
availability, same goes for memory space access.

Damn, those quirks should really be either more careful or be made
platform specific if they are x86 junk workarounds.

> > > Of course the _need_ to do such a thing might be another PPC-specific
> > > (or OpenFirmware-specific?) PCI thing ... we've hit other cases where
> > > PPC breaks things that work on other PCI systems (and vice versa).
> > 
> > "ppc" doens't do anything fancy that other archs don't do too, please
> > stop with your "ppc specific" thing all over the place.
> 
> When the only problem reports come from PPC hardware, it sure looks
> PPC-specific to me

Bla bla bla bla... can you stop the crackpipe please ?

>  If such issues get reported on non-PPC hardware
> (with those unique-to-ppc changes to PCI enumeration) then I'll stop
> thinking of it as PPC-specific.  Until then ... ;)

I prefer not replying...

> > It is illegal, whatever the platform is, to tap a PCI device MMIO like
> > that without calling pci_enable_device(), requesting resources etc... or
> > at the very least, testing if MMIO decoding is enabled on the chip.
> > Period. It has nothing to do with PPC and all to do with correctness.
> 
> I could easily believe that all that quirk code has been buggy since
> day one, yes.

Probably. Hopefully, it usually is specifically targeted to chpisets
that often exist only on x86 where they happen to work. Your USB quirk
happen to be broad enough to affect pretty much any platform with USB in
it, and thus the brokenness appears more widely.

>   Certainly it's always had bugs in how it dealt with the
> USB functionality; so why shouldn't it have bugs in how it deals with
> the PCI functionality too?  Even if it was being maintained by the
> PCI maintainers!

More bla bla bla ...

> > What _That_ code is doing in the quirks... shouldn't it be in the
> > {U,O,E}HCI drivers instead ?
> 
> Not for PCI.  Vojtech, this is your cue to explain some of how late handoff
> borks the input layer, as observed by SuSE on way too many BIOS/hardware combos
> for me to remember ... :)

Well, at the _very_ least then read the PCI command register and check
that memory access is enabled before going to your quirk.

> Actually any "sophisticated" boot loader nowadays will know something
> about USB, to handle keyboards, mice, or maybe boot disks.  (Didn't I
> just write that?)  On some platforms, u-Boot understands OHCI ... so that's
> not just x86 BIOS or other closed-source firmware.  (Though to be sure,
> that u-Boot code acts more like Linux 2.4 than anything else; it doesn't
> follow the standard firmare-uses-USB rules.)  And I sure thought some of
> the OpenFirmware systems had USB support too.  (Written in FORTH?)

Yes, it has, and it properly shuts things down before calling the
operating system, thus doesn't require those "handoff" hacks.

Ben.

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[David Brownell]

> > > "ppc" doens't do anything fancy that other archs don't do too, please
> > > stop with your "ppc specific" thing all over the place.
> > 
> > When the only problem reports come from PPC hardware, it sure looks
> > PPC-specific to me.
> 
> Bla bla bla bla... can you stop the crackpipe please ?

Maybe you should first pay attention to what I pointed out:  that
the problem reports I've seen have ONLY been on PPC systems.

Like the powerbook in $SUBJECT ... people without PPCs are unlikely
to hit such issues, judging by the evidence so far.  (Though of course
it'd be possible...)


> >                   If such issues get reported on non-PPC hardware
> > (with those unique-to-ppc changes to PCI enumeration) then I'll stop
> > thinking of it as PPC-specific.  Until then ... ;)


I guess one point to be made is that although x86 gets the most testing
right away, PPC lately isn't far behind.  Latent bugs in usb-handoff
logic got surfaced by this patch ... there could be others.  I suspect
nobody except x86 users have been running that code much at all.

- Dave

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Paul Mackerras]

David Brownell writes:

> Maybe you should first pay attention to what I pointed out:  that
> the problem reports I've seen have ONLY been on PPC systems.

Well, there is a problem in the code which is clearly visible just by
inspection: that it is touching a pci device without having called
pci_enable_device on it.  That is well known to cause problems on many
platforms, and it is not guaranteed to work on any platform.

With a clearly visible bug like that in there, it doesn't matter what
platform(s) the problem is reported on.

Paul.

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Benjamin Herrenschmidt]

On Tue, 2005-11-01 at 15:52 +1100, Paul Mackerras wrote:
> David Brownell writes:
> 
> > Maybe you should first pay attention to what I pointed out:  that
> > the problem reports I've seen have ONLY been on PPC systems.
> 
> Well, there is a problem in the code which is clearly visible just by
> inspection: that it is touching a pci device without having called
> pci_enable_device on it.  That is well known to cause problems on many
> platforms, and it is not guaranteed to work on any platform.
> 
> With a clearly visible bug like that in there, it doesn't matter what
> platform(s) the problem is reported on.

Yup, though I agree that considering the purpose of that code, it might
make sense for it to just "peek" to check if the device was enabled
rather than force-enabling it. If it was not, there is obviously no
handoff to do from the BIOS.

Ben.

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Alan Cox]

On Maw, 2005-11-01 at 14:30 +1100, Benjamin Herrenschmidt wrote:
> Damn, those quirks should really be either more careful or be made
> platform specific if they are x86 junk workarounds.

USB handoff is fairly x86 specific. The x86 folks took great care to
handle back compatibility while Apple was content to just dump the users
and machines.

> > > It is illegal, whatever the platform is, to tap a PCI device MMIO like

Not "illegal" -> "invalid".

Please get that right as we have far too many incorrect uses of
"illegal" in publically visible printk calls. Illegal means "prohibited
by law".

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Glenn Maynard]

On Tue, Nov 01, 2005 at 09:28:14AM +0000, Alan Cox wrote:
> > > > It is illegal, whatever the platform is, to tap a PCI device MMIO like
> 
> Not "illegal" -> "invalid".
> 
> Please get that right as we have far too many incorrect uses of
> "illegal" in publically visible printk calls. Illegal means "prohibited
> by law".

"Illegal" means "prohibited"; not necessarily "by law", though that's the
most common use.  It's valid in other contexts, such as "prohibited by
the API".  "int *p = 1.5f" is illegal C code.  Google for "illegal chess
moves".

There may be political reasons to prefer "invalid" to "illegal", but that
doesn't make it incorrect.

-- 
Glenn Maynard

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Benjamin Herrenschmidt]

On Tue, 2005-11-01 at 09:28 +0000, Alan Cox wrote:
> On Maw, 2005-11-01 at 14:30 +1100, Benjamin Herrenschmidt wrote:
> > Damn, those quirks should really be either more careful or be made
> > platform specific if they are x86 junk workarounds.
> 
> USB handoff is fairly x86 specific. The x86 folks took great care to
> handle back compatibility while Apple was content to just dump the users
> and machines.

What is this comment supposed to mean ? Backward compatiblity on Macs is
very high, thanks to mostly software not relying on stupid & broken
hardware implementation details...

> Not "illegal" -> "invalid".
> 
> Please get that right as we have far too many incorrect uses of
> "illegal" in publically visible printk calls. Illegal means "prohibited
> by law".

Oh well, whatever you say..

Ben.

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Dmitry Torokhov]

On Monday 31 October 2005 22:09, David Brownell wrote:
> > > > I'm not sure it's legal to do pci_enable_device() from within a pci
> > > > quirk anyway. I really wonder what that code is doing in the quirks, I
> > > > don't think it's the right place, but I may be wrong.
> > > 
> > > Erm, what "code is doing" what, that you mean ??
> > 
> > What _That_ code is doing in the quirks... shouldn't it be in the
> > {U,O,E}HCI drivers instead ?
> 
> Not for PCI.  Vojtech, this is your cue to explain some of how late handoff
> borks the input layer, as observed by SuSE on way too many BIOS/hardware combos
> for me to remember ... :)
> 

Not Vojtech, but here is goes... Not everyone has USB compiled in and
even then I think USB is registered after serio. So when we probe for
i8042 BIOS still has its dirty hands on USB controllers and pretends
that they are in fact PS/2 devices. Crazy stuff like that... That's
why we can't keep that code in HCI drivers. 

-- 
Dmitry

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[Kyle Moffett]

On Oct 31, 2005, at 22:09:32, David Brownell wrote:
>>> Which logic?  The fundamental thing those USB handoff functions  
>>> do is make sure that BIOS code lets go of the host controllers.   
>>> The main reason it'd be using a controller is because of USB  
>>> keyboards, mice, or maybe boot disks.  Secondarily, that code  
>>> needs to make sure the controller is really quiesced before Linux  
>>> starts using it.
>>
>> So you rant about "ppc specific" whatever while the entire point  
>> of this code is to workaround x86 specific BIOS junk ...
>
> Actually any "sophisticated" boot loader nowadays will know  
> something about USB, to handle keyboards, mice, or maybe boot disks.

OpenFirmware is quite knowledgeable about USB devices, both disks,  
mice, keyboards, and IIRC there's even a USB<=>serial bridge useable  
as an OpenFirmware console.

> On some platforms, u-Boot understands OHCI ... so that's not just  
> x86 BIOS or other closed-source firmware.

On other platforms, OpenFirmware supports direct ELF loading without  
any extra code.  If you want initrd support, you need a little Forth  
script (IE: yaboot) to load it into some RAM first.

The difference is, OpenFirmware is nice and clean and stops messing  
with hardware before handing off to the new kernel.  If you ever try  
to boot from an invalid ELF file on an OpenFirmware machine, you'll  
see that's fairly obvious, because the screen flashes and changes  
state slightly during the failed boot attempt (after which it  
reconnects to the hardware again to display messages).

Why should x86-specific-BIOS-USB-handoff-specific-crap-PCI-quirks be  
even _compiled_ on PowerPC systems that have nothing remotely like  
the affected hardware (BIOS & PS/2 serio chip)?

Cheers,
Kyle Moffett

--
Simple things should be simple and complex things should be possible
   -- Alan Kay

Re: [linux-usb-devel] Re: Commit "[PATCH] USB: Always do usb-handoff" breaks my powerbook

[David Brownell]

> Why should x86-specific-BIOS-USB-handoff-specific-crap-PCI-quirks be  
> even _compiled_ on PowerPC systems that have nothing remotely like  
> the affected hardware (BIOS & PS/2 serio chip)?

For starters, none of the controller specs say that the handshaking
is x86-specific.  There's a certain amount of "x86 Linux gets the
most testing" going on here.  Plus a lot of "nobody really used that
usb-handoff code before, except to fix semi-broken x86 systems".

One requirement coming from x86/DOS legacy support though is that the
system probably expects to "work like DOS" at various boot stages.
Hence the way some systems take kbd/mouse input from USB and jam it
through PS2 serio hardware, so DOS will see it.  Which is why x86
hardware generally _does_ need to use these handhaking mechanisms,
to kick the BIOS off the hardware.  (And why the USB folk have been
very used to telling folk to disable BIOS support for USB.  That's
fine advice unless you've got a USB keyboard or mouse.)


> The difference is, OpenFirmware is nice and clean and stops messing  
> with hardware before handing off to the new kernel. 

That's a nice design policy (IMO) but sometimes folk also like to
draw the firmware/OS boundary in different ways.

In any case ... let's all just blame this on DOS, and move on to
something that's not as twentieth-century.  :)

- Dave