| Message ID | 20190802123956.2450-1-sergey.senozhatsky@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] i915: convert to new mount API | expand |
Quoting Sergey Senozhatsky (2019-08-02 13:39:55) > tmpfs does not set ->remount_fs() anymore and its users need > to be converted to new mount API. > > BUG: kernel NULL pointer dereference, address: 0000000000000000 > PF: supervisor instruction fetch in kernel mode > PF: error_code(0x0010) - not-present page > RIP: 0010:0x0 > Code: Bad RIP value. > Call Trace: > i915_gemfs_init+0x6e/0xa0 [i915] > i915_gem_init_early+0x76/0x90 [i915] > i915_driver_probe+0x30a/0x1640 [i915] > ? kernfs_activate+0x5a/0x80 > ? kernfs_add_one+0xdd/0x130 > pci_device_probe+0x9e/0x110 > really_probe+0xce/0x230 > driver_probe_device+0x4b/0xc0 > device_driver_attach+0x4e/0x60 > __driver_attach+0x47/0xb0 > ? device_driver_attach+0x60/0x60 > bus_for_each_dev+0x61/0x90 > bus_add_driver+0x167/0x1b0 > driver_register+0x67/0xaa > > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > --- > drivers/gpu/drm/i915/gem/i915_gemfs.c | 28 ++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c > index 099f3397aada..cf05ba72df9d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c > +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c > @@ -7,14 +7,17 @@ > #include <linux/fs.h> > #include <linux/mount.h> > #include <linux/pagemap.h> > +#include <linux/fs_context.h> > > #include "i915_drv.h" > #include "i915_gemfs.h" > > int i915_gemfs_init(struct drm_i915_private *i915) > { > + struct fs_context *fc = NULL; > struct file_system_type *type; > struct vfsmount *gemfs; > + bool ok = true; Start with ok = false, we only need to set to true if we succeed in reconfiguring. > type = get_fs_type("tmpfs"); > if (!type) > @@ -36,18 +39,29 @@ int i915_gemfs_init(struct drm_i915_private *i915) > struct super_block *sb = gemfs->mnt_sb; > /* FIXME: Disabled until we get W/A for read BW issue. */ > char options[] = "huge=never"; > - int flags = 0; > - int err; Hmm, we could avoid this if we used vfs_kernel_mount() directly rather than the kern_mount wrapper, as then we pass options through to parse_monotithic_mount_data(). Or am I barking up the wrong tree? > > - err = sb->s_op->remount_fs(sb, &flags, options); > - if (err) { > - kern_unmount(gemfs); > - return err; > + fc = fs_context_for_reconfigure(sb->s_root, 0, 0); > + if (IS_ERR(fc)) { > + ok = false; > + goto out; > } > + > + if (!fc->ops->parse_monolithic || > + fc->ops->parse_monolithic(fc, options)) { checkpatch.pl will complain that this should line up with the '(' > + ok = false; > + goto out; > + } > + > + if (!fc->ops->reconfigure || fc->ops->reconfigure(fc)) > + ok = false; > } > > +out: > + if (!ok) > + pr_err("i915 gemfs reconfiguration failed\n"); Let's make it a bit more user friendly, dev_err(i915->drm.dev, "Unable to reconfigure internal shmemfs for preferred" " allocation strategy; continuing, but performance may suffer.\n"); Assuming that we can't just use vfs_kern_mount() instead, with the nits Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On (08/02/19 13:54), Chris Wilson wrote: [..] > > int i915_gemfs_init(struct drm_i915_private *i915) > > { > > + struct fs_context *fc = NULL; > > struct file_system_type *type; > > struct vfsmount *gemfs; > > + bool ok = true; > > Start with ok = false, we only need to set to true if we succeed in > reconfiguring. OK, makes sense. > > type = get_fs_type("tmpfs"); > > if (!type) > > @@ -36,18 +39,29 @@ int i915_gemfs_init(struct drm_i915_private *i915) > > struct super_block *sb = gemfs->mnt_sb; > > /* FIXME: Disabled until we get W/A for read BW issue. */ > > char options[] = "huge=never"; > > - int flags = 0; > > - int err; > > Hmm, we could avoid this if we used vfs_kernel_mount() directly rather > than the kern_mount wrapper, as then we pass options through to > parse_monotithic_mount_data(). Or am I barking up the wrong tree? Hmm. Wouldn't this error on !TRANSPARENT_HUGE_PAGECACHE systems? "huge=never" should be an invalid option when system does not know about THP. [..] > > + if (!fc->ops->parse_monolithic || > > + fc->ops->parse_monolithic(fc, options)) { > > checkpatch.pl will complain that this should line up with the '(' It doesn't. ------------------------------------------------- outgoing/0001-i915-convert-to-new-mount-API.patch ------------------------------------------------- total: 0 errors, 0 warnings, 53 lines checked outgoing/0001-i915-convert-to-new-mount-API.patch has no obvious style problems and is ready for submission. ------------------------------------------------------- outgoing/0002-i915-do-not-leak-module-ref-counter.patch ------------------------------------------------------- total: 0 errors, 0 warnings, 11 lines checked outgoing/0002-i915-do-not-leak-module-ref-counter.patch has no obvious style problems and is ready for submission. [..] > > + if (!ok) > > + pr_err("i915 gemfs reconfiguration failed\n"); > > Let's make it a bit more user friendly, > > dev_err(i915->drm.dev, > "Unable to reconfigure internal shmemfs for preferred" > " allocation strategy; continuing, but performance may suffer.\n"); I guess now checkpatch will complain :) > Assuming that we can't just use vfs_kern_mount() instead, with the nits > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Thanks. I'll sit on it for several days, just to see if more feedback will come. -ss
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c index 099f3397aada..cf05ba72df9d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gemfs.c +++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c @@ -7,14 +7,17 @@ #include <linux/fs.h> #include <linux/mount.h> #include <linux/pagemap.h> +#include <linux/fs_context.h> #include "i915_drv.h" #include "i915_gemfs.h" int i915_gemfs_init(struct drm_i915_private *i915) { + struct fs_context *fc = NULL; struct file_system_type *type; struct vfsmount *gemfs; + bool ok = true; type = get_fs_type("tmpfs"); if (!type) @@ -36,18 +39,29 @@ int i915_gemfs_init(struct drm_i915_private *i915) struct super_block *sb = gemfs->mnt_sb; /* FIXME: Disabled until we get W/A for read BW issue. */ char options[] = "huge=never"; - int flags = 0; - int err; - err = sb->s_op->remount_fs(sb, &flags, options); - if (err) { - kern_unmount(gemfs); - return err; + fc = fs_context_for_reconfigure(sb->s_root, 0, 0); + if (IS_ERR(fc)) { + ok = false; + goto out; } + + if (!fc->ops->parse_monolithic || + fc->ops->parse_monolithic(fc, options)) { + ok = false; + goto out; + } + + if (!fc->ops->reconfigure || fc->ops->reconfigure(fc)) + ok = false; } +out: + if (!ok) + pr_err("i915 gemfs reconfiguration failed\n"); + if (!IS_ERR_OR_NULL(fc)) + put_fs_context(fc); i915->mm.gemfs = gemfs; - return 0; }
tmpfs does not set ->remount_fs() anymore and its users need to be converted to new mount API. BUG: kernel NULL pointer dereference, address: 0000000000000000 PF: supervisor instruction fetch in kernel mode PF: error_code(0x0010) - not-present page RIP: 0010:0x0 Code: Bad RIP value. Call Trace: i915_gemfs_init+0x6e/0xa0 [i915] i915_gem_init_early+0x76/0x90 [i915] i915_driver_probe+0x30a/0x1640 [i915] ? kernfs_activate+0x5a/0x80 ? kernfs_add_one+0xdd/0x130 pci_device_probe+0x9e/0x110 really_probe+0xce/0x230 driver_probe_device+0x4b/0xc0 device_driver_attach+0x4e/0x60 __driver_attach+0x47/0xb0 ? device_driver_attach+0x60/0x60 bus_for_each_dev+0x61/0x90 bus_add_driver+0x167/0x1b0 driver_register+0x67/0xaa Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> --- drivers/gpu/drm/i915/gem/i915_gemfs.c | 28 ++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)