Re: [PATCH 2/2] drm/i915/gvt: Avoid full proxy f_ops for vgpu_status debug attributes

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

On Wed, Jan 11, 2023 at 08:23:49PM +0530, Deepak R Varma wrote:
> On Wed, Jan 11, 2023 at 05:02:02AM -0500, Rodrigo Vivi wrote:
> > On Tue, Jan 10, 2023 at 01:49:57PM -0500, Rodrigo Vivi wrote:
> > > On Wed, Jan 11, 2023 at 12:00:12AM +0530, Deepak R Varma wrote:
> > > > Using DEFINE_SIMPLE_ATTRIBUTE macro with the debugfs_create_file()
> > > > function adds the overhead of introducing a proxy file operation
> > > > functions to wrap the original read/write inside file removal protection
> > > > functions. This adds significant overhead in terms of introducing and
> > > > managing the proxy factory file operations structure and function
> > > > wrapping at runtime.
> > > > As a replacement, a combination of DEFINE_DEBUGFS_ATTRIBUTE macro paired
> > > > with debugfs_create_file_unsafe() is suggested to be used instead.  The
> > > > DEFINE_DEBUGFS_ATTRIBUTE utilises debugfs_file_get() and
> > > > debugfs_file_put() wrappers to protect the original read and write
> > > > function calls for the debug attributes. There is no need for any
> > > > runtime proxy file operations to be managed by the debugfs core.
> > > > Following coccicheck make command helped identify this change:
> > > > 
> > > > make coccicheck M=drivers/gpu/drm/i915/ MODE=patch COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> > > > 
> > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > 
> > > I believe these 2 gvt cases could be done in one patch.
> > > But anyways,
> > > 
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > for both patches... and will leave these 2 patches for gvt folks
> > > to apply. Unless they ack and I apply in the drm-intel along with the other ones.
> > 
> > Actually, could you please address the checkpatch issues before we can push?
> > Sorry about that, but just noticed now when I was going to push the other ones.
> 
> Hello Rodrigo,
> The checkpatch warning is associated with the long "make coccicheck ..." command
> in the commit message. It is not part of the code, so is should not be carried
> forward into the code base.
> If you still want me to correct it, I will need to split it into two lines which
> I think still violates the commit description guidelines.

This part I would just ignore or fix myself while merging. But the next one about
the parenthesis alignment need to be fixed in the code so we need another version.
Since we try to avoid touching the code between CI and merge.

Then, since you need to change that, while changing that, also please break
the coccinelle line in the commit msg.

I'd appreciate to have the patch for the pxp as well :)

Thanks a lot,
Rodrigo.


> 
> Let me know what you think.
> 
> Thank you,
> ./drv
> 
> > 
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/debugfs.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c
> > > > index 03f081c3d9a4..baccbf1761b7 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/debugfs.c
> > > > @@ -165,7 +165,7 @@ static int vgpu_status_get(void *data, u64 *val)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -DEFINE_SIMPLE_ATTRIBUTE(vgpu_status_fops, vgpu_status_get, NULL, "0x%llx\n");
> > > > +DEFINE_DEBUGFS_ATTRIBUTE(vgpu_status_fops, vgpu_status_get, NULL, "0x%llx\n");
> > > >  
> > > >  /**
> > > >   * intel_gvt_debugfs_add_vgpu - register debugfs entries for a vGPU
> > > > @@ -182,8 +182,8 @@ void intel_gvt_debugfs_add_vgpu(struct intel_vgpu *vgpu)
> > > >  			    &vgpu_mmio_diff_fops);
> > > >  	debugfs_create_file_unsafe("scan_nonprivbb", 0644, vgpu->debugfs, vgpu,
> > > >  				   &vgpu_scan_nonprivbb_fops);
> > > > -	debugfs_create_file("status", 0644, vgpu->debugfs, vgpu,
> > > > -			    &vgpu_status_fops);
> > > > +	debugfs_create_file_unsafe("status", 0644, vgpu->debugfs, vgpu,
> > > > +				   &vgpu_status_fops);
> > > >  }
> > > >  
> > > >  /**
> > > > -- 
> > > > 2.34.1
> > > > 
> > > > 
> > > > 
> 
>