Am 05.12.2016 um 09:39 schrieb Nicolai Stange:
Christian König <christian.koenig@xxxxxxx> 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@xxxxxxxxx>
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@xxxxxxx>.
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.
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.
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.
Regards,
Christian.
Thanks,
Nicolai
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
