| Message ID | 1431683064-29584-1-git-send-email-cristian.stoica@freescale.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Hi Greg, Can you have a look at this patch? https://patchwork.kernel.org/patch/6413191/ Thanks, Cristian S. On 05/15/2015 12:44 PM, Cristian Stoica wrote: > Use resource_size_t to accommodate sizes greater than the size of an > unsigned long int on platforms that have more than 32 bit > physical addresses > > Signed-off-by: Cristian Stoica <cristian.stoica@freescale.com> > > Changes in v2: > - update definitions is pgtable_64.c as well - or else we get broken builds on 64 > - minor reformatting in pgtable_64.c to satisfy checkpatch > > --- > arch/powerpc/include/asm/io.h | 14 +++++++------- > arch/powerpc/mm/pgtable_32.c | 10 +++++----- > arch/powerpc/mm/pgtable_64.c | 22 +++++++++++----------- > 3 files changed, 23 insertions(+), 23 deletions(-) > > diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h > index a8d2ef3..749c66e 100644 > --- a/arch/powerpc/include/asm/io.h > +++ b/arch/powerpc/include/asm/io.h > @@ -716,24 +716,24 @@ static inline void iosync(void) > * be hooked (but can be used by a hook on iounmap) > * > */ > -extern void __iomem *ioremap(phys_addr_t address, unsigned long size); > -extern void __iomem *ioremap_prot(phys_addr_t address, unsigned long size, > +extern void __iomem *ioremap(phys_addr_t address, resource_size_t size); > +extern void __iomem *ioremap_prot(phys_addr_t address, resource_size_t size, > unsigned long flags); > -extern void __iomem *ioremap_wc(phys_addr_t address, unsigned long size); > +extern void __iomem *ioremap_wc(phys_addr_t address, resource_size_t size); > #define ioremap_nocache(addr, size) ioremap((addr), (size)) > > extern void iounmap(volatile void __iomem *addr); > > -extern void __iomem *__ioremap(phys_addr_t, unsigned long size, > +extern void __iomem *__ioremap(phys_addr_t, resource_size_t size, > unsigned long flags); > -extern void __iomem *__ioremap_caller(phys_addr_t, unsigned long size, > +extern void __iomem *__ioremap_caller(phys_addr_t, resource_size_t size, > unsigned long flags, void *caller); > > extern void __iounmap(volatile void __iomem *addr); > > extern void __iomem * __ioremap_at(phys_addr_t pa, void *ea, > - unsigned long size, unsigned long flags); > -extern void __iounmap_at(void *ea, unsigned long size); > + resource_size_t size, unsigned long flags); > +extern void __iounmap_at(void *ea, resource_size_t size); > > /* > * When CONFIG_PPC_INDIRECT_PIO is set, we use the generic iomap implementation > diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c > index 7692d1b..a7d5137 100644 > --- a/arch/powerpc/mm/pgtable_32.c > +++ b/arch/powerpc/mm/pgtable_32.c > @@ -135,7 +135,7 @@ pgtable_t pte_alloc_one(struct mm_struct *mm, unsigned long address) > } > > void __iomem * > -ioremap(phys_addr_t addr, unsigned long size) > +ioremap(phys_addr_t addr, resource_size_t size) > { > return __ioremap_caller(addr, size, _PAGE_NO_CACHE | _PAGE_GUARDED, > __builtin_return_address(0)); > @@ -143,7 +143,7 @@ ioremap(phys_addr_t addr, unsigned long size) > EXPORT_SYMBOL(ioremap); > > void __iomem * > -ioremap_wc(phys_addr_t addr, unsigned long size) > +ioremap_wc(phys_addr_t addr, resource_size_t size) > { > return __ioremap_caller(addr, size, _PAGE_NO_CACHE, > __builtin_return_address(0)); > @@ -151,7 +151,7 @@ ioremap_wc(phys_addr_t addr, unsigned long size) > EXPORT_SYMBOL(ioremap_wc); > > void __iomem * > -ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long flags) > +ioremap_prot(phys_addr_t addr, resource_size_t size, unsigned long flags) > { > /* writeable implies dirty for kernel addresses */ > if ((flags & (_PAGE_RW | _PAGE_RO)) != _PAGE_RO) > @@ -173,13 +173,13 @@ ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long flags) > EXPORT_SYMBOL(ioremap_prot); > > void __iomem * > -__ioremap(phys_addr_t addr, unsigned long size, unsigned long flags) > +__ioremap(phys_addr_t addr, resource_size_t size, unsigned long flags) > { > return __ioremap_caller(addr, size, flags, __builtin_return_address(0)); > } > > void __iomem * > -__ioremap_caller(phys_addr_t addr, unsigned long size, unsigned long flags, > +__ioremap_caller(phys_addr_t addr, resource_size_t size, unsigned long flags, > void *caller) > { > unsigned long v, i; > diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c > index 59daa5e..64dd7a9 100644 > --- a/arch/powerpc/mm/pgtable_64.c > +++ b/arch/powerpc/mm/pgtable_64.c > @@ -168,8 +168,8 @@ int map_kernel_page(unsigned long ea, unsigned long pa, int flags) > * __ioremap_at - Low level function to establish the page tables > * for an IO mapping > */ > -void __iomem * __ioremap_at(phys_addr_t pa, void *ea, unsigned long size, > - unsigned long flags) > +void __iomem *__ioremap_at(phys_addr_t pa, void *ea, resource_size_t size, > + unsigned long flags) > { > unsigned long i; > > @@ -202,7 +202,7 @@ void __iomem * __ioremap_at(phys_addr_t pa, void *ea, unsigned long size, > * are manipulated manually, like partial unmapping of > * PCI IOs or ISA space. > */ > -void __iounmap_at(void *ea, unsigned long size) > +void __iounmap_at(void *ea, resource_size_t size) > { > WARN_ON(((unsigned long)ea) & ~PAGE_MASK); > WARN_ON(size & ~PAGE_MASK); > @@ -210,8 +210,8 @@ void __iounmap_at(void *ea, unsigned long size) > unmap_kernel_range((unsigned long)ea, size); > } > > -void __iomem * __ioremap_caller(phys_addr_t addr, unsigned long size, > - unsigned long flags, void *caller) > +void __iomem *__ioremap_caller(phys_addr_t addr, resource_size_t size, > + unsigned long flags, void *caller) > { > phys_addr_t paligned; > void __iomem *ret; > @@ -255,13 +255,13 @@ void __iomem * __ioremap_caller(phys_addr_t addr, unsigned long size, > return ret; > } > > -void __iomem * __ioremap(phys_addr_t addr, unsigned long size, > - unsigned long flags) > +void __iomem *__ioremap(phys_addr_t addr, resource_size_t size, > + unsigned long flags) > { > return __ioremap_caller(addr, size, flags, __builtin_return_address(0)); > } > > -void __iomem * ioremap(phys_addr_t addr, unsigned long size) > +void __iomem *ioremap(phys_addr_t addr, resource_size_t size) > { > unsigned long flags = _PAGE_NO_CACHE | _PAGE_GUARDED; > void *caller = __builtin_return_address(0); > @@ -271,7 +271,7 @@ void __iomem * ioremap(phys_addr_t addr, unsigned long size) > return __ioremap_caller(addr, size, flags, caller); > } > > -void __iomem * ioremap_wc(phys_addr_t addr, unsigned long size) > +void __iomem *ioremap_wc(phys_addr_t addr, resource_size_t size) > { > unsigned long flags = _PAGE_NO_CACHE; > void *caller = __builtin_return_address(0); > @@ -281,8 +281,8 @@ void __iomem * ioremap_wc(phys_addr_t addr, unsigned long size) > return __ioremap_caller(addr, size, flags, caller); > } > > -void __iomem * ioremap_prot(phys_addr_t addr, unsigned long size, > - unsigned long flags) > +void __iomem *ioremap_prot(phys_addr_t addr, resource_size_t size, > + unsigned long flags) > { > void *caller = __builtin_return_address(0); > >
On Thu, Jun 11, 2015 at 05:42:00PM +0300, Cristian Stoica wrote: > Hi Greg, > > Can you have a look at this patch? > > https://patchwork.kernel.org/patch/6413191/ Why?
On 06/11/2015 06:38 PM, Greg KH wrote: > On Thu, Jun 11, 2015 at 05:42:00PM +0300, Cristian Stoica wrote: > > Why? > This patch matches the input argument "size" of ioremap() with the output of request_mem_region() (which is resource_size_t). Since the latter is used as input to the former, the types should match (even though mapping more than 4G is not usually expected). There are a lot of such differences in the code and this is an attempt to reduce that. Cristian S.
On Thu, Jun 11, 2015 at 07:10:36PM +0300, Cristian Stoica wrote: > On 06/11/2015 06:38 PM, Greg KH wrote: > > On Thu, Jun 11, 2015 at 05:42:00PM +0300, Cristian Stoica wrote: > > > > Why? > > > > This patch matches the input argument "size" of ioremap() with the output of request_mem_region() (which is > resource_size_t). > Since the latter is used as input to the former, the types should match (even though mapping more than 4G is not usually > expected). There are a lot of such differences in the code and this is an attempt to reduce that. No, why do I care about a ppc-only patch? There's nothing that I can do about this, please work with the proper maintainers and don't randomly poke people who have no responsibility about the code in question as it doesn't do any good. greg k-h
On Thu, 2015-06-11 at 19:10 +0300, Cristian Stoica wrote: > On 06/11/2015 06:38 PM, Greg KH wrote: > > On Thu, Jun 11, 2015 at 05:42:00PM +0300, Cristian Stoica wrote: > > > > Why? > > > > This patch matches the input argument "size" of ioremap() with the > output of request_mem_region() (which is > resource_size_t). > Since the latter is used as input to the former, the types should > match (even though mapping more than 4G is not usually > expected). There are a lot of such differences in the code and this > is an attempt to reduce that. Dropping the upper bits of the size harms the ability to detect error scenarios where unmappably large -- but not power-of-two -- regions are requested to be mapped. However, this patch doesn't fix that. It just postpones the loss of the upper 32 bits until __ioremap_caller() calls get_vm_area_caller(). There's also no error checking at all for the size of ioremap() done during early boot (!slab_is_available()). Don't just blindly turn static analyzer reports into patches -- and why didn't the analyzer complain about the call to get_vm_area_caller() after this patch? -Scott
On 06/12/2015 12:27 AM, Scott Wood wrote: > Dropping the upper bits of the size harms the ability to detect error > scenarios where unmappably large -- but not power-of-two -- regions > are requested to be mapped. > > However, this patch doesn't fix that. It just postpones the loss of > the upper 32 bits until __ioremap_caller() calls get_vm_area_caller(). > > There's also no error checking at all for the size of ioremap() done > during early boot (!slab_is_available()). Thanks for the explanation. I'll have a another look at the code. > Don't just blindly turn static analyzer reports into patches -- and > why didn't the analyzer complain about the call to > get_vm_area_caller() after this patch? The analysis that lead to this patch was targeted to a specific driver - in hindsight this is probably not the best approach. Cristian S.
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h index a8d2ef3..749c66e 100644 --- a/arch/powerpc/include/asm/io.h +++ b/arch/powerpc/include/asm/io.h @@ -716,24 +716,24 @@ static inline void iosync(void) * be hooked (but can be used by a hook on iounmap) * */ -extern void __iomem *ioremap(phys_addr_t address, unsigned long size); -extern void __iomem *ioremap_prot(phys_addr_t address, unsigned long size, +extern void __iomem *ioremap(phys_addr_t address, resource_size_t size); +extern void __iomem *ioremap_prot(phys_addr_t address, resource_size_t size, unsigned long flags); -extern void __iomem *ioremap_wc(phys_addr_t address, unsigned long size); +extern void __iomem *ioremap_wc(phys_addr_t address, resource_size_t size); #define ioremap_nocache(addr, size) ioremap((addr), (size)) extern void iounmap(volatile void __iomem *addr); -extern void __iomem *__ioremap(phys_addr_t, unsigned long size, +extern void __iomem *__ioremap(phys_addr_t, resource_size_t size, unsigned long flags); -extern void __iomem *__ioremap_caller(phys_addr_t, unsigned long size, +extern void __iomem *__ioremap_caller(phys_addr_t, resource_size_t size, unsigned long flags, void *caller); extern void __iounmap(volatile void __iomem *addr); extern void __iomem * __ioremap_at(phys_addr_t pa, void *ea, - unsigned long size, unsigned long flags); -extern void __iounmap_at(void *ea, unsigned long size); + resource_size_t size, unsigned long flags); +extern void __iounmap_at(void *ea, resource_size_t size); /* * When CONFIG_PPC_INDIRECT_PIO is set, we use the generic iomap implementation diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c index 7692d1b..a7d5137 100644 --- a/arch/powerpc/mm/pgtable_32.c +++ b/arch/powerpc/mm/pgtable_32.c @@ -135,7 +135,7 @@ pgtable_t pte_alloc_one(struct mm_struct *mm, unsigned long address) } void __iomem * -ioremap(phys_addr_t addr, unsigned long size) +ioremap(phys_addr_t addr, resource_size_t size) { return __ioremap_caller(addr, size, _PAGE_NO_CACHE | _PAGE_GUARDED, __builtin_return_address(0)); @@ -143,7 +143,7 @@ ioremap(phys_addr_t addr, unsigned long size) EXPORT_SYMBOL(ioremap); void __iomem * -ioremap_wc(phys_addr_t addr, unsigned long size) +ioremap_wc(phys_addr_t addr, resource_size_t size) { return __ioremap_caller(addr, size, _PAGE_NO_CACHE, __builtin_return_address(0)); @@ -151,7 +151,7 @@ ioremap_wc(phys_addr_t addr, unsigned long size) EXPORT_SYMBOL(ioremap_wc); void __iomem * -ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long flags) +ioremap_prot(phys_addr_t addr, resource_size_t size, unsigned long flags) { /* writeable implies dirty for kernel addresses */ if ((flags & (_PAGE_RW | _PAGE_RO)) != _PAGE_RO) @@ -173,13 +173,13 @@ ioremap_prot(phys_addr_t addr, unsigned long size, unsigned long flags) EXPORT_SYMBOL(ioremap_prot); void __iomem * -__ioremap(phys_addr_t addr, unsigned long size, unsigned long flags) +__ioremap(phys_addr_t addr, resource_size_t size, unsigned long flags) { return __ioremap_caller(addr, size, flags, __builtin_return_address(0)); } void __iomem * -__ioremap_caller(phys_addr_t addr, unsigned long size, unsigned long flags, +__ioremap_caller(phys_addr_t addr, resource_size_t size, unsigned long flags, void *caller) { unsigned long v, i; diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c index 59daa5e..64dd7a9 100644 --- a/arch/powerpc/mm/pgtable_64.c +++ b/arch/powerpc/mm/pgtable_64.c @@ -168,8 +168,8 @@ int map_kernel_page(unsigned long ea, unsigned long pa, int flags) * __ioremap_at - Low level function to establish the page tables * for an IO mapping */ -void __iomem * __ioremap_at(phys_addr_t pa, void *ea, unsigned long size, - unsigned long flags) +void __iomem *__ioremap_at(phys_addr_t pa, void *ea, resource_size_t size, + unsigned long flags) { unsigned long i; @@ -202,7 +202,7 @@ void __iomem * __ioremap_at(phys_addr_t pa, void *ea, unsigned long size, * are manipulated manually, like partial unmapping of * PCI IOs or ISA space. */ -void __iounmap_at(void *ea, unsigned long size) +void __iounmap_at(void *ea, resource_size_t size) { WARN_ON(((unsigned long)ea) & ~PAGE_MASK); WARN_ON(size & ~PAGE_MASK); @@ -210,8 +210,8 @@ void __iounmap_at(void *ea, unsigned long size) unmap_kernel_range((unsigned long)ea, size); } -void __iomem * __ioremap_caller(phys_addr_t addr, unsigned long size, - unsigned long flags, void *caller) +void __iomem *__ioremap_caller(phys_addr_t addr, resource_size_t size, + unsigned long flags, void *caller) { phys_addr_t paligned; void __iomem *ret; @@ -255,13 +255,13 @@ void __iomem * __ioremap_caller(phys_addr_t addr, unsigned long size, return ret; } -void __iomem * __ioremap(phys_addr_t addr, unsigned long size, - unsigned long flags) +void __iomem *__ioremap(phys_addr_t addr, resource_size_t size, + unsigned long flags) { return __ioremap_caller(addr, size, flags, __builtin_return_address(0)); } -void __iomem * ioremap(phys_addr_t addr, unsigned long size) +void __iomem *ioremap(phys_addr_t addr, resource_size_t size) { unsigned long flags = _PAGE_NO_CACHE | _PAGE_GUARDED; void *caller = __builtin_return_address(0); @@ -271,7 +271,7 @@ void __iomem * ioremap(phys_addr_t addr, unsigned long size) return __ioremap_caller(addr, size, flags, caller); } -void __iomem * ioremap_wc(phys_addr_t addr, unsigned long size) +void __iomem *ioremap_wc(phys_addr_t addr, resource_size_t size) { unsigned long flags = _PAGE_NO_CACHE; void *caller = __builtin_return_address(0); @@ -281,8 +281,8 @@ void __iomem * ioremap_wc(phys_addr_t addr, unsigned long size) return __ioremap_caller(addr, size, flags, caller); } -void __iomem * ioremap_prot(phys_addr_t addr, unsigned long size, - unsigned long flags) +void __iomem *ioremap_prot(phys_addr_t addr, resource_size_t size, + unsigned long flags) { void *caller = __builtin_return_address(0);
Use resource_size_t to accommodate sizes greater than the size of an unsigned long int on platforms that have more than 32 bit physical addresses Signed-off-by: Cristian Stoica <cristian.stoica@freescale.com> Changes in v2: - update definitions is pgtable_64.c as well - or else we get broken builds on 64 - minor reformatting in pgtable_64.c to satisfy checkpatch --- arch/powerpc/include/asm/io.h | 14 +++++++------- arch/powerpc/mm/pgtable_32.c | 10 +++++----- arch/powerpc/mm/pgtable_64.c | 22 +++++++++++----------- 3 files changed, 23 insertions(+), 23 deletions(-)