[Intel-gfx] [PATCH v2 3/5] drm/i915/guc: Provide debugfs for log relay sub-buf info

[Teres Alexis, Alan Previn]

Finally got some time to relook at this series. Responses.
I'll re-rev this and re-connect with Ashutosh offline considering how long i've been silent on this.


On Wed, 2022-12-07 at 08:43 -0800, Dixit, Ashutosh wrote:
> On Tue, 06 Dec 2022 01:20:58 -0800, Alan Previn wrote:
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
> > index ddfbe334689f..27756640338e 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log_debugfs.c
> > @@ -105,6 +105,38 @@ DEFINE_SIMPLE_ATTRIBUTE(guc_log_level_fops,
> > 			guc_log_level_get, guc_log_level_set,
> > 			"%lld\n");
> > 
> > +static int guc_log_relay_subbuf_size_get(void *data, u64 *val)
> > +{
> > +	struct intel_guc_log *log = data;
> > +
> > +	if (!log->vma)
> > +		return -ENODEV;
> 
> For the record, from the other email thread, the issue here is whether this
> check is needed.
> 
> Also, the issue is what happens if the relay is open and we unload the
> module, what happens?
> 
I'll retest this - but I clearly remember that if the user space app was stil holding
onto the debugfs handle, the i915 unload would go through most of the driver unload /
unregister steps, while the app doesnt get any signals but if the app were to close that
handle after that, (guc_log_relay_ctl_release gets called), we do get invalid ptr access
in kernel. Take note the logger tool runs with sudo. That said something "like" above check
is required but perhaps hanging off a still-valid ptr (like i915->foo - maybe gt-struct validity
- but needs something that is explicitly cleared on unload, not left around with stale ptrs.

>