Re: [PATCH 1/3] drm/xe: Store pointer to struct xe_gt in gt/ debugfs directory

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Mar 27, 2024 at 06:20:22PM -0500, Lucas De Marchi wrote:
> On Mon, Mar 25, 2024 at 06:34:01PM +0100, Michal Wajdeczko wrote:
> > 
> > 
> > On 25.03.2024 18:01, Rodrigo Vivi wrote:
> > > On Wed, Feb 14, 2024 at 12:57:54PM +0100, Michal Wajdeczko wrote:
> > > > Attributes added under 'gt/' directories may wish to use that
> > > > in case they can't obtain it from elsewhere.
> > > > 
> > > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_gt_debugfs.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > > > index c4b67cf09f8f..207b992f1240 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> > > > @@ -225,6 +225,9 @@ void xe_gt_debugfs_register(struct xe_gt *gt)
> > > >  		return;
> > > >  	}
> > > > 
> > > > +	/* other attributes may use parent->d_inode->i_private */
> > > 
> > > what did you mean with this comment?
> > > if others are using, what would be the risks?
> > > is this a good thing? is this a bad thing?
> > 
> > maybe better wording should be:
> > 
> > /*
> > * Store the xe_gt pointer as private data of the gt/ directory node
> > * so other GT specific attributes under that directory may refer to
> > * it by looking at its parent node private data.
> > */
> > 
> > > 
> > > > +	root->d_inode->i_private = gt;
> > > 
> > > At first I thought this was intrusive, but then the following
> > > patches made me realize that we are already being intrusive
> > > when disrespecting the data:
> > > 
> > > include/drm/drm_debugfs.h
> > > struct drm_debugfs_info
> > > /** @data: Driver-private data, should not be device-specific. */
> > > 
> > > 
> > > So it looks that we do need something else.
> > > 
> > > Looking the i_private that you pointed out seems an alternative
> > > 
> > > include/linux/fs.h
> > > struct inode {
> > > void                    *i_private; /* fs or device private pointer */
> > > 
> > > it is a 'device' pointer rather then a 'driver', but I'm still confident
> > > that it is the right one to use.
> > 
> > GT aka xe_gt is more a device than a driver, no ?
> > 
> > > 
> > > It looks like the debugfs_create_file functions would override that
> > > anyway with the data. Also other places in the fs code where this is
> > > used for other checks.
> > 
> > the drm_debugfs will use i_private only on nodes that represent
> > individual attributes, it will not touch the parent node i_private
> > (which is our gt/ directory - and this where we set pointer to xe_gt)
> 
> I completely agree with this. It seems nice to be able to easily
> retrieve xe_gt. I´d just double check the lifecycle if we may not end up
> freeing something that's being used during unbind.
> 
> I can't do a thorough review of this and the other patches right now,
> but ack on the approach in this patch.
> 
> 
> Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>

for the series:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> 
> Lucas De Marchi