Re: [PATCH i-g-t v3 2/2] tests/intel/i915_pm_rpm: check drmModeGetConnector() before use

["Coelho, Luciano" <luciano.coelho@intel.com>]

On Thu, 2024-06-13 at 15:32 +0200, Kamil Konieczny wrote:
> Hi Coelho,,
> On 2024-06-12 at 14:10:56 +0000, Coelho, Luciano wrote:
> > On Fri, 2024-06-07 at 17:11 +0200, Kamil Konieczny wrote:
> > > Hi Luca,
> > > On 2024-05-30 at 11:59:08 +0300, Luca Coelho wrote:
> > > > 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.
> > > > 
> > > > From the bug report:
> > > > 
> > > > Starting subtest: module-reload
> > > > Received signal SIGSEGV.
> > > > Stack trace:
> > > 
> > > Now I think that line 'Stack trace' is not needed, only those
> > > two with subtest name and signal are enough.
> > 
> > Okay... it's mostly noise IMHO, anyway.
> > 
> > 
> > > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10911
> > > > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > > > Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > > > Cc: Jani Saarinen <jani.saarinen@intel.com>
> > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > Signed-off-by: Luca Coelho <luciano.coelho@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..e0f395fa8a84 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]) {
> > > > +				igt_warn("Could not read connector %u\n",
> > > ----------------^^^^^^^^
> > > igt_debug
> > > 
> > > > +					 data->res->connectors[i]);
> > > > +				continue;
> > > > +			}
> > > > +
> > > 
> > > We generally do not warn and fail a test in case some prerequistics
> > > are not present, that is why it is better to igt_debug here
> > 
> > This is not about pre-requisites.  This is about a race condition where
> > we get a larger number of connectors than what is actually available
> > later.
> 
> You didn't write that in a comment in code nor in commit message,
> looks like it could help later on when this happen.

Right, that is one known case, but it's not the only time it could
happen, actually.  That's why there's no mention in the commit message.
The connector may have disappeared (disconnected physically, for
example), so the drmModeGetConnector() may return NULL at any time. 
What I did was check all callers of drmModeGetConnector() to see if
they were checking for NULL or safe in some other way.  This is what I
tried to convey in the commit message.


> > I think the test should fail in this case, because then we will look at
> > what is going on and try to figure out what's causing it.
> > 
> > I had igt_warn(), then you told me to change to igt_debug(), then you
> > told me to change it back to igt_warn() and now you're asking me to
> > change it to igt_debug, *again*?!
> > 
> 
> Nevermind, let it be warn.

Okay.


> > > and make checks in few other places, like:
> > > 			if (data->connectors[i]) {
> > >                     // here we can safely use/free connector or edid
> > >             }
> > > or
> > > 			if (!data->connectors[i])
> > >                 continue;
> > 
> > I don't get it.  Can you explain in more details?
> > 
> 
> I did a check locally and it seems NULLs there are not a problem,
> so your change seems ok,
> 
> Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

Thanks for the reviews, Kamil!

--
Cheers,
Luca.