On Thu, Mar 28, 2024 at 07:40:49PM -0500, Stephen Boyd wrote: > Quoting Stephan Gerhold (2024-03-28 02:58:56) > > > > FWIW: This old patch series from Stephen Boyd is closely related: > > https://lore.kernel.org/linux-arm-msm/20190910160903.65694-1-swboyd@xxxxxxxxxxxx/ > > > > > The main use case I have is to map the command-db memory region on > > > Qualcomm devices with a read-only mapping. It's already a const marked > > > pointer and the API returns const pointers as well, so this series > > > makes sure that even stray writes can't modify the memory. > > > > Stephen, what was the end result of that patch series? Mapping the > > cmd-db read-only sounds cleaner than trying to be lucky with the right > > set of cache flags. > > > > I dropped it because I got busy with other stuff. Feel free to pick it > back up. It looks like the part where I left off was where we had to > make the API fallback to mapping the memory as writeable if read-only > isn't supported on the architecture. [...] > The other weird thing was that we passed both MEMREMAP_RO and > MEMREMAP_WB to indicate what sort of fallback we want. Perhaps that can > be encoded in the architecture layer so that you get the closest thing > to read-only memory (i.e. any sort of write side caching is removed) and > you don't have to pass a fallback mapping type. Was there any motivation for having the fallback? I suspect driver owners that want to use MEMREMAP_RO will know which architectures the driver is applicable to and also know whether MEMREMAP_RO would work. > I also wanted to return a const pointer. The stash looks mostly complete. Do you think it should be part of the series or submitted separately? > > Here's my stash patch on top of the branch (from 2019!). > > ---8<---- > From: Stephen Boyd <swboyd@xxxxxxxxxxxx> > Date: Tue, 14 May 2019 13:22:01 -0700 > Subject: [PATCH] stash const iounmap > > --- > arch/arm64/include/asm/io.h | 2 +- > arch/arm64/mm/ioremap.c | 9 +++++---- > arch/x86/mm/ioremap.c | 2 +- > drivers/firmware/google/vpd.c | 22 +++++++++++----------- > drivers/soc/qcom/rmtfs_mem.c | 5 ++--- > include/asm-generic/io.h | 2 +- > include/linux/io.h | 2 +- > include/linux/mmiotrace.h | 2 +- > kernel/iomem.c | 2 +- > 9 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index 245bd371e8dc..0fd4f1678300 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -178,7 +178,7 @@ extern void __memset_io(volatile void __iomem *, > int, size_t); > * I/O memory mapping functions. > */ > extern void __iomem *__ioremap(phys_addr_t phys_addr, size_t size, > pgprot_t prot); > -extern void __iounmap(volatile void __iomem *addr); > +extern void __iounmap(const volatile void __iomem *addr); > extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size); > > #define ioremap(addr, size) __ioremap((addr), (size), > __pgprot(PROT_DEVICE_nGnRE)) > diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c > index c4c8cd4c31d4..e39fb2cb6042 100644 > --- a/arch/arm64/mm/ioremap.c > +++ b/arch/arm64/mm/ioremap.c > @@ -80,16 +80,17 @@ void __iomem *__ioremap(phys_addr_t phys_addr, > size_t size, pgprot_t prot) > } > EXPORT_SYMBOL(__ioremap); > > -void __iounmap(volatile void __iomem *io_addr) > +void __iounmap(const volatile void __iomem *io_addr) > { > - unsigned long addr = (unsigned long)io_addr & PAGE_MASK; > + const unsigned long addr = (const unsigned long)io_addr & PAGE_MASK; > + const void *vaddr = (const void __force *)addr; > > /* > * We could get an address outside vmalloc range in case > * of ioremap_cache() reusing a RAM mapping. > */ > - if (is_vmalloc_addr((void *)addr)) > - vunmap((void *)addr); > + if (is_vmalloc_addr(vaddr)) > + vunmap(vaddr); > } > EXPORT_SYMBOL(__iounmap); > > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c > index 0029604af8a4..e9a2910d0c63 100644 > --- a/arch/x86/mm/ioremap.c > +++ b/arch/x86/mm/ioremap.c > @@ -392,7 +392,7 @@ EXPORT_SYMBOL(ioremap_prot); > * > * Caller must ensure there is only one unmapping for the same pointer. > */ > -void iounmap(volatile void __iomem *addr) > +void iounmap(const volatile void __iomem *addr) > { > struct vm_struct *p, *o; > > diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c > index c0c0b4e4e281..7428f189999e 100644 > --- a/drivers/firmware/google/vpd.c > +++ b/drivers/firmware/google/vpd.c > @@ -48,7 +48,7 @@ struct vpd_section { > const char *name; > char *raw_name; /* the string name_raw */ > struct kobject *kobj; /* vpd/name directory */ > - char *baseaddr; > + const char *baseaddr; > struct bin_attribute bin_attr; /* vpd/name_raw bin_attribute */ > struct list_head attribs; /* key/value in vpd_attrib_info list */ > }; > @@ -187,19 +187,19 @@ static int vpd_section_create_attribs(struct > vpd_section *sec) > return 0; > } > > -static int vpd_section_init(const char *name, struct vpd_section *sec, > +static int vpd_section_init(struct device *dev, const char *name, > struct vpd_section *sec, > phys_addr_t physaddr, size_t size) > { > int err; > > - sec->baseaddr = memremap(physaddr, size, MEMREMAP_WB); > - if (!sec->baseaddr) > - return -ENOMEM; > + sec->baseaddr = devm_memremap(physaddr, size, MEMREMAP_RO | MEMREMAP_WB); > + if (IS_ERR(sec->baseaddr)) > + return PTR_ERR(sec->baseaddr); > > sec->name = name; > > /* We want to export the raw partition with name ${name}_raw */ > - sec->raw_name = kasprintf(GFP_KERNEL, "%s_raw", name); > + sec->raw_name = devm_kasprintf(GFP_KERNEL, "%s_raw", name); > if (!sec->raw_name) { > err = -ENOMEM; > goto err_memunmap; > @@ -252,11 +252,12 @@ static int vpd_section_destroy(struct vpd_section *sec) > return 0; > } > > -static int vpd_sections_init(phys_addr_t physaddr) > +static int vpd_sections_init(struct coreboot_device *cdev) > { > struct vpd_cbmem __iomem *temp; > struct vpd_cbmem header; > int ret = 0; > + phys_addr_t physaddr = cdev->cbmem_ref.cbmem_addr; > > temp = memremap(physaddr, sizeof(struct vpd_cbmem), MEMREMAP_WB); > if (!temp) > @@ -269,7 +270,7 @@ static int vpd_sections_init(phys_addr_t physaddr) > return -ENODEV; > > if (header.ro_size) { > - ret = vpd_section_init("ro", &ro_vpd, > + ret = vpd_section_init(&cdev->dev, "ro", &ro_vpd, > physaddr + sizeof(struct vpd_cbmem), > header.ro_size); > if (ret) > @@ -294,10 +295,9 @@ static int vpd_probe(struct coreboot_device *dev) > int ret; > > vpd_kobj = kobject_create_and_add("vpd", firmware_kobj); > - if (!vpd_kobj) > - return -ENOMEM; > + if (!vpd_kobj) return -ENOMEM; > > - ret = vpd_sections_init(dev->cbmem_ref.cbmem_addr); > + ret = vpd_sections_init(dev); > if (ret) { > kobject_put(vpd_kobj); > return ret; > diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c > index 6f5e8be9689c..137e5240d916 100644 > --- a/drivers/soc/qcom/rmtfs_mem.c > +++ b/drivers/soc/qcom/rmtfs_mem.c > @@ -212,10 +212,9 @@ static int qcom_rmtfs_mem_probe(struct > platform_device *pdev) > rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups; > rmtfs_mem->dev.release = qcom_rmtfs_mem_release_device; > > - rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr, > - rmtfs_mem->size, MEMREMAP_WC); > + rmtfs_mem->base = devm_memremap_reserved_mem(&pdev->dev, > + MEMREMAP_WC); > if (IS_ERR(rmtfs_mem->base)) { > - dev_err(&pdev->dev, "failed to remap rmtfs_mem region\n"); > ret = PTR_ERR(rmtfs_mem->base); > goto put_device; > } > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index 303871651f8a..d675a574eeb3 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -982,7 +982,7 @@ static inline void __iomem *__ioremap(phys_addr_t > offset, size_t size, > #ifndef iounmap > #define iounmap iounmap > > -static inline void iounmap(void __iomem *addr) > +static inline void iounmap(const void __iomem *addr) > { > } > #endif > diff --git a/include/linux/io.h b/include/linux/io.h > index 16c7f4498869..82e6c4d6bdd3 100644 > --- a/include/linux/io.h > +++ b/include/linux/io.h > @@ -163,7 +163,7 @@ enum { > }; > > void *memremap(resource_size_t offset, size_t size, unsigned long flags); > -void memunmap(void *addr); > +void memunmap(const void *addr); > > /* > * On x86 PAT systems we have memory tracking that keeps track of > diff --git a/include/linux/mmiotrace.h b/include/linux/mmiotrace.h > index 88236849894d..04607c468b73 100644 > --- a/include/linux/mmiotrace.h > +++ b/include/linux/mmiotrace.h > @@ -47,7 +47,7 @@ extern int kmmio_handler(struct pt_regs *regs, > unsigned long addr); > /* Called from ioremap.c */ > extern void mmiotrace_ioremap(resource_size_t offset, unsigned long size, > void __iomem *addr); > -extern void mmiotrace_iounmap(volatile void __iomem *addr); > +extern void mmiotrace_iounmap(const volatile void __iomem *addr); > > /* For anyone to insert markers. Remember trailing newline. */ > extern __printf(1, 2) int mmiotrace_printk(const char *fmt, ...); > diff --git a/kernel/iomem.c b/kernel/iomem.c > index 8d3cf74a32cb..22d0fa336360 100644 > --- a/kernel/iomem.c > +++ b/kernel/iomem.c > @@ -130,7 +130,7 @@ void *memremap(resource_size_t offset, size_t > size, unsigned long flags) > } > EXPORT_SYMBOL(memremap); > > -void memunmap(void *addr) > +void memunmap(const void *addr) > { > if (is_vmalloc_addr(addr)) > iounmap((void __iomem *) addr); > > base-commit: 9e98c678c2d6ae3a17cb2de55d17f69dddaa231b > prerequisite-patch-id: 62119e27c0c0686e02f0cb55c296b878fb7f5e47 > prerequisite-patch-id: bda32cfc1733c245ae3f141d7c27b18e4adcc628 > prerequisite-patch-id: b8f8097161bd15e87d54dcfbfa67b9ca1abc7204 > prerequisite-patch-id: cd374fb6e39941b8613d213b4a75909749409d63 > prerequisite-patch-id: d8dbc8485a0f86353a314ab5c22fc92d8eac1cc0 > prerequisite-patch-id: e539621b0eccf82aabf9891de69c30398abf76a0 > prerequisite-patch-id: 59d60033b80dec940201edd5aefed22726122a37 > prerequisite-patch-id: 0d16b23cec20eaab7f45ee84fd8d2950657dc72e > -- > https://chromeos.dev >
