On Wed, Feb 15, 2017 at 12:12:47PM +0800, Shawn Lin wrote:
> Hi Russell,
>
> On 2017/2/15 3:34, Russell King - ARM Linux wrote:
> >On Tue, Feb 14, 2017 at 09:18:43AM -0700, Jens Axboe wrote:
> >>The current situation seems like a bit of a mess. Why don't you have two
> >>entry points, one for DMA and one for PIO. If the caller doesn't know if
> >>he can use DMA, he'd better call the PIO variant. Either that, or audit
> >>all callers and ensure they do the right thing wrt having a dma capable
> >>buffer.
> >
> >It really shouldn't matter. MMC interfaces are just like USB - you
> >have a host controller, which interfaces what is a multi-lane serial
> >bus to the system. The SDIO card shouldn't care one bit whether
> >the host controller is using DMA or PIO.
> >
> >However, I think that the DMA vs PIO thing is actually misleading here,
> >that's really not the issue at all.
> >
> >Looking at the oops, I see it uses sdio_memcpy_toio(). Tracing that
> >code leads me to here:
> >
> > for_each_sg(data.sg, sg_ptr, data.sg_len, i) {
> > sg_set_page(sg_ptr, virt_to_page(buf + (i *
> > seg_size)),
> > min(seg_size, left_size),
> > offset_in_page(buf + (i *
> > seg_size)));
> >
> >so the buffer that is passed into sdio_memcpy_toio() gets passed into
> >virt_to_page().
> >
> >Firstly, the fact that it's passed to virt_to_page() means that "buf"
> >must _only_ _ever_ be a lowmem address. It can't ever be a vmalloc
> >address (virt_to_page() is invalid on anything but lowmem.) Just like
> >certain kernel interfaces, passing pointers to memory of different types
> >from the one intended by the interface produces invalid results, and
> >that seems to be what's happening here.
> >
> >Secondly, it's a scatterlist, and scatterlists can be passed to DMA
> >mapping operations, which also implies that _if_ a host driver decides
> >to use DMA on it, the buffer better be DMA-able.
> >
> >Thirdly, while PIO may work (or even appear to work) because _maybe_
> >converting a vmalloc address to a ficticious struct page + offset, and
> >then converting that back again _might_ result in hitting the correct
> >memory, but it's not guaranteed to.
> >
>
> [1]:
> If no DMA involved, the host drivers usually use memcpy or readl/writel
> to transfer the data between MMIO address and buffer coming from the
> caller. So, is it also not guaranteed when using memcpy or readl to
> transfer data between MMIO address and vmalloc/heap buffer?
The point here is, if buf is a vmalloc address:
v = kmap_atomic(virt_to_page(buf))
v may be _anything_ at all. The kmap_atomic() will be done by the MMC
host driver, the virt_to_page() is done by the code I quoted above.
v may be a lowmem page address. v may be somewhere in userspace. v may
be some device mapping. v may be (if you're lucky) the same address as
"buf". There's no guarantees what it will be.
Note that the scatterlist above does _not_ store the virtual address
that was passed into it, but only the struct page, offset and length.
So, drivers can not know what the original virtual address was.
> >I suspect that virt_to_page() + kmap_atomic() is likely to try to
> >dereference a struct page pointer that does not point at a legal entry
> >in the memmap arrays, and result in scribbling over some random part
> >of kernel memory.
>
> If that is the fact, so what I am concerned mostly is that by
> seraching the APIs, sdio_writeb and sdio_readb, under the drivers/net
> /wireless/, I could see almost all sdio based WLAN drivers passed in
> a vmalloc area(actually when built as moudle, it should be located in
> MODULE range which also be included as vmalloc area, no?) or heap
> buffer.
sdio_readb() and sdio_writeb() convey the data in the command stream,
not the data stream. Firstly, sdio_writeb() takes the actual value to
be written, so the memory storage is irrelevant (on many platforms, it
will be passed as a value in CPU registers.) Eventually, it's written
into the MMC command structure as the command argument, which typically
ends up being written to a host controller register.
In the case of sdio_readb(), the returned value comes from the command
status, which is typically read from registers in the host controller.
mmc_io_rw_direct_host() writes the value to the passed address. Hence,
the host controller, again, never sees the address.
> I assume my question[1] above is fine, then thanks to none of the mmc
> host drivers use DMA for sdio_writeb and sdio_readb since it only
> request one byte which didn't be fetched from host FIFO and the host
> controller HW didn't support this kind of request to use DMA(but may be
> not in the future). Otherwise, it may result in scribbling over some
> random part of kernel memory.
See above, your understanding of how sdio_readb() and sdio_writeb() is
not correct. These are single value transfer functions, using
mmc_io_rw_direct() as the underlying method of access, and do not
transfer anything over the data lines.
These functions are quite different from sdio_memcpy_toio(),
sdio_memcpy_fromio(), sdio_readsb() and sdio_writesb(), which are
multiple value transfer functions, and these perform the transfer using
the data lines. These use sdio_io_rw_ext_helper(), which then uses
mmc_io_rw_extended() to perform the transfer. The code I quoted in my
original email is from mmc_io_rw_extended().
The command path is entirely separate from the data path in most MMC
host controllers. The command path is commonly PIO, the data path is
commonly DMA, but host controllers _may_ choose PIO for small transfers
or when they have no DMA support.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
