On Mon, Oct 26, 2026 at 01:16:34PM -0700, John Harrison wrote: > On 10/26/26 11:57, John Harrison wrote: > > On 10/23/26 00:48, Albert Esteve wrote: > > > 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? > > 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. > > > > 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!
I am as certain as the code exploration allowed me to be, which means to a limited extent... Also, not wanting to remove test coverage is a valid point. I mean, to me changing production code just because a test needs fixing tickles my spidy sense so I questioned it... > > > > John. > 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. > > 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. ... but you are right here. Checking for NULL is a defensive check with no real downside. Plus all the problems you raised with changing the test. Thanks for exploring that further. Reviewed-by: Albert Esteve <[email protected]> Thanks! Albert > > John. > > > > > > BR, > > > Albert > > > > > > > Fixes: 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.c > > > > index 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 > > > > > > >
