Re: [PATCH v6] drm/i915: Fix NULL pointer dereference in capture_engine

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 04 Dec 2024, Andi Shyti <andi.shyti@linux.intel.com> wrote:
> Hi Michal,
>
>> > +	if (rq && !i915_request_started(rq)) {
>> > +		/*
>> > +		* We want to know also what is the gcu_id of the context,
>> 
>> typo: guc_id
>> 
>> > +		* but if we don't have the context reference, then skip
>> > +		* printing it.
>> > +		*/
>> 
>> but IMO this comment is redundant as it's quite obvious that without
>> context pointer you can't print guc_id member
>
> I recommended to add a comment because there is some code
> redundancy that, I think, needs some explanation; someone might
> not spot immediately the need for ce, unless goes a the end of
> the drm_info parameter's list.
>
>> > +		if (ce)
>> > +			drm_info(&engine->gt->i915->drm,
>> > +				"Got hung context on %s with active request %lld:%lld [0x%04X] not yet started\n",
>> > +				engine->name, rq->fence.context, rq->fence.seqno, ce->guc_id.id);
>> > +		else
>> > +			drm_info(&engine->gt->i915->drm,
>> > +				"Got hung context on %s with active request %lld:%lld not yet started\n",
>> > +				engine->name, rq->fence.context, rq->fence.seqno);
>> 
>> since you are touching drm_info() where we use engine->gt then maybe
>> it's good time to switch to gt_info() to get better per-GT message?
>
> I think the original reason for using drm_info is because we are
> outside the GT. But, because the engine belongs to the GT, it
> makes also sense to use gt_info(), I don't oppose.
>
> It would make more sense to move this function completely into
> gt/.

Can we converge on the patch instead of diverge, please?

It's a Cc: stable null pointer dereference fix.

It's already been iterated for two weeks to reach v6.

Fix the comment typo while applying, but there's nothing inherently
wrong here AFAICT. Merge it and move on.


BR,
Jani.


-- 
Jani Nikula, Intel