linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: linux@arm.linux.org.uk (Russell King - ARM Linux)
To: linux-arm-kernel@lists.infradead.org
Subject: Unhandled fault: page domain fault (0x81b) at 0x00e41008
Date: Wed, 27 Jan 2016 10:48:12 +0000	[thread overview]
Message-ID: <20160127104811.GX10826@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <56A89DC3.8010309@free.fr>

On Wed, Jan 27, 2016 at 11:36:51AM +0100, Mason wrote:
> On 24/01/2016 14:27, Mason wrote:
> 
> > Our "software stack" provides the kernel API under discussion:
> > 
> >   {read,write}_data{8,16,32}
> > 
> > These 6 functions must be implemented, because they are part
> > of the API we provide to customers. As the "page domain fault"
> > underscores, my own implementation is incorrect. I am grateful
> > for the implementation you suggested up-thread, and will test
> > its performance on Monday.
> 
> For the record, I've now changed the implementation as follows.
> I'll benchmark performance as soon as I fix the other bug in
> the module.
> 
> #define DEFINE_BLOCK_READ(N)							\
> static int read##N(void __user *dest, void *buf, void __iomem *io, size_t len)	\
> {										\
> 	size_t i; u##N *temp = buf;						\
> 	for (i = 0; i < len; i += N/8) temp[i] = RD##N(io + i);			\
> 	return copy_to_user(dest, temp, len) ? -EFAULT : 0;			\
> }
> 
> #define RD8	readb_relaxed
> #define RD16	readw_relaxed
> #define RD32	readl_relaxed
> 
> DEFINE_BLOCK_READ(8)
> DEFINE_BLOCK_READ(16)
> DEFINE_BLOCK_READ(32)
> 
> #define DEFINE_BLOCK_WRITE(N)							\
> static int write##N(void __user *src, void *buf, void __iomem *io, size_t len)	\
> {										\
> 	size_t i; u##N *temp = buf;						\
> 	if (copy_from_user(temp, src, len)) return -EFAULT;			\
> 	for (i = 0; i < len; i += N/8) WR##N(temp[i], io + i);			\
> 	return 0;								\
> }
> 
> #define WR8	writeb_relaxed
> #define WR16	writew_relaxed
> #define WR32	writel_relaxed
> 
> DEFINE_BLOCK_WRITE(8)
> DEFINE_BLOCK_WRITE(16)
> DEFINE_BLOCK_WRITE(32)
> 
> #define TILE_SIZE (16u << 10)
> typedef int fun_t(void __user *ua, void *buf, void __iomem *io, size_t len);
> 
> static int block_copy(void __user *ua, phys_addr_t pa, size_t bytes, fun_t *fun)
> {
> 	int err = 0;
> 	size_t pos = 0;
> 	void *buf = kmalloc(TILE_SIZE, GFP_KERNEL);
> 	if (buf == NULL) err = -ENOMEM;
> 
> 	while (pos < bytes && !err)
> 	{
> 		size_t tile = min(bytes-pos, TILE_SIZE);
> 		void __iomem *va = ioremap(pa + pos, tile);
> 
> 		err = va ? fun(ua + pos, buf, va, tile) : -EFAULT;
> 		iounmap(va);
> 		pos += tile;
> 	}
> 
> 	kfree(buf);
> 	return err;
> }
> 
> 
> and then the ioctl dispatcher calls e.g.
> 	block_copy(user_addr, phys_addr, count*4, read32);
> 
> 
> IIUC, Arnd mentioned that there might be an issue using readl_relaxed
> on a memory region with a big-endian kernel.

I think you're confused there, or Arnd's comment was incorrect.

In any case, I'm even more pissed off with you.  Let me quote from
your earlier emails:

> The problem is that the kernel module's API is already set
> in stone, and it requires block copies with specific access
> sizes, e.g. block_copy8, block_copy16, block_copy32.

and:

> So, a little over a decade ago, someone decided that these
> functions would have the following prototype:
> 
> int read_data8  (u8  *user_addr, u8  *phys_addr, int count)
> int read_data16 (u16 *user_addr, u16 *phys_addr, int count)
> int read_data32 (u32 *user_addr, u32 *phys_addr, int count)
> 
> int write_data8 (u8  *user_addr, u8  *phys_addr, int count)
> int write_data16(u16 *user_addr, u16 *phys_addr, int count)
> int write_data32(u32 *user_addr, u32 *phys_addr, int count)

However, you've done away with those prototypes, and instead come up
with something that looks like:

int block_copy(void __user *ua, phys_addr_t pa, size_t bytes, fun_t *fun)

?

So, let me get this straight.  You demand that the API can't be changed,
and I provide you with a solution which results in very little change to
the API.

However, rather than testing the version I carefully created for you,
you've decided that you're not going to, instead coming up with your
own solution which breaks all your previous demands.

This is totally rediculous.  Why should I waste any more time with _any_
of your questions?

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

  reply	other threads:[~2016-01-27 10:48 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-22 17:37 Unhandled fault: page domain fault (0x81b) at 0x00e41008 Mason
2016-01-22 17:48 ` Russell King - ARM Linux
2016-01-22 18:59   ` Mason
2016-01-22 19:34     ` Russell King - ARM Linux
2016-01-22 23:15       ` Mason
2016-01-22 23:57         ` Russell King - ARM Linux
2016-01-23 11:14           ` Mason
2016-01-23 11:34             ` Russell King - ARM Linux
2016-01-23 20:53               ` Mason
2016-01-23 22:46                 ` Mason
2016-01-23 23:59                 ` Russell King - ARM Linux
2016-01-24 13:27                   ` Mason
2016-01-27 10:36                     ` Mason
2016-01-27 10:48                       ` Russell King - ARM Linux [this message]
2016-01-27 12:04                         ` Mason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160127104811.GX10826@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).