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

[Kamil Konieczny]

Hi Luca,
On 2024-05-30 at 00:45:12 +0300, Luca Coelho wrote:
> 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

It is better to give such info here in patch itself,
especially as this is needed for reproduction.

Regards,
Kamil

> 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.