Re: [20/80] ALSA: lx6464es - fix device communication via command bus

[Linus Torvalds]

On Wed, Dec 7, 2011 at 8:06 AM, Greg KH <gregkh@xxxxxxx> wrote:
>
> From: Tim Blechmann <tim@xxxxxxxxxx>
>
> commit a29878553a9a7b4c06f93c7e383527cf014d4ceb upstream.
>
> commit 6175ddf06b6172046a329e3abfd9c901a43efd2e optimized the mem*io
> functions that have been used to send commands to the device. these
> optimizations somehow corrupted the communication with the lx6464es,
> that resulted the device to be unusable with kernels after 2.6.33.

Uhhuh. Looking at this, I understand why the driver can't really use
memcpy_toio/fromio any more, and I never noticed this problem because
it was worked around in drivers instead (or being discussed elsewhere
and I just overlooked it)

That commit (6175ddf06b61) is really bad and buggy. It probably wasn't
horribly bad back when it was done, but it's getting increasingly bad
as we change how "memcpy()" works.

For example, memcpy some day will be just "rep movsb" on newer CPU's
("enhanced fast string memcpy"), which can be optimal for memory. But
it is completely unacceptable for IO devices, so saying that "Iomem
has no special significance on x86" is just total crap.

iomem can work with regular accessors, yes, but it still has
significance even on x86: iomem is *not* RAM.

And using memcpy() is wrong, wrong, wrong. It's wrong even aside from
any future "rep movsb" issues - it's already wrong due to the crazy
"optimized x86 memcpy" that works around Atom crap. Copying stuff
backwards is not what memcpy_toio/fromio is supposed to do, partly
because it can confuse devices, but partly simply because it can
easily destroy things like PCI bursting etc. I also would not be
totally shocked to hear that some devices dislike 8-byte writes.

So I understand why the driver works around the issue, but I think the
real fix is to undo much of that broken commit in the first place.

So we should probably make memcpy_fromio/toio do the
"__inline_memcpy()" loop that we used to use: "rep movsl" followed by
conditional movsw/movsb. No, it's not necessarily optimal, but it's
*safe*, and it's quite likely *more* optimal than memcpy that may well
end up doing it a byte at a time!

So I think we should just re-instate "arch/x86/lib/io.c, copy the
"inline_memcpy" thing in there, and add a similar "rep stosl" version
for the "memset_io()". IOW, undo most of 6175ddf06b61, just make it
use the same code for 32-bit and 64-bit.

Ingo, Peter, Thomas? Comments?

                                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/