| Message ID | 1566221892-16744-1-git-send-email-yihuaijie@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | mtd: fix oops when writing to phram device on arm64 | expand |
On Mon, Aug 19, 2019 at 3:39 PM Huaijie Yi <yihuaijie@huawei.com> wrote: > > From: yihuaijie <yihuaijie@huawei.com> > > memcpy() to memory remaped by ioremap() at an address not aligned > to 8-bytes will cause oops on arm64 platform, fixing this by using > ioremap_wc(). This might be the right thing on arm64, but I fear not for all other archs and configurations. We had a similar chain of problems already with our sram driver. See: eb43e023130b ("misc: sram: add optional ioremap without write combining") 0ab163ad1ea0 ("misc: sram: switch to ioremap_wc from ioremap")
>On Mon, Aug 19, 2019 at 3:39 PM Huaijie Yi <yihuaijie@huawei.com> wrote: >> >> From: yihuaijie <yihuaijie@huawei.com> >> >> memcpy() to memory remaped by ioremap() at an address not aligned to >> 8-bytes will cause oops on arm64 platform, fixing this by using >> ioremap_wc(). > >This might be the right thing on arm64, but I fear not for all other archs and configurations. > >We had a similar chain of problems already with our sram driver. >See: >eb43e023130b ("misc: sram: add optional ioremap without write combining") >0ab163ad1ea0 ("misc: sram: switch to ioremap_wc from ioremap") > >-- >Thanks, >//richard Hi, richard I verified this on x86_64, and there isn't the problem. I will resend a V2 to fix this only on ARM64. -- Regards //Huaijie Yi
On 9/21/2019 11:03 PM, Huaijie Yi wrote: >> On Mon, Aug 19, 2019 at 3:39 PM Huaijie Yi <yihuaijie@huawei.com> wrote: >>> >>> From: yihuaijie <yihuaijie@huawei.com> >>> >>> memcpy() to memory remaped by ioremap() at an address not aligned to >>> 8-bytes will cause oops on arm64 platform, fixing this by using >>> ioremap_wc(). >> >> This might be the right thing on arm64, but I fear not for all other archs and configurations. >> >> We had a similar chain of problems already with our sram driver. >> See: >> eb43e023130b ("misc: sram: add optional ioremap without write combining") >> 0ab163ad1ea0 ("misc: sram: switch to ioremap_wc from ioremap") >> >> -- >> Thanks, >> //richard > > Hi, richard > > I verified this on x86_64, and there isn't the problem. > > I will resend a V2 to fix this only on ARM64. The SRAM driver also takes care of using memcpy_{to,from}io() which the PHRAM does not make use, that sounds like a possible issue too. There is the alignment that is important, but also how you access the underlying memory (e.g.: beats that you generate on the bus connecting you to that memory area). ARM64 may be forgiving and allow memcpy() to work against a WC mapping, other architectures, maybe not so much. It sounds like adopting the solution that the SRAM driver did, whereby adding an optional boolean Device Tree property to indicate whether ioremap_wc() or ioremap() is to be used seems like the most flexible solution.
PHRAM does't use Device Tree property, so I'm going to use a boolean module param to achieve it. -- Thanks, //Huaijie Yi
>On 9/21/2019 11:03 PM, Huaijie Yi wrote: >>> On Mon, Aug 19, 2019 at 3:39 PM Huaijie Yi <yihuaijie@huawei.com> wrote: >>>> >>>> From: yihuaijie <yihuaijie@huawei.com> >>>> >>>> memcpy() to memory remaped by ioremap() at an address not aligned to >>>> 8-bytes will cause oops on arm64 platform, fixing this by using >>>> ioremap_wc(). >>> >>> This might be the right thing on arm64, but I fear not for all other archs and configurations. >>> >>> We had a similar chain of problems already with our sram driver. >>> See: >>> eb43e023130b ("misc: sram: add optional ioremap without write >>> combining") >>> 0ab163ad1ea0 ("misc: sram: switch to ioremap_wc from ioremap") >>> >>> -- >>> Thanks, >>> //richard >> >> Hi, richard >> >> I verified this on x86_64, and there isn't the problem. >> >> I will resend a V2 to fix this only on ARM64. > >The SRAM driver also takes care of using memcpy_{to,from}io() which the PHRAM does not make use, that sounds like a possible issue too. There is the alignment that is important, but also how you access the underlying memory (e.g.: beats that you generate on the bus connecting you to that memory area). ARM64 may be forgiving and allow memcpy() to work against a WC mapping, other architectures, maybe not so much. > >It sounds like adopting the solution that the SRAM driver did, whereby adding an optional boolean Device Tree property to indicate whether >ioremap_wc() or ioremap() is to be used seems like the most flexible solution. >-- >Florian PHRAM does't use Device Tree property, so I'm going to use a boolean module param to achieve it. -- Thanks, //Huaijie Yi
diff --git a/drivers/mtd/devices/phram.c b/drivers/mtd/devices/phram.c index c467286..32355cb 100644 --- a/drivers/mtd/devices/phram.c +++ b/drivers/mtd/devices/phram.c @@ -98,7 +98,7 @@ static int register_device(char *name, phys_addr_t start, size_t len) goto out0; ret = -EIO; - new->mtd.priv = ioremap(start, len); + new->mtd.priv = ioremap_wc(start, len); if (!new->mtd.priv) { pr_err("ioremap failed\n"); goto out1;