Super Top USB SD card reader resets and corruption - XHCI
Hi, I came across a problem with an USB SD card reader apparently causing USB resets and corrupting data on the card. The reader in question is a very cheap: 14cd:1212 Super Top microSD card reader (SY-T18) The card is an old SDHC, 32 GB, Sandisk brand. The problems don't seem to exist now if I connect the reader to an EHCI-equipped machine (an old Core2 duo + ICH10). I can read and write MBs of data, however older logs suggest that this computer also had the issue in the past (with Linux 4.6.3-300.fc24.x86_64, with both EHCI and UHCI). I can't use it with an XHCI. Both: Intel Corp 8 Series/C220 Series Chipset Family USB xHCI (rev 04) (prog-if 30 [XHCI]) (SandyBridge laptop with only USB2.0 connectors) and: Intel Corp Sunrise Point-H USB 3.0 xHCI Controller (rev 31) (a Skylake + Z170 and USB3, though the reader is USB2) have the issue. I have spotted the following writted to the SD card, I guess it's some USB mass storage write command: 3d085000 55 53 42 43 ca 08 00 00 00 e0 01 00 00 00 0a 2a |USBC...*| 3d085010 00 00 1e 83 70 00 00 f0 00 00 00 00 00 00 00 b2 |p...| 3d085020 2d 00 38 00 a9 c8 f4 ab 10 8d cc 39 e1 8f 43 45 |-.89..CE| I think the problem doesn't manifest itself if I don't write (RO mount) to the SD card. However, even light writes seem to trigger the issue. The kernels are basically recent Fedora, 4.7.4-200.fc24.x86_64, 4.7.9-200.fc24.x86_64 but it seems e.g. 4.6.3-300.fc24.x86_64 was also affected. The one which doesn't cause problems now (EHCI) is 4.7.2-201.fc24.x86_64 (though EHCI did cause problems previously). Could it be faulty hw? What can I do with this? [80125.418020] usb 1-1: new high-speed USB device number 9 using xhci_hcd [80125.583307] usb 1-1: New USB device found, idVendor=14cd, idProduct=1212 [80125.583315] usb 1-1: New USB device strings: Mfr=1, Product=3, SerialNumber=2 [80125.583320] usb 1-1: Product: Mass Storage Device [80125.583324] usb 1-1: Manufacturer: Generic [80125.583327] usb 1-1: SerialNumber: 121220130416 [80125.585139] usb-storage 1-1:1.0: USB Mass Storage device detected [80125.585392] scsi host6: usb-storage 1-1:1.0 [80126.586670] scsi 6:0:0:0: Direct-Access Mass Storage Device 1.00 PQ: 0 ANSI: 0 CCS [80126.587547] sd 6:0:0:0: Attached scsi generic sg3 type 0 [80126.775260] sd 6:0:0:0: [sdc] 62333952 512-byte logical blocks: (31.9 GB/29.7 GiB) [80126.775504] sd 6:0:0:0: [sdc] Write Protect is off [80126.775510] sd 6:0:0:0: [sdc] Mode Sense: 03 00 00 00 [80126.775686] sd 6:0:0:0: [sdc] No Caching mode page found [80126.775696] sd 6:0:0:0: [sdc] Assuming drive cache: write through [80126.787956] sdc: sdc1 [80126.789309] sd 6:0:0:0: [sdc] Attached SCSI removable disk [80396.165947] usb 1-1: reset high-speed USB device number 9 using xhci_hcd [80408.251009] usb 1-1: USB disconnect, device number 9 [80408.256545] sd 6:0:0:0: [sdc] tag#0 FAILED Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK [80408.256555] sd 6:0:0:0: [sdc] tag#0 CDB: Write(10) 2a 00 00 02 82 50 00 00 f0 00 [80408.256560] blk_update_request: I/O error, dev sdc, sector 164432 [80408.256567] Buffer I/O error on dev sdc1, logical block 19530, lost async page write [80408.256578] Buffer I/O error on dev sdc1, logical block 19531, lost async page write [80408.256583] Buffer I/O error on dev sdc1, logical block 19532, lost async page write [80408.256587] Buffer I/O error on dev sdc1, logical block 19533, lost async page write [80408.256592] Buffer I/O error on dev sdc1, logical block 19534, lost async page write [80408.256596] Buffer I/O error on dev sdc1, logical block 19535, lost async page write [80408.256600] Buffer I/O error on dev sdc1, logical block 19536, lost async page write [80408.256604] Buffer I/O error on dev sdc1, logical block 19537, lost async page write [80408.256608] Buffer I/O error on dev sdc1, logical block 19538, lost async page write [80408.256611] Buffer I/O error on dev sdc1, logical block 19539, lost async page write [80408.340407] VFS: Dirty inode writeback failed for block device sdc1 (err=-5). [80408.618642] usb 1-1: new high-speed USB device number 10 using xhci_hcd [80408.783479] usb 1-1: New USB device found, idVendor=14cd, idProduct=1212 [80408.783487] usb 1-1: New USB device strings: Mfr=1, Product=3, SerialNumber=2 [80408.783492] usb 1-1: Product: Mass Storage Device [80408.783496] usb 1-1: Manufacturer: Generic [80408.784034] usb-storage 1-1:1.0: USB Mass Storage device detected [80408.784253] scsi host6: usb-storage 1-1:1.0 [80409.786335] scsi 6:0:0:0: Direct-Access Mass Storage Device 1.00 PQ: 0 ANSI: 0 CCS [80409.787089] sd 6:0:0:0: Attached scsi generic sg3 type 0 [80409.975129] sd 6:0:0:0: [sdc] 62333952 512-byte logical blocks: (31.9 GB/29.7 GiB) [80409.975380] sd 6:0:0:0: [sdc] Write Protect is off [80409.975389] sd 6:0:0:0: [sdc] Mode Sense: 03 00 00 00 [80409.976494] sd 6:0:0:0: [sdc] No Caching mode page found [80409.976503] sd 6:0:0:0: [sdc] Assuming drive cache: wri
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes:
> Ok, so I guess what this means is that ixp4xx (or xscale in general)
> implements its big-endian mode by adding a byteswap on its DRAM
> and PCI interfaces in be32 mode, rather than by changing the behavior of
> the load/store operations (as be8 mode does) or by having the byteswap
> in its load/store pipeline or the top-level AHB bridge?
Hmm... IIRC, there is normally no swapping on DRAM bus. E.g. if you
write 0x12345678 to RAM and change endianness, it will still read as
0x12345678. The CPU will still be able to execute opcodes after
switching endianness, but byte-oriented data will be messed up.
PCI swaps in BE mode, so byte order is preserved but readl() must
"unswap" it.
> I'm still unsure about
> __indirect_readsl()/ioread32_rep()/insl()/readsl().
Indirect ops should behave the same as direct. I think I have tested
them at some point. The "string" operations don't have to swap (on PCI)
because the PCI bus controller does it for them (in BE mode).
> insl() does a double-swap on big-endian, which seems right, as we
> end up with four swaps total, preserving correct byte order.
static inline void insl(u32 io_addr, void *p, u32 count)
{
u32 *vaddr = p;
while (count--)
*vaddr++ = le32_to_cpu(inl(io_addr));
}
inl() does indirect input (preserving value, not byte order), so there seem
to be just one swap here (le32_to_cpu) preserving byte order.
> __raw_readsl() performs no swap, which would be correct for PCI
> (same swap on PCI and RAM, so byteorder is preserved),
No, a single swap on PCI, this means the byte order is preserved :-)
> but wrong
> for on-chip FIFO registers (one swap on RAM, no swap on MMIO).
But there aren't any such registers. Basically, almost all registers are
32-bit, even if they only hold an 8-bit value.
Exceptions such as 16550 UARTs are taken care of in platform structs
(using offset = 3).
> However, when CONFIG_IXP4XX_INDIRECT_PCI is set, both
> ioread32_rep() and readsl() call __indirect_readsl(), which
> in turn swaps the data once, so I think we actually need this patch:
>
> diff --git a/arch/arm/mach-ixp4xx/include/mach/io.h
> b/arch/arm/mach-ixp4xx/include/mach/io.h
> @@ -100,7 +102,7 @@ static inline void __indirect_writesw(volatile void
> __iomem *bus_addr,
> const u16 *vaddr = p;
>
> while (count--)
> - writew(*vaddr++, bus_addr);
> + writew((u16 __force)cpu_to_le32(*vaddr++), bus_addr);
> }
...
> Does that make sense to you? This is essentially the same thing we already
> do for inw/inl/outw/outl.
Well, we may need something like this. It seems writesw() (and thus
__indirect_writesw()) etc. use le16 values (preserving byte order), so
the above should probably use le16_to_cpu() instead (and le32_to_cpu in
__indirect_writesl()). I think the only thing that can use it on my hw
is VIA PATA adapter (throught ioread32_rep() etc). I will have to dig it
up as well.
I wouldn't rather touch this stuff without verifying that it fixes
things up.
Thanks for looking into this.
--
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > ixp4xx is really special in that it performs hardware swapping for > internal devices based on CPU endianess but not on PCI devices. Again, IXP4xx does not perform hardware (nor any other) swapping for registers of on-chip devices. The registers are connected 1:1, bit 0 to bit 0 etc. (Yes, IXP4xx can be optionally programmed for such swapping, depending on silicon revision, but it is not used in mainline kernel). The only hardware swapping happens on PCI bus (in BE mode), to be compatible with other platforms and non-IXP4xx-specific PCI drivers. > Coming back to the specific pxa25x_udc case: using __raw_* accessors > in the driver would possibly end up breaking the PXA25x machines in > the (very unlikely) case that someone wants to make it work with > big-endian kernels, assuming it does not have the same hardware > byteswap logic as ixp4xx. I'd expect both CPUs to behave in exactly the same manner, i.e., to not swap anything on the internal bus. If true, it would mean it should "just work" in both BE and LE modes (including BE mode on PXA, should it be actually possible). -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > Both writes leave the CPU core within the spinlock but are not serialized > with anything else, so there is no ordering between the CPUs when they > enter the shared bus, other than having address before data. You'd > expect to see address0, data0, address1, data1, but it could also > be e.g. address0, address1, data1, data0. Ah, so it's a matter of flushing the write buffers before exiting the spinlock-protected code. > The point is more what the common case is. Almost all machines we support > have fixed-endian devices, and the drivers are correct when using readl() > or in_le32() but are not endian-safe when using __raw_readl(). Sure, e.g. PCI does it this way (eventually swapping the data lanes if needed). > Using __raw_readl() has the big danger of someone accidentally "fixing" > the driver to work like all the others in order to solve a theoretical > endian problem, while it should really be made obvious that the hardware > actively swaps its data on the bus. Sure - if this is the case. On-chip IXP4xx peripherals don't swap data at all (i.e., they match CPU endianess) - accessing their registers is like accessing a normal CPU register. That's why they don't use PCI-style readl() etc. - however a better name than __raw_* would probably help here. Using __raw_* in a PCI driver would be generally wrong. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > The barriers on a spinlock synchronize between CPUs but not an external > bus, so (on some architectures) a spinlock protecting an MMIO register > does not guarantee that two CPUs doing > > spin_lock(); > __raw_writel(address); > __raw_writel(data); > spin_unlock(); > > would cause pairs of address/data to be seen on the bus. > > Of course this is meaningless on ixp4xx, as there is only one CPU. I still don't get it. If the spinlocks synchronize between CPUs, there can only be one CPU (or core) doing the pair of raw_writel(), so how would it be possible to not get the address/data pair written out? IOW, how is it different from a system with a single CPU? > On powerpc, we have in_le32/in_be32 for SoC-internal register access, > while only PCI devices are allowed to be accessed using readl(). Yeah, this seems like a sane solution. > I would suggest using an ixp4xx specific set of accessors that comes down > to either readl() or ioread32_be(), depending on whether CONFIG_CPU_BIG_ENDIAN > is set. That makes it clear that there is a magic bus involved and that it > works on this platform but not in portable code. Hmm. This is actually the opposite - while there may be some magic (swapping) in readl() and friends, there is absolutely no magic in the __raw_readl() etc. They are essentially equivalent to *(volatile u32 *)ptr. This is constant and doesn't depend on endianess, PCI, anything. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > I consider the use of __raw_* accessors a bug, I don't think we should > ever do that because it hides how the hardware actually works, it doesn't > work with spinlocks, and it can lead to the compiler splitting up accesses > into byte sized ones (not on ARM with the current definition, but > possible in general). Well, then maybe we should fix them, or add another set. Why don't they work with spinlocks? To be honest, I remember this was already discussed a bit years ago. I think I proposed back then a set of read_le32 (which would be equivalent of current readl(), and could be named pci_readl() as well), read_be32, read_host (without swapping). The names could be better, though. > Almost all hardware is fixed-endian, so you have to use swapping accessors > when the CPU is the other way, except for device RAM and FIFO registers > that are always used to transfer a byte stream (see the definition of > readsl() and memcpy_fromio()). When you have hardware that adds byteswaps > on the bus interface, you typically end up with MMIO registers requiring > no swap (or double swap) and readsl()/memcpy_fromio()) suddenly requiring > a swap that is counterintuitive. Sure, but the __raw_* are used just to be sure there is absolutely no swapping. E.g. for IXP4xx, the registers never require swapping, thus readl() etc. are not suitable for this. What is needed here is simple "atomic" 32-bit straight to/from register (MM)IO (assuming 4-byte address alignment). If not __raw_* then what? -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: >> Anyway, I think readl()/writel() do the right thing: in BE mode they >> swap PCI accesses and don't swap normal registers, in LE mode nothing is >> swapped. > > This seems to be true when CONFIG_IXP4XX_INDIRECT_PCI is set, but > not otherwise. For the indirect variant, writel() is a __raw_writel() > without barrier or byteswap on non-PCI memory, while with that > option disabled, we use the standard implementation that has both > a byteswap and a barrier. > > According to your description, that would mean the version without > indirect PCI access is broken and it appears to have been that way > since before the start of git history in 2.6.12. > > It's possible that nobody cared because all drivers for non-PCI > devices on ixp4xx (the on chip ones) just use __raw_readl or > direct pointer references. Well, it is possible. I recall I probably used __raw_* instead of readl()/writel() in qmgr/npe/Ethernet drivers for this very reason. I think Direct pointer references are only used for locations in RAM (DMA buffers), they have their own share of problems because the peripherals are always big endian. I think I still have an early IXP425 board with UDC connector so I will try to dig it up and test this code. I wonder if we should switch the driver to use __raw_*, too. The problem is almost nobody uses UDC with this CPU. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio
Arnd Bergmann writes: > The unclear part here is for IXP4xx, which supports both big-endian > and little-endian configurations. So far, the driver has done > no byteswap in either case. I suspect that is wrong and it would > actually need to swap in one or the other case, but I don't know > which. If at all, I guess it should swap in LE mode. But it's far from certain. > It's also possible that there is some magic setting in > the chip that makes the endianess of the MMIO register match the > CPU, and in that case, the code actually does the right thing > for all configurations, both before and after this patch. This is IMHO most probable. Actually, the IXP4xx is "natural" in BE mode (except for PCI) and normally in LE mode it's order-coherent, meaning 32-bit "integer" accesses need no swapping, but 8-bit and (mostly unused) 16-bit transfers need swapping. Anyway, I think readl()/writel() do the right thing: in BE mode they swap PCI accesses and don't swap normal registers, in LE mode nothing is swapped. LE data-coherent mode (which has never landed in the official kernel) is a bit different, but still, readl()/writel() do the right thing. I remember the "string" (block) functions preserve 8-bit ordering, and thus 32-bit values transfered using them may need swapping. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch
Arnd Bergmann writes: > This addresses both issues by moving all the definitions into the > pxa25x_udc driver itself. It turns out the only difference between > them was 'UDCCS_IO_ROF', and that could well be a mistake when it > was incorrectly copied from pxa25x to ixp4xx. > > Signed-off-by: Arnd Bergmann Both the programming manual and the Intel's library code say it's bit 2. Must be a mistake in the old code. Unfortunately I think we won't be able to check it on real hardware, the UDC on IXP4xx boards isn't usually connected. Also, this bug wouldn't likely affect operation (unless there is an overflow event, which would be currently undetected). I guess we as well very safely change it to 1 << 2. For the IXP4xx part, Acked-by: Krzysztof Halasa -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
