[PATCH i-g-t v2 2/2] tests/intel: check drmModeGetConnector() return in i915_pm_rpm

[Luca Coelho]

On Wed, 2024-05-29 at 18:37 +0200, Kamil Konieczny wrote:
> Hi Luca,
> On 2024-05-28 at 10:58:55 +0300, Luca Coelho wrote:
> 
> this subject imho is too long:
> 
> [PATCH i-g-t v2 2/2] tests/intel: check drmModeGetConnector() return in i915_pm_rpm
> 
> It should have test name at begin, not at end:
> 
> tests/intel/i915_pm_rpm: Fix bug in init_mode_set_data()
> 
> or
> 
> tests/intel/i915_pm_rpm: check drmModeGetConnector() before use
> 
> or find out something better.

This is the same as in the previous patch.  So, just tell me which one
of your options you want me to use and I'll do it.  No need to bike-
shed on this here.


> > The drmModeGetConnector() function can return NULL in some cases, so
> > we need to check the return value before accessing it.  This is not
> > being checked in init_mode_set_data(), so fix that.
> 
> Please cite few lines from bug report:
> 
> Starting subtest: module-reload
> Received signal SIGSEGV.
> Stack trace: 
> #0 [fatal_sig_handler+0x10f]
> #1 [killpg+0x40]
> #2 [setup_environment+0x33f]

This is one click away, in the link I added below, but I see the
potential usefulness to copy it here.  It could make it easier to find
patches that fix bugs in specific tests, if you use git grep, for
instance.  Though I'm in general of the opinion that duplicating
information that is already linked in one way or another.

But sure, I'll add it. :)


> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10911
> > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> 
> Add here also some KMS devs to Cc, I added Jani, Juha-Pekka and Bhanuprakash.
> 
> Cc: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> Cc: Jani Saarinen <jani.saarinen at intel.com>
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>

Okay. :)

Do you have something like the kernel's get_maintainer.pl script?
Otherwise it's hard to know who should be justifiably spammed. ;)


> > Signed-off-by: Luca Coelho <luciano.coelho at intel.com>
> > ---
> >  tests/intel/i915_pm_rpm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/intel/i915_pm_rpm.c b/tests/intel/i915_pm_rpm.c
> > index 0ea5fbd8afd9..3b4034f24a57 100644
> > --- a/tests/intel/i915_pm_rpm.c
> > +++ b/tests/intel/i915_pm_rpm.c
> > @@ -515,6 +515,12 @@ static void init_mode_set_data(struct mode_set_data *data)
> >  			data->connectors[i] =
> >  				drmModeGetConnector(drm_fd,
> >  						    data->res->connectors[i]);
> > +			if (!data->connectors[i]) {
> 
> imho we should also check it later before using this or edids[]
> so maybe this should really be a warn? Or even igt_assert? Or maybe
> we should lower number of available connectors? Please ask KMS devs
> for possible solution here.

There probably are other places where this could be a problem, for
sure.  But I'm fixing a specific bug and maybe other changes should be
in separate patches? I actually checked all the places where
drmModeGetConnector() is called and only this part and the one in
chamelium code had to be changed.

I did discuss this with the kernel display driver team, I'm part of it
after all. ;) The driver can't guarantee the validity of a connector
returned in the list when we call drmModeGetConnector(), because it can
be disconnected at any time an return NULL.  Another option we
considered was to simply restart the test silently (or at least get the
list of connector IDs again), because this happens because of race
conditions.  But at this stage, I wanted a fix to prevent a crash, not
a full refactor of the test case.

One of my goals was to change the test as little as possible.  Without
my change, if this happened, the test would just crash.  I originally
made it warn, which, like the crash, would cause the test to fail,
making it visible.  But you asked me to change it to igt_debug(), so I
did.


> Above code should work if you also fix fini_mode_set_data() below
> and also at one more function:
> 
> grep -n count_connectors tests/intel/i915_pm_rpm.c
> 
> 319:    for (int i = 0; i < data->res->count_connectors; i++) {
> 513:            igt_assert(data->res->count_connectors <= MAX_CONNECTORS);
> 514:            for (int i = 0; i < data->res->count_connectors; i++) {
> 531:            for (int i = 0; i < data->res->count_connectors; i++) {

I don't think the checks should be where count_connectors is used, but
in the functions that may fail if the connector is (or becomse)
invalid.  That's where the crash can happen if we assume it returns a
valid pointer and not NULL.

--
Cheers,
Luca.