Date:	Thu, 1 Feb 2007 16:35:44 -0800
From:	"Michael K. Edwards" <medwards.linux@...il.com>
To:	"Linux Kernel List" <linux-kernel@...r.kernel.org>
Subject: Should io(read|write)(8|16|32)_rep take (const|) volatile u(8|16|32) __iomem *addr?

Sorry, I wrote this:
> (This is not an ARM-specific question, although the example is on ARM.)
and then sent it to linux-arm-kernel anyway.  :-P


I'm writing an ALSA driver for an ARM SoC whose PCM audio interface is
MMIO.  ALSA provides copy_from_user_toio and copy_to_user_fromio
routines for this purpose, but the results are not sane, perhaps
because STMIA to I/O addresses is bollixed on this core.  I suspect
that the right thing to do is to use iowrite32_rep and ioread32_rep,
approximately like so (slightly modified ALSA code):

/**
 * copy_u32s_to_user_fromio - copy data from mmio-space to user-space
 * @dst: the destination pointer on user-space
 * @src: the source pointer on mmio
 * @count: the data size to copy in u32s
 *
 * Copies the data from mmio-space to user-space.
 *
 * Returns zero if successful, or non-zero on failure.
 */
static int copy_u32s_to_user_fromio(void __user *dst, const volatile
void __iomem *src, size_t count)
{
        u32 buf[80];
        count <<= 2;
        while (count) {
                size_t c = count;
                if (c > sizeof(buf))
                        c = sizeof(buf);
                ioread32_rep(src, buf, c>>2);
                if (copy_to_user(dst, buf, c))
                        return -EFAULT;
                count -= c;
                dst += c;
                src += c;
        }
        return 0;
}

/**
 * copy_u32s_from_user_toio - copy data from user-space to mmio-space
 * @dst: the destination pointer on mmio-space
 * @src: the source pointer on user-space
 * @count: the data size to copy in u32s
 *
 * Copies the data from user-space to mmio-space.
 *
 * Returns zero if successful, or non-zero on failure.
 */
static int copy_u32s_from_user_toio(volatile void __iomem *dst, const
void __user *src, size_t count)
{
        u32 buf[80];
        count <<= 2;
        while (count) {
                size_t c = count;
                if (c > sizeof(buf))
                        c = sizeof(buf);
                if (copy_from_user(buf, src, c))
                        return -EFAULT;
                iowrite32_rep(dst, buf, c>>2);
                count -= c;
                dst += c;
                src += c;
        }
        return 0;
}

Compiling (with GCC 4.1.2 RC1) gives these warnings:

foo.c: In function 'copy_u32s_to_user_fromio':
foo.c:56: warning: passing argument 1 of '__raw_readsl' discards
qualifiers from pointer target type
foo.c: In function 'copy_u32s_from_user_toio':
foo.c:86: warning: passing argument 1 of '__raw_writesl' discards
qualifiers from pointer target type

The qualifiers being discarded are the "volatile"s, which are indeed
not present in the declarations of these functions (which happen to be
ARM's implementation of io(read|write)32_rep):

include/asm-arm/io.h:extern void __raw_writesb(void __iomem *addr,
const void *data, int bytelen);
include/asm-arm/io.h:extern void __raw_writesw(void __iomem *addr,
const void *data, int wordlen);
include/asm-arm/io.h:extern void __raw_writesl(void __iomem *addr,
const void *data, int longlen);
include/asm-arm/io.h:extern void __raw_readsb(const void __iomem
*addr, void *data, int bytelen);
include/asm-arm/io.h:extern void __raw_readsw(const void __iomem
*addr, void *data, int wordlen);
include/asm-arm/io.h:extern void __raw_readsl(const void __iomem
*addr, void *data, int longlen);

It looks to me, by comparison to memcpy_(from|to)io, as if the
volatiles ought to be there.  It also looks to me like the void *
parameters should be u(8|16|32) * instead, so the compiler knows what
alignment to expect (the implementations would generally fail or suck
on non-aligned arguments).  (That would also be more consistent with
the fact that the length parameters are in (8|16|32)-bit units, not
octets.)

Opinions?

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

Powered by blists - more mailing lists