On Tue, 21 Jul 2020 13:19:36 -0400 "Michael J. Ruhl" <michael.j.ruhl@xxxxxxxxx> wrote:
> The !ATOMIC_IOMAP version of io_maping_init_wc will always return
> success, even when the ioremap fails.
>
> Since the ATOMIC_IOMAP version returns NULL when the init fails, and
> callers check for a NULL return on error this is unexpected.
>
> During a device probe, where the ioremap failed, a crash can look
> like this:
>
> BUG: unable to handle page fault for address: 0000000000210000
> #PF: supervisor write access in kernel mode
> #PF: error_code(0x0002) - not-present page
> Oops: 0002 [#1] PREEMPT SMP
> CPU: 0 PID: 177 Comm:
> RIP: 0010:fill_page_dma [i915]
> gen8_ppgtt_create [i915]
> i915_ppgtt_create [i915]
> intel_gt_init [i915]
> i915_gem_init [i915]
> i915_driver_probe [i915]
> pci_device_probe
> really_probe
> driver_probe_device
>
> The remap failure occurred much earlier in the probe. If it had
> been propagated, the driver would have exited with an error.
>
> Return NULL on ioremap failure.
>
> ...
>
> --- a/include/linux/io-mapping.h
> +++ b/include/linux/io-mapping.h
> @@ -118,7 +118,7 @@ io_mapping_init_wc(struct io_mapping *iomap,
> iomap->prot = pgprot_noncached(PAGE_KERNEL);
> #endif
>
> - return iomap;
> + return iomap->iomem ? iomap : NULL;
> }
>
> static inline void
LGTM. However I do think it would be stylistically better/typical to
detect and handle the error early, rather than to blunder on,
pointlessly initializing things?
--- a/include/linux/io-mapping.h~io-mapping-indicate-mapping-failure-fix
+++ a/include/linux/io-mapping.h
@@ -107,9 +107,12 @@ io_mapping_init_wc(struct io_mapping *io
resource_size_t base,
unsigned long size)
{
+ iomap->iomem = ioremap_wc(base, size);
+ if (!iomap->iomem)
+ return NULL;
+
iomap->base = base;
iomap->size = size;
- iomap->iomem = ioremap_wc(base, size);
#if defined(pgprot_noncached_wc) /* archs can't agree on a name ... */
iomap->prot = pgprot_noncached_wc(PAGE_KERNEL);
#elif defined(pgprot_writecombine)
@@ -118,7 +121,7 @@ io_mapping_init_wc(struct io_mapping *io
iomap->prot = pgprot_noncached(PAGE_KERNEL);
#endif
- return iomap->iomem ? iomap : NULL;
+ return iomap;
}
static inline void
_
