Alan Stern wrote:
On Fri, 22 Jan 2010, Jan Andersson wrote:
Why is this a problem? With the exception of some old code in
ehci-au1xxx.c, this register is always read via ehci_readl(), which
automatically takes the endianness into account.
This issue would not have come up if CAPLENGTH and HCIVERSION were
specified as members of a 32-bit register (as most of the other
registers are). If you do not agree with me think of the case where
CAPLENGTH, as specified, is read out using a byte access. The
specification says that it is located at offset 0. Should a 32-bit read
on a big endian system return CAPLENGTH on any other position that 31:24
on a 32-bit read to offset 0 then that byte will not be located at the
specified address.
So change the places that compute HC_VERSION and HC_LENGTH. Instead of
doing
HC_VERSION(ehci_readl(ehci, &ehci->caps->hc_capbase))
make them do
HC_VERSION(le32_to_cpu(readl(&ehci->caps->hc_capbase)));
and similarly for HC_LENGTH. That'll do what you want, won't it?
Apart from bus glue, these things occur only once in ehci-dbg.c and
ehci-hcd.c.
Include a comment explaining that ehci_readl() can't be used here
because the register is defined in terms of byte offsets rather than as
a 32-bit word.
That is a solution that is more beautiful than what I first came up with
and it should fix the problem in my case for big endian mode. However,
when running with the controller hardware in little endian mode (where
it essentially byte swaps all the registers since this is a big endian
system) and ehci->big_endian_mmio = 0, ehci_readl will byte swap back
the data for me and return a value that is suitable for the macros.
Introducing a le32_to_cpu would swap the value one more time..
I am also worried that the addition of le32_to_cpu() could break the
existing drivers. If a driver works today on a _big endian_ cpu,
wouldn't the addition of le32_to_cpu() break it? I'm guessing that this
can be an issue for ehci-ppc-of? - But I may be wrong..
I am unable to produce a (software) solution that does not require user
configuration other than my first suggestion of assigning ehci->version
and ehci->caplength in the initialization code and replacing HC_LENGTH
and HC_VERSION with these variables. Is there any chance of such a patch
getting accepted?
There is also another solution; Since I work for the vendor of this host
controller, I could redefine the host controller hardware to use a
"little endian layout" of capbase, even when in big endian mode. I
suppose that this could be acceptable if this is what other vendors have
done. On the other hand, it is not what is described by the
specification and it is possible that this issue comes up again when
someone wants to add support for yet another big endian host controller
implementation..
Best regards,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
