On 10/26/26 11:57, John Harrison wrote:
I'm finding more and more tests that explicitly test a null DC. Also, a stub DC with a link_count of zero is not sufficient. All the IRQ code goes boom trying to configure interrupts because dc->res_pool->irqs is dc->[null]->[null]. Hacking up a fake res_pool and a fake irq structure would presumably be possible but then the driver will be attempting to enable interrupts in a non-hardware kunit test. I haven't gone further than that to see if anything else goes boom with a partial DC yet.On 10/23/26 00:48, Albert Esteve wrote:I was meaning the later comment "DC is absent (NULL), so the HW writes are NULL-safe no-ops.". To me that reads as the test is explicitly checking that a NULL DC is safe. Also, the dm_test_hpd_init_empty_connectors test says "init must complete without touching the DC". Which also seems to be saying that a null DC is a valid thing to happen and must be allowed.Hi John,On Thu, Oct 22, 2026 at 10:31:54AM -0700, John Harrison wrote:A bunch of the KUNIT tests (dm_test_hpd_init_*) expect that the init function can be called with adev->dm.dc set to null. Indeed, the dm_test_hpd_init_fini_with_connectors test has a comment explicitly saying that the null is intentional. Unfortunately, since the patch mentioned below, the first thing that the init function does is access adev->dm.dc->link_count. Thus it is a null pointer dereference and a boom (at least if running with KASAN). I'm not sure if a null DC can ever occur outside of the KUNIT tests, but there is presumably a reason for the KUNITs explicitly testing it. So add a null check around the link_count loop.This feels backwards reasoning. The tests assume in comments that "a NULL dc that fallback is a safe no-op", which isn't true after commit e38159437a91. Also, in amdgpu_dm_init() the DC is created and checked, and if it is NULL errors out. Thus, DC is never NULL at amdgpu_dm_hpd_init() in production. The fix should be in the tests, maybe allocating a stub with link_count 0?If you are certain that there is never a possibility nor a reason for having a null DC in reality, then sure, the tests can be updated instead. This is not code I am especially familiar with - I'm just trying to run the tests to ensure that other changes don't break existing drivers. I just don't want to go removing test coverage simply because a driver change breaks a test - that is generally the backwards direction!John.
It seems like trying to force a fake DC is an unravelling thread that will mean a lot of changes to a lot of tests and maybe to the driver too.
As the driver currently stands, the tests explode with null pointer dereferences. Fixing up the one patch that broke a fundamental requirement of so many tests by adding a simple null-pointer check and returning the original functionality seems like a sensible first step. We get the test coverage back with no real downside to the driver. If the people that really know this code want to do a more involved fix then that is up to them to decide how it should be done.
John.
BR, AlbertFixes: e38159437a91 ("drm/amd/display: only clear the hotplug sources the ASIC has")Cc: Mattia Tadini <[email protected]> Cc: Roman Li <[email protected]> Cc: Alex Deucher <[email protected]> Cc: Alex Hung <[email protected]> Cc: George Zhang <[email protected]> Cc: Bhawanpreet Lakha <[email protected]> Cc: "Mario Limonciello (AMD)" <[email protected]> Cc: Leo Li <[email protected]> Cc: "Timur Kristóf" <[email protected]> Cc: Albert Esteve <[email protected]> Cc: Geoffrey McRae <[email protected]> Cc: Harry Wentland <[email protected]> Signed-off-by: John Harrison <[email protected]> ---.../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 26 ++++++++++---------1 file changed, 14 insertions(+), 12 deletions(-)diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.cindex 5b4ff079cd55..427f7bba343d 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c@@ -989,18 +989,20 @@ void amdgpu_dm_hpd_init(struct amdgpu_device *adev) * has rather than over the whole enum: the unused slots of the DC* interrupt table hold dummy handlers that assert when touched. */ - for (i = 0; i < adev->dm.dc->link_count; i++) { - const struct dc_link *link = adev->dm.dc->links[i]; - - if (link->irq_source_hpd != DC_IRQ_SOURCE_INVALID && - !amdgpu_dm_irq_set(adev, link->irq_source_hpd, false)) - drm_err(dev, "Failed to clear hpd source=%d on init\n", - link->irq_source_hpd); - - if (link->irq_source_hpd_rx != DC_IRQ_SOURCE_INVALID && - !amdgpu_dm_irq_set(adev, link->irq_source_hpd_rx, false)) - drm_err(dev, "Failed to clear hpdrx source=%d on init\n", - link->irq_source_hpd_rx); + if (adev->dm.dc) { + for (i = 0; i < adev->dm.dc->link_count; i++) { + const struct dc_link *link = adev->dm.dc->links[i]; + + if (link->irq_source_hpd != DC_IRQ_SOURCE_INVALID && + !amdgpu_dm_irq_set(adev, link->irq_source_hpd, false))+ drm_err(dev, "Failed to clear hpd source=%d on init\n",+ link->irq_source_hpd); + + if (link->irq_source_hpd_rx != DC_IRQ_SOURCE_INVALID &&+ !amdgpu_dm_irq_set(adev, link->irq_source_hpd_rx, false)) + drm_err(dev, "Failed to clear hpdrx source=%d on init\n",+ link->irq_source_hpd_rx); + } } drm_connector_list_iter_begin(dev, &iter); -- 2.43.0
