On Mon, Dec 05, 2016 at 09:48:02AM +0100, Christian König wrote:
Am 05.12.2016 um 09:39 schrieb Nicolai Stange:
Christian König christian.koenig@amd.com writes:
Am 05.12.2016 um 08:27 schrieb Daniel Vetter:
On Sat, Dec 03, 2016 at 03:47:00PM +0100, Nicolai Stange wrote:
Since commit 8a357d10043c ("drm: Nerf DRM_CONTROL nodes"), a struct drm_device's ->control member is always NULL.
In the case of CONFIG_DEBUG_FS=y, radeon_debugfs_add_files() accesses ->control->debugfs_root though. This results in the following Oops:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000018 IP: radeon_debugfs_add_files+0x90/0x100 [radeon] PGD 0 Oops: 0000 [#1] SMP [...] Call Trace: ? work_on_cpu+0xb0/0xb0 radeon_fence_driver_init+0x120/0x150 [radeon] si_init+0x122/0xd50 [radeon] ? _raw_spin_unlock_irq+0x2c/0x40 ? device_pm_check_callbacks+0xb3/0xc0 radeon_device_init+0x958/0xda0 [radeon] radeon_driver_load_kms+0x9a/0x210 [radeon] drm_dev_register+0xa9/0xd0 [drm] drm_get_pci_dev+0x9c/0x1e0 [drm] radeon_pci_probe+0xb8/0xe0 [radeon] [...]Fix this by omitting the drm_debugfs_create_files() call for the control minor debugfs directory which is now non-existent anyway.
Fixes: 8a357d10043c ("drm: Nerf DRM_CONTROL nodes") Signed-off-by: Nicolai Stange nicstange@gmail.com
Applied to drm-misc with Dave's irc ack, thanks for your patch.
If it's still worth it the patch is Reviewed-by: Christian König christian.koenig@amd.com.
On the other hand when ->control is always NULL, why do we still have ->control anyway?
Yes, I was wondering about that, too.
Quoting from 8a357d10043c ("drm: Nerf DRM_CONTROL nodes"):
Since I don't like dead uabi, let's remove it. But since this would be a really big change I think it's better to start out small by simply not registering anything. We can garbage-collect the dead code later on, once we're sure it's really not used anywhere.
I'd too prefer compile time errors by purging ->control here. Daniel?
Seconded.
We're super-late for 4.10, I think that'd would need to be postponened for 4.11. Not need to wait with creating the patches (drm-misc is always open), but wee need to plug the oopses first.
And BTW: Please double check the other drivers as well.
# git grep '->control' -- drivers/gpu/ drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: adev->ddev->control->debugfs_root, drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: adev->ddev->control); drivers/gpu/drm/amd/amdgpu/amdgpu_device.c: adev->ddev->control);
Oops.
Yeah, that's what I expected as well but Daniel said it would only affect qxl.
No idea why I've missed that, I guess coffee wasn't working.
drivers/gpu/drm/amd/amdkfd/kfd_dbgdev.c: ib_packet->control = (1 << 23) | (1 << 31) | drivers/gpu/drm/drm_drv.c: return &dev->control;
That's drm_minor_get_slot(dev, type), but grepping for DRM_MINOR_CONTROL doesn't yield anything -> dead code.
drivers/gpu/drm/gma500/psb_intel_sdvo.c: switch (sdvo->controlled_output) { drivers/gpu/drm/gma500/psb_intel_sdvo.c: psb_intel_sdvo->controlled_output |= SDVO_OUTPUT_TMDS0; drivers/gpu/drm/gma500/psb_intel_sdvo.c: psb_intel_sdvo->controlled_output |= SDVO_OUTPUT_TMDS1; drivers/gpu/drm/gma500/psb_intel_sdvo.c: psb_intel_sdvo->controlled_output |= type; drivers/gpu/drm/gma500/psb_intel_sdvo.c: psb_intel_sdvo->controlled_output |= SDVO_OUTPUT_RGB0; drivers/gpu/drm/gma500/psb_intel_sdvo.c: psb_intel_sdvo->controlled_output |= SDVO_OUTPUT_RGB1; drivers/gpu/drm/gma500/psb_intel_sdvo.c: psb_intel_sdvo->controlled_output |= SDVO_OUTPUT_LVDS0; drivers/gpu/drm/gma500/psb_intel_sdvo.c: psb_intel_sdvo->controlled_output |= SDVO_OUTPUT_LVDS1; drivers/gpu/drm/gma500/psb_intel_sdvo.c: psb_intel_sdvo->controlled_output = 0; drivers/gpu/drm/i915/intel_sdvo.c: switch (sdvo->controlled_output) { drivers/gpu/drm/i915/intel_sdvo.c: intel_sdvo->controlled_output |= SDVO_OUTPUT_TMDS0; drivers/gpu/drm/i915/intel_sdvo.c: intel_sdvo->controlled_output |= SDVO_OUTPUT_TMDS1; drivers/gpu/drm/i915/intel_sdvo.c: intel_sdvo->controlled_output |= type; drivers/gpu/drm/i915/intel_sdvo.c: intel_sdvo->controlled_output |= SDVO_OUTPUT_RGB0; drivers/gpu/drm/i915/intel_sdvo.c: intel_sdvo->controlled_output |= SDVO_OUTPUT_RGB1; drivers/gpu/drm/i915/intel_sdvo.c: intel_sdvo->controlled_output |= SDVO_OUTPUT_LVDS0; drivers/gpu/drm/i915/intel_sdvo.c: intel_sdvo->controlled_output |= SDVO_OUTPUT_LVDS1; drivers/gpu/drm/i915/intel_sdvo.c: intel_sdvo->controlled_output = 0; drivers/gpu/drm/msm/msm_debugfs.c: ret = late_init_minor(dev->control);
Not an oops but dead code.
drivers/gpu/drm/qxl/qxl_debugfs.c: qdev->ddev->control->debugfs_root, drivers/gpu/drm/qxl/qxl_debugfs.c: qdev->ddev->control); drivers/gpu/drm/qxl/qxl_debugfs.c: qdev->ddev->control);
Oops.
I'll send compile-only tested patches either tonight or tomorrow. Shall they cover the oopses only or the dead code as well?
Please start with the ops, cause we certainly will get complains about that rather fast.
I sent out qxl already, and yes oops patches first pls. -Daniel